On Mon, Mar 22, 2021 at 11:09 PM Martin Sebor <msebor@xxxxxxxxx> wrote: > On 3/22/21 2:29 PM, Ingo Molnar wrote: > > * Arnd Bergmann <arnd@xxxxxxxxxx> wrote: > > > > I.e. the real workaround might be to turn off the -Wstringop-overread-warning, > > until GCC-11 gets fixed? > > In GCC 10 -Wstringop-overread is a subset of -Wstringop-overflow. > GCC 11 breaks it out as a separate warning to make it easier to > control. Both warnings have caught some real bugs but they both > have a nonzero rate of false positives. Other than bug reports > we don't have enough data to say what their S/N ratio might be > but my sense is that it's fairly high in general. > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overread > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overflow Unfortunately, stringop-overflow is one of the warnings that is completely disabled in the kernel at the moment, rather than enabled at one of the user-selectable higher warning levels. I have a patch series to change that and to pull some of these into the lower W=1 or W=2 levels or even enable them by default. To do this though, I first need to ensure that the actual output is empty for the normal builds. I added -Wstringop-overflow to the list of warnings I wanted to address because it is a new warning and only has a dozen or so occurrences throughout the kernel. > In GCC 11, all access warnings expect objects to be either declared > or allocated. Pointers with constant values are taken to point to > nothing valid (as Arnd mentioned above, this is to detect invalid > accesses to members of structs at address zero). > > One possible solution to the known address problem is to extend GCC > attributes address and io that pin an object to a hardwired address > to all targets (at the moment they're supported on just one or two > targets). I'm not sure this can still happen before GCC 11 releases > sometime in April or May. > > Until then, another workaround is to convert the fixed address to > a volatile pointer before using it for the access, along the lines > below. It should have only a negligible effect on efficiency. > > diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c > index 4c09ba110204..76326b906010 100644 > --- a/arch/x86/kernel/tboot.c > +++ b/arch/x86/kernel/tboot.c > @@ -67,7 +67,9 @@ void __init tboot_probe(void) > /* Map and check for tboot UUID. */ > set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr); > tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE); > - if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) { > + if (memcmp(&tboot_uuid, > + (*(struct tboot* volatile *)(&tboot))->uuid, > + sizeof(tboot->uuid))) { > pr_warn("tboot at 0x%llx is invalid\n", I think a stray 'volatile' would raise too many red flags here, but I checked that the RELOC_HIDE() macro has the same effect, e.g. @@ -66,7 +67,8 @@ void __init tboot_probe(void) /* Map and check for tboot UUID. */ set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr); - tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE); + /* RELOC_HIDE to prevent gcc warnings about NULL pointer */ + tboot = RELOC_HIDE(NULL, fix_to_virt(FIX_TBOOT_BASE)); if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) { pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr); tboot = NULL; Arnd
