[PATCH net-next] net/mlx5e: Ensure macsec_rule is always initiailized in macsec_fs_{r,t}x_add_rule()

[PATCH net-next] net/mlx5e: Ensure macsec_rule is always initiailized in macsec_fs_{r,t}x_add_rule()

[Nathan Chancellor]

Clang warns:

  drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:539:6: error: variable 'macsec_rule' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
          if (err)
              ^~~
  drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:598:9: note: uninitialized use occurs here
          return macsec_rule;
                ^~~~~~~~~~~
  drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:539:2: note: remove the 'if' if its condition is always false
          if (err)
          ^~~~~~~~
  drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:523:38: note: initialize the variable 'macsec_rule' to silence this warning
          union mlx5e_macsec_rule *macsec_rule;
                                              ^
                                              = NULL
  drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1131:6: error: variable 'macsec_rule' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
          if (err)
              ^~~
  drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1215:9: note: uninitialized use occurs here
          return macsec_rule;
                ^~~~~~~~~~~
  drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1131:2: note: remove the 'if' if its condition is always false
          if (err)
          ^~~~~~~~
  drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1118:38: note: initialize the variable 'macsec_rule' to silence this warning
          union mlx5e_macsec_rule *macsec_rule;
                                              ^
                                              = NULL
  2 errors generated.

If macsec_fs_{r,t}x_ft_get() fail, macsec_rule will be uninitialized.
Use the existing initialization to NULL in the existing error path to
ensure macsec_rule is always initialized.

Fixes: e467b283ffd5 ("net/mlx5e: Add MACsec TX steering rules")
Fixes: 3b20949cb21b ("net/mlx5e: Add MACsec RX steering rules")
Link: https://github.com/ClangBuiltLinux/linux/issues/1706
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---

The other fix I considered was shuffling the two if statements so that
the allocation of macsec_rule came before the call to
macsec_fs_{r,t}x_ft_get() but I was not sure what the implications of
that change were.

Also, I thought netdev was doing testing with clang so that new warnings
do not show up. Did something break or stop working since this is the
second time in two weeks that new warnings have appeared in -next?

 .../net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c    | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
index 608fbbaa5a58..4467e88d7e7f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
@@ -537,7 +537,7 @@ macsec_fs_tx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
 
 	err = macsec_fs_tx_ft_get(macsec_fs);
 	if (err)
-		goto out_spec;
+		goto out_spec_no_rule;
 
 	macsec_rule = kzalloc(sizeof(*macsec_rule), GFP_KERNEL);
 	if (!macsec_rule) {
@@ -591,6 +591,7 @@ macsec_fs_tx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
 
 err:
 	macsec_fs_tx_del_rule(macsec_fs, tx_rule);
+out_spec_no_rule:
 	macsec_rule = NULL;
 out_spec:
 	kvfree(spec);
@@ -1129,7 +1130,7 @@ macsec_fs_rx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
 
 	err = macsec_fs_rx_ft_get(macsec_fs);
 	if (err)
-		goto out_spec;
+		goto out_spec_no_rule;
 
 	macsec_rule = kzalloc(sizeof(*macsec_rule), GFP_KERNEL);
 	if (!macsec_rule) {
@@ -1209,6 +1210,7 @@ macsec_fs_rx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
 
 err:
 	macsec_fs_rx_del_rule(macsec_fs, rx_rule);
+out_spec_no_rule:
 	macsec_rule = NULL;
 out_spec:
 	kvfree(spec);

base-commit: 75554fe00f941c3c3d9344e88708093a14d2b4b8
-- 
2.37.3

Re: [PATCH net-next] net/mlx5e: Ensure macsec_rule is always initiailized in macsec_fs_{r,t}x_add_rule()

[Tom Rix]

On 9/8/22 8:32 AM, Nathan Chancellor wrote:
> Clang warns:
>
>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:539:6: error: variable 'macsec_rule' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>            if (err)
>                ^~~
>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:598:9: note: uninitialized use occurs here
>            return macsec_rule;
>                  ^~~~~~~~~~~
>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:539:2: note: remove the 'if' if its condition is always false
>            if (err)
>            ^~~~~~~~
>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:523:38: note: initialize the variable 'macsec_rule' to silence this warning
>            union mlx5e_macsec_rule *macsec_rule;
>                                                ^
>                                                = NULL
>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1131:6: error: variable 'macsec_rule' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>            if (err)
>                ^~~
>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1215:9: note: uninitialized use occurs here
>            return macsec_rule;
>                  ^~~~~~~~~~~
>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1131:2: note: remove the 'if' if its condition is always false
>            if (err)
>            ^~~~~~~~
>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1118:38: note: initialize the variable 'macsec_rule' to silence this warning
>            union mlx5e_macsec_rule *macsec_rule;
>                                                ^
>                                                = NULL
>    2 errors generated.
>
> If macsec_fs_{r,t}x_ft_get() fail, macsec_rule will be uninitialized.
> Use the existing initialization to NULL in the existing error path to
> ensure macsec_rule is always initialized.
>
> Fixes: e467b283ffd5 ("net/mlx5e: Add MACsec TX steering rules")
> Fixes: 3b20949cb21b ("net/mlx5e: Add MACsec RX steering rules")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1706
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Tom Rix <trix@redhat.com>
> ---
>
> The other fix I considered was shuffling the two if statements so that
> the allocation of macsec_rule came before the call to
> macsec_fs_{r,t}x_ft_get() but I was not sure what the implications of
> that change were.
>
> Also, I thought netdev was doing testing with clang so that new warnings
> do not show up. Did something break or stop working since this is the
> second time in two weeks that new warnings have appeared in -next?
>
>   .../net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c    | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
> index 608fbbaa5a58..4467e88d7e7f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
> @@ -537,7 +537,7 @@ macsec_fs_tx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
>   
>   	err = macsec_fs_tx_ft_get(macsec_fs);
>   	if (err)
> -		goto out_spec;
> +		goto out_spec_no_rule;
>   
>   	macsec_rule = kzalloc(sizeof(*macsec_rule), GFP_KERNEL);
>   	if (!macsec_rule) {
> @@ -591,6 +591,7 @@ macsec_fs_tx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
>   
>   err:
>   	macsec_fs_tx_del_rule(macsec_fs, tx_rule);
> +out_spec_no_rule:
>   	macsec_rule = NULL;
>   out_spec:
>   	kvfree(spec);
> @@ -1129,7 +1130,7 @@ macsec_fs_rx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
>   
>   	err = macsec_fs_rx_ft_get(macsec_fs);
>   	if (err)
> -		goto out_spec;
> +		goto out_spec_no_rule;
>   
>   	macsec_rule = kzalloc(sizeof(*macsec_rule), GFP_KERNEL);
>   	if (!macsec_rule) {
> @@ -1209,6 +1210,7 @@ macsec_fs_rx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
>   
>   err:
>   	macsec_fs_rx_del_rule(macsec_fs, rx_rule);
> +out_spec_no_rule:
>   	macsec_rule = NULL;
>   out_spec:
>   	kvfree(spec);
>
> base-commit: 75554fe00f941c3c3d9344e88708093a14d2b4b8

RE: [PATCH net-next] net/mlx5e: Ensure macsec_rule is always initiailized in macsec_fs_{r,t}x_add_rule()

[Raed Salem]

On 9/8/22 8:32 AM, Nathan Chancellor wrote:
>> Clang warns:
>>
>>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:539:6: error: variable 'macsec_rule' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>>            if (err)
>>                ^~~
>>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:598:9: note: uninitialized use occurs here
>>            return macsec_rule;
>>                  ^~~~~~~~~~~
>>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:539:2: note: remove the 'if' if its condition is always false
>>            if (err)
>>            ^~~~~~~~
>>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:523:38: note: initialize the variable 'macsec_rule' to silence this warning
>>            union mlx5e_macsec_rule *macsec_rule;
>>                                                ^
>>                                                = NULL
>>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1131:6: error: variable 'macsec_rule' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>>            if (err)
>>                ^~~
>>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1215:9: note: uninitialized use occurs here
>>            return macsec_rule;
>>                  ^~~~~~~~~~~
>>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1131:2: note: remove the 'if' if its condition is always false
>>            if (err)
>>            ^~~~~~~~
>>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1118:38: note: initialize the variable 'macsec_rule' to silence this warning
>>            union mlx5e_macsec_rule *macsec_rule;
>>                                                ^
>>                                                = NULL
Why not do as suggested and initialize the macsec_rule to NULL (and change placement to comply with reversed Christmas tree parameters order) ?
it is cleaner and adhering to similar error paths in the mlx5 driver, thanks for the catch.
>>    2 errors generated.
>>
>> If macsec_fs_{r,t}x_ft_get() fail, macsec_rule will be uninitialized.
>> Use the existing initialization to NULL in the existing error path to 
>> ensure macsec_rule is always initialized.
>>
>> Fixes: e467b283ffd5 ("net/mlx5e: Add MACsec TX steering rules")
>> Fixes: 3b20949cb21b ("net/mlx5e: Add MACsec RX steering rules")
>> Link: https://github.com/ClangBuiltLinux/linux/issues/1706
>> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
>Reviewed-by: Tom Rix <trix@redhat.com>
>> ---
>>
>> The other fix I considered was shuffling the two if statements so that 
>> the allocation of macsec_rule came before the call to
>> macsec_fs_{r,t}x_ft_get() but I was not sure what the implications of 
>> that change were.
>>
>> Also, I thought netdev was doing testing with clang so that new 
>> warnings do not show up. Did something break or stop working since 
>> this is the second time in two weeks that new warnings have appeared in -next?
>>
>>   .../net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c    | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git 
>> a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c 
>> b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
>> index 608fbbaa5a58..4467e88d7e7f 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
>> @@ -537,7 +537,7 @@ macsec_fs_tx_add_rule(struct mlx5e_macsec_fs 
>> *macsec_fs,
>>
>>      err = macsec_fs_tx_ft_get(macsec_fs);
>>       if (err)
>> -             goto out_spec;
>> +             goto out_spec_no_rule;
>>
>>       macsec_rule = kzalloc(sizeof(*macsec_rule), GFP_KERNEL);
>>       if (!macsec_rule) {
>> @@ -591,6 +591,7 @@ macsec_fs_tx_add_rule(struct mlx5e_macsec_fs 
>> *macsec_fs,
>>
>>   err:
>>       macsec_fs_tx_del_rule(macsec_fs, tx_rule);
>> +out_spec_no_rule:
>>       macsec_rule = NULL;
>>   out_spec:
>>       kvfree(spec);
>> @@ -1129,7 +1130,7 @@ macsec_fs_rx_add_rule(struct mlx5e_macsec_fs 
>> *macsec_fs,
>>
>>       err = macsec_fs_rx_ft_get(macsec_fs);
>>       if (err)
>> -             goto out_spec;
>> +             goto out_spec_no_rule;
>>
>>       macsec_rule = kzalloc(sizeof(*macsec_rule), GFP_KERNEL);
>>       if (!macsec_rule) {
>> @@ -1209,6 +1210,7 @@ macsec_fs_rx_add_rule(struct mlx5e_macsec_fs 
>> *macsec_fs,
>>
>>   err:
>>       macsec_fs_rx_del_rule(macsec_fs, rx_rule);
>> +out_spec_no_rule:
>>       macsec_rule = NULL;
>>   out_spec:
>>       kvfree(spec);
>>
>> base-commit: 75554fe00f941c3c3d9344e88708093a14d2b4b8

Re: [PATCH net-next] net/mlx5e: Ensure macsec_rule is always initiailized in macsec_fs_{r,t}x_add_rule()

[Nathan Chancellor]

On Sun, Sep 11, 2022 at 05:37:26AM +0000, Raed Salem wrote:
> On 9/8/22 8:32 AM, Nathan Chancellor wrote:
> >> Clang warns:
> >>
> >>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:539:6: error: variable 'macsec_rule' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
> >>            if (err)
> >>                ^~~
> >>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:598:9: note: uninitialized use occurs here
> >>            return macsec_rule;
> >>                  ^~~~~~~~~~~
> >>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:539:2: note: remove the 'if' if its condition is always false
> >>            if (err)
> >>            ^~~~~~~~
> >>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:523:38: note: initialize the variable 'macsec_rule' to silence this warning
> >>            union mlx5e_macsec_rule *macsec_rule;
> >>                                                ^
> >>                                                = NULL
> >>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1131:6: error: variable 'macsec_rule' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
> >>            if (err)
> >>                ^~~
> >>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1215:9: note: uninitialized use occurs here
> >>            return macsec_rule;
> >>                  ^~~~~~~~~~~
> >>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1131:2: note: remove the 'if' if its condition is always false
> >>            if (err)
> >>            ^~~~~~~~
> >>    drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c:1118:38: note: initialize the variable 'macsec_rule' to silence this warning
> >>            union mlx5e_macsec_rule *macsec_rule;
> >>                                                ^
> >>                                                = NULL
> Why not do as suggested and initialize the macsec_rule to NULL (and change placement to comply with reversed Christmas tree parameters order) ?
> it is cleaner and adhering to similar error paths in the mlx5 driver, thanks for the catch.

No particular reason. I tend to avoid initializing variables at the top
when they might be used in error paths because it will hide warnings if
the variable needs to be set to something different (for example, error
codes). That is not too relevant here from what I can tell so I can just
initialize it at the top as you suggested. I will send a v2 shortly.
Thanks for the input!

Cheers,
Nathan

> >>    2 errors generated.
> >>
> >> If macsec_fs_{r,t}x_ft_get() fail, macsec_rule will be uninitialized.
> >> Use the existing initialization to NULL in the existing error path to 
> >> ensure macsec_rule is always initialized.
> >>
> >> Fixes: e467b283ffd5 ("net/mlx5e: Add MACsec TX steering rules")
> >> Fixes: 3b20949cb21b ("net/mlx5e: Add MACsec RX steering rules")
> >> Link: https://github.com/ClangBuiltLinux/linux/issues/1706
> >> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> >Reviewed-by: Tom Rix <trix@redhat.com>
> >> ---
> >>
> >> The other fix I considered was shuffling the two if statements so that 
> >> the allocation of macsec_rule came before the call to
> >> macsec_fs_{r,t}x_ft_get() but I was not sure what the implications of 
> >> that change were.
> >>
> >> Also, I thought netdev was doing testing with clang so that new 
> >> warnings do not show up. Did something break or stop working since 
> >> this is the second time in two weeks that new warnings have appeared in -next?
> >>
> >>   .../net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c    | 6 ++++--
> >>   1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git 
> >> a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c 
> >> b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
> >> index 608fbbaa5a58..4467e88d7e7f 100644
> >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
> >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
> >> @@ -537,7 +537,7 @@ macsec_fs_tx_add_rule(struct mlx5e_macsec_fs 
> >> *macsec_fs,
> >>
> >>      err = macsec_fs_tx_ft_get(macsec_fs);
> >>       if (err)
> >> -             goto out_spec;
> >> +             goto out_spec_no_rule;
> >>
> >>       macsec_rule = kzalloc(sizeof(*macsec_rule), GFP_KERNEL);
> >>       if (!macsec_rule) {
> >> @@ -591,6 +591,7 @@ macsec_fs_tx_add_rule(struct mlx5e_macsec_fs 
> >> *macsec_fs,
> >>
> >>   err:
> >>       macsec_fs_tx_del_rule(macsec_fs, tx_rule);
> >> +out_spec_no_rule:
> >>       macsec_rule = NULL;
> >>   out_spec:
> >>       kvfree(spec);
> >> @@ -1129,7 +1130,7 @@ macsec_fs_rx_add_rule(struct mlx5e_macsec_fs 
> >> *macsec_fs,
> >>
> >>       err = macsec_fs_rx_ft_get(macsec_fs);
> >>       if (err)
> >> -             goto out_spec;
> >> +             goto out_spec_no_rule;
> >>
> >>       macsec_rule = kzalloc(sizeof(*macsec_rule), GFP_KERNEL);
> >>       if (!macsec_rule) {
> >> @@ -1209,6 +1210,7 @@ macsec_fs_rx_add_rule(struct mlx5e_macsec_fs 
> >> *macsec_fs,
> >>
> >>   err:
> >>       macsec_fs_rx_del_rule(macsec_fs, rx_rule);
> >> +out_spec_no_rule:
> >>       macsec_rule = NULL;
> >>   out_spec:
> >>       kvfree(spec);
> >>
> >> base-commit: 75554fe00f941c3c3d9344e88708093a14d2b4b8