re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
> http://netbsd.org/~kamil/kubsan/0007-boot-real-hardware.txt i fixed the hdafg.c ones here. not sure about the hdaudio.c ones, since they are already 1u << 31. leaving: x86/pci/pci_machdep.c ahcisata_core.c amd64/kobj_machdep.c netinet/tcp_input.c beyond the xhci one, that actually doesn't matter since the alignment is not required in the copy of the structure. .mrg.
Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
On Jan 6, 9:53am, nick.hud...@gmx.co.uk (Nick Hudson) wrote: -- Subject: Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys | I'm pretty sure this is the same as http://gnats.netbsd.org/50038 Yes, this looks like the same issue; we should not be patching individual drivers like this. The compiler should be producing correct code that handles unaligned access. | Maybe I should be brave enough to stop using PMAP_NOCACHE in bus_dma.c | for armv6+ and fix any missing bus_dmamap_sync calls. Isn't that orthogonal? christos
Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
On Jan 6, 3:59pm, nick.hud...@gmx.co.uk (Nick Hudson) wrote: -- Subject: Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys | > Isn't that orthogonal? | | Nope, because normal cached memory allows unaligned access (kernel and | userland). | I did not realize that the i_axe case was referencing uncached memory. If that's the case, then we should turn on the pmap flag and start fixing the drivers :-) But perhaps we don't want to inflict that pain to everyone until things are mostly fixed... christos
Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
On 06/01/2019 15:31, Christos Zoulas wrote: On Jan 6, 9:53am, nick.hud...@gmx.co.uk (Nick Hudson) wrote: -- Subject: Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys | I'm pretty sure this is the same as http://gnats.netbsd.org/50038 Yes, this looks like the same issue; we should not be patching individual drivers like this. The compiler should be producing correct code that handles unaligned access. | Maybe I should be brave enough to stop using PMAP_NOCACHE in bus_dma.c | for armv6+ and fix any missing bus_dmamap_sync calls. Isn't that orthogonal? Nope, because normal cached memory allows unaligned access (kernel and userland). Nick
Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
On Jan 6, 5:46pm, rokuy...@rk.phys.keio.ac.jp (Rin Okuyama) wrote:
-- Subject: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev
| I guess other codes can be miscompiled if -mno-unaligned-access is
| not specified. Can I commit the patch?
I believe this is the right way to do it, since we don't want unaligned
accesses in the kernel.
Best,
christos
|
| Thanks,
| rin
|
| Index: sys/arch/arm/conf/Makefile.arm
| ===
| RCS file: /home/netbsd/src/sys/arch/arm/conf/Makefile.arm,v
| retrieving revision 1.49
| diff -p -u -r1.49 Makefile.arm
| --- sys/arch/arm/conf/Makefile.arm22 Sep 2018 12:24:01 - 1.49
| +++ sys/arch/arm/conf/Makefile.arm6 Jan 2019 08:14:56 -
| @@ -53,6 +53,13 @@ CPPFLAGS.cpufunc_asm_armv6.S+= -mcpu=arm
| CPPFLAGS.cpufunc_asm_arm11.S+= -mcpu=arm1136j-s
| CPPFLAGS.cpufunc_asm_xscale.S+= -mcpu=xscale
|
| +# For GCC, -munaligned-access is enabled by default for ARMv6+.
| +# But the unaligned access is forbidden in the supervisor mode.
| +.if (!empty(MACHINE_ARCH:Mearmv6*) || !empty(MACHINE_ARCH:Mearmv7*)) \
| +&& ${ACTIVE_CC} == "gcc"
| +CFLAGS+= -mno-unaligned-access
| +.endif
| +
| ##
| ## (3) libkern and compat
| ##
-- End of excerpt from Rin Okuyama
Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
kUBSan detected a number of unaligned accesses in USB code:
http://netbsd.org/~kamil/kubsan/0007-boot-real-hardware.txt
On 06.01.2019 09:46, Rin Okuyama wrote:
> (CC added to port-...@netbsd.org)
>
> Let me summarize the problem briefly. In axe(4), there is a code where
> memcpy() is carried out from 2-byte aligned buffer to 4-byte structure:
>
> https://nxr.netbsd.org/xref/src/sys/dev/usb/if_axe.c#1284
>
> This results in kernel panic due to alignment fault on earmv[67]hf:
>
> https://twitter.com/furandon_pig/status/1071771151418908672
>
> In short, this is because -munaligned-access is enabled on ARMv6+ by
> default for GCC. As the unaligned memory access is forbidden in the
> supervisor mode unlike in the user mode, we need to explicitly specify
> -mno-unaligned-access for kernel on ARMv6+.
>
> On 2019/01/06 12:59, matthew green wrote:
>> Christos Zoulas writes:
>>> In article <20190106003905.60969f...@cvs.netbsd.org>,
>>> Rin Okuyama wrote:
-=-=-=-=-=-
Module Name: src
Committed By: rin
Date: Sun Jan 6 00:39:05 UTC 2019
Modified Files:
src/sys/dev/usb: if_axe.c
Log Message:
Fix kernel panic on arm reported by @furandon_pig on Twitter.
Hardware header is 2-byte aligned in RX buffer, not 4-byte.
For some architectures, __builtin_memcpy() of GCC 6 attempts to
copy 4-byte header at once, which results in alignment error.
>>>
>>> This is really ugly..
>>>
>>> https://stackoverflow.com/questions/24883410/armcc-problems-with-memcpy-alignment-exceptions
>>>
>>>
>>> Perhaps there is a better solution? Can't memcpy be smarter?
>>
>> hmmm, what happens if struct axe_sframe_hdr is not marked
>> "packed"? this feels like a compiler bug, but perhaps it
>> is assuming it can write 4 bytes to the structure when it
>> is only 2 byte aligned.
>>
>> is there a small test case that reproduces the problem?
>> preferably in user land?
>
> On 2019/01/06 15:25, m...@netbsd.org wrote:
>> Are we building ARM with -mstrict-alignment?
>
> I tried to reproduce the problem on userland. objdump(1) shows an
> unaligned load is generated. However, alignment fault does not occur:
>
> % uname -p
> earmv7hf
> % cat test.c
> #include
> #include
>
> int
> main()
> {
> char buf[sizeof(int) + 1];
> int i;
>
> fread(buf, 1, sizeof(buf), stdin);
> memcpy(, [1], sizeof(i));
> printf("0x%x\n", i);
> return 0;
> }
> % cc -g -O2 test.c && cc test.o
> % objdump test.o
> ...
> 28: e51b1013 ldr r1, [fp, #-19] ; 0xffed
> ...
> % ./a.out
> 01234
> 0x34333231
>
> This is because unaligned access is permitted for the user mode on
> ARMv6+. For GCC, -munaligned-access is enabled by default on ARMv6+.
> However, the unaligned access is forbidden in the supervisor mode.
> So, we need to explicitly specify -mno-unaligned-access for kernel
> on ARMv6+.
>
> By reverting if_axe.c r1.94 and applying the attached patch, axe(4)
> works fine on earmv7hf. We can see that the instruction is changed
> from word-wise load to byte-wise load by specifying
> -mno-unaligned-access:
>
> % armv7--netbsdelf-eabihf-objdump -d if_axe.o
> (before) 364: e4983004 ldr r3, [r8], #4
> --->
> (after) 364: e5d6 ldrb r0, [r6]
>
> I guess other codes can be miscompiled if -mno-unaligned-access is
> not specified. Can I commit the patch?
>
> Thanks,
> rin
>
> Index: sys/arch/arm/conf/Makefile.arm
> ===
> RCS file: /home/netbsd/src/sys/arch/arm/conf/Makefile.arm,v
> retrieving revision 1.49
> diff -p -u -r1.49 Makefile.arm
> --- sys/arch/arm/conf/Makefile.arm 22 Sep 2018 12:24:01 - 1.49
> +++ sys/arch/arm/conf/Makefile.arm 6 Jan 2019 08:14:56 -
> @@ -53,6 +53,13 @@ CPPFLAGS.cpufunc_asm_armv6.S+= -mcpu=arm
> CPPFLAGS.cpufunc_asm_arm11.S+= -mcpu=arm1136j-s
> CPPFLAGS.cpufunc_asm_xscale.S+= -mcpu=xscale
>
> +# For GCC, -munaligned-access is enabled by default for ARMv6+.
> +# But the unaligned access is forbidden in the supervisor mode.
> +.if (!empty(MACHINE_ARCH:Mearmv6*) || !empty(MACHINE_ARCH:Mearmv7*)) \
> + && ${ACTIVE_CC} == "gcc"
> +CFLAGS+= -mno-unaligned-access
> +.endif
> +
> ##
> ## (3) libkern and compat
> ##
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/sys/arch/x86/x86
Maxime Villard writes: > Can we do something about it now? It's been more than a week, and the issue is > still there. NVMM still doesn't modload, same for procfs, and GENERIC_KASLR > doesn't work either. > > This needs to be fixed now, and we should not start adding random hacks all > over the place (like the one below). > Sorry for the delay - I've rolled back the changes. http://mail-index.netbsd.org/source-changes/2019/01/06/msg102113.html The XEN related ones I will roll back if enough people complain. I'm meanwhile investigating other options. Thanks for your patience. -- ~cherry
Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
On Sun, Jan 06, 2019 at 05:52:55AM -0800, Jason Thorpe wrote: > That's probably a good idea in any case, because there will almost > certainly be a performance benefit, but I still think ensuring that > drivers don't perform unaligned accesses is desirable. It is a bit tricky. We do this only when compiling for CPUs that can do the unaligned access, i.e. when compiling kernels for arm v5 we tell gcc to not generate unaligned access ops. IIUC in this case the driver code was innocent (using proper memcpy), but gcc optimized the memcpy (which was fine too, given the flags we pass to it). Martin
Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
> On Jan 6, 2019, at 5:36 AM, Martin Husemann wrote: > > On Sun, Jan 06, 2019 at 08:31:53AM -0500, Greg Troxel wrote: >> Why do we generate code with unaligned access in user space? That seems >> surprising, if the processor isn't happy about it. > > The processor is happy with it, both in user- and kernel space. > Only special memory regions mapped uncached make it trap. There is a performance penalty for unaligned accesses, and not even all ARM versions can do it in a way that produces the expected results. The device in question here is a USB device, and we support pre-ARMv6 systems that have USB capability. > Nick suggest to change the mapping for bus_dma to cached, which would avoid > the issue but may expose bugs in device drivers. That's probably a good idea in any case, because there will almost certainly be a performance benefit, but I still think ensuring that drivers don't perform unaligned accesses is desirable. -- thorpej
Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
On Sun, Jan 06, 2019 at 08:31:53AM -0500, Greg Troxel wrote: > Why do we generate code with unaligned access in user space? That seems > surprising, if the processor isn't happy about it. The processor is happy with it, both in user- and kernel space. Only special memory regions mapped uncached make it trap. Nick suggest to change the mapping for bus_dma to cached, which would avoid the issue but may expose bugs in device drivers. Martin
Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
matthew green writes: >> In short, this is because -munaligned-access is enabled on ARMv6+ by >> default for GCC. As the unaligned memory access is forbidden in the >> supervisor mode unlike in the user mode, we need to explicitly specify >> -mno-unaligned-access for kernel on ARMv6+. > > i think this seems like the right thing to do here. > > othewise we'd have to patch this all over.. Why do we generate code with unaligned access in user space? That seems surprising, if the processor isn't happy about it.
re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
> In short, this is because -munaligned-access is enabled on ARMv6+ by > default for GCC. As the unaligned memory access is forbidden in the > supervisor mode unlike in the user mode, we need to explicitly specify > -mno-unaligned-access for kernel on ARMv6+. i think this seems like the right thing to do here. othewise we'd have to patch this all over.. .mrg.
Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
On 06/01/2019 08:46, Rin Okuyama wrote: (CC added to port-...@netbsd.org) Let me summarize the problem briefly. In axe(4), there is a code where memcpy() is carried out from 2-byte aligned buffer to 4-byte structure: https://nxr.netbsd.org/xref/src/sys/dev/usb/if_axe.c#1284 This results in kernel panic due to alignment fault on earmv[67]hf: https://twitter.com/furandon_pig/status/1071771151418908672 In short, this is because -munaligned-access is enabled on ARMv6+ by default for GCC. As the unaligned memory access is forbidden in the supervisor mode unlike in the user mode, we need to explicitly specify -mno-unaligned-access for kernel on ARMv6+. I'm pretty sure this is the same as http://gnats.netbsd.org/50038 Maybe I should be brave enough to stop using PMAP_NOCACHE in bus_dma.c for armv6+ and fix any missing bus_dmamap_sync calls. Nick
Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
(CC added to port-...@netbsd.org)
Let me summarize the problem briefly. In axe(4), there is a code where
memcpy() is carried out from 2-byte aligned buffer to 4-byte structure:
https://nxr.netbsd.org/xref/src/sys/dev/usb/if_axe.c#1284
This results in kernel panic due to alignment fault on earmv[67]hf:
https://twitter.com/furandon_pig/status/1071771151418908672
In short, this is because -munaligned-access is enabled on ARMv6+ by
default for GCC. As the unaligned memory access is forbidden in the
supervisor mode unlike in the user mode, we need to explicitly specify
-mno-unaligned-access for kernel on ARMv6+.
On 2019/01/06 12:59, matthew green wrote:
Christos Zoulas writes:
In article <20190106003905.60969f...@cvs.netbsd.org>,
Rin Okuyama wrote:
-=-=-=-=-=-
Module Name:src
Committed By: rin
Date: Sun Jan 6 00:39:05 UTC 2019
Modified Files:
src/sys/dev/usb: if_axe.c
Log Message:
Fix kernel panic on arm reported by @furandon_pig on Twitter.
Hardware header is 2-byte aligned in RX buffer, not 4-byte.
For some architectures, __builtin_memcpy() of GCC 6 attempts to
copy 4-byte header at once, which results in alignment error.
This is really ugly..
https://stackoverflow.com/questions/24883410/armcc-problems-with-memcpy-alignment-exceptions
Perhaps there is a better solution? Can't memcpy be smarter?
hmmm, what happens if struct axe_sframe_hdr is not marked
"packed"? this feels like a compiler bug, but perhaps it
is assuming it can write 4 bytes to the structure when it
is only 2 byte aligned.
is there a small test case that reproduces the problem?
preferably in user land?
On 2019/01/06 15:25, m...@netbsd.org wrote:
Are we building ARM with -mstrict-alignment?
I tried to reproduce the problem on userland. objdump(1) shows an
unaligned load is generated. However, alignment fault does not occur:
% uname -p
earmv7hf
% cat test.c
#include
#include
int
main()
{
char buf[sizeof(int) + 1];
int i;
fread(buf, 1, sizeof(buf), stdin);
memcpy(, [1], sizeof(i));
printf("0x%x\n", i);
return 0;
}
% cc -g -O2 test.c && cc test.o
% objdump test.o
...
28: e51b1013ldr r1, [fp, #-19] ; 0xffed
...
% ./a.out
01234
0x34333231
This is because unaligned access is permitted for the user mode on
ARMv6+. For GCC, -munaligned-access is enabled by default on ARMv6+.
However, the unaligned access is forbidden in the supervisor mode.
So, we need to explicitly specify -mno-unaligned-access for kernel
on ARMv6+.
By reverting if_axe.c r1.94 and applying the attached patch, axe(4)
works fine on earmv7hf. We can see that the instruction is changed
from word-wise load to byte-wise load by specifying
-mno-unaligned-access:
% armv7--netbsdelf-eabihf-objdump -d if_axe.o
(before) 364: e4983004ldr r3, [r8], #4
--->
(after) 364: e5d6ldrbr0, [r6]
I guess other codes can be miscompiled if -mno-unaligned-access is
not specified. Can I commit the patch?
Thanks,
rin
Index: sys/arch/arm/conf/Makefile.arm
===
RCS file: /home/netbsd/src/sys/arch/arm/conf/Makefile.arm,v
retrieving revision 1.49
diff -p -u -r1.49 Makefile.arm
--- sys/arch/arm/conf/Makefile.arm 22 Sep 2018 12:24:01 - 1.49
+++ sys/arch/arm/conf/Makefile.arm 6 Jan 2019 08:14:56 -
@@ -53,6 +53,13 @@ CPPFLAGS.cpufunc_asm_armv6.S+= -mcpu=arm
CPPFLAGS.cpufunc_asm_arm11.S+= -mcpu=arm1136j-s
CPPFLAGS.cpufunc_asm_xscale.S+=-mcpu=xscale
+# For GCC, -munaligned-access is enabled by default for ARMv6+.
+# But the unaligned access is forbidden in the supervisor mode.
+.if (!empty(MACHINE_ARCH:Mearmv6*) || !empty(MACHINE_ARCH:Mearmv7*)) \
+&& ${ACTIVE_CC} == "gcc"
+CFLAGS+= -mno-unaligned-access
+.endif
+
##
## (3) libkern and compat
##
