Re: CVS commit: src/sys/kern
On 02.08.2018 12:23, Kamil Rytarowski wrote: > 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. > For the record, I've landed the kUBSan implementation. Please uncomment the "kubsan" kernel config option and rebuild from scratch the kernel. It will detect misalignment issues. I've observed that GCC is more sensible to these checks than Clang/LLVM. signature.asc Description: OpenPGP digital signature
Re: ubsan.c (was: CVS commit: src/common/lib/libc/misc)
On 03.08.2018 04:48, Paul Goyette wrote: > If there are no licensing issues or concerns, then please describe the > real reason(s) for avoiding KNF. > > As discussed, we have removed the comment and drop the unnecessary part from CVS log. signature.asc Description: OpenPGP digital signature
Re: ubsan.c (was: CVS commit: src/common/lib/libc/misc)
Module Name:src Committed By: kamil Date: Fri Aug 3 02:05:43 UTC 2018 Added Files: src/common/lib/libc/misc: ubsan.c Log Message: Import micro-UBSan (ubsan.c) This file does not follow the regular KNF style, due to potential licensing concerns. If there are potential licensing issues, they should be discussed first, before the potentially offending code is committed. Avoiding KNF (nor any other sort of reformatting) is not going to avoid a license issue. If there are no licensing issues or concerns, then please describe the real reason(s) for avoiding KNF. +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org | +--+--++
Re: CVS commit: src/sys/kern
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=src > > > 12 M_REGION_GET()s: > https://nxr.netbsd.org/search?q=M_REGION_GET=src > > > 21 m_pulldown()s: > https://nxr.netbsd.org/search?q=m_pulldown=src > > > 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. >>> >>> >>> >> >> > > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
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:#defineIP_HDR_ALIGNED_P(ip)1 netinet/ip_private.h:#defineIP_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=src 12 M_REGION_GET()s: https://nxr.netbsd.org/search?q=M_REGION_GET=src 21 m_pulldown()s: https://nxr.netbsd.org/search?q=m_pulldown=src 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 (msai...@execsw.org msai...@netbsd.org)
Re: CVS commit: src/sys/kern
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. > >> >> 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. >> > > > signature.asc Description: OpenPGP digital signature
