Re: CVS commit: src/sys/kern

[Masanobu SAITOH <msaitoh%execsw.org@localhost>]

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.


> > > 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.


--
-----------------------------------------------
                SAITOH Masanobu (msaitoh%execsw.org@localhost
                                 msaitoh%netbsd.org@localhost)