init/Kconfig: extend -Wno-array-bounds to gcc 13

Message ID 20230306220947.1982272-1-trix@redhat.com
State New
Headers
Series init/Kconfig: extend -Wno-array-bounds to gcc 13 |

Commit Message

Tom Rix March 6, 2023, 10:09 p.m. UTC
  With gcc 13.0.1 on x86, there are several false positives like

drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c:167:31:
  error: array subscript 4 is above array bounds of ‘const struct sparx5_psfp_gce[4]’ [-Werror=array-bounds=]
  167 |                 gce = &sg->gce[i];
      |                        ~~~~~~~^~~
In file included from drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c:8:
drivers/net/ethernet/microchip/sparx5/sparx5_main.h:506:32: note: while referencing ‘gce’
  506 |         struct sparx5_psfp_gce gce[SPX5_PSFP_GCE_CNT];
      |                                ^~~

The code lines for the reported problem
	/* For each scheduling entry */
	for (i = 0; i < sg->num_entries; i++) {
		gce = &sg->gce[i];

i is bounded by num_entries, which is set in sparx5_tc_flower.c
	if (act->gate.num_entries >= SPX5_PSFP_GCE_CNT) {
		NL_SET_ERR_MSG_MOD(extack, "Invalid number of gate entries");
		return -EINVAL;
	}
..
	sg->num_entries = act->gate.num_entries;

So disable array-bounds as was done on gcc 11 and 12

Signed-off-by: Tom Rix <trix@redhat.com>
---
 init/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)
  

Comments

Nick Desaulniers March 6, 2023, 10:20 p.m. UTC | #1
+ Kees
https://lore.kernel.org/lkml/20230306220947.1982272-1-trix@redhat.com/

On Mon, Mar 6, 2023 at 2:10 PM Tom Rix <trix@redhat.com> wrote:
>
> With gcc 13.0.1 on x86, there are several false positives like
>
> drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c:167:31:
>   error: array subscript 4 is above array bounds of ‘const struct sparx5_psfp_gce[4]’ [-Werror=array-bounds=]
>   167 |                 gce = &sg->gce[i];
>       |                        ~~~~~~~^~~
> In file included from drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c:8:
> drivers/net/ethernet/microchip/sparx5/sparx5_main.h:506:32: note: while referencing ‘gce’
>   506 |         struct sparx5_psfp_gce gce[SPX5_PSFP_GCE_CNT];
>       |                                ^~~
>
> The code lines for the reported problem
>         /* For each scheduling entry */
>         for (i = 0; i < sg->num_entries; i++) {
>                 gce = &sg->gce[i];
>
> i is bounded by num_entries, which is set in sparx5_tc_flower.c
>         if (act->gate.num_entries >= SPX5_PSFP_GCE_CNT) {
>                 NL_SET_ERR_MSG_MOD(extack, "Invalid number of gate entries");
>                 return -EINVAL;
>         }
> ..
>         sg->num_entries = act->gate.num_entries;
>
> So disable array-bounds as was done on gcc 11 and 12
>
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  init/Kconfig | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 1fb5f313d18f..10d0a0020726 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -898,10 +898,14 @@ config GCC11_NO_ARRAY_BOUNDS
>  config GCC12_NO_ARRAY_BOUNDS
>         def_bool y
>
> +config GCC13_NO_ARRAY_BOUNDS
> +       def_bool y
> +
>  config CC_NO_ARRAY_BOUNDS
>         bool
>         default y if CC_IS_GCC && GCC_VERSION >= 110000 && GCC_VERSION < 120000 && GCC11_NO_ARRAY_BOUNDS
>         default y if CC_IS_GCC && GCC_VERSION >= 120000 && GCC_VERSION < 130000 && GCC12_NO_ARRAY_BOUNDS
> +       default y if CC_IS_GCC && GCC_VERSION >= 130000 && GCC_VERSION < 140000 && GCC13_NO_ARRAY_BOUNDS
>
>  #
>  # For architectures that know their GCC __int128 support is sound
> --
> 2.27.0
>
  
Kees Cook March 6, 2023, 11:02 p.m. UTC | #2
On March 6, 2023 2:20:50 PM PST, Nick Desaulniers <ndesaulniers@google.com> wrote:
>+ Kees
>https://lore.kernel.org/lkml/20230306220947.1982272-1-trix@redhat.com/
>
>On Mon, Mar 6, 2023 at 2:10 PM Tom Rix <trix@redhat.com> wrote:
>>
>> With gcc 13.0.1 on x86, there are several false positives like
>>
>> drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c:167:31:
>>   error: array subscript 4 is above array bounds of ‘const struct sparx5_psfp_gce[4]’ [-Werror=array-bounds=]
>>   167 |                 gce = &sg->gce[i];
>>       |                        ~~~~~~~^~~
>> In file included from drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c:8:
>> drivers/net/ethernet/microchip/sparx5/sparx5_main.h:506:32: note: while referencing ‘gce’
>>   506 |         struct sparx5_psfp_gce gce[SPX5_PSFP_GCE_CNT];
>>       |                                ^~~
>>
>> The code lines for the reported problem
>>         /* For each scheduling entry */
>>         for (i = 0; i < sg->num_entries; i++) {
>>                 gce = &sg->gce[i];
>>
>> i is bounded by num_entries, which is set in sparx5_tc_flower.c
>>         if (act->gate.num_entries >= SPX5_PSFP_GCE_CNT) {
>>                 NL_SET_ERR_MSG_MOD(extack, "Invalid number of gate entries");
>>                 return -EINVAL;
>>         }
>> ..
>>         sg->num_entries = act->gate.num_entries;
>>
>> So disable array-bounds as was done on gcc 11 and 12

GCC 13 isn't released yet, and we've been working to make Linux warning-free under -Wareay-bounds. (And we succeeded briefly with GCC 11.)

I'd much rather get GCC fixed. This is due to the shift sanitizer reducing the scope of num_entries (via macro args) to 0-31, which is still >4. This seems like a hinting bug in GCC: just because the variable was used in a shift doesn't mean the compiler can make any value assumptions.

-Kees
  
Tom Rix March 7, 2023, 1:07 a.m. UTC | #3
On 3/6/23 3:02 PM, Kees Cook wrote:
> On March 6, 2023 2:20:50 PM PST, Nick Desaulniers <ndesaulniers@google.com> wrote:
>> + Kees
>> https://lore.kernel.org/lkml/20230306220947.1982272-1-trix@redhat.com/
>>
>> On Mon, Mar 6, 2023 at 2:10 PM Tom Rix <trix@redhat.com> wrote:
>>> With gcc 13.0.1 on x86, there are several false positives like
>>>
>>> drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c:167:31:
>>>    error: array subscript 4 is above array bounds of ‘const struct sparx5_psfp_gce[4]’ [-Werror=array-bounds=]
>>>    167 |                 gce = &sg->gce[i];
>>>        |                        ~~~~~~~^~~
>>> In file included from drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c:8:
>>> drivers/net/ethernet/microchip/sparx5/sparx5_main.h:506:32: note: while referencing ‘gce’
>>>    506 |         struct sparx5_psfp_gce gce[SPX5_PSFP_GCE_CNT];
>>>        |                                ^~~
>>>
>>> The code lines for the reported problem
>>>          /* For each scheduling entry */
>>>          for (i = 0; i < sg->num_entries; i++) {
>>>                  gce = &sg->gce[i];
>>>
>>> i is bounded by num_entries, which is set in sparx5_tc_flower.c
>>>          if (act->gate.num_entries >= SPX5_PSFP_GCE_CNT) {
>>>                  NL_SET_ERR_MSG_MOD(extack, "Invalid number of gate entries");
>>>                  return -EINVAL;
>>>          }
>>> ..
>>>          sg->num_entries = act->gate.num_entries;
>>>
>>> So disable array-bounds as was done on gcc 11 and 12
> GCC 13 isn't released yet, and we've been working to make Linux warning-free under -Wareay-bounds. (And we succeeded briefly with GCC 11.)
>
> I'd much rather get GCC fixed. This is due to the shift sanitizer reducing the scope of num_entries (via macro args) to 0-31, which is still >4. This seems like a hinting bug in GCC: just because the variable was used in a shift doesn't mean the compiler can make any value assumptions.

The build with fail generally with gcc 13.

The warnings could be cleaned without having an error, but I looked at 
multiple errors, none of them were real.

imo this is a broken compiler option.

Tom

>
> -Kees
>
>
>
  
Miguel Ojeda March 7, 2023, 11:42 a.m. UTC | #4
On Tue, Mar 7, 2023 at 2:07 AM Tom Rix <trix@redhat.com> wrote:
>
> The build with fail generally with gcc 13.
>
> The warnings could be cleaned without having an error, but I looked at
> multiple errors, none of them were real.
>
> imo this is a broken compiler option.

I am not sure I understand -- my reading of Kees' message is that he
would prefer to get the warning (rather than the kernel) fixed before
GCC 13 releases.

Are you asking to have the option disabled until GCC 13 releases and
reevaluate then? How many warnings are you getting? Are those actual
errors or `-Werror`?

Cheers,
Miguel
  
Tom Rix March 7, 2023, 1:28 p.m. UTC | #5
On 3/7/23 3:42 AM, Miguel Ojeda wrote:
> On Tue, Mar 7, 2023 at 2:07 AM Tom Rix <trix@redhat.com> wrote:
>> The build with fail generally with gcc 13.
>>
>> The warnings could be cleaned without having an error, but I looked at
>> multiple errors, none of them were real.
>>
>> imo this is a broken compiler option.
> I am not sure I understand -- my reading of Kees' message is that he
> would prefer to get the warning (rather than the kernel) fixed before
> GCC 13 releases.

yes, that would be ideal.

But anyone using gcc 13 in the meanwhile will have a broken build

and any other aspect of testing the kernel with gcc 13 would have

to be deferred to after the fix, if it happens.

>
> Are you asking to have the option disabled until GCC 13 releases and
> reevaluate then? How many warnings are you getting? Are those actual
> errors or `-Werror`?

With W=1, there are 40 error:'s, sorry I do not have the default logs at 
hand.

I looked about 10 of them over the weekend, they we all bogus.

So gcc needs to be fixed first, just disabling with -Wnoerror will

give folks bad problems that do not need fixing.


I am asking that we at least temporarily disable this option so

the build break is fixed.

Tom

>
> Cheers,
> Miguel
>
  

Patch

diff --git a/init/Kconfig b/init/Kconfig
index 1fb5f313d18f..10d0a0020726 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -898,10 +898,14 @@  config GCC11_NO_ARRAY_BOUNDS
 config GCC12_NO_ARRAY_BOUNDS
 	def_bool y
 
+config GCC13_NO_ARRAY_BOUNDS
+	def_bool y
+
 config CC_NO_ARRAY_BOUNDS
 	bool
 	default y if CC_IS_GCC && GCC_VERSION >= 110000 && GCC_VERSION < 120000 && GCC11_NO_ARRAY_BOUNDS
 	default y if CC_IS_GCC && GCC_VERSION >= 120000 && GCC_VERSION < 130000 && GCC12_NO_ARRAY_BOUNDS
+	default y if CC_IS_GCC && GCC_VERSION >= 130000 && GCC_VERSION < 140000 && GCC13_NO_ARRAY_BOUNDS
 
 #
 # For architectures that know their GCC __int128 support is sound