netdev - Re: [PATCH net-next 1/5] net: ethtool: only allow set_rxnfc with rss + ring

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4139c530-d75c-4e39-974f-514b552a7082@intel.com>
Date: Mon, 2 Dec 2024 08:19:52 -0700
From: Ahmed Zaki <ahmed.zaki@...el.com>
To: Edward Cree <ecree.xilinx@...il.com>, Gal Pressman <gal@...dia.com>,
	<edward.cree@....com>, <davem@...emloft.net>, <kuba@...nel.org>,
	<edumazet@...gle.com>, <pabeni@...hat.com>, "Samudrala, Sridhar"
	<sridhar.samudrala@...el.com>
CC: <netdev@...r.kernel.org>, <habetsm.xilinx@...il.com>,
	<linux-net-drivers@....com>, <horms@...nel.org>, <andrew+netdev@...n.ch>,
	<shuah@...nel.org>, <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH net-next 1/5] net: ethtool: only allow set_rxnfc with rss
 + ring_cookie if driver opts in



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.