So the reason mikeb@'s mbuf changes caused issues is that the way we
define struct mbuf is inherently fragile because it doesn't take
structure padding into account. Adding an int64_t member to struct
pkthdr changed the alignment from 4 bytes to 8 bytes on most 32-bit
architectures (but not i386). Since struct m_hdr is only 20 bytes
this means the compiler has to add padding between struct m_hdr and
union that includes struct pkthdr in struct mbuf. But this padding is
not included in the calculation of MLEN so struct mbuf gets too big.
FreeBSD solved the issue by adding explicit padding when !__LP64__.
But that's rather fragile. The padding would have to be reconsidered
whenever the defenition of the mbuf header changes.
NetBSD solved this by using offsetoff() instead of sizeof() in its
definition of MLEN. But that requires the definition of a dummy
structure through a macro that isn't exactly pretty. But it should be
fairly robust.
The diff below takes a different approach. It marks struct m_hdr as
8-byte aligned. That has the effect of padding out the struct to be a
multiple of 8 bytes (because otherways the elements of arrays of
struct m_hdr wouldn't be properly aligned). This makes us rely on
compiler extensions (C99 doesn't include the possibility to specify
alignment), but all compilers we care of provide such an extension and
it is nicely abstracted in <sys/cdefs.h>. That somewhat matters since
<sys/mbuf.h> exposes the struct mbuf definition to userland. This
approach should be robus (until somebody decides they absolutely need
a 128-bit type in their packet header).
Fun detail; gcc on armv7 crashes if I use __aligned(sizeof(int64_t))
instead of __aligned(8). The armv7 kernel boots fine with this diff,
but I'm defenitely doing a full build before I even consider
committing this.
Thoughts?
Index: sys/mbuf.h
===================================================================
RCS file: /cvs/src/sys/sys/mbuf.h,v
retrieving revision 1.226
diff -u -p -r1.226 mbuf.h
--- sys/mbuf.h 7 May 2017 17:53:30 -0000 1.226
+++ sys/mbuf.h 8 May 2017 11:11:09 -0000
@@ -86,7 +86,7 @@ struct m_hdr {
u_int mh_len; /* amount of data in this mbuf */
short mh_type; /* type of data in this mbuf */
u_short mh_flags; /* flags; see below */
-};
+} __aligned(8);
/* pf stuff */
struct pf_state_key;
@@ -122,6 +122,7 @@ struct pkthdr_pf {
struct pkthdr {
void *ph_cookie; /* additional data */
SLIST_HEAD(, m_tag) ph_tags; /* list of packet tags */
+ int64_t ph_timestamp; /* packet timestamp */
int len; /* total packet length */
u_int16_t ph_tagsset; /* mtags attached */
u_int16_t ph_flowid; /* pseudo unique flow id */
