Hi David On 03/02/2021 21:45, David Young wrote: > > This change looks a little hasty to me. > > It looks to me like some of these structs were __packed so that > they could be read/written directly from/to any offset in a packet > chain using mtod(), which does not pay any mind to the alignment > of `*t`: > > #define mtod(m, t) ((t)((m)->m_data)) > > I see gre_h is accessed in that way, just for one example.
Looking at the other places where this is handled, does this patch to gre_h address your concerns? I tested this on aarch64 which does not define __NO_STRICT_ALIGNMENT and it passes the KASSERT.
Roy diff -r e3c82b1d9c2e sys/net/if_gre.c --- a/sys/net/if_gre.c Mon Feb 08 01:00:49 2021 +0000 +++ b/sys/net/if_gre.c Tue Feb 09 06:55:44 2021 +0000 @@ -395,10 +395,26 @@ sc->sc_error_ev.ev_count++; return; } - if (m->m_len < sizeof(*gh) && (m = m_pullup(m, sizeof(*gh))) == NULL) { - GRE_DPRINTF(sc, "m_pullup failed\n"); - sc->sc_pullup_ev.ev_count++; - return; + + /* If the GRE header is not aligned, slurp it up into a new + * mbuf with space for link headers, in the event we forward + * it. Otherwise, if it is aligned, make sure the entire + * base GRE header is in the first mbuf of the chain. + */ + if (GRE_HDR_ALIGNED_P(mtod(m, void *)) == 0) { + if ((m = m_copyup(m, sizeof(struct gre_h), + (max_linkhdr + 3) & ~3)) == NULL) { + /* XXXJRT new stat, please */ + GRE_DPRINTF(sc, "m_copyup failed\n"); + sc->sc_pullup_ev.ev_count++; + return; + } + } else if (__predict_false(m->m_len < sizeof(struct gre_h))) { + if ((m = m_pullup(m, sizeof(struct gre_h))) == NULL) { + GRE_DPRINTF(sc, "m_pullup failed\n"); + sc->sc_pullup_ev.ev_count++; + return; + } } gh = mtod(m, const struct gre_h *); @@ -940,7 +956,6 @@ #endif M_PREPEND(m, sizeof(*gh), M_DONTWAIT); - if (m == NULL) { IF_DROP(&ifp->if_snd); error = ENOBUFS; @@ -948,6 +963,7 @@ } gh = mtod(m, struct gre_h *); + KASSERT(GRE_HDR_ALIGNED_P(gh)); gh->flags = 0; gh->ptype = etype; /* XXX Need to handle IP ToS. Look at how I handle IP TTL. */ diff -r e3c82b1d9c2e sys/net/if_gre.h --- a/sys/net/if_gre.h Mon Feb 08 01:00:49 2021 +0000 +++ b/sys/net/if_gre.h Tue Feb 09 06:55:44 2021 +0000 @@ -131,6 +131,11 @@ Present if (rt_pres == 1) */ }; +#ifdef __NO_STRICT_ALIGNMENT +#define GRE_HDR_ALIGNED_P(gh) 1 +#else +#define GRE_HDR_ALIGNED_P(gh) ((((vaddr_t) (gh)) & 3) == 0) +#endif #ifdef __CTASSERT __CTASSERT(sizeof(struct gre_h) == 4); #endif