From: Arnd Bergmann <arnd@kernel.org> To: Eric Biggers <ebiggers@kernel.org> Cc: linux-arch <linux-arch@vger.kernel.org>, Linus Torvalds <torvalds@linux-foundation.org>, Vineet Gupta <vgupta@synopsys.com>, Russell King <linux@armlinux.org.uk>, Herbert Xu <herbert@gondor.apana.org.au>, "David S. Miller" <davem@davemloft.net>, Thomas Bogendoerfer <tsbogend@alpha.franken.de>, Linux ARM <linux-arm-kernel@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" <linux-crypto@vger.kernel.org>, "open list:BROADCOM NVRAM DRIVER" <linux-mips@vger.kernel.org> Subject: Re: [PATCH v2 07/13] asm-generic: unaligned always use struct helpers Date: Tue, 18 May 2021 09:25:54 +0200 [thread overview] Message-ID: <CAK8P3a0iqe5V6uvaW+Eo0qiwzvyUVavVEfZGwXh4s8ad+0RdCg@mail.gmail.com> (raw) In-Reply-To: <YKLlyQnR+3uW4ETD@gmail.com> On Mon, May 17, 2021 at 11:53 PM Eric Biggers <ebiggers@kernel.org> wrote: > On Fri, May 14, 2021 at 12:00:55PM +0200, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > As found by Vineet Gupta and Linus Torvalds, gcc has somewhat unexpected > > behavior when faced with overlapping unaligned pointers. The kernel's > > unaligned/access-ok.h header technically invokes undefined behavior > > that happens to usually work on the architectures using it, but if the > > compiler optimizes code based on the assumption that undefined behavior > > doesn't happen, it can create output that actually causes data corruption. > > > > A related problem was previously found on 32-bit ARMv7, where most > > instructions can be used on unaligned data, but 64-bit ldrd/strd causes > > an exception. The workaround was to always use the unaligned/le_struct.h > > helper instead of unaligned/access-ok.h, in commit 1cce91dfc8f7 ("ARM: > > 8715/1: add a private asm/unaligned.h"). > > > > The same solution should work on all other architectures as well, so > > remove the access-ok.h variant and use the other one unconditionally on > > all architectures, picking either the big-endian or little-endian version. > > FYI, gcc 10 had a bug where it miscompiled code that uses "packed structs" to > copy between overlapping unaligned pointers > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94994). Thank you for pointing this out > I'm not sure whether the kernel will run into that or not, and gcc has since > fixed it. But it's worth mentioning, especially since the issue mentioned in > this commit sounds very similar (overlapping unaligned pointers), and both > involved implementations of DEFLATE decompression. I tried reproducing this on the kernel deflate code with the kernel.org gcc-10.1 and gcc-10.3 crosstool versions but couldn't quite get there with Vineet's preprocessed source https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100363 Trying with both the original get_unaligned() version in there and the packed-struct variant, I get the same output from gcc-10.1 and gcc-10.3 when I compile those myself for arc hs4x , but it's rather different from the output that Vineet got and I don't know how to spot whether the problem exists in any of those versions. > Anyway, partly due to the above, in userspace I now only use memcpy() to > implement {get,put}_unaligned_*, since these days it seems to be compiled > optimally and have the least amount of problems. > > I wonder if the kernel should do the same, or whether there are still cases > where memcpy() isn't compiled optimally. armv6/7 used to be one such case, but > it was fixed in gcc 6. It would have to be memmove(), not memcpy() in this case, right? My feeling is that if gcc-4.9 and gcc-5 produce correct but slightly slower code, we can live with that, unlike the possibility of gcc-10.{1,2} producing incorrect code. Since the new asm/unaligned.h has a single implementation across all architectures, we could probably fall back to a memmove based version for the compilers affected by the 94994 bug, but I'd first need to have a better way to test regarding whether given combination of asm/unaligned.h and compiler version runs into this bug. I have checked your reproducer and confirmed that it does affect x86_64 gcc-10.1 -O3 with my proposed version of asm-generic/unaligned.h, but does not trigger on any other version (4.9 though 9.3, 10.3 or 11.1), and not on -O2 or "-O3 -mno-sse" builds or on arm64, but that doesn't necessarily mean it's safe on these. Arnd
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@kernel.org> To: Eric Biggers <ebiggers@kernel.org> Cc: linux-arch <linux-arch@vger.kernel.org>, Linus Torvalds <torvalds@linux-foundation.org>, Vineet Gupta <vgupta@synopsys.com>, Russell King <linux@armlinux.org.uk>, Herbert Xu <herbert@gondor.apana.org.au>, "David S. Miller" <davem@davemloft.net>, Thomas Bogendoerfer <tsbogend@alpha.franken.de>, Linux ARM <linux-arm-kernel@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" <linux-crypto@vger.kernel.org>, "open list:BROADCOM NVRAM DRIVER" <linux-mips@vger.kernel.org> Subject: Re: [PATCH v2 07/13] asm-generic: unaligned always use struct helpers Date: Tue, 18 May 2021 09:25:54 +0200 [thread overview] Message-ID: <CAK8P3a0iqe5V6uvaW+Eo0qiwzvyUVavVEfZGwXh4s8ad+0RdCg@mail.gmail.com> (raw) In-Reply-To: <YKLlyQnR+3uW4ETD@gmail.com> On Mon, May 17, 2021 at 11:53 PM Eric Biggers <ebiggers@kernel.org> wrote: > On Fri, May 14, 2021 at 12:00:55PM +0200, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > As found by Vineet Gupta and Linus Torvalds, gcc has somewhat unexpected > > behavior when faced with overlapping unaligned pointers. The kernel's > > unaligned/access-ok.h header technically invokes undefined behavior > > that happens to usually work on the architectures using it, but if the > > compiler optimizes code based on the assumption that undefined behavior > > doesn't happen, it can create output that actually causes data corruption. > > > > A related problem was previously found on 32-bit ARMv7, where most > > instructions can be used on unaligned data, but 64-bit ldrd/strd causes > > an exception. The workaround was to always use the unaligned/le_struct.h > > helper instead of unaligned/access-ok.h, in commit 1cce91dfc8f7 ("ARM: > > 8715/1: add a private asm/unaligned.h"). > > > > The same solution should work on all other architectures as well, so > > remove the access-ok.h variant and use the other one unconditionally on > > all architectures, picking either the big-endian or little-endian version. > > FYI, gcc 10 had a bug where it miscompiled code that uses "packed structs" to > copy between overlapping unaligned pointers > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94994). Thank you for pointing this out > I'm not sure whether the kernel will run into that or not, and gcc has since > fixed it. But it's worth mentioning, especially since the issue mentioned in > this commit sounds very similar (overlapping unaligned pointers), and both > involved implementations of DEFLATE decompression. I tried reproducing this on the kernel deflate code with the kernel.org gcc-10.1 and gcc-10.3 crosstool versions but couldn't quite get there with Vineet's preprocessed source https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100363 Trying with both the original get_unaligned() version in there and the packed-struct variant, I get the same output from gcc-10.1 and gcc-10.3 when I compile those myself for arc hs4x , but it's rather different from the output that Vineet got and I don't know how to spot whether the problem exists in any of those versions. > Anyway, partly due to the above, in userspace I now only use memcpy() to > implement {get,put}_unaligned_*, since these days it seems to be compiled > optimally and have the least amount of problems. > > I wonder if the kernel should do the same, or whether there are still cases > where memcpy() isn't compiled optimally. armv6/7 used to be one such case, but > it was fixed in gcc 6. It would have to be memmove(), not memcpy() in this case, right? My feeling is that if gcc-4.9 and gcc-5 produce correct but slightly slower code, we can live with that, unlike the possibility of gcc-10.{1,2} producing incorrect code. Since the new asm/unaligned.h has a single implementation across all architectures, we could probably fall back to a memmove based version for the compilers affected by the 94994 bug, but I'd first need to have a better way to test regarding whether given combination of asm/unaligned.h and compiler version runs into this bug. I have checked your reproducer and confirmed that it does affect x86_64 gcc-10.1 -O3 with my proposed version of asm-generic/unaligned.h, but does not trigger on any other version (4.9 though 9.3, 10.3 or 11.1), and not on -O2 or "-O3 -mno-sse" builds or on arm64, but that doesn't necessarily mean it's safe on these. Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-05-18 7:27 UTC|newest] Thread overview: 98+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-14 10:00 [PATCH v2 00/13] Unify asm/unaligned.h around struct helper Arnd Bergmann 2021-05-14 10:00 ` [OpenRISC] " Arnd Bergmann 2021-05-14 10:00 ` Arnd Bergmann 2021-05-14 10:00 ` Arnd Bergmann 2021-05-14 10:00 ` [PATCH v2 01/13] asm-generic: use asm-generic/unaligned.h for most architectures Arnd Bergmann 2021-05-14 10:00 ` Arnd Bergmann 2021-05-14 10:00 ` Arnd Bergmann 2021-05-14 10:00 ` [PATCH v2 02/13] openrisc: always use unaligned-struct header Arnd Bergmann 2021-05-14 10:00 ` [OpenRISC] " Arnd Bergmann 2021-05-14 10:00 ` [PATCH v2 03/13] sh: remove unaligned access for sh4a Arnd Bergmann 2021-05-14 10:34 ` John Paul Adrian Glaubitz 2021-05-14 12:22 ` Arnd Bergmann 2021-05-15 15:36 ` John Paul Adrian Glaubitz 2021-05-15 20:10 ` Arnd Bergmann 2021-05-14 10:00 ` [PATCH v2 04/13] m68k: select CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS Arnd Bergmann 2021-05-14 10:00 ` [PATCH v2 05/13] powerpc: use linux/unaligned/le_struct.h on LE power7 Arnd Bergmann 2021-05-14 10:00 ` Arnd Bergmann 2021-05-14 11:48 ` Segher Boessenkool 2021-05-14 11:48 ` Segher Boessenkool 2021-05-14 13:02 ` Arnd Bergmann 2021-05-14 10:00 ` [PATCH v2 06/13] asm-generic: unaligned: remove byteshift helpers Arnd Bergmann 2021-05-14 10:00 ` Arnd Bergmann 2021-05-14 10:00 ` [PATCH v2 07/13] asm-generic: unaligned always use struct helpers Arnd Bergmann 2021-05-14 10:00 ` Arnd Bergmann 2021-05-17 21:53 ` Eric Biggers 2021-05-17 21:53 ` Eric Biggers 2021-05-18 7:25 ` Arnd Bergmann [this message] 2021-05-18 7:25 ` Arnd Bergmann 2021-05-18 14:56 ` Linus Torvalds 2021-05-18 14:56 ` Linus Torvalds 2021-05-18 15:41 ` Arnd Bergmann 2021-05-18 15:41 ` Arnd Bergmann 2021-05-18 16:12 ` Linus Torvalds 2021-05-18 16:12 ` Linus Torvalds 2021-05-18 18:09 ` Jason A. Donenfeld 2021-05-18 18:09 ` Jason A. Donenfeld 2021-05-18 20:51 ` Arnd Bergmann 2021-05-18 20:51 ` Arnd Bergmann 2021-05-18 21:31 ` Eric Biggers 2021-05-18 21:31 ` Eric Biggers 2021-05-18 21:14 ` David Laight 2021-05-18 21:14 ` David Laight 2021-05-14 10:00 ` [PATCH v2 08/13] partitions: msdos: fix one-byte get_unaligned() Arnd Bergmann 2021-05-17 10:28 ` Christoph Hellwig 2021-05-17 10:44 ` Arnd Bergmann 2021-05-14 10:00 ` [PATCH v2 09/13] apparmor: use get_unaligned() only for multi-byte words Arnd Bergmann 2021-05-14 10:00 ` [PATCH v2 10/13] mwifiex: re-fix for unaligned accesses Arnd Bergmann 2021-05-15 6:22 ` Kalle Valo 2021-05-15 9:01 ` Arnd Bergmann 2021-05-15 18:23 ` Kalle Valo 2021-05-14 10:00 ` [PATCH v2 11/13] netpoll: avoid put_unaligned() on single character Arnd Bergmann 2021-05-14 10:01 ` [PATCH v2 12/13] asm-generic: uaccess: 1-byte access is always aligned Arnd Bergmann 2021-05-15 18:41 ` Randy Dunlap 2021-05-15 20:16 ` Arnd Bergmann 2021-05-14 10:01 ` [PATCH v2 13/13] asm-generic: simplify asm/unaligned.h Arnd Bergmann 2021-05-14 10:35 ` David Laight 2021-05-14 17:32 ` [PATCH v2 00/13] Unify asm/unaligned.h around struct helper Linus Torvalds 2021-05-14 17:32 ` [OpenRISC] " Linus Torvalds 2021-05-14 17:32 ` Linus Torvalds 2021-05-14 17:32 ` Linus Torvalds 2021-05-14 18:51 ` Vineet Gupta 2021-05-14 18:51 ` [OpenRISC] " Vineet Gupta 2021-05-14 18:51 ` Vineet Gupta 2021-05-14 18:51 ` Vineet Gupta 2021-05-14 19:22 ` Linus Torvalds 2021-05-14 19:22 ` [OpenRISC] " Linus Torvalds 2021-05-14 19:22 ` Linus Torvalds 2021-05-14 19:22 ` Linus Torvalds 2021-05-14 19:45 ` Vineet Gupta 2021-05-14 19:45 ` [OpenRISC] " Vineet Gupta 2021-05-14 19:45 ` Vineet Gupta 2021-05-14 19:45 ` Vineet Gupta 2021-05-14 20:19 ` Linus Torvalds 2021-05-14 20:19 ` [OpenRISC] " Linus Torvalds 2021-05-14 20:19 ` Linus Torvalds 2021-05-14 20:19 ` Linus Torvalds 2021-05-14 19:31 ` Arnd Bergmann 2021-05-14 19:31 ` [OpenRISC] " Arnd Bergmann 2021-05-14 19:31 ` Arnd Bergmann 2021-05-14 19:31 ` Arnd Bergmann 2021-12-16 17:29 ` Ard Biesheuvel 2021-12-16 17:29 ` [OpenRISC] " Ard Biesheuvel 2021-12-16 17:29 ` Ard Biesheuvel 2021-12-16 17:42 ` Linus Torvalds 2021-12-16 17:42 ` [OpenRISC] " Linus Torvalds 2021-12-16 17:42 ` Linus Torvalds 2021-12-16 17:49 ` David Laight 2021-12-16 17:49 ` [OpenRISC] " David Laight 2021-12-16 17:49 ` David Laight 2021-12-16 18:56 ` Segher Boessenkool 2021-12-16 18:56 ` [OpenRISC] " Segher Boessenkool 2021-12-16 18:56 ` Segher Boessenkool 2021-12-17 12:34 ` David Laight 2021-12-17 12:34 ` [OpenRISC] " David Laight 2021-12-17 12:34 ` David Laight 2021-12-17 13:35 ` Segher Boessenkool 2021-12-17 13:35 ` [OpenRISC] " Segher Boessenkool 2021-12-17 13:35 ` Segher Boessenkool
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAK8P3a0iqe5V6uvaW+Eo0qiwzvyUVavVEfZGwXh4s8ad+0RdCg@mail.gmail.com \ --to=arnd@kernel.org \ --cc=davem@davemloft.net \ --cc=ebiggers@kernel.org \ --cc=herbert@gondor.apana.org.au \ --cc=linux-arch@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-crypto@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mips@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=torvalds@linux-foundation.org \ --cc=tsbogend@alpha.franken.de \ --cc=vgupta@synopsys.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.