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
