On Sun, Dec 08, 2024 at 09:42:28PM +0530, Nilay Shroff wrote:
> While building the powerpc code using gcc 13.x, I came across following
> errors generated for kernel/padata.c file:
>
> 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,
> | ~~~~~~~~~~~~~~^~~~~~~~
>
> The above error appears to be false-positive:
> >From the distro relevant config,
> CONFIG_PPC64=y
> CONFIG_CC_IS_GCC=y
> CONFIG_GCC_VERSION=130301
> CONFIG_NR_CPUS=2048
> CONFIG_FORTIFY_SOURCE=y
>
> >From the source code,
> unsigned long name[BITS_TO_LONGS(bits)]
> ...
> typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
> typedef struct cpumask cpumask_var_t[1];
> ...
>
> extern unsigned int nr_cpu_ids;
> ...
> static __always_inline
> void bitmap_copy(unsigned long *dst, const unsigned long *src, unsigned
> int nbits)
> {
> unsigned int len = bitmap_size(nbits);
>
> if (small_const_nbits(nbits))
> *dst = *src;
> else
> memcpy(dst, src, len);
> }
>
> static __always_inline
> void cpumask_copy(struct cpumask *dstp, const struct cpumask *srcp)
> {
> bitmap_copy(cpumask_bits(dstp), cpumask_bits(srcp), large_cpumask_bits);
> }
> ...
> static int __padata_set_cpumasks(struct padata_instance *pinst,
> cpumask_var_t pcpumask,
> cpumask_var_t cbcpumask)
> {
> ...
> cpumask_copy(pinst->cpumask.pcpu, pcpumask);
> cpumask_copy(pinst->cpumask.cbcpu, cbcpumask);
> ...
> }
>
> So the above statements expands to:
> memcpy(pinst->cpumask.pcpu->bits, pcpumask->bits, nr_cpu_ids)
> memcpy(pinst->cpumask.cbcpu->bits, cbcpumask->bits, nr_cpu_ids)
>
> Now the compiler complains about "error: ‘__builtin_memcpy’ reading
> between 257 and 536870904 bytes from a region of size 256". So the
> value of nr_cpu_ids which gcc calculated is between 257 and 536870904.
> This looks strange and incorrect.
Thanks for the detour into internals. I did the same by myself, and
spent quite a lot of my time trying to understand why GCC believes
that here we're trying to access memory beyond idx == 256 and up to
a pretty random 536870904.
256 is most likely NR_CPUS/8, and that makes sense. But I have no ideas
what does this 536870904 mean. OK, it's ((u32)-64)>>3, but to me it's a
random number. I'm quite sure cpumasks machinery can't be involved in
generating it.
Well, we maybe have to spend more time on this, at least try to
understand how exactly CONFIG_FORTIFY_SOURCE is involved. But to me
it's OK to silence the warning this way. Moreover, you're pointing to
undergoing discussions.
> Later similar errors[1] were also observed on x86-64 platform using
> gcc-14. Apparently, above errors menifests using gcc-13+ and config
> option CONFIG_FORTIFY_SOURCE=y. Moreover, I don't encounter above
> error when,
> - using gcc 11.x or gcc 12.x or LLVM/Clang compiler
> or
> - disabling CONFIG_FORTIFY_SOURCE option
>
> So for now, silence above errors globally while using gcc-13+ and
> CONFIG_FORTIFY_SOURCE=y. We may later revert this change when we
> find root cause of the error. Per this discussion[2], GCC developers
> are working to add extra diagnostics and context when this error
> menifests and that might help to find the root cause.
>
> [1] https://lore.kernel.org/all/2024120757-lustrous-equinox-77f0@gregkh/
> [2] https://gcc.gnu.org/pipermail/gcc-patches/2024-October/666870.html
>
> Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Greg acked v2, which differs from v3 quite a lot. I would check with
him if he's still OK.
> Cc: briannorris@xxxxxxxxxxxx
> Cc: yury.norov@xxxxxxxxx
> Cc: kees@xxxxxxxxxx
> Cc: gustavoars@xxxxxxxxxx
> Cc: nathan@xxxxxxxxxx
> Cc: steffen.klassert@xxxxxxxxxxx
> Cc: daniel.m.jordan@xxxxxxxxxx
> Cc: gjoyce@xxxxxxx
> Cc: linux-crypto@xxxxxxxxxxxxxxx
> Cc: linux@xxxxxxxxxxxxxx
> Signed-off-by: Nilay Shroff <nilay@xxxxxxxxxxxxx>
> ---
> Changes from v2:
> - Globally disable -Werror-stringop-overread while using gcc-13+ and
> FORTIFY_SOURCE (Yury Norov)
> - Updated the patch subject line to make it clear that this is
> possiblt gcc error and not related to cpumask.
> Changes from v1:
> - Fix spell error in the commit message (Brian Norris)
> - Add commentary around change to note that changes are needed to
> avoid false positive on gcc 13+ (Brian Norris)
> - Add the kerenl/padata.c file maintainers (Brian Norris)
> ---
> init/Kconfig | 8 ++++++++
> scripts/Makefile.extrawarn | 1 +
> 2 files changed, 9 insertions(+)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index a20e6efd3f0f..ff2f54520551 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -920,6 +920,14 @@ config CC_STRINGOP_OVERFLOW
> bool
> default y if CC_IS_GCC && !CC_NO_STRINGOP_OVERFLOW
>
> +# Currently, disable -Wstringop-overread for gcc-13+ and FORTIFY_SOURCE globally.
> +config GCC13_NO_STRINGOP_OVERREAD
> + def_bool y
This is an issue for GCC14 as well, as Greg mentioned, and you say the
same in the commit message. I'd name this config GCC_NO_STRINGOP_OVERREAD.
But I don't think we need this extra knob at all. Those who want to
disable CC_NO_STRINGOP_OVERREAD can disable it explicitly.
Anyways, it's minor. For cpumasks:
Acked-by: Yury Norov <yury.norov@xxxxxxxxx>
> +
> +config CC_NO_STRINGOP_OVERREAD
> + bool
> + default y if CC_IS_GCC && GCC_VERSION >= 130000 && GCC13_NO_STRINGOP_OVERREAD && FORTIFY_SOURCE
> +
> #
> # For architectures that know their GCC __int128 support is sound
> #
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index 1d13cecc7cc7..1abd41269fd0 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -27,6 +27,7 @@ endif
> KBUILD_CPPFLAGS-$(CONFIG_WERROR) += -Werror
> KBUILD_CPPFLAGS += $(KBUILD_CPPFLAGS-y)
> KBUILD_CFLAGS-$(CONFIG_CC_NO_ARRAY_BOUNDS) += -Wno-array-bounds
> +KBUILD_CFLAGS-$(CONFIG_CC_NO_STRINGOP_OVERREAD) += -Wno-stringop-overread
>
> ifdef CONFIG_CC_IS_CLANG
> # The kernel builds with '-std=gnu11' so use of GNU extensions is acceptable.
> --
> 2.45.2
