Date: Tue, 27 Aug 2013 20:52:03 +0000 (UTC) From: Andre Oppermann <andre@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r254973 - in head/sys: kern sys Message-ID: <201308272052.r7RKq3Uq051699@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: andre Date: Tue Aug 27 20:52:02 2013 New Revision: 254973 URL: http://svnweb.freebsd.org/changeset/base/254973 Log: Pad m_hdr on 32bit architectures to to prevent alignment and padding problems with the way MLEN, MHLEN, and struct mbuf are set up. CTASSERT's are provided to detect such issues at compile time in the future. The #define MLEN and MHLEN calculation do not take actual compiler- induced alignment and padding inside the complete struct mbuf into account. Accordingly appropriate attention is required when changing members of struct mbuf. Ideally one would calculate MLEN as (MSIZE - sizeof(((struct mbuf *)0)->m_hdr) but that doesn't work as the compiler refuses to operate on an as of yet incomplete structure. In particular ARM 32bit has more strict alignment requirements which caused 4 bytes of padding between m_hdr and pkthdr in struct mbuf because of the 64bit members in pkthdr. This wasn't picked up by MLEN and MHLEN causing an overflow of the mbuf provided data storage by overestimating its size. I386 didn't show this problem because it handles unaligned access just fine, albeit at a small performance penalty. On 64bit architectures the struct mbuf layout is 64bit aligned in all places. Reported by: Thomas Skibo <ThomasSkibo-at-sbcglobal-dot-net> Tested by: tuexen, ian, Thomas Skibo (extended patch) Sponsored by: The FreeBSD Foundation Modified: head/sys/kern/uipc_mbuf.c head/sys/sys/mbuf.h Modified: head/sys/kern/uipc_mbuf.c ============================================================================== --- head/sys/kern/uipc_mbuf.c Tue Aug 27 20:43:27 2013 (r254972) +++ head/sys/kern/uipc_mbuf.c Tue Aug 27 20:52:02 2013 (r254973) @@ -85,6 +85,14 @@ SYSCTL_INT(_kern_ipc, OID_AUTO, m_defrag #endif /* + * Ensure the correct size of various mbuf parameters. It could be off due + * to compiler-induced padding and alignment artifacts. + */ +CTASSERT(sizeof(struct mbuf) == MSIZE); +CTASSERT(MSIZE - offsetof(struct mbuf, m_dat) == MLEN); +CTASSERT(MSIZE - offsetof(struct mbuf, m_pktdat) == MHLEN); + +/* * m_get2() allocates minimum mbuf that would fit "size" argument. */ struct mbuf * Modified: head/sys/sys/mbuf.h ============================================================================== --- head/sys/sys/mbuf.h Tue Aug 27 20:43:27 2013 (r254972) +++ head/sys/sys/mbuf.h Tue Aug 27 20:52:02 2013 (r254973) @@ -53,6 +53,10 @@ * externally and attach it to the mbuf in a way similar to that of mbuf * clusters. * + * NB: These calculation do not take actual compiler-induced alignment and + * padding inside the complete struct mbuf into account. Appropriate + * attention is required when changing members of struct mbuf. + * * MLEN is data length in a normal mbuf. * MHLEN is data length in an mbuf with pktheader. * MINCLSIZE is a smallest amount of data that should be put into cluster. @@ -84,7 +88,7 @@ struct mb_args { /* * Header present at the beginning of every mbuf. - * Size ILP32: 20 + * Size ILP32: 24 * LP64: 32 */ struct m_hdr { @@ -94,6 +98,9 @@ struct m_hdr { int32_t mh_len; /* amount of data in this mbuf */ uint32_t mh_type:8, /* type of data in this mbuf */ mh_flags:24; /* flags; see below */ +#if !defined(__LP64__) + uint32_t mh_pad; /* pad for 64bit alignment */ +#endif }; /*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201308272052.r7RKq3Uq051699>