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

From: Nathan Chancellor <nathan@kernel.org>
To: Raed Salem <raeds@nvidia.com>
Cc: Tom Rix <trix@redhat.com>, Saeed Mahameed <saeedm@nvidia.com>,
	Leon Romanovsky <leon@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Boris Pismenny <borisp@nvidia.com>,
	Lior Nahmanson <liorna@nvidia.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"llvm@lists.linux.dev" <llvm@lists.linux.dev>,
	"patches@lists.linux.dev" <patches@lists.linux.dev>
Subject: Re: [PATCH net-next] net/mlx5e: Ensure macsec_rule is always initiailized in macsec_fs_{r,t}x_add_rule()
Date: Sun, 11 Sep 2022 01:54:04 -0700	[thread overview]
Message-ID: <Yx2iLJafMDKYZ0jL@dev-arch.thelio-3990X> (raw)
In-Reply-To: <DM4PR12MB53570DA53F0DA74F132698EAC9459@DM4PR12MB5357.namprd12.prod.outlook.com>

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