Message-ID: <2d9f4b56-3a8f-4fd7-a356-022f973da5e0@linux.ibm.com>
Date: Tue, 10 Dec 2024 13:58:00 +0530
From: Nilay Shroff <nilay@...ux.ibm.com>
To: Nathan Chancellor <nathan@...nel.org>, Yury Norov <yury.norov@...il.com>
Cc: linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        briannorris@...omium.org, kees@...nel.org, gustavoars@...nel.org,
        steffen.klassert@...unet.com, daniel.m.jordan@...cle.com,
        gjoyce@....com, linux-crypto@...r.kernel.org, linux@...ssschuh.net
Subject: Re: [PATCHv3] gcc: disable '-Wstrignop-overread' universally for
 gcc-13+ and FORTIFY_SOURCE



On 12/10/24 01:05, Nathan Chancellor wrote:
> On Sun, Dec 08, 2024 at 10:25:21AM -0800, Yury Norov wrote:
>> On Sun, Dec 08, 2024 at 09:42:28PM +0530, Nilay Shroff wrote:
>>> 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.
> 
> That can also be written as (UINT_MAX - 63) / 8, which I believe matches
> the ultimate math of bitmap_size() if nbits is UINT_MAX (but I did not
> fully verify) in bitmap_copy(). I tried building this code with the
> in-review -fdiagnostics-details option from GCC [1] but it does not
> really provide any other insight here. UINT_MAX probably comes from the
> fact that for this configuration, large_cpumask_bits is an indeterminate
> value for the compiler without link time optimization because it is an
> extern in kernel/padata.c:
> 
> | #if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
> | #define nr_cpu_ids ((unsigned int)NR_CPUS)
> | #else
> | extern unsigned int nr_cpu_ids;
> | #endif
> | ...
> | #if NR_CPUS <= BITS_PER_LONG
> |   #define small_cpumask_bits ((unsigned int)NR_CPUS)
> |   #define large_cpumask_bits ((unsigned int)NR_CPUS)
> | #elif NR_CPUS <= 4*BITS_PER_LONG
> |   #define small_cpumask_bits nr_cpu_ids
> |   #define large_cpumask_bits ((unsigned int)NR_CPUS)
> | #else
> |   #define small_cpumask_bits nr_cpu_ids
> |   #define large_cpumask_bits nr_cpu_ids
> | #endif
> 
> From what I can tell, nothing in this callchain asserts to the compiler
> that nr_cpu_ids cannot be larger than the compile time value of NR_CPUS
> (I assume there is a check for this somewhere?), so it assumes that this
> memcpy() can overflow if nr_cpu_ids is larger than NR_CPUS, which is
> where that range appears to come from. I am able to kill this warning
> with
> 
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 9278a50d514f..a1b0e213c638 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -836,6 +836,7 @@ void cpumask_shift_left(struct cpumask *dstp, const struct cpumask *srcp, int n)
>  static __always_inline
>  void cpumask_copy(struct cpumask *dstp, const struct cpumask *srcp)
>  {
> +	BUG_ON(large_cpumask_bits > NR_CPUS);
>  	bitmap_copy(cpumask_bits(dstp), cpumask_bits(srcp), large_cpumask_bits);
>  }
>  
> 
> although I am sure that is not going to be acceptable but it might give
> a hint about what could be done to deal with this.
> 
> Another option would be taking advantage of the __diag infrastructure to
> silence this warning around the bitmap_copy() in cpumask_copy(), stating
> that we know this can never overflow because of <reason>. I think that
> would be much more palpable than disabling the warning globally for the
> kernel, much like Greg said.
> 
Okay so I think you (and Greg) were suggesting instead of disabling 
-Wstringop-overread globally or tuning it off for a particular source
file, lets disable it on gcc-13+ while we invoke bitmap_copy() as shown
below: 

diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index d0ed9583743f..e61b9f3ff6a7 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -139,6 +139,18 @@
 #define __diag_GCC_8(s)
 #endif
 
+#if GCC_VERSION >= 130000
+#define __diag_GCC_13(s)       __diag(s)
+#else
+#define __diag_GCC_13(s)
+#endif
+
+#if GCC_VERSION >= 140000
+#define __diag_GCC_14(s)       __diag(s)
+#else
+#define __diag_GCC_14(s)
+#endif
+
 #define __diag_ignore_all(option, comment) \
        __diag(__diag_GCC_ignore option)
 
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 9278a50d514f..6885856e38b0 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -836,7 +836,23 @@ void cpumask_shift_left(struct cpumask *dstp, const struct cpumask *srcp, int n)
 static __always_inline
 void cpumask_copy(struct cpumask *dstp, const struct cpumask *srcp)
 {
+       /*
+        * Silence -Wstringop-overead warning generated while copying cpumask
+        * bits on gcc-13+ and CONFIG_FORTIFY_SOURCE=y. The gcc-13+ emits
+        * warning suggesting "we're trying to copy nbits which potentially
+        * exceeds NR_CPUS. Apparently, this seems false positive and might be
+        * a gcc bug as we know that large_cpumask_bits should never exceed
+        * NR_CPUS.
+        */
+       __diag_push();
+       __diag_ignore(GCC, 13, "-Wstringop-overread",
+               "Ignore string overflow warning while copying cpumask bits");
+       __diag_ignore(GCC, 14, "-Wstringop-overread",
+               "Ignore string overflow warning while copying cpumask bits");
+
        bitmap_copy(cpumask_bits(dstp), cpumask_bits(srcp), large_cpumask_bits);
+
+       __diag_pop();
 }

Does the above change look good to everyone?

Thanks,
--Nilay

Powered by blists - more mailing lists