Re: CVS commit: src/sys/arch/x86/x86
>>> Misaligned pointer is explicitly documented as undefined behavior >>> in the C standard (C11 6.3.2.3 p7). >> So what? Do you have reason to think that sys/arch/x86 will at some >> point be ported to a compiler [] that does something unexpected with >> that code? > Due to UB a compiler is free to perform optimization and treat x86 > like a RISC architecture... Indeed. But a compiler is also free to not do so. Writing machine-dependent code in something other than assembler is dependent on having a compiler that produces the desired result. A compiler that doesn't, whether because it's broken or because it takes advantage of latitude in the languageg spec, simply is not suitable for that use. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: CVS commit: src/sys/arch/x86/x86
On 08.07.2018 17:30, Mouse wrote: > Caveat: this is all opinion. I'm not the one doing the work here. > > src/sys/arch/x86/x86: mpbios.c > > Remove unaligned access to mpbios_page[] > > sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address > 0x800031c7a413 for type 'const __uint16_t' which requires 2 byte > alignment > >>> Can we please do NOT do such stupid changes? > >> Kernel Undefined Behavior Sanitizer detects various portability >> issues including alignment. > > Portability issues are, in general, not issues when they are in code > that is inherently already nonportable - such as almost everything > under sys/arch/. > >> Misaligned pointer is explicitly documented as undefined behavior in >> the C standard (C11 6.3.2.3 p7). > > So what? Do you have reason to think that sys/arch/x86 will at some > point be ported to a compiler (I would say "and architecture", but it's > already tightly bound to a very small set of CPU architectures) that > does something unexpected with that code? Expecting the MD code in the > low levels of an OS to never produce formally implementation-defined or > undefined behaviour is a fool's dream. > > Programs such as undefined-behaviour detectors are tools to serve us, > not shackles to bind us. Intelligence should be applied when using > their results, including not expecting portability from inherently > nonportable code. > Due to UB a compiler is free to perform optimization and treat x86 like a RISC architecture... especially with more instruction sets like SSE (as noted by Christos). x86 sanitizers experienced alignment trouble too. MOVDQA used to break sanitizers in the interceptor of __tls_get_addr(). https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58066 I will try to propose a macro as noted in a reply to Jason Thorpe. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/arch/x86/x86
On 08.07.2018 17:16, Jason Thorpe wrote: > > >> On Jul 8, 2018, at 6:30 AM, Kamil Rytarowski wrote: >> >> In future __NO_STRICT_ALIGNMENT could be defined for aarch64, at least >> for the use of acpica (which still contains a fallback for Itanium >> without misaligned access, but not actively maintained). >> >> Linux uses a different approach and ships get_unaligned() and >> set_unaligned() macros and implements it on per ABI and CPU-basis. > > In general, I get the utility of UBSan, but for these unaligned access > issues, an ad hoc approach like what was done in mpbios is the wrong way to > go. > > Here's my $0.02: > > -- Define a set of standard unaligned-accessors/mutators. I propose: > > uint16_t__unaligned_load16(const uint16_t *); > uint32_t__unaligned_load32(const uint32_t *); > uint64_t__unaligned_load64(const uint64_t *); > > void__unaligned_store16(uint16_t *, uint16_t); > void__unaligned_store32(uint32_t *, uint32_t); > void__unaligned_store64(uint64_t *, uint64_t); > > ...and maybe you need to have another set for the signed value flavor, dunno. > (I guess probably, because you want to preserve the type information as much > as possible ... no casting.) > > -- Implement them as static inlines in a suitable system header > (, maybe, although these types are types, yah? So, > adjust the types to __uint16_t or whatever as necessary). Decorate only > these static inlines with the "don't sanitize me" directives. Implementing > them as inline functions rather than macros has 2 benefits: avoids unintended > multiple-evaluation of arguments, allows the smallest possible "don't > sanitize" footprint. > > -- Implement 2 standard sets, one for __NO_STRICT_ALIGNMENT platforms, and > one for the strict-alignment case. > I did something similar once upon a time when I was working on libo(verflow). https://github.com/krytarowski/libo/blob/nbsd/overflow.h There is a compiler switch for all C types: char, signed char, short, int, long, long long, unsigned char, unsigned short, unsigned int, unsigned long, unsigned long long. However at that point of time, C11 (_Generic) wasn't adopted so widely and pcc was in more active development so I've deferred the libo library for future. Today a compiler without C11 is not an issue. I can reuse the same idea and template (no licensing issues) for unaligned accessor and I will propose a patch. BTW. we are using a subset of the get_unaligned() feature macro in DRMKMS -- sys/external/bsd/drm2/include/asm/unaligned.h this file is even (c) TNF. I will try to scratch a new header unaligned.h with the set of macros and submit it to evaluation. > -- For changes like what would be needed in acpica, GET INPUT FROM THE > UPSTREAM as to how to integrate use of these macros, and preferably push the > changes to the upstream FIRST so that we can simply import them in a regular > update. > The discussion about the ACPICA case is ongoing: https://github.com/acpica/acpica/issues/393 "NetBSD Kernel Undefined Behavior Sanitizer: acpica reports" It will probably end up as a new macro for decoration of a function. Harry (luserx0) is working on it as a GSoC (sub)task. I intend to import the fix for acpica together with a new version of this software. > -- thorpej > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/arch/x86/x86
Caveat: this is all opinion. I'm not the one doing the work here. src/sys/arch/x86/x86: mpbios.c Remove unaligned access to mpbios_page[] sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address 0x800031c7a413 for type 'const __uint16_t' which requires 2 byte alignment >> Can we please do NOT do such stupid changes? > Kernel Undefined Behavior Sanitizer detects various portability > issues including alignment. Portability issues are, in general, not issues when they are in code that is inherently already nonportable - such as almost everything under sys/arch/. > Misaligned pointer is explicitly documented as undefined behavior in > the C standard (C11 6.3.2.3 p7). So what? Do you have reason to think that sys/arch/x86 will at some point be ported to a compiler (I would say "and architecture", but it's already tightly bound to a very small set of CPU architectures) that does something unexpected with that code? Expecting the MD code in the low levels of an OS to never produce formally implementation-defined or undefined behaviour is a fool's dream. Programs such as undefined-behaviour detectors are tools to serve us, not shackles to bind us. Intelligence should be applied when using their results, including not expecting portability from inherently nonportable code. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: CVS commit: src/sys/arch
On Sun, Jul 08, 2018 at 04:49:51PM +0200, Kamil Rytarowski wrote:
> On 08.07.2018 12:01, Martin Husemann wrote:
> >
> > This is unecessary churn for no good reason, please stop it.
> >
> > But worse are the other changes you are doing where kubsan insists on
> > natural alignement for {u,}int_{16,32,64} types, which is plain wrong,
> > these CPUs do not require that alignment (and it is not even clear
> > if kubsan propagates the alignment of structures correctly).
> >
> > Martin
> >
>
> I've started a thread on it in tech-kern@. Please move the discussion there.
OK, let's talk about it here. I would suggest that making changes to
MD code to conform to alignment constraints not actually present on the
"M" in question is not the right thing to do. I'd further suggest that
there are in fact cases where it can break things or harm performance
(the trick conditionally used in some device drivers of intentionally
misaligning structures so their fields accessed by DMA meet DMA alignment
constraints comes to mind).
--
Thor Lancelot Simont...@panix.com
"The two most common variations translate as follows:
illegitimi non carborundum = the unlawful are not silicon carbide
illegitimis non carborundum = the unlawful don't have silicon carbide."
Re: CVS commit: src/sys/arch/x86/x86
> On Jul 8, 2018, at 6:30 AM, Kamil Rytarowski wrote: > > In future __NO_STRICT_ALIGNMENT could be defined for aarch64, at least > for the use of acpica (which still contains a fallback for Itanium > without misaligned access, but not actively maintained). > > Linux uses a different approach and ships get_unaligned() and > set_unaligned() macros and implements it on per ABI and CPU-basis. In general, I get the utility of UBSan, but for these unaligned access issues, an ad hoc approach like what was done in mpbios is the wrong way to go. Here's my $0.02: -- Define a set of standard unaligned-accessors/mutators. I propose: uint16_t__unaligned_load16(const uint16_t *); uint32_t__unaligned_load32(const uint32_t *); uint64_t__unaligned_load64(const uint64_t *); void__unaligned_store16(uint16_t *, uint16_t); void__unaligned_store32(uint32_t *, uint32_t); void__unaligned_store64(uint64_t *, uint64_t); ...and maybe you need to have another set for the signed value flavor, dunno. (I guess probably, because you want to preserve the type information as much as possible ... no casting.) -- Implement them as static inlines in a suitable system header (, maybe, although these types are types, yah? So, adjust the types to __uint16_t or whatever as necessary). Decorate only these static inlines with the "don't sanitize me" directives. Implementing them as inline functions rather than macros has 2 benefits: avoids unintended multiple-evaluation of arguments, allows the smallest possible "don't sanitize" footprint. -- Implement 2 standard sets, one for __NO_STRICT_ALIGNMENT platforms, and one for the strict-alignment case. -- For changes like what would be needed in acpica, GET INPUT FROM THE UPSTREAM as to how to integrate use of these macros, and preferably push the changes to the upstream FIRST so that we can simply import them in a regular update. -- thorpej
Re: CVS commit: src/sys/arch/x86/x86
On 08.07.2018 15:53, Jaromír Doleček wrote: > Le dim. 8 juil. 2018 à 15:29, Kamil Rytarowski a écrit : >> I've introduced the change to mpbios.c as it was small, selfcontained >> and without the need to decorate the whole function. > > Am I reading the code wrong or you actually introduced bug in mpbios.c? > > Shouldn't this: > > memtop |= (uint16_t)mpbios_page[0x414] << 8; > > be actually << 16 to keep the same semantics? > > Jaromir > We are moving a 16-bit unaligned integer into a variable. The first byte is 0 bits shifted, the second one 8. According to my checks old and new versions produce the same result. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/arch/x86/x86
Le dim. 8 juil. 2018 à 15:29, Kamil Rytarowski a écrit : > I've introduced the change to mpbios.c as it was small, selfcontained > and without the need to decorate the whole function. Am I reading the code wrong or you actually introduced bug in mpbios.c? Shouldn't this: memtop |= (uint16_t)mpbios_page[0x414] << 8; be actually << 16 to keep the same semantics? Jaromir
Re: CVS commit: src/sys/arch/x86/x86
On 08.07.2018 11:24, Martin Husemann wrote: > On Sun, Jul 08, 2018 at 10:49:53AM +0200, Jaromír Dole?ek wrote: >>> Module Name:src >>> Committed By: kamil >>> Date: Sat Jul 7 23:05:50 UTC 2018 >>> >>> Modified Files: >>> src/sys/arch/x86/x86: mpbios.c >>> >>> Log Message: >>> Remove unaligned access to mpbios_page[] >>> >>> Replace unaligned pointer dereference with a more portable construct that >>> is free from Undefined Behavior semantics. >>> >>> sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address >>> 0x800031c7a413 for type 'const __uint16_t' which requires 2 byte >>> alignment > > > Can we please do NOT do such stupid changes? > > This is a bogus error message, please restore the original code! > > Martin > On the request by Martin, I'm moving this discussion to tech-kern. Kernel Undefined Behavior Sanitizer detects various portability issues including alignment. Misaligned pointer is explicitly documented as undefined behavior in the C standard (C11 6.3.2.3 p7). (In C++ it's basically the same.) Depending on the CPU, misaligned access can be either supported (with or without performance hit) or unsupported (CPU exception, reinterpret instruction to perform a different operation etc). For some CPUs it's possible to define a dedicated exception handler to align the data access on the performance cost. There is a NetBSD header symbol that marks certain architectures as capable to handle miaslignment: __NO_STRICT_ALIGNMENT. It's defined right now for m68k, vax, amd64 and i386. In future __NO_STRICT_ALIGNMENT could be defined for aarch64, at least for the use of acpica (which still contains a fallback for Itanium without misaligned access, but not actively maintained). Linux uses a different approach and ships get_unaligned() and set_unaligned() macros and implements it on per ABI and CPU-basis. Almost all of the remaining issues detected by Kernel UBsan come from unaligned memory access: Report from a boot in qemu: http://netbsd.org/~kamil/kubsan/0006-boot.txt Report from a boot on a real hardware (with drmkms disabled): http://netbsd.org/~kamil/kubsan/0007-boot-real-hardware.txt The plan with Kernel Sanitizers is to mark the issues detected with them as fatal, with a warning/fatal switch option for UBSan. Marking UBSan issues as fatal makes this sanitizer useful for fuzzing and regular checks for regressions (ATF execution), because it will detect the issues immediately or just detect at all. However in order to make Kernel UBSan more useful there is need to address the reports, some of them might be cautious. Handling cautious KUBSan reports can be treated similarly to handling compiler warnings. We are already marking a lot of variables as initialized, signed/unsigned, long/short or even properly intended. Some of them are overcautious. UBSan detects a similar set of problems during the runtime execution. Regarding the unaligned access, there is an option to: - mark certain kernel functions with no-sanitize flags, - address the reported issues if possible. I've introduced the change to mpbios.c as it was small, selfcontained and without the need to decorate the whole function. Various device drivers (vge(4), vr(4), sip(4), wdc(4) etc) contain fixups for misalignment access under ifdef. Other ones like bwfm(4) [1] contain fixups inlined and were detecting when a driver was in the process of porting from x86 to a port with strict alignment. Such issues as bwfm(4) could be detected with the sanitizer aid more easily. [1] commit 44bb30584312c2fa6bc0661178986c4965b67c0c Author: jmcneill Date: Fri Oct 20 23:38:21 2017 + Fix an alignment problem with scan results within an escan event signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/arch
On Sat, Jul 07, 2018 at 09:35:16PM +, Kamil Rytarowski wrote:
> Module Name: src
> Committed By: kamil
> Date: Sat Jul 7 21:35:16 UTC 2018
>
> Modified Files:
> src/sys/arch/amd64/include: tss.h
> src/sys/arch/i386/include: tss.h
>
> Log Message:
> Correct unportable signed integer left shift in i386/amd64 tss code
This change itself is OK, but your reasoning is broken. What do you
want to port the MD i386/amd64 code to?
This is unecessary churn for no good reason, please stop it.
But worse are the other changes you are doing where kubsan insists on
natural alignement for {u,}int_{16,32,64} types, which is plain wrong,
these CPUs do not require that alignment (and it is not even clear
if kubsan propagates the alignment of structures correctly).
Martin
Re: new errno ?
On Sat, Jul 7, 2018, 11:43 AM Jason Thorpe wrote: > > > On Jul 6, 2018, at 2:49 PM, Eitan Adler wrote: > > For those interested in some of the history: > https://lists.freebsd.org/pipermail/freebsd-hackers/2003-May/000791.html > > > ...and the subsequent thread went just as I expected it might. Sigh. > > Anyway... in what situations is this absurd error code used in the 802.11 > code? > ENOTTY is best for how 802.11 uses it. Warner EFAULT seems wrong because it means something very specific. Actually, > that brings me to a bigger point... rather than having a generic error code > for "lulz I could have panic'd here, heh", why not simply return an error > code appropriate for the situation that would have otherwise resulted in > calling panic()? There are many to choose from :-) > > -- thorpej > >
