| Message ID | 20220908153207.4048871-1-nathan@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net/mlx5e: Ensure macsec_rule is always initiailized in macsec_fs_{r,t}x_add_rule() | expand |
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
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
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
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);
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(-) base-commit: 75554fe00f941c3c3d9344e88708093a14d2b4b8