Received-SPF: pass (google.com: domain of linux-kernel+bounces-161111-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sat, 27 Apr 2024 18:36:22 +0100
From: Simon Horman <horms@kernel.org>
To: =?utf-8?Q?Asbj=C3=B8rn_Sloth_T=C3=B8nnesen?= <ast@fiberby.net>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Ariel Elior <aelior@marvell.com>,
	Manish Chopra <manishc@marvell.com>, Jiri Pirko <jiri@resnulli.us>,
	Pablo Neira Ayuso <pablo@netfilter.org>
Subject: Re: [PATCH net 0/4] net: qede: avoid overruling error codes
Message-ID: <20240427173622.GU516117@kernel.org>
References: <20240426091227.78060-1-ast@fiberby.net>
 <20240427114813.GG516117@kernel.org>
 <ab165657-2c69-4b30-a371-6ad7fd28c539@fiberby.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ab165657-2c69-4b30-a371-6ad7fd28c539@fiberby.net>

On Sat, Apr 27, 2024 at 12:58:38PM +0000, Asbjørn Sloth Tønnesen wrote:
> Hi Simon,
> 
> Thank you for your review effort.
> 
> On 4/27/24 11:48 AM, Simon Horman wrote:
> > On Fri, Apr 26, 2024 at 09:12:22AM +0000, Asbjørn Sloth Tønnesen wrote:
> > > This series fixes the qede driver, so that
> > > qede_parse_flow_attr() and it's subfunctions
> > > doesn't get their error codes overruled
> > > (ie. turning -EOPNOTSUPP into -EINVAL).
> > > 
> > > ---
> > > I have two more patches along the same lines,
> > > but they are not yet causing any issues,
> > > so I have them destined for net-next.
> > > (those are for qede_flow_spec_validate_unused()
> > > and qede_flow_parse_ports().)
> > > 
> > > After that I have a series for converting to
> > > extack + the final one for validating control
> > > flags.
> > 
> > Hi,
> > 
> > I'm fine with these patches so far as the code changes go.
> > But it is not clear to me that they are fixing a bug.
> > 
> > If so, I think some explanation should go in the commit messages.
> > If not, I think these should be targeted at net-next
> > (and not have Fixes tags.
> 
> Since I don't have the hardware I didn't try to construct commands, showing
> the wrong error code being returned. I could make up some hypothetical commands,
> and simulate how they would error. I assumed that the bug, was clear based on
> the list of possible return values for each function.
> 
> As an example, in qede_parse_flow_attr() it validates dissector->used_keys,
> and if an unsupported FLOW_DISSECTOR_KEY_* is set, then ede_parse_flow_attr()
> returns -EOPNOTSUPP, which is returned to qede_add_tc_flower_fltr(),
> and only check for non-zero, and since -EOPNOTSUPP is non zero,
> then it returns -EINVAL. So if you try to match on a vlan tag,
> then FLOW_DISSECTOR_KEY_VLAN would be set, and cause a -EOPNOTSUPP
> to be returned, which then gets converted into a -EINVAL.
> 
> All drivers generally returns -EOPNOTSUPP in their used_keys checks, and
> this driver clearly intended to do that as well.
> 
> The -EINVAL override was introduced in the same commit as the above check,
> so it was broken from the start.
> 
> Another example is 319a1d19471e (blamed in 4th patch), Jiri added
> a call to flow_action_basic_hw_stats_types_check() across multiple drivers,
> and since -EINVAL was returned only a few lines above, then he assumed
> that he could just return -EOPNOTSUPP, but that return value gets overruled
> into a -EINVAL. It is clear from the commit that Jiri intended to return
> -EOPNOTSUPP, but this part of the driver didn't follow the principle of
> least astonishment, so that function could only fail with -EINVAL.
> 
> I think it's a bug, when another error code is returned than the one that
> was clearly intended, but it's properly a low impact one.

Thanks, now that you point this out I agree this should have been obvious
to me.

I agree that the patches resolve issues around -EOPNOTSUPP (and other error
values; that these errors are, in general, propagated to user-space; and
that especially in the case of -EOPNOTSUPP, this may effect the behaviour
of the system as it is intended to indicate that offload of an action is not
supported (at this time, for any reason).

> > Also, if you do end posting a v2, blamed, is misspelt several
> > times in commit messages.
> 
> Sorry about that, will fix that if a v2 turns out to be needed.
> 
> -- 
> Best regards
> Asbjørn Sloth Tønnesen
> Network Engineer
> Fiberby - AS42541
>