Re: drivers/net/ethernet/sfc/tc.c:450 efx_tc_flower_replace() warn: missing unwind goto?

[Edward Cree]

On 20/05/2023 09:32, Dan Carpenter wrote:
> b7f5e17b3bb961 Edward Cree 2023-03-27  432  	if (efx_tc_match_is_encap(&match.mask)) {
> b7f5e17b3bb961 Edward Cree 2023-03-27  433  		NL_SET_ERR_MSG_MOD(extack, "Ingress enc_key matches not supported");
> b7f5e17b3bb961 Edward Cree 2023-03-27  434  		rc = -EOPNOTSUPP;
> b7f5e17b3bb961 Edward Cree 2023-03-27  435  		goto release;
> 
> This goto confuses Smatch.  It could be converted to a direct return.
[...]
> d902e1a737d44e Edward Cree 2022-09-26  456  	if (old) {
> d902e1a737d44e Edward Cree 2022-09-26  457  		netif_dbg(efx, drv, efx->net_dev,
> d902e1a737d44e Edward Cree 2022-09-26  458  			  "Already offloaded rule (cookie %lx)\n", tc->cookie);
> d902e1a737d44e Edward Cree 2022-09-26  459  		rc = -EEXIST;
> d902e1a737d44e Edward Cree 2022-09-26  460  		NL_SET_ERR_MSG_MOD(extack, "Rule already offloaded");
> d902e1a737d44e Edward Cree 2022-09-26  461  		goto release;
> 
> It looks like this error path is problematic because it will remove the
> existing rule from the list.  Better to just do:
> 
> 	if (old) {
> 		netif_dbg(...);
> 		NL_SET_ERR_MSG_MOD(extack, "Rule already offloaded");
> 		kfree(rule);
> 		return -EEXIST;
> 	}

Agreed on both counts.
(Technically we could add a new goto label in the failure ladder to
 just do the kfree and return, but this is probably clearer.)
Guess efx_tc_flower_replace_foreign() now has the latter problem
 too?  (And it _does_ want to use the failure ladder, because we've
 got to release the encap match.)

Thanks for catching this.  Patch for 'net' to follow shortly.
-ed