Message ID | 20230616090844.2677815-1-arnd@kernel.org (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [1/2] sfc: fix uninitialized variable use | expand |
On Fri, Jun 16, 2023 at 11:08:18AM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The new efx_bind_neigh() function contains a broken code path when IPV6 is > disabled: > > drivers/net/ethernet/sfc/tc_encap_actions.c:144:7: error: variable 'n' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] > if (encap->type & EFX_ENCAP_FLAG_IPV6) { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/sfc/tc_encap_actions.c:184:8: note: uninitialized use occurs here > if (!n) { > ^ > drivers/net/ethernet/sfc/tc_encap_actions.c:144:3: note: remove the 'if' if its condition is always false > if (encap->type & EFX_ENCAP_FLAG_IPV6) { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/sfc/tc_encap_actions.c:141:22: note: initialize the variable 'n' to silence this warning > struct neighbour *n; > ^ > = NULL > > Change it to use the existing error handling path here. > > Fixes: 7e5e7d800011a ("sfc: neighbour lookup for TC encap action offload") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On 16/06/2023 10:08, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The new efx_bind_neigh() function contains a broken code path when IPV6 is > disabled: > > drivers/net/ethernet/sfc/tc_encap_actions.c:144:7: error: variable 'n' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] > if (encap->type & EFX_ENCAP_FLAG_IPV6) { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/sfc/tc_encap_actions.c:184:8: note: uninitialized use occurs here > if (!n) { > ^ > drivers/net/ethernet/sfc/tc_encap_actions.c:144:3: note: remove the 'if' if its condition is always false > if (encap->type & EFX_ENCAP_FLAG_IPV6) { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/sfc/tc_encap_actions.c:141:22: note: initialize the variable 'n' to silence this warning > struct neighbour *n; > ^ > = NULL > > Change it to use the existing error handling path here. > > Fixes: 7e5e7d800011a ("sfc: neighbour lookup for TC encap action offload") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/net/ethernet/sfc/tc_encap_actions.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/sfc/tc_encap_actions.c b/drivers/net/ethernet/sfc/tc_encap_actions.c > index aac259528e73e..cfd76d5bbdd48 100644 > --- a/drivers/net/ethernet/sfc/tc_encap_actions.c > +++ b/drivers/net/ethernet/sfc/tc_encap_actions.c > @@ -163,6 +163,7 @@ static int efx_bind_neigh(struct efx_nic *efx, > * enabled how did someone create an IPv6 tunnel_key? > */ > rc = -EOPNOTSUPP; > + n = NULL; > NL_SET_ERR_MSG_MOD(extack, "No IPv6 support (neigh bind)"); > #endif > } else { > Nack. There is a bug here, as you've identified, but the right fix is to add a 'goto out_free;' after setting the rc and extack msg. Setting n to NULL and relying on the subsequent error path will not only produce the wrong rc and error message, it will also attempt to drop a reference on neigh->egdev that was never taken. -ed
diff --git a/drivers/net/ethernet/sfc/tc_encap_actions.c b/drivers/net/ethernet/sfc/tc_encap_actions.c index aac259528e73e..cfd76d5bbdd48 100644 --- a/drivers/net/ethernet/sfc/tc_encap_actions.c +++ b/drivers/net/ethernet/sfc/tc_encap_actions.c @@ -163,6 +163,7 @@ static int efx_bind_neigh(struct efx_nic *efx, * enabled how did someone create an IPv6 tunnel_key? */ rc = -EOPNOTSUPP; + n = NULL; NL_SET_ERR_MSG_MOD(extack, "No IPv6 support (neigh bind)"); #endif } else {