Messages in this thread | | | Date | Sat, 27 Apr 2024 12:58:38 +0000 | Subject | Re: [PATCH net 0/4] net: qede: avoid overruling error codes | From | 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
| |