Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: drivers/net/ethernet/sfc/tc.c:450 efx_tc_flower_replace() warn:
 missing unwind goto?
To:     Dan Carpenter <dan.carpenter@linaro.org>, oe-kbuild@lists.linux.dev
Cc:     lkp@intel.com, oe-kbuild-all@lists.linux.dev,
        linux-kernel@vger.kernel.org,
        Simon Horman <simon.horman@corigine.com>
References: <cbbbf576-6788-4049-b1e8-a05862f62cc2@kili.mountain>
From:   Edward Cree <ecree.xilinx@gmail.com>
Message-ID: <4eabd7b1-66b3-7b3a-cabd-d1876767c49c@gmail.com>
Date:   Tue, 30 May 2023 19:26:46 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <cbbbf576-6788-4049-b1e8-a05862f62cc2@kili.mountain>
Content-Type: text/plain; charset=utf-8
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Precedence: bulk

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