On 02.08.2018 12:09, Masanobu SAITOH wrote: > On 2018/08/02 18:10, Kamil Rytarowski wrote: >> On 02.08.2018 06:33, Masanobu SAITOH wrote: >>> On 2018/08/02 13:28, SAITOH Masanobu wrote: >>>> Module Name: src >>>> Committed By: msaitoh >>>> Date: Thu Aug 2 04:28:56 UTC 2018 >>>> >>>> Modified Files: >>>> src/sys/kern: uipc_mbuf2.c >>>> >>>> Log Message: >>>> Adjust alignment in m_pulldown(). >>>> >>>> IP6_EXTHDR_GET() and M_REGION_GET() do m_pulldown(). When >>>> m_pulldown() copies >>>> data into M_TRAILINGSPACE, the alignment might be changed. There are a >>>> lot of >>>> IP6_EXTHDR_GET() calls, so I think it's not good to check the >>>> alignment after >>>> every IP6_EXTHDR_GET() call. This change fixes this problem in >>>> m_pulldown(). >>>> In this commit, the next mbuf are 4 byte aligned. For networking, I've >>>> never >>>> heard that 64bit alignment is required, so I think it would be OK. >>>> >>>> I don't know this is the best solution, but it's better than >>>> nothing. >>>> >>>> OK'd by maxv@. >>>> >>>> After committing this change, the workaround code for PR#50776 can >>>> be removed. >>> >>> Not PR#50776 but PR#50766 >>> (panic in tcp_input.c on the banana pi (earm7hf) when trying to connect >>> an ipv6 address through a gif ipv6 in ipv4 tunnel) >>> >> >> I've identified various similar misalignment in the kernel. >> >> I will land the utility (micro-UBSan) to the sources soon (this week?). >> It got delayed few days because rebase to HEAD with newer clang >> introduced new checks and I needed to cover them with handlers and tests. > > The problem is that many people (include me) didn't know > IP6_EXTHDR_GET() and M_REGION_GET() (and m_pulldown()) might change > the next mbuf's alignment. It's not documented. > > > Currently, >> grep _ALIGNED_P */*.h >> netinet/icmp_private.h:#define ICMP_HDR_ALIGNED_P(ic) 1 >> netinet/icmp_private.h:#define ICMP_HDR_ALIGNED_P(ic) ((((vaddr_t) >> (ic)) & 3) == 0) >> netinet/igmp_var.h:#define IGMP_HDR_ALIGNED_P(ig) 1 >> netinet/igmp_var.h:#define IGMP_HDR_ALIGNED_P(ig) ((((vaddr_t) >> (ig)) & 3) == 0) >> netinet/ip_private.h:#define IP_HDR_ALIGNED_P(ip) 1 >> netinet/ip_private.h:#define IP_HDR_ALIGNED_P(ip) ((((vaddr_t) >> (ip)) & 3) == 0) >> netinet/tcp_private.h:#define TCP_HDR_ALIGNED_P(th) 1 >> netinet/tcp_private.h:#define TCP_HDR_ALIGNED_P(th) >> ((((vaddr_t)(th)) & 3) == 0) >> netinet/udp_private.h:#define UDP_HDR_ALIGNED_P(uh) 1 >> netinet/udp_private.h:#define UDP_HDR_ALIGNED_P(uh) ((((vaddr_t) >> (uh)) & 3) == 0) >> netinet6/ip6_private.h:#define IP6_HDR_ALIGNED_P(ip) 1 >> netinet6/ip6_private.h:#define IP6_HDR_ALIGNED_P(ip) ((((vaddr_t) >> (ip)) & 3) == 0) > > so 4 byte alignment is OK. I heard that OpenFlow's packet has uint64_t > values, > so this solution might not work in future. > > Correct solution is to check all code around IP6_EXTHDR_GET(), > M_REGION_GET() and m_pulldown(). If a code before those call expects > the mbuf is aligned, (re-)check the alignment with xxx_HDR_ALIGNED_P(). > If it's not aligned, (re-)align with m_copyup(). > > > 28 IP6_EXTHDR_GET()s: > https://nxr.netbsd.org/search?q=IP6_EXTHDR_GET&project=src&defs=&refs=&path=&hist= > > > 12 M_REGION_GET()s: > https://nxr.netbsd.org/search?q=M_REGION_GET&project=src&defs=&refs=&path=&hist= > > > 21 m_pulldown()s: > https://nxr.netbsd.org/search?q=m_pulldown&project=src&defs=&refs=&path=&hist= > > > Not so much? > > Let me know if my understanding is incorrect. > I will defer the research on a proper solution in the networking code as I'm swamped by the development and improving of tool catching misalignment access. I will be done with it soon. With the tool aboard, there will be an option to double check the code for portability issues: Please see some of my older logs for examples of misaligned access: http://netbsd.org/~kamil/kubsan/0005-atf-run.txt (UBSan in the kernel) or http://netbsd.org/~kamil/mksanitizer-reports/atf-mklibcsanitizer-2018-07-25.txt (UBSan in libc) > >>> >>>> >>>> To generate a diff of this commit: >>>> cvs rdiff -u -r1.32 -r1.33 src/sys/kern/uipc_mbuf2.c >>>> >>>> Please note that diffs are not public domain; they are subject to the >>>> copyright notices on the relevant files. >>>> >>> >>> >>> >> >> > >
Attachment:
signature.asc
Description: OpenPGP digital signature