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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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.
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
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
>
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 <[email protected]>
> ---
> drivers/net/ethernet/qlogic/qede/qede_filter.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
Reviewed-by: Simon Horman <[email protected]>
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 <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
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 <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
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 <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Hello:
This series was applied to netdev/net.git (main)
by David S. Miller <[email protected]>:
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