[PATCH net 0/4] net: qede: avoid overruling error codes

[PATCH net 0/4] net: qede: avoid overruling error codes

[Asbjørn Sloth Tønnesen]

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.

Asbjørn Sloth Tønnesen (4):
  net: qede: sanitize 'rc' in qede_add_tc_flower_fltr()
  net: qede: use return from qede_parse_flow_attr() for flower
  net: qede: use return from qede_parse_flow_attr() for flow_spec
  net: qede: use return from qede_parse_actions()

 drivers/net/ethernet/qlogic/qede/qede_filter.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH net 1/4] net: qede: sanitize 'rc' in qede_add_tc_flower_fltr()

[Asbjørn Sloth Tønnesen]

Explicitly set 'rc' (return code), before jumping to the
unlock and return path.

By not having any code depend on that 'rc' remains at
it's initial value of -EINVAL, then we can re-use 'rc' for
the return code of function calls in subsequent patches.

Only compile tested.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 drivers/net/ethernet/qlogic/qede/qede_filter.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index a5ac21a0ee33..8ecdfa36a685 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -1868,8 +1868,8 @@ int qede_add_tc_flower_fltr(struct qede_dev *edev, __be16 proto,
 			    struct flow_cls_offload *f)
 {
 	struct qede_arfs_fltr_node *n;
-	int min_hlen, rc = -EINVAL;
 	struct qede_arfs_tuple t;
+	int min_hlen, rc;
 
 	__qede_lock(edev);
 
@@ -1879,8 +1879,10 @@ int qede_add_tc_flower_fltr(struct qede_dev *edev, __be16 proto,
 	}
 
 	/* parse flower attribute and prepare filter */
-	if (qede_parse_flow_attr(edev, proto, f->rule, &t))
+	if (qede_parse_flow_attr(edev, proto, f->rule, &t)) {
+		rc = -EINVAL;
 		goto unlock;
+	}
 
 	/* Validate profile mode and number of filters */
 	if ((edev->arfs->filter_count && edev->arfs->mode != t.mode) ||
@@ -1888,12 +1890,15 @@ int qede_add_tc_flower_fltr(struct qede_dev *edev, __be16 proto,
 		DP_NOTICE(edev,
 			  "Filter configuration invalidated, filter mode=0x%x, configured mode=0x%x, filter count=0x%x\n",
 			  t.mode, edev->arfs->mode, edev->arfs->filter_count);
+		rc = -EINVAL;
 		goto unlock;
 	}
 
 	/* parse tc actions and get the vf_id */
-	if (qede_parse_actions(edev, &f->rule->action, f->common.extack))
+	if (qede_parse_actions(edev, &f->rule->action, f->common.extack)) {
+		rc = -EINVAL;
 		goto unlock;
+	}
 
 	if (qede_flow_find_fltr(edev, &t)) {
 		rc = -EEXIST;
-- 
2.43.0

Re: [PATCH net 1/4] net: qede: sanitize 'rc' in qede_add_tc_flower_fltr()

[Simon Horman]

On Fri, Apr 26, 2024 at 09:12:23AM +0000, Asbjørn Sloth Tønnesen wrote:
> Explicitly set 'rc' (return code), before jumping to the
> unlock and return path.
> 
> By not having any code depend on that 'rc' remains at
> it's initial value of -EINVAL, then we can re-use 'rc' for
> the return code of function calls in subsequent patches.
> 
> Only compile tested.
> 
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---
>  drivers/net/ethernet/qlogic/qede/qede_filter.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net 2/4] net: qede: use return from qede_parse_flow_attr() for flower

[Asbjørn Sloth Tønnesen]

In qede_add_tc_flower_fltr(), when calling
qede_parse_flow_attr() then the return code
was only used for a non-zero check, and then
-EINVAL was returned.

qede_parse_flow_attr() can currently fail with:
* -EINVAL
* -EOPNOTSUPP
* -EPROTONOSUPPORT

This patch changes the code to use the actual
return code, not just return -EINVAL.

The blaimed commit introduced these functions.

Only compile tested.

Fixes: 2ce9c93eaca6 ("qede: Ingress tc flower offload (drop action) support.")
Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 drivers/net/ethernet/qlogic/qede/qede_filter.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index 8ecdfa36a685..25ef0f4258cb 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -1879,10 +1879,9 @@ int qede_add_tc_flower_fltr(struct qede_dev *edev, __be16 proto,
 	}
 
 	/* parse flower attribute and prepare filter */
-	if (qede_parse_flow_attr(edev, proto, f->rule, &t)) {
-		rc = -EINVAL;
+	rc = qede_parse_flow_attr(edev, proto, f->rule, &t);
+	if (rc)
 		goto unlock;
-	}
 
 	/* Validate profile mode and number of filters */
 	if ((edev->arfs->filter_count && edev->arfs->mode != t.mode) ||
-- 
2.43.0

Re: [PATCH net 2/4] net: qede: use return from qede_parse_flow_attr() for flower

[Simon Horman]

On Fri, Apr 26, 2024 at 09:12:24AM +0000, Asbjørn Sloth Tønnesen wrote:
> In qede_add_tc_flower_fltr(), when calling
> qede_parse_flow_attr() then the return code
> was only used for a non-zero check, and then
> -EINVAL was returned.
> 
> qede_parse_flow_attr() can currently fail with:
> * -EINVAL
> * -EOPNOTSUPP
> * -EPROTONOSUPPORT
> 
> This patch changes the code to use the actual
> return code, not just return -EINVAL.
> 
> The blaimed commit introduced these functions.
> 
> Only compile tested.
> 
> Fixes: 2ce9c93eaca6 ("qede: Ingress tc flower offload (drop action) support.")
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net 3/4] net: qede: use return from qede_parse_flow_attr() for flow_spec

[Asbjørn Sloth Tønnesen]

In qede_flow_spec_to_rule(), when calling
qede_parse_flow_attr() then the return code
was only used for a non-zero check, and then
-EINVAL was returned.

qede_parse_flow_attr() can currently fail with:
* -EINVAL
* -EOPNOTSUPP
* -EPROTONOSUPPORT

This patch changes the code to use the actual
return code, not just return -EINVAL.

The blaimed commit introduced qede_flow_spec_to_rule(),
and this call to qede_parse_flow_attr(), it looks
like it just duplicated how it was already used.

Only compile tested.

Fixes: 37c5d3efd7f8 ("qede: use ethtool_rx_flow_rule() to remove duplicated parser code")
Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 drivers/net/ethernet/qlogic/qede/qede_filter.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index 25ef0f4258cb..377d661f70f7 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -2002,10 +2002,9 @@ static int qede_flow_spec_to_rule(struct qede_dev *edev,
 	if (IS_ERR(flow))
 		return PTR_ERR(flow);
 
-	if (qede_parse_flow_attr(edev, proto, flow->rule, t)) {
-		err = -EINVAL;
+	err = qede_parse_flow_attr(edev, proto, flow->rule, t);
+	if (err)
 		goto err_out;
-	}
 
 	/* Make sure location is valid and filter isn't already set */
 	err = qede_flow_spec_validate(edev, &flow->rule->action, t,
-- 
2.43.0

Re: [PATCH net 3/4] net: qede: use return from qede_parse_flow_attr() for flow_spec

[Simon Horman]

On Fri, Apr 26, 2024 at 09:12:25AM +0000, Asbjørn Sloth Tønnesen wrote:
> In qede_flow_spec_to_rule(), when calling
> qede_parse_flow_attr() then the return code
> was only used for a non-zero check, and then
> -EINVAL was returned.
> 
> qede_parse_flow_attr() can currently fail with:
> * -EINVAL
> * -EOPNOTSUPP
> * -EPROTONOSUPPORT
> 
> This patch changes the code to use the actual
> return code, not just return -EINVAL.
> 
> The blaimed commit introduced qede_flow_spec_to_rule(),
> and this call to qede_parse_flow_attr(), it looks
> like it just duplicated how it was already used.
> 
> Only compile tested.
> 
> Fixes: 37c5d3efd7f8 ("qede: use ethtool_rx_flow_rule() to remove duplicated parser code")
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net 4/4] net: qede: use return from qede_parse_actions()

[Asbjørn Sloth Tønnesen]

When calling qede_parse_actions() then the
return code was only used for a non-zero check,
and then -EINVAL was returned.

qede_parse_actions() can currently fail with:
* -EINVAL
* -EOPNOTSUPP

This patch changes the code to use the actual
return code, not just return -EINVAL.

The blaimed commit broke the implicit assumption
that only -EINVAL would ever be returned.

Only compile tested.

Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type")
Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 drivers/net/ethernet/qlogic/qede/qede_filter.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index 377d661f70f7..cb6b33a228ea 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -1894,10 +1894,9 @@ int qede_add_tc_flower_fltr(struct qede_dev *edev, __be16 proto,
 	}
 
 	/* parse tc actions and get the vf_id */
-	if (qede_parse_actions(edev, &f->rule->action, f->common.extack)) {
-		rc = -EINVAL;
+	rc = qede_parse_actions(edev, &f->rule->action, f->common.extack);
+	if (rc)
 		goto unlock;
-	}
 
 	if (qede_flow_find_fltr(edev, &t)) {
 		rc = -EEXIST;
-- 
2.43.0

Re: [PATCH net 4/4] net: qede: use return from qede_parse_actions()

[Simon Horman]

On Fri, Apr 26, 2024 at 09:12:26AM +0000, Asbjørn Sloth Tønnesen wrote:
> When calling qede_parse_actions() then the
> return code was only used for a non-zero check,
> and then -EINVAL was returned.
> 
> qede_parse_actions() can currently fail with:
> * -EINVAL
> * -EOPNOTSUPP
> 
> This patch changes the code to use the actual
> return code, not just return -EINVAL.
> 
> The blaimed commit broke the implicit assumption
> that only -EINVAL would ever be returned.
> 
> Only compile tested.
> 
> Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type")
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 0/4] net: qede: avoid overruling error codes

[Simon Horman]

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.

Also, if you do end posting a v2, blamed, is misspelt several
times in commit messages.

Re: [PATCH net 0/4] net: qede: avoid overruling error codes

[Asbjørn Sloth Tønnesen]

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.


> 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

Re: [PATCH net 0/4] net: qede: avoid overruling error codes

[Simon Horman]

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
> 

Re: [PATCH net 0/4] net: qede: avoid overruling error codes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri, 26 Apr 2024 09:12:22 +0000 you 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().)
> 
> [...]

Here is the summary with links:
  - [net,1/4] net: qede: sanitize 'rc' in qede_add_tc_flower_fltr()
    https://git.kernel.org/netdev/net/c/e25714466abd
  - [net,2/4] net: qede: use return from qede_parse_flow_attr() for flower
    https://git.kernel.org/netdev/net/c/fcee2065a178
  - [net,3/4] net: qede: use return from qede_parse_flow_attr() for flow_spec
    https://git.kernel.org/netdev/net/c/27b44414a34b
  - [net,4/4] net: qede: use return from qede_parse_actions()
    https://git.kernel.org/netdev/net/c/f26f719a36e5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html