Re: [PATCHv3] gcc: disable '-Wstrignop-overread' universally for gcc-13+ and FORTIFY_SOURCE

[Yury Norov]

On Thu, Dec 12, 2024 at 10:47:40AM -0800, Kees Cook wrote:
> On Thu, Dec 12, 2024 at 10:24:36AM -0800, Kees Cook wrote:
> > Or we could unconditionally put the OPTIMIZER_HIDE_VAR() inside
> > bitmap_copy() itself:
> > 
> > 
> > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > index 262b6596eca5..5503ccabe05a 100644
> > --- a/include/linux/bitmap.h
> > +++ b/include/linux/bitmap.h
> > @@ -251,12 +251,14 @@ static __always_inline void bitmap_fill(unsigned long *dst, unsigned int nbits)
> >  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))
> > +	if (small_const_nbits(nbits)) {
> >  		*dst = *src;
> > -	else
> > +	} else {
> > +		unsigned int len = bitmap_size(nbits);
> > +
> > +		OPTIMIZER_HIDE_VAR(len);
> >  		memcpy(dst, src, len);
> > +	}
> >  }
> >  
> >  /*
> > 
> > I prefer any of these to doing the build-system disabling of the
> > warning.
> 
> Actually, this should probably be done in the FORTIFY macro instead --
> it's what actually tripping the GCC warning since it is the code that is
> gleefully issuing a warning and then continuing with a overflowing copy
> anyway...
> 
> 
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index 0d99bf11d260..7203acfb9f17 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -630,7 +630,13 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
>  		  __fortify_size,					\
>  		  "field \"" #p "\" at " FILE_LINE,			\
>  		  __p_size_field);					\
> -	__underlying_##op(p, q, __fortify_size);			\
> +	if (__builtin_constant_p(__fortify_size)) {			\
> +		__underlying_##op(p, q, __fortify_size);		\
> +	} else {							\
> +		size_t ___fortify_size = __fortify_size;		\
> +		OPTIMIZER_HIDE_VAR(___fortify_size);			\
> +		__underlying_##op(p, q, ___fortify_size);		\
> +	}								\
>  })

I like this more than anything else. Bitmap API is an innocent victim,
and trashing it with unrelated warning suppressors is just bad. If GCC
will get more aggressive, we'll end up with the kernel all trashed with
this OPTIMIZER_HIDE_VAR() stuff.

I tried to formulate it when discussed __always_inline story, but now I
think I can do it clear. Bitmaps is an example of very basic coding. It's
not very complicated, but it's the 2nd most popular kernel API after
spinlocks.

People literally spent hundreds hours on every single line of core APIs.
I think all that people expect that the most reviewed system code in
the world will be somewhat a reference for compilers and other tools.

If compiler can't inline a pure wrapper with no additional
functionality at all, or if modpost can't understand that inline
function declared in header can't be a 'section mismatch', or 
if CONFIG_FORTIFY complains about an overread that can never happen,
it's not a reason to pollute bitmaps, spinlocks, atomics or whatever
else, or even touch them. The tools should be fixed, not the code.

Acked-by: Yury Norov <yury.norov@xxxxxxxxx>