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@xxxxxxxxxx>
>Reviewed-by: Tom Rix <trix@xxxxxxxxxx>
>> ---
>>
>> 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