Re: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning
From: Arnd Bergmann <arnd@kernel.org>
Date: 2021-03-22 22:50:55
Also in:
ath11k, cgroups, dri-devel, intel-gfx, linux-arm-kernel, linux-scsi, linux-wireless, lkml, netdev
On Mon, Mar 22, 2021 at 11:09 PM Martin Sebor [off-list ref] wrote:
On 3/22/21 2:29 PM, Ingo Molnar wrote:quoted
* Arnd Bergmann [off-list ref] 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.
quoted hunk ↗ jump to hunk
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