[PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12

[PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12

[Christophe de Dinechin]

Compiling with GCC 12 using defconfig generates a number of build errors
due to new warnings, notably array-bounds checks. Some of these warnings appear
legitimate and relatively easy to fix.

Note that this series is not sufficient for a clean build yet. There are
in particular a number of warnings reported by the array-bounds check
that appear bogus, like:

| In function ‘__native_read_cr3’,
|     inlined from ‘__read_cr3’
|         at ./arch/x86/include/asm/special_insns.h:169:9,
|     inlined from ‘read_cr3_pa’
|         at ./arch/x86/include/asm/processor.h:252:9,
|     inlined from ‘relocate_restore_code’
|         at arch/x86/power/hibernate.c:165:17:
| ./arch/x86/include/asm/special_insns.h:48:9: error:
|    array subscript 0 is outside array bounds of ‘unsigned int[0]’
|    [-Werror=array-bounds]
|    48 | asm volatile("mov %%cr3,%0\n\t" : "=r" (val) : __FORCE_ORDER);
|       | ^~~
| cc1: all warnings being treated as errors

The error above is for an instruction that does not obviously address any
C array, in particular since the asm constraint is "=r" and not "=rm".

Consequently, the series here only addresses a few low hanging fruits that
appear legitimate and relatively easy to fix.

Christophe de Dinechin (3):
  sched/headers: Fix compilation error with GCC 12
  nodemask.h: Fix compilation error with GCC12
  virtio-pci: Use cpumask_available to fix compilation error

 drivers/virtio/virtio_pci_common.c |  2 +-
 include/linux/nodemask.h           | 13 ++++++-------
 kernel/sched/sched.h               | 11 +++++++++--
 3 files changed, 16 insertions(+), 10 deletions(-)

-- 
2.35.1

[PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Christophe de Dinechin]

With gcc version 12.0.1 20220401 (Red Hat 12.0.1-0) (GCC), the following
errors are reported in sched.h when building after `make defconfig`:

|   CC      kernel/sched/core.o
| In file included from kernel/sched/core.c:81:
| kernel/sched/core.c: In function ‘set_rq_online.part.0’:
| kernel/sched/sched.h:2197:52: error: array subscript -1 is outside
|  array bounds of ‘struct sched_class[44343134792571037]’
|  [-Werror=array-bounds]
|  2197 | #define sched_class_lowest  (__begin_sched_classes - 1)
|       |                                                    ^
| kernel/sched/sched.h:2200:41: note: in definition of macro
|  ‘for_class_range’
|  2200 |         for (class = (_from); class != (_to); class--)
|       |                                         ^~~
| kernel/sched/sched.h:2203:53: note: in expansion of macro
|  ‘sched_class_lowest’
|  2203 |for_class_range(class, sched_class_highest, sched_class_lowest)
|       |                                            ^~~~~~~~~~~~~~~~~~
| kernel/sched/core.c:9115:17: note: in expansion of macro
|  ‘for_each_class’
|  9115 |                 for_each_class(class) {
|       |                 ^~~~~~~~~~~~~~
| kernel/sched/sched.h:2193:27: note: at offset -208 into object
|   ‘__begin_sched_classes’ of size [0, 9223372036854775807]
|  2193 | extern struct sched_class __begin_sched_classes[];
|       |                           ^~~~~~~~~~~~~~~~~~~~~
| kernel/sched/core.c: In function ‘set_rq_offline.part.0’:
| kernel/sched/sched.h:2197:52: error: array subscript -1 is outside
|   array bounds of ‘struct sched_class[44343134792571037]’
|   [-Werror=array-bounds]
|  2197 | #define sched_class_lowest  (__begin_sched_classes - 1)
|       |                                                    ^
| kernel/sched/sched.h:2200:41: note: in definition of macro
|  ‘for_class_range’
|  2200 |         for (class = (_from); class != (_to); class--)
|       |                                         ^~~
| kernel/sched/sched.h:2203:53: note: in expansion of macro
|  ‘sched_class_lowest’
|  2203 |for_class_range(class, sched_class_highest, sched_class_lowest)
|       |                                            ^~~~~~~~~~~~~~~~~~
| kernel/sched/core.c:9127:17: note: in expansion of macro
|  ‘for_each_class’
|  9127 |                 for_each_class(class) {
|       |                 ^~~~~~~~~~~~~~
| kernel/sched/sched.h:2193:27: note: at offset -208 into object
|  ‘__begin_sched_classes’ of size [0, 9223372036854775807]
|  2193 | extern struct sched_class __begin_sched_classes[];
|       |                           ^~~~~~~~~~~~~~~~~~~~~
| In function ‘__pick_next_task’,
|     inlined from ‘pick_next_task’ at kernel/sched/core.c:6204:9,
|     inlined from ‘__schedule’ at kernel/sched/core.c:6352:9:
| kernel/sched/sched.h:2197:52: error: array subscript -1 is outside
|  array bounds of ‘struct sched_class[44343134792571037]’
|  [-Werror=array-bounds]
|  2197 | #define sched_class_lowest  (__begin_sched_classes - 1)
|       |                                                    ^
| kernel/sched/sched.h:2200:41: note: in definition of macro
|  ‘for_class_range’
|  2200 |         for (class = (_from); class != (_to); class--)
|       |                                         ^~~
| kernel/sched/sched.h:2203:53: note: in expansion of macro
|  ‘sched_class_lowest’
|  2203 |for_class_range(class, sched_class_highest, sched_class_lowest)
|       |                                            ^~~~~~~~~~~~~~~~~~
| kernel/sched/core.c:5711:9: note: in expansion of macro
|  ‘for_each_class’
|  5711 |         for_each_class(class) {
|       |         ^~~~~~~~~~~~~~
| kernel/sched/sched.h: In function ‘__schedule’:
| kernel/sched/sched.h:2193:27: note: at offset -208 into object
|  ‘__begin_sched_classes’ of size [0, 9223372036854775807]
|  2193 | extern struct sched_class __begin_sched_classes[];
|       |                           ^~~~~~~~~~~~~~~~~~~~~
| cc1: all warnings being treated as errors

Rewrite the definitions of sched_class_highest and for_class_range to
avoid this error as follows:

1/ The sched_class_highest is rewritten to be relative to
  __begin_sched_classes, so that GCC sees it as being part of the
  __begin_sched_classes array and not a distinct __end_sched_classes
  array.

2/ The for_class_range macro is modified to replace the comparison with
  an out-of-bound pointer __begin_sched_classes - 1 with an equivalent,
  but in-bounds comparison.

In that specific case, I believe that the GCC analysis is correct and
potentially valuable for other arrays, so it makes sense to keep it
enabled.

Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 kernel/sched/sched.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 8dccb34eb190..6350fbc7418d 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2193,11 +2193,18 @@ const struct sched_class name##_sched_class \
 extern struct sched_class __begin_sched_classes[];
 extern struct sched_class __end_sched_classes[];
 
-#define sched_class_highest (__end_sched_classes - 1)
+/*
+ * sched_class_highests is really __end_sched_classes - 1, but written in a way
+ * that makes it clear that it is within __begin_sched_classes[] and not outside
+ * of __end_sched_classes[].
+ */
+#define sched_class_highest (__begin_sched_classes + \
+			     (__end_sched_classes - __begin_sched_classes - 1))
 #define sched_class_lowest  (__begin_sched_classes - 1)
 
+/* The + 1 below places the pointers within the range of their array */
 #define for_class_range(class, _from, _to) \
-	for (class = (_from); class != (_to); class--)
+	for (class = (_from); class + 1 != (_to) + 1; class--)
 
 #define for_each_class(class) \
 	for_class_range(class, sched_class_highest, sched_class_lowest)
-- 
2.35.1

[PATCH 2/3] nodemask.h: Fix compilation error with GCC12

[Christophe de Dinechin]

With gcc version 12.0.1 20220401 (Red Hat 12.0.1-0), building with
defconfig results in the following compilation error:

|   CC      mm/swapfile.o
| mm/swapfile.c: In function ‘setup_swap_info’:
| mm/swapfile.c:2291:47: error: array subscript -1 is below array bounds
|  of ‘struct plist_node[]’ [-Werror=array-bounds]
|  2291 |                                 p->avail_lists[i].prio = 1;
|       |                                 ~~~~~~~~~~~~~~^~~
| In file included from mm/swapfile.c:16:
| ./include/linux/swap.h:292:27: note: while referencing ‘avail_lists’
|   292 |         struct plist_node avail_lists[]; /*
|       |                           ^~~~~~~~~~~

This is due to the compiler detecting that the mask in
node_states[__state] could theoretically be zero, which would lead to
first_node() returning -1 through find_first_bit.

I believe that the warning/error is legitimate. I first tried
adding a test to check that the node mask is not emtpy, since a
similar test exists in the case where MAX_NUMNODES == 1.

However, adding the if statement causes other warnings to appear in
for_each_cpu_node_but, because it introduces a dangling else
ambiguity. And unfortunately, GCC is not smart enough to detect that the
added test makes the case where (node) == -1 impossible, so it still
complains with the same message.

This is why I settled on replacing that with a harmless, but relatively
useless (node) >= 0 test. Based on the warning for the dangling else, I
also decided to fix the case where MAX_NUMNODES == 1 by moving the
condition inside the for loop. It will still only be tested once. This
ensures that the meaning of an else following for_each_node_mask
or derivatives would not silently have a different meaning depending on
the configuration.

Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 include/linux/nodemask.h | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h
index 567c3ddba2c4..c6199dbe2591 100644
--- a/include/linux/nodemask.h
+++ b/include/linux/nodemask.h
@@ -375,14 +375,13 @@ static inline void __nodes_fold(nodemask_t *dstp, const nodemask_t *origp,
 }
 
 #if MAX_NUMNODES > 1
-#define for_each_node_mask(node, mask)			\
-	for ((node) = first_node(mask);			\
-		(node) < MAX_NUMNODES;			\
-		(node) = next_node((node), (mask)))
+#define for_each_node_mask(node, mask)				    \
+	for ((node) = first_node(mask);				    \
+	     (node >= 0) && (node) < MAX_NUMNODES;		    \
+	     (node) = next_node((node), (mask)))
 #else /* MAX_NUMNODES == 1 */
-#define for_each_node_mask(node, mask)			\
-	if (!nodes_empty(mask))				\
-		for ((node) = 0; (node) < 1; (node)++)
+#define for_each_node_mask(node, mask)                                  \
+	for ((node) = 0; (node) < 1 && !nodes_empty(mask); (node)++)
 #endif /* MAX_NUMNODES */
 
 /*
-- 
2.35.1

[PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error

[Christophe de Dinechin]

With GCC 12 and defconfig, we get the following error:

|   CC      drivers/virtio/virtio_pci_common.o
| drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
| drivers/virtio/virtio_pci_common.c:257:29: error: the comparison will
|  always evaluate as ‘true’ for the pointer operand in
|  ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 8)’
|  must not be NULL [-Werror=address]
|   257 |                         if (vp_dev->msix_affinity_masks[i])
|       |                             ^~~~~~

This happens in the case where CONFIG_CPUMASK_OFFSTACK is not defined,
since we typedef cpumask_var_t as an array. The compiler is essentially
complaining that an array pointer cannot be NULL. This is not a very
important warning, but there is a function called cpumask_available that
seems to be defined just for that case, so the fix is easy.

Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 drivers/virtio/virtio_pci_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index d724f676608b..5c44a2f13c93 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -254,7 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
 
 	if (vp_dev->msix_affinity_masks) {
 		for (i = 0; i < vp_dev->msix_vectors; i++)
-			if (vp_dev->msix_affinity_masks[i])
+			if (cpumask_available(vp_dev->msix_affinity_masks[i]))
 				free_cpumask_var(vp_dev->msix_affinity_masks[i]);
 	}
 
-- 
2.35.1

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Peter Zijlstra]

On Thu, Apr 14, 2022 at 05:08:53PM +0200, Christophe de Dinechin wrote:
> With gcc version 12.0.1 20220401 (Red Hat 12.0.1-0) (GCC), the following
> errors are reported in sched.h when building after `make defconfig`:

<snip tons of noise>

> Rewrite the definitions of sched_class_highest and for_class_range to
> avoid this error as follows:
> 
> 1/ The sched_class_highest is rewritten to be relative to
>   __begin_sched_classes, so that GCC sees it as being part of the
>   __begin_sched_classes array and not a distinct __end_sched_classes
>   array.
> 
> 2/ The for_class_range macro is modified to replace the comparison with
>   an out-of-bound pointer __begin_sched_classes - 1 with an equivalent,
>   but in-bounds comparison.
> 
> In that specific case, I believe that the GCC analysis is correct and
> potentially valuable for other arrays, so it makes sense to keep it
> enabled.
> 
> Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  kernel/sched/sched.h | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 8dccb34eb190..6350fbc7418d 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -2193,11 +2193,18 @@ const struct sched_class name##_sched_class \
>  extern struct sched_class __begin_sched_classes[];
>  extern struct sched_class __end_sched_classes[];
>  
> -#define sched_class_highest (__end_sched_classes - 1)
> +/*
> + * sched_class_highests is really __end_sched_classes - 1, but written in a way
> + * that makes it clear that it is within __begin_sched_classes[] and not outside
> + * of __end_sched_classes[].
> + */
> +#define sched_class_highest (__begin_sched_classes + \
> +			     (__end_sched_classes - __begin_sched_classes - 1))
>  #define sched_class_lowest  (__begin_sched_classes - 1)
>  
> +/* The + 1 below places the pointers within the range of their array */
>  #define for_class_range(class, _from, _to) \
> -	for (class = (_from); class != (_to); class--)
> +	for (class = (_from); class + 1 != (_to) + 1; class--)

Urgh, so now we get less readable code, just because GCC is being
stupid?

What's wrong with negative array indexes? memory is memory, stuff works.

Re: [PATCH 2/3] nodemask.h: Fix compilation error with GCC12

[Peter Zijlstra]

On Thu, Apr 14, 2022 at 05:08:54PM +0200, Christophe de Dinechin wrote:

> diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h
> index 567c3ddba2c4..c6199dbe2591 100644
> --- a/include/linux/nodemask.h
> +++ b/include/linux/nodemask.h
> @@ -375,14 +375,13 @@ static inline void __nodes_fold(nodemask_t *dstp, const nodemask_t *origp,
>  }
>  
>  #if MAX_NUMNODES > 1
> -#define for_each_node_mask(node, mask)			\
> -	for ((node) = first_node(mask);			\
> -		(node) < MAX_NUMNODES;			\
> -		(node) = next_node((node), (mask)))
> +#define for_each_node_mask(node, mask)				    \
> +	for ((node) = first_node(mask);				    \
> +	     (node >= 0) && (node) < MAX_NUMNODES;		    \
> +	     (node) = next_node((node), (mask)))
>  #else /* MAX_NUMNODES == 1 */
> -#define for_each_node_mask(node, mask)			\
> -	if (!nodes_empty(mask))				\
> -		for ((node) = 0; (node) < 1; (node)++)
> +#define for_each_node_mask(node, mask)                                  \
> +	for ((node) = 0; (node) < 1 && !nodes_empty(mask); (node)++)
>  #endif /* MAX_NUMNODES */

Again, less readable code :/ And why?

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Andrew Morton]

On Thu, 14 Apr 2022 17:21:01 +0200 Peter Zijlstra <peterz@infradead.org> wrote:

> > +/* The + 1 below places the pointers within the range of their array */
> >  #define for_class_range(class, _from, _to) \
> > -	for (class = (_from); class != (_to); class--)
> > +	for (class = (_from); class + 1 != (_to) + 1; class--)
> 
> Urgh, so now we get less readable code, just because GCC is being
> stupid?
> 
> What's wrong with negative array indexes? memory is memory, stuff works.

What's more, C is C.  Glorified assembly language in which people do odd
stuff.

But this is presumably a released gcc version and we need to do
something.  And presumably, we need to do a backportable something, so
people can compile older kernels with gcc-12.

Is it possible to suppress just this warning with a gcc option?  And if
so, are we confident that this warning will never be useful in other
places in the kernel?

If no||no then we'll need to add workarounds such as these?

Re: [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error

[Michael S. Tsirkin]

On Thu, Apr 14, 2022 at 05:08:55PM +0200, Christophe de Dinechin wrote:
> With GCC 12 and defconfig, we get the following error:
> 
> |   CC      drivers/virtio/virtio_pci_common.o
> | drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
> | drivers/virtio/virtio_pci_common.c:257:29: error: the comparison will
> |  always evaluate as ‘true’ for the pointer operand in
> |  ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 8)’
> |  must not be NULL [-Werror=address]
> |   257 |                         if (vp_dev->msix_affinity_masks[i])
> |       |                             ^~~~~~
> 
> This happens in the case where CONFIG_CPUMASK_OFFSTACK is not defined,
> since we typedef cpumask_var_t as an array. The compiler is essentially
> complaining that an array pointer cannot be NULL. This is not a very
> important warning, but there is a function called cpumask_available that
> seems to be defined just for that case, so the fix is easy.
> 
> Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

There was an alternate patch proposed for this by
Murilo Opsfelder Araujo. What do you think about that approach?


> ---
>  drivers/virtio/virtio_pci_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index d724f676608b..5c44a2f13c93 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -254,7 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
>  
>  	if (vp_dev->msix_affinity_masks) {
>  		for (i = 0; i < vp_dev->msix_vectors; i++)
> -			if (vp_dev->msix_affinity_masks[i])
> +			if (cpumask_available(vp_dev->msix_affinity_masks[i]))
>  				free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>  	}
>  
> -- 
> 2.35.1

RE: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[David Laight]

From: Peter Zijlstra
> Sent: 14 April 2022 16:21
...
> <snip tons of noise>
> 
..
> > -#define sched_class_highest (__end_sched_classes - 1)
> > +/*
> > + * sched_class_highests is really __end_sched_classes - 1, but written in a way
> > + * that makes it clear that it is within __begin_sched_classes[] and not outside
> > + * of __end_sched_classes[].
> > + */
> > +#define sched_class_highest (__begin_sched_classes + \
> > +			     (__end_sched_classes - __begin_sched_classes - 1))
> >  #define sched_class_lowest  (__begin_sched_classes - 1)
> >
> > +/* The + 1 below places the pointers within the range of their array */
> >  #define for_class_range(class, _from, _to) \
> > -	for (class = (_from); class != (_to); class--)
> > +	for (class = (_from); class + 1 != (_to) + 1; class--)

That is still technically broken because you are still calculating
the address of array[-1] - even though it is probably optimised out.

> Urgh, so now we get less readable code, just because GCC is being
> stupid?
> 
> What's wrong with negative array indexes? memory is memory, stuff works.

Consider segmented x86 where malloc() always returns {segment:0..segment:size).
Pointer arithmetic will only change the offset.
So &array[-1] is likely to be greater than &array[0].
So it has never been valid C to create pointers to before a data item.

OTOH I've NFI why gcc and clang have started generating warnings for
portability issues that really don't affect mainstream systems.

I'm just waiting for them to warn about memset(p, 0 sizeof *p) when
the structure contains pointers - because the NULL pointer doesn't
have to be the all-zero bit pattern.
The only reason (int)&(struct foo *)0->member is non-portable is because
NULL might not be 0.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Peter Zijlstra]

On Thu, Apr 14, 2022 at 01:30:50PM -0700, Andrew Morton wrote:
> On Thu, 14 Apr 2022 17:21:01 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > +/* The + 1 below places the pointers within the range of their array */
> > >  #define for_class_range(class, _from, _to) \
> > > -	for (class = (_from); class != (_to); class--)
> > > +	for (class = (_from); class + 1 != (_to) + 1; class--)
> > 
> > Urgh, so now we get less readable code, just because GCC is being
> > stupid?
> > 
> > What's wrong with negative array indexes? memory is memory, stuff works.
> 
> What's more, C is C.  Glorified assembly language in which people do odd
> stuff.
> 
> But this is presumably a released gcc version and we need to do
> something.  And presumably, we need to do a backportable something, so
> people can compile older kernels with gcc-12.
> 
> Is it possible to suppress just this warning with a gcc option?  And if
> so, are we confident that this warning will never be useful in other
> places in the kernel?
> 
> If no||no then we'll need to add workarounds such as these?

-Wno-array-bounds

Is the obvious fix-all cure. The thing is, I want to hear if this new
warning has any actual use or is just crack induced madness like many of
the warnings we turn off.

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Kees Cook]

On Sun, Apr 17, 2022 at 05:52:05PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 14, 2022 at 01:30:50PM -0700, Andrew Morton wrote:
> > On Thu, 14 Apr 2022 17:21:01 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > > +/* The + 1 below places the pointers within the range of their array */
> > > >  #define for_class_range(class, _from, _to) \
> > > > -	for (class = (_from); class != (_to); class--)
> > > > +	for (class = (_from); class + 1 != (_to) + 1; class--)
> > > 
> > > Urgh, so now we get less readable code, just because GCC is being
> > > stupid?
> > > 
> > > What's wrong with negative array indexes? memory is memory, stuff works.
> > 
> > What's more, C is C.  Glorified assembly language in which people do odd
> > stuff.
> > 
> > But this is presumably a released gcc version and we need to do
> > something.  And presumably, we need to do a backportable something, so
> > people can compile older kernels with gcc-12.
> > 
> > Is it possible to suppress just this warning with a gcc option?  And if
> > so, are we confident that this warning will never be useful in other
> > places in the kernel?
> > 
> > If no||no then we'll need to add workarounds such as these?
> 
> -Wno-array-bounds

Please no; we just spent two years fixing all the old non-flexible array
definitions and so many other things fixed for this to be enable because
it finds actual flaws (but we turned it off when it was introduced
because of how much sloppy old code we had).

> Is the obvious fix-all cure. The thing is, I want to hear if this new
> warning has any actual use or is just crack induced madness like many of
> the warnings we turn off.

Yes, it finds real flaws. And also yes, it is rather opinionated about
some "tricks" that have worked in C, but frankly, most of those tricks
end up being weird/accidentally-correct and aren't great for long-term
readability or robustness. Though I'm not speaking specifically to this
proposed patch; I haven't looked closely at it yet.

I quickly went back through commits; here's a handful I found:

20314bacd2f9 ("staging: r8188eu: Fix PPPoE tag insertion on little endian systems")
dcf500065fab ("net: bnxt_ptp: fix compilation error")
5f7dc7d48c94 ("octeontx2-af: fix array bound error")
c7d58971dbea ("ALSA: mixart: Reduce size of mixart_timer_notify")
b3f1dd52c991 ("ARM: vexpress/spc: Avoid negative array index when !SMP")
a2151490cc6c ("drm/dp: Fix OOB read when handling Post Cursor2 register")
d4da1f27396f ("drm/dp: Fix off-by-one in register cache size")
47307c31d90a ("crypto: octeontx2 - Avoid stack variable overflow")
a6501e4b380f ("eeprom: at25: Restore missing allocation")
33ce7f2f95ca ("drm/imx: imx-ldb: fix out of bounds array access warning")
f051ae4f6c73 ("Input: cyapa_gen6 - fix out-of-bounds stack access")
f3217d6f2f7a ("firmware: xilinx: fix out-of-bounds access")
8a03447dd311 ("rtw88: fix subscript above array bounds compiler warning")
ad82a928eb58 ("s390/perf: fix gcc 8 array-bounds warning")
6038aa532a22 ("nvme: target: fix buffer overflow")
50a0d71a5d20 ("cros_ec: fix nul-termination for firmware build info")
43d15c201313 ("staging: rtl8822be: Keep array subscript no lower than zero")

The important part is that with this enabled now, we won't get _new_
problems introduced. Making the C code clear enough that the compiler
can understand the intent, though, can be a little annoying, but makes
things much easier to automatically check. Getting our code-base arranged
so the toolchain can actually do the analysis is well worth it.

-- 
Kees Cook

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Peter Zijlstra]

On Wed, Apr 20, 2022 at 11:45:05AM -0700, Kees Cook wrote:

> > -Wno-array-bounds
> 
> Please no; we just spent two years fixing all the old non-flexible array
> definitions and so many other things fixed for this to be enable because
> it finds actual flaws (but we turned it off when it was introduced
> because of how much sloppy old code we had).
> 
> > Is the obvious fix-all cure. The thing is, I want to hear if this new
> > warning has any actual use or is just crack induced madness like many of
> > the warnings we turn off.
> 
> Yes, it finds real flaws. And also yes, it is rather opinionated about
> some "tricks" that have worked in C, but frankly, most of those tricks
> end up being weird/accidentally-correct and aren't great for long-term
> readability or robustness. Though I'm not speaking specifically to this
> proposed patch; I haven't looked closely at it yet.

So the whole access outside object is UB thing in C is complete rubbish
from an OS perspective. The memory is there and there are geniune uses
for it.

And so far, the patches I've seen for it make the code actively worse.
So we need a sane annotation to tell the compiler to shut up already
without making the code an unreadable mess.

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Christophe de Dinechin]

> On 14 Apr 2022, at 17:21, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Thu, Apr 14, 2022 at 05:08:53PM +0200, Christophe de Dinechin wrote:
>> With gcc version 12.0.1 20220401 (Red Hat 12.0.1-0) (GCC), the following
>> errors are reported in sched.h when building after `make defconfig`:
> 
> <snip tons of noise>

I don’t mind removing the detailed error message.
What do others think?

> 
>> Rewrite the definitions of sched_class_highest and for_class_range to
>> avoid this error as follows:
>> 
>> 1/ The sched_class_highest is rewritten to be relative to
>>  __begin_sched_classes, so that GCC sees it as being part of the
>>  __begin_sched_classes array and not a distinct __end_sched_classes
>>  array.
>> 
>> 2/ The for_class_range macro is modified to replace the comparison with
>>  an out-of-bound pointer __begin_sched_classes - 1 with an equivalent,
>>  but in-bounds comparison.
>> 
>> In that specific case, I believe that the GCC analysis is correct and
>> potentially valuable for other arrays, so it makes sense to keep it
>> enabled.
>> 
>> Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> kernel/sched/sched.h | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>> 
>> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
>> index 8dccb34eb190..6350fbc7418d 100644
>> --- a/kernel/sched/sched.h
>> +++ b/kernel/sched/sched.h
>> @@ -2193,11 +2193,18 @@ const struct sched_class name##_sched_class \
>> extern struct sched_class __begin_sched_classes[];
>> extern struct sched_class __end_sched_classes[];
>> 
>> -#define sched_class_highest (__end_sched_classes - 1)
>> +/*
>> + * sched_class_highests is really __end_sched_classes - 1, but written in a way
>> + * that makes it clear that it is within __begin_sched_classes[] and not outside
>> + * of __end_sched_classes[].
>> + */
>> +#define sched_class_highest (__begin_sched_classes + \
>> +			     (__end_sched_classes - __begin_sched_classes - 1))
>> #define sched_class_lowest  (__begin_sched_classes - 1)
>> 
>> +/* The + 1 below places the pointers within the range of their array */
>> #define for_class_range(class, _from, _to) \
>> -	for (class = (_from); class != (_to); class--)
>> +	for (class = (_from); class + 1 != (_to) + 1; class--)
> 
> Urgh, so now we get less readable code,

You consider the original code readable? It actually relies on a
precise layout that is not enforced in this code, not even documented,
but actually enforced by the linker script.

> just because GCC is being
> stupid?

I think that GCC is actually remarkably smart there. It tells you
that you are building pointers to A[] from B[], when there is a legit
way to say that the pointer is in A[] (which is what my patch does)

> What's wrong with negative array indexes? memory is memory, stuff works.

What’s wrong is that the compiler cannot prove theorems anymore.
These theorems are used to optimise code. When you write -1[B], the
compiler cannot optimise based on knowing this refers to A[B-A-1].

While at first, you might think that disabling a warning is a win, what comes next
is the compiler optimizing in a way you did not anticipate, mysterious bugs showing up,
and/or having to turn off some potentially useful optimisation.

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Christophe de Dinechin]

> On 14 Apr 2022, at 22:30, Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> On Thu, 14 Apr 2022 17:21:01 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
> 
>>> +/* The + 1 below places the pointers within the range of their array */
>>> #define for_class_range(class, _from, _to) \
>>> -	for (class = (_from); class != (_to); class--)
>>> +	for (class = (_from); class + 1 != (_to) + 1; class--)
>> 
>> Urgh, so now we get less readable code, just because GCC is being
>> stupid?
>> 
>> What's wrong with negative array indexes? memory is memory, stuff works.
> 
> What's more, C is C.  Glorified assembly language in which people do odd
> stuff.

Notably since the advent of clang, we moved a bit beyond glorified assembly language.
There is no 1 on 1 correspondence between what you write and the generated
assembly anymore, by a long shot. I’m sure you know that ;-), but that’s an
opportunity to plug Jason Turner’s conference on writing a C64 pong game using C++17
(https://www.youtube.com/watch?v=zBkNBP00wJE). That demonstrates,
in a funny way, just how far compilers go these days to massage your code.

> 
> But this is presumably a released gcc version and we need to do
> something.  And presumably, we need to do a backportable something, so
> people can compile older kernels with gcc-12.

Hmm, I must admit I had not considered the backporting implications.

> 
> Is it possible to suppress just this warning with a gcc option? And if
> so, are we confident that this warning will never be useful in other
> places in the kernel?

I would advise against it, and not just because of warnings.

With GCC’s ability to track pointers to individual C objects, you can expect
that they will soon start optimising based on that collected knowledge.

An example of useful optimisation based on that knowledge is to
avoid memory reloads, The idea is that a write in array B[] does
not force you to reload all data you already fetched from array A[].
But that requires the compiler to prove that pointers to A[] stay in A[]
and that you don’t purposely build negative indexes from B[] or
anything weird like that.


> If no||no then we'll need to add workarounds such as these?

It is definitely possible to silence that warning. I would still recommend
adding this kind of changes, which I would personally describe more as
“accurate description intended of memory accesses” rather than “workarounds”.
To me, it’s on the same level as putting memory fences, for example.

> 

Re: [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error

[Christophe Marie Francois Dupont de Dinechin]

> On 15 Apr 2022, at 10:48, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Thu, Apr 14, 2022 at 05:08:55PM +0200, Christophe de Dinechin wrote:
>> With GCC 12 and defconfig, we get the following error:
>> 
>> |   CC      drivers/virtio/virtio_pci_common.o
>> | drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
>> | drivers/virtio/virtio_pci_common.c:257:29: error: the comparison will
>> |  always evaluate as ‘true’ for the pointer operand in
>> |  ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 8)’
>> |  must not be NULL [-Werror=address]
>> |   257 |                         if (vp_dev->msix_affinity_masks[i])
>> |       |                             ^~~~~~
>> 
>> This happens in the case where CONFIG_CPUMASK_OFFSTACK is not defined,
>> since we typedef cpumask_var_t as an array. The compiler is essentially
>> complaining that an array pointer cannot be NULL. This is not a very
>> important warning, but there is a function called cpumask_available that
>> seems to be defined just for that case, so the fix is easy.
>> 
>> Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> 
> There was an alternate patch proposed for this by
> Murilo Opsfelder Araujo. What do you think about that approach?

I responded on the other thread, but let me share the response here:

[to muriloo@linux.ibm.com]
Apologies for the delay in responding, broken laptop…

In the case where CONFIG_CPUMASK_OFFSTACK is not defined, we have:

	typedef struct cpumask cpumask_var_t[1];

So that vp_dev->msix_affinity_masks[i] is statically not null (that’s the warning)
but also a static pointer, so not kfree-safe IMO.

> 
> 
>> ---
>> drivers/virtio/virtio_pci_common.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>> index d724f676608b..5c44a2f13c93 100644
>> --- a/drivers/virtio/virtio_pci_common.c
>> +++ b/drivers/virtio/virtio_pci_common.c
>> @@ -254,7 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
>> 
>> 	if (vp_dev->msix_affinity_masks) {
>> 		for (i = 0; i < vp_dev->msix_vectors; i++)
>> -			if (vp_dev->msix_affinity_masks[i])
>> +			if (cpumask_available(vp_dev->msix_affinity_masks[i]))
>> 				free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>> 	}
>> 
>> -- 
>> 2.35.1
> 

Re: [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error

[Michael S. Tsirkin]

On Thu, Apr 28, 2022 at 11:48:01AM +0200, Christophe Marie Francois Dupont de Dinechin wrote:
> 
> 
> > On 15 Apr 2022, at 10:48, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Thu, Apr 14, 2022 at 05:08:55PM +0200, Christophe de Dinechin wrote:
> >> With GCC 12 and defconfig, we get the following error:
> >> 
> >> |   CC      drivers/virtio/virtio_pci_common.o
> >> | drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
> >> | drivers/virtio/virtio_pci_common.c:257:29: error: the comparison will
> >> |  always evaluate as ‘true’ for the pointer operand in
> >> |  ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 8)’
> >> |  must not be NULL [-Werror=address]
> >> |   257 |                         if (vp_dev->msix_affinity_masks[i])
> >> |       |                             ^~~~~~
> >> 
> >> This happens in the case where CONFIG_CPUMASK_OFFSTACK is not defined,
> >> since we typedef cpumask_var_t as an array. The compiler is essentially
> >> complaining that an array pointer cannot be NULL. This is not a very
> >> important warning, but there is a function called cpumask_available that
> >> seems to be defined just for that case, so the fix is easy.
> >> 
> >> Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
> >> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > 
> > There was an alternate patch proposed for this by
> > Murilo Opsfelder Araujo. What do you think about that approach?
> 
> I responded on the other thread, but let me share the response here:
> 
> [to muriloo@linux.ibm.com]
> Apologies for the delay in responding, broken laptop…
> 
> In the case where CONFIG_CPUMASK_OFFSTACK is not defined, we have:
> 
> 	typedef struct cpumask cpumask_var_t[1];
> 
> So that vp_dev->msix_affinity_masks[i] is statically not null (that’s the warning)
> but also a static pointer, so not kfree-safe IMO.


Not sure I understand what you are saying here.

> > 
> > 
> >> ---
> >> drivers/virtio/virtio_pci_common.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> >> index d724f676608b..5c44a2f13c93 100644
> >> --- a/drivers/virtio/virtio_pci_common.c
> >> +++ b/drivers/virtio/virtio_pci_common.c
> >> @@ -254,7 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
> >> 
> >> 	if (vp_dev->msix_affinity_masks) {
> >> 		for (i = 0; i < vp_dev->msix_vectors; i++)
> >> -			if (vp_dev->msix_affinity_masks[i])
> >> +			if (cpumask_available(vp_dev->msix_affinity_masks[i]))
> >> 				free_cpumask_var(vp_dev->msix_affinity_masks[i]);
> >> 	}
> >> 
> >> -- 
> >> 2.35.1
> > 

Re: [PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12

[Davidlohr Bueso]

Hello - What is the status of this? Currently gcc 12 (tumbleweed) is unable to
build Linus' latest because of splats in the scheduler headers...

Thanks,
Davidlohr

On Thu, 14 Apr 2022, Christophe de Dinechin wrote:

>Compiling with GCC 12 using defconfig generates a number of build errors
>due to new warnings, notably array-bounds checks. Some of these warnings appear
>legitimate and relatively easy to fix.
>
>Note that this series is not sufficient for a clean build yet. There are
>in particular a number of warnings reported by the array-bounds check
>that appear bogus, like:
>
>| In function ???__native_read_cr3???,
>|     inlined from ???__read_cr3???
>|         at ./arch/x86/include/asm/special_insns.h:169:9,
>|     inlined from ???read_cr3_pa???
>|         at ./arch/x86/include/asm/processor.h:252:9,
>|     inlined from ???relocate_restore_code???
>|         at arch/x86/power/hibernate.c:165:17:
>| ./arch/x86/include/asm/special_insns.h:48:9: error:
>|    array subscript 0 is outside array bounds of ???unsigned int[0]???
>|    [-Werror=array-bounds]
>|    48 | asm volatile("mov %%cr3,%0\n\t" : "=r" (val) : __FORCE_ORDER);
>|       | ^~~
>| cc1: all warnings being treated as errors
>
>The error above is for an instruction that does not obviously address any
>C array, in particular since the asm constraint is "=r" and not "=rm".
>
>Consequently, the series here only addresses a few low hanging fruits that
>appear legitimate and relatively easy to fix.
>
>Christophe de Dinechin (3):
>  sched/headers: Fix compilation error with GCC 12
>  nodemask.h: Fix compilation error with GCC12
>  virtio-pci: Use cpumask_available to fix compilation error
>
> drivers/virtio/virtio_pci_common.c |  2 +-
> include/linux/nodemask.h           | 13 ++++++-------
> kernel/sched/sched.h               | 11 +++++++++--
> 3 files changed, 16 insertions(+), 10 deletions(-)
>
>--
>2.35.1
>
>

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Peter Zijlstra]

On Mon, Apr 25, 2022 at 04:07:43PM +0200, Christophe de Dinechin wrote:

> >> extern struct sched_class __begin_sched_classes[];
> >> extern struct sched_class __end_sched_classes[];
> >> 
> >> -#define sched_class_highest (__end_sched_classes - 1)
> >> +/*
> >> + * sched_class_highests is really __end_sched_classes - 1, but written in a way
> >> + * that makes it clear that it is within __begin_sched_classes[] and not outside
> >> + * of __end_sched_classes[].
> >> + */
> >> +#define sched_class_highest (__begin_sched_classes + \
> >> +			     (__end_sched_classes - __begin_sched_classes - 1))
> >> #define sched_class_lowest  (__begin_sched_classes - 1)
> >> 
> >> +/* The + 1 below places the pointers within the range of their array */
> >> #define for_class_range(class, _from, _to) \
> >> -	for (class = (_from); class != (_to); class--)
> >> +	for (class = (_from); class + 1 != (_to) + 1; class--)
> > 
> > Urgh, so now we get less readable code,
> 
> You consider the original code readable? 

Yeah, because: x + y - x - 1 == y - 1, so wth would you want to write it
with the x on. That's just silly.

> It actually relies on a
> precise layout that is not enforced in this code, not even documented,
> but actually enforced by the linker script.

It has a comment pointing at the linker script, and we have:

	/* Make sure the linker didn't screw up */
	BUG_ON(&idle_sched_class + 1 != &fair_sched_class ||
	       &fair_sched_class + 1 != &rt_sched_class ||
	       &rt_sched_class + 1   != &dl_sched_class);
#ifdef CONFIG_SMP
	BUG_ON(&dl_sched_class + 1 != &stop_sched_class);
#endif

On boot to verify the layout is as we expect.

> > just because GCC is being
> > stupid?
> 
> I think that GCC is actually remarkably smart there. It tells you
> that you are building pointers to A[] from B[], when there is a legit
> way to say that the pointer is in A[] (which is what my patch does)

We build with -fno-strict-aliasing, it must not assume anything like
that, unless restrict is used.

In this case, they're not two objects but the same one. Just because
linker script can't really get us a sensible array definition.

> > What's wrong with negative array indexes? memory is memory, stuff works.
> 
> What’s wrong is that the compiler cannot prove theorems anymore.
> These theorems are used to optimise code. When you write -1[B], the
> compiler cannot optimise based on knowing this refers to A[B-A-1].
> 
> While at first, you might think that disabling a warning is a win,
> what comes next is the compiler optimizing in a way you did not
> anticipate, mysterious bugs showing up, and/or having to turn off some
> potentially useful optimisation.

We're usually fairly quick to call a compiler broken if doesn't do what
we want it to. Dodgy optimizations go out the window real fast.