linux-kernel - Re: drivers/net/ethernet/sfc/tc.c:450 efx_tc_flower

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 30 May 2023 19:26:46 +0100
From:   Edward Cree <ecree.xilinx@...il.com>
To:     Dan Carpenter <dan.carpenter@...aro.org>, oe-kbuild@...ts.linux.dev
Cc:     lkp@...el.com, oe-kbuild-all@...ts.linux.dev,
        linux-kernel@...r.kernel.org,
        Simon Horman <simon.horman@...igine.com>
Subject: Re: drivers/net/ethernet/sfc/tc.c:450 efx_tc_flower_replace() warn:
 missing unwind goto?

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