Re: [PATCH net-next 1/5] net: ethtool: only allow set_rxnfc with rss + ring_cookie if driver opts in

[Ahmed Zaki <ahmed.zaki@intel.com>]

On 2024-11-25 12:01 p.m., Ahmed Zaki wrote:
> 
> 
> On 2024-11-25 7:42 a.m., Edward Cree wrote:
>> On 25/11/2024 14:20, Ahmed Zaki wrote:
>>> On 2024-11-25 7:10 a.m., Gal Pressman wrote:
>>>> On 25/11/2024 15:21, Edward Cree wrote:
>>>>> Also, the check below it, dealing with sym-xor, looks like it's only
>>>>>    relevant to ETHTOOL_SRXFH, since info.data is garbage for other 
>>>>> commands.
>>>>>    Ahmed, is my understanding correct there?
>>>>>
>>>>
>>>> Speaking of the below check, the sanity check depends on the order of
>>>> operations, for example:
>>>> 1. Enable symmetric xor
>>>> 2. Request hash on src only
>>>> = Error as expected, however:
>>>
>>> Correct. The check below is to make sure that no ntuple that does not 
>>> cover symmetric fields is added if symm-xor is enabled.
>> But symm-xor is about hashing, and is only relevant to traffic being
>>   directed by RSS.  The user should still be allowed to, and the NIC
>>   should be able to handle, setting an ntuple filter (SRXCLSRLINS)
>>   that is asymmetric, to override the symmetric hashing for selected
>>   traffic.
> 
> I agree, and in its first version, the sym-xor series was setting sym- 
> xor per ntuple, not per netdev. So the NIC can support different RSS 
> functions for different filters. Unfortunately, this was then changed to 
> be per-netdev during review. At that point, these checks were added in 
> nxfc path.
> 
>> symm-xor should only constrain RXFH settings.  And in fact even if
>>   you wanted to block asymm ntuple filters, the current code does not
>>   do that, since the info.data fields it looks at aren't populated for
>>   ntuple filters (whose filter fields are defined by info.fs).
>> So the xfrm check should be under `if (info.cmd == ETHTOOL_SRXFH)`.
> 
> If it is not, then it is a bug. I will try to test later this week and 
> send a fix if needed.
> 

Sorry, I misunderstood your original question. The check was actually 
intended for "rx-flow-hash":

# ethtool -X eth0 xfrm symmetric-xor
# ethtool -N eth0 rx-flow-hash udp4 sdf
Cannot change RX network flow hashing options: Invalid argument

and the NICs that support symm-xor do not use RSS on ntuple filters. So 
you are right, we should:


--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -997,7 +997,7 @@ static noinline_for_stack int 
ethtool_set_rxnfc(struct net_device *dev,
             ethtool_get_flow_spec_ring(info.fs.ring_cookie))
                 return -EINVAL;

-       if (ops->get_rxfh) {
+       if (info.cmd == ETHTOOL_SRXFH && ops->get_rxfh) {
                 struct ethtool_rxfh_param rxfh = {};

                 rc = ops->get_rxfh(dev, &rxfh);


Let me know if you want me to send this.

Thanks.