Re: [PATCHv2] cpumask: work around false-postive stringop-overread errors

Yury Norov
2024-12-06T20:49:19Z

On Fri, Dec 06, 2024 at 02:39:58PM +0530, Nilay Shroff wrote:
>
> Thank you Yuri for insightful comments! Please see my responses inline...
>
> On 12/5/24 21:53, Yury Norov wrote:
> > On Thu, Dec 05, 2024 at 06:04:09PM +0530, Nilay Shroff wrote:
> >> While building the powerpc code using gcc 13, 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,
> >> | ~~~~~~~~~~~~~~^~~~~~~~
> >>
> >> Apparently, above errors only manifests with GCC 13.x and config option
> >> CONFIG_FORTIFY_SOURCE. Furthermore, if I use gcc 11.x or gcc 12.x then I
> >> don't encounter above errors. Prima facie, these errors appear to be false-
> >
> > If it works for pre-GCC13, and likely for clang, you shouldn't disable it
> > for them. It should be enabled for CONFIG_FORTIFY_SOURCE=n, as well.
> >
> > Check config CC_NO_ARRAY_BOUNDS for an example of how versioned flags
> > are implemented.
> >
>
> >> positive. Brian informed me that currently some efforts are underway by
> >> GCC developers to emit more verbose information when GCC detects string
> >> overflow errors and that might help to further narrow down the root cause
> >> of this error.
> >
> > I'm 100% sure that Brian is a great person and his information is
> > absolutely correct and complete. But this is just not how we write
> > commit messages.
> >
> > Please avoid personal references, vague statements and news from
> > the future.
> >
> Sure, I would do the needful for future patches.
>
> >> So for now, silence these errors using -Wno-stringop-
> >> overread gcc option while building kernel/padata.c file until we find the
> >> root cause.
> >
> > You didn't provide any evidence that this warning is specific for padata.
> >
>
> Let me just show you the test matrix for the stringop-overread error:
>
> ARCH PowerPC:
> compiler CONFIG_FORTIFY_SOURCE -Werror=stringop-overread
> gcc 11.x Y N
> gcc 11.x N N
> gcc 12.x Y N
> gcc 12.x N N
> gcc 13.x Y Y
> gcc 13.x N N
> clang 18.x Y N
> clang 18.x N N
>
> ARCH x86_64:
> compiler CONFIG_FORTIFY_SOURCE -Werror=stringop-overread
> gcc 11.x Y N
> gcc 11.x N N
> gcc 12.x Y N
> gcc 12.x N N
> gcc 13.x Y N
> gcc 13.x N N
> clang 18.x Y N
> clang 18.x N N
>
> >From the above matrix, we could see that the sringop-overread error is only encountered
> when we use gcc 13 on PowerPC machine and the stringop-overread error is seen only when we
> enable CONFIG_FORTIFY_SOURCE. Furthermore, so far I've only encountered this error while
> compiling kernel/padata.c file.
>
> > And indeed the subject states that this is a cpumasks-related warninig.
> > Cpumask is a global subsystem. If you believe that this warning is
> > false-positive, it may show up for any other random victim. Please
> > suppress it globally.
> >
> Yes you were correct, this warning might appear for any other random victims. But as
> I mentioned earlier, so far I've only encountered it with kernel/padata.c file.
> So, if we want to reduce the blast radius then wouldn't it be sufficient to just silence
> it only while compiling kernel/padata.c file? Or do you still suggest disabling it at
> global level would be more helpful? I'm OK with either way moving forward. Please let
> me know.

You will reduce the radius significantly if you limit sringop-overread
suppression to a specific config, compiler and architecture. Silencing
random files is a gambling.