Message-ID: <7ddc3a12-1d8f-4226-895e-1f484087ab73@linux.ibm.com>
Date: Thu, 7 Nov 2024 19:00:39 +0530
From: Nilay Shroff <nilay@...ux.ibm.com>
To: Yury Norov <yury.norov@...il.com>
Cc: briannorris@...omium.org, kees@...nel.org, nathan@...nel.org,
        linux-kernel@...r.kernel.org, Gregory Joyce <gjoyce@....com>
Subject: Re: [bug report] cpumask: gcc 13.x emits compilation error on PowerPC


On 11/6/24 21:42, Yury Norov wrote:
> On Wed, Nov 06, 2024 at 06:32:23PM +0530, Nilay Shroff wrote:
>> Hi,
>>
>> Of late, I've been encountering the following compilation error while using GCC 13.x and latest upstream code:
>>
>> Compilation error:
>> ==================
>>   <snip>
>>   CC      kernel/padata.o
>> In file included from ./include/linux/string.h:390,
>>                  from ./arch/powerpc/include/asm/paca.h:16,
>>                  from ./arch/powerpc/include/asm/current.h:13,
>>                  from ./include/linux/thread_info.h:23,
>>                  from ./include/asm-generic/preempt.h:5,
>>                  from ./arch/powerpc/include/generated/asm/preempt.h:1,
>>                  from ./include/linux/preempt.h:79,
>>                  from ./include/linux/spinlock.h:56,
>>                  from ./include/linux/swait.h:7,
>>                  from ./include/linux/completion.h:12,
>>                  from kernel/padata.c:14:
>> In function ‘bitmap_copy’,
>>     inlined from ‘cpumask_copy’ at ./include/linux/cpumask.h:839:2,
>>     inlined from ‘__padata_set_cpumasks’ at kernel/padata.c:730:2:
>> ./include/linux/fortify-string.h:114:33: error: ‘__builtin_memcpy’ reading between 257 and 536870904 bytes from a region of size 256 [-Werror=stringop-overread]
>>   114 | #define __underlying_memcpy     __builtin_memcpy
>>       |                                 ^
>> ./include/linux/fortify-string.h:633:9: note: in expansion of macro ‘__underlying_memcpy’
>>   633 |         __underlying_##op(p, q, __fortify_size);                        \
>>       |         ^~~~~~~~~~~~~
>> ./include/linux/fortify-string.h:678:26: note: in expansion of macro ‘__fortify_memcpy_chk’
>>   678 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
>>       |                          ^~~~~~~~~~~~~~~~~~~~
>> ./include/linux/bitmap.h:259:17: note: in expansion of macro ‘memcpy’
>>   259 |                 memcpy(dst, src, len);
>>       |                 ^~~~~~
>> kernel/padata.c: In function ‘__padata_set_cpumasks’:
>> kernel/padata.c:713:48: note: source object ‘pcpumask’ of size [0, 256]
>>   713 |                                  cpumask_var_t pcpumask,
>>       |                                  ~~~~~~~~~~~~~~^~~~~~~~
>> In function ‘bitmap_copy’,
>>     inlined from ‘cpumask_copy’ at ./include/linux/cpumask.h:839:2,
>>     inlined from ‘__padata_set_cpumasks’ at kernel/padata.c:730:2:
>> ./include/linux/fortify-string.h:114:33: error: ‘__builtin_memcpy’ reading between 257 and 536870904 bytes from a region of size 256 [-Werror=stringop-overread]
>>   114 | #define __underlying_memcpy     __builtin_memcpy
>>       |                                 ^
>> ./include/linux/fortify-string.h:633:9: note: in expansion of macro ‘__underlying_memcpy’
>>   633 |         __underlying_##op(p, q, __fortify_size);                        \
>>       |         ^~~~~~~~~~~~~
>> ./include/linux/fortify-string.h:678:26: note: in expansion of macro ‘__fortify_memcpy_chk’
>>   678 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
>>       |                          ^~~~~~~~~~~~~~~~~~~~
>> ./include/linux/bitmap.h:259:17: note: in expansion of macro ‘memcpy’
>>   259 |                 memcpy(dst, src, len);
>>       |                 ^~~~~~
>> kernel/padata.c: In function ‘__padata_set_cpumasks’:
>> kernel/padata.c:713:48: note: source object ‘pcpumask’ of size [0, 256]
>>   713 |                                  cpumask_var_t pcpumask,
>>       |                                  ~~~~~~~~~~~~~~^~~~~~~~
>> cc1: all warnings being treated as errors
>> make[3]: *** [scripts/Makefile.build:229: kernel/padata.o] Error 1
>> make[2]: *** [scripts/Makefile.build:478: kernel] Error 2
>> make[1]: *** [/root/linux/Makefile:1936: .] Error 2
>> make: *** [Makefile:224: __sub-make] Error 2
>>
>> # gcc --version 
>> gcc (GCC) 13.2.1 20231205 (Red Hat 13.2.1-6)
>> Copyright (C) 2023 Free Software Foundation, Inc.
>> This is free software; see the source for copying conditions.  There is NO
>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>>
>> Note:
>> =====
>> I don't encounter above error using GCC 11.x and 12.x on PowerPC.
>> Moreover, I don't encounter above error using GCC 11.x or 12.x or 13.x on x86_64.
>>
>> Git bisect:
>> ===========
>> The git bisect points to the following commit causing the above compilation error:
>>
>> commit ab6b1010dab68f6d4bf063517db4ce2d63554bc6 (HEAD)
>> Author: Brian Norris <briannorris@...omium.org>
>> Date:   Thu Jul 18 17:50:39 2024 -0700
>>
>>     cpumask: Switch from inline to __always_inline
>>     
>>     On recent (v6.6+) builds with Clang (based on Clang 18.0.0) and certain
>>     configurations [0], I'm finding that (lack of) inlining decisions may
>>     lead to section mismatch warnings like the following:
>>     
>>       WARNING: modpost: vmlinux.o: section mismatch in reference:
>>       cpumask_andnot (section: .text) ->
>>       cpuhp_bringup_cpus_parallel.tmp_mask (section: .init.data) ERROR:
>>       modpost: Section mismatches detected.
>>     
>>     or more confusingly:
>>     
>>       WARNING: modpost: vmlinux: section mismatch in reference:
>>       cpumask_andnot+0x5f (section: .text) -> efi_systab_phys (section:
>>       .init.data)
>>     
>>     The first warning makes a little sense, because
>>     cpuhp_bringup_cpus_parallel() (an __init function) calls
>>     cpumask_andnot() on tmp_mask (an __initdata symbol). If the compiler
>>     doesn't inline cpumask_andnot(), this may appear like a mismatch.
>>     
>>     The second warning makes less sense, but might be because efi_systab_phys
>>     and cpuhp_bringup_cpus_parallel.tmp_mask are laid out near each other,
>>     and the latter isn't a proper C symbol definition.
>>     
>>     In any case, it seems a reasonable solution to suggest more strongly to
>>     the compiler that these cpumask macros *must* be inlined, as 'inline' is
>>     just a recommendation.
>>     
>>     This change has been previously proposed in the past as:
>>       Subject: [PATCH 1/3] bitmap: switch from inline to __always_inline
>>       https://lore.kernel.org/all/20221027043810.350460-2-yury.norov@gmail.com/
>>     
>>     But the change has been split up, to separately justify the cpumask
>>     changes (which drive my work) and the bitmap/const optimizations (that
>>     Yury separately proposed for other reasons). This ends up as somewhere
>>     between a "rebase" and "rewrite" -- I had to rewrite most of the patch.
>>     
>>     According to bloat-o-meter, vmlinux decreases minimally in size (-0.00%
>>     to -0.01%, depending on the version of GCC or Clang and .config in
>>     question) with this series of changes:
>>     
>>     gcc 13.2.0, x86_64_defconfig
>>     -3005 bytes, Before=21944501, After=21941496, chg -0.01%
>>     
>>     clang 16.0.6, x86_64_defconfig
>>     -105 bytes, Before=22571692, After=22571587, chg -0.00%
>>     
>>     gcc 9.5.0, x86_64_defconfig
>>     -1771 bytes, Before=21557598, After=21555827, chg -0.01%
>>     
>>     clang 18.0_pre516547 (ChromiumOS toolchain), x86_64_defconfig
>>     -191 bytes, Before=22615339, After=22615148, chg -0.00%
>>     
>>     clang 18.0_pre516547 (ChromiumOS toolchain), based on ChromiumOS config + gcov
>>     -979 bytes, Before=76294783, After=76293804, chg -0.00%
>>     
>>     [0] CONFIG_HOTPLUG_PARALLEL=y ('select'ed for x86 as of [1]) and
>>         CONFIG_GCOV_PROFILE_ALL.
>>     
>>     [1] commit 0c7ffa32dbd6 ("x86/smpboot/64: Implement
>>         arch_cpuhp_init_parallel_bringup() and enable it")
>>     
>>     Co-developed-by: Brian Norris <briannorris@...omium.org>
>>     Signed-off-by: Brian Norris <briannorris@...omium.org>
>>     Reviewed-by: Kees Cook <kees@...nel.org>
>>     Reviewed-by: Nathan Chancellor <nathan@...nel.org>
>>     Signed-off-by: Yury Norov <yury.norov@...il.com>
>>
>> So it appears that changing cpumask_copy() from inline to __always_inline causing the 
>> above error using Gcc 13.x. I am not gcc expert but it seems some issue with GCC 13.x?
>>
>> I tried the following patch which helps fix the above error but I'm not sure if this
>> is the proper fix or do we need to fix it differently.
>>
>> Patch:
>> ======
>> diff --git a/kernel/padata.c b/kernel/padata.c
>> index d899f34558af..86aad2f71890 100644
>> --- a/kernel/padata.c
>> +++ b/kernel/padata.c
>> @@ -710,8 +710,8 @@ static bool padata_validate_cpumask(struct padata_instance *pinst,
>>  }
>>  
>>  static int __padata_set_cpumasks(struct padata_instance *pinst,
>> -                                cpumask_var_t pcpumask,
>> -                                cpumask_var_t cbcpumask)
>> +                                struct cpumask *pcpumask,
>> +                                struct cpumask *cbcpumask)
>>  {
>>         int valid;
>>         int err;
>  
> This only works if CONFIG_CPUMASK_OFFSTACK=y. Otherwise, cpumask_var_t
> is declared as:
> 
> typedef struct cpumask cpumask_var_t[1];
> 
> and your hack wouldn't work. You can read a comment starting with "Oh, the
> wicked games we play!" in include/linux/cpumask_types.h for details. :)
>  
>> Please let me know if you need any further information.
> 
> config usually helps. Is it defconfig? What instrumentation is
> enabled? Can you try the same without *ASAN and friends?
> 
Thank you for your suggestions!

It is not defconfig. It is redhat/distro config and I don't have *ASAN and friends 
configured in this distro config. BTW, today I tried with defconfig and I couldn't 
re-create the error but then later I closely reviewed both redhat/distro config and 
defconfig and one config option which drew my attention was CONFIG_FORTIFY_SOURCE. 
The CONFIG_FORTIFY_SOURCE was not enabled in defconfig  however it was enabled on 
my redhat/distro config. 

Later, I enabled CONFIG_FORTIFY_SOURCE in defconfig and I could recreate the same 
compilation error. I also tried disabling CONFIG_FORTIFY_SOURCE in running redhat/distro 
config and the above compilation error went away. So now it seems to me that if we 
include "fortify-string.h" file while compiling kernel/padata.c file then above compiler 
(GCC 13.x) error manifests. Having said that, how about following patch which helps avoid 
including "fortify-string.h" file while compiling kernel/padata.c?

diff --git a/kernel/padata.c b/kernel/padata.c
index d899f34558af..bebf081d7f65 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -11,6 +11,7 @@
  * Author: Daniel Jordan <daniel.m.jordan@...cle.com>
  */
 
+#define __NO_FORTIFY
 #include <linux/completion.h>
 #include <linux/export.h>
 #include <linux/cpumask.h>

Later when we find the fix (maybe GCC fixes it or we fix it in fortify-string), we
may revert the above change. What do you think?

Thanks,
--Nilay

PS: I have attached my running redhat/distro config, for reference.
View attachment "config-distro.txt" of type "text/plain" (139506 bytes)

Powered by blists - more mailing lists