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

Commit Message

[e-mail] Nov. 13, 2024, 12:13 p.m. UTC

From: Edward Cree <[e-mail]>

Ethtool ntuple filters with FLOW_RSS were originally defined as adding
 the base queue ID (ring_cookie) to the value from the indirection table,
 so that the same table could distribute over more than one set of queues
 when used by different filters.
However, some drivers / hardware ignore the ring_cookie, and simply use
 the indirection table entries as queue IDs directly.  Thus, for drivers
 which have not opted in by setting ethtool_ops.cap_rss_rxnfc_adds to
 declare that they support the original (addition) semantics, reject in
 ethtool_set_rxnfc any filter which combines FLOW_RSS and a nonzero ring.
(For a ring_cookie of zero, both behaviours are equivalent.)
Set the cap bit in sfc, as it is known to support this feature.

Signed-off-by: Edward Cree <[e-mail]>
---
 drivers/net/ethernet/sfc/ef100_ethtool.c | 1 +
 drivers/net/ethernet/sfc/ethtool.c       | 1 +
 include/linux/ethtool.h                  | 4 ++++
 net/ethtool/ioctl.c                      | 5 +++++
 4 files changed, 11 insertions(+)

Comments

Martin Habets Nov. 14, 2024, 9:21 a.m. UTC | #1

On Wed, Nov 13, 2024 at 12:13:09PM +0000, [e-mail] wrote:
> 
> From: Edward Cree <[e-mail]>
> 
> Ethtool ntuple filters with FLOW_RSS were originally defined as adding
>  the base queue ID (ring_cookie) to the value from the indirection table,
>  so that the same table could distribute over more than one set of queues
>  when used by different filters.
> However, some drivers / hardware ignore the ring_cookie, and simply use
>  the indirection table entries as queue IDs directly.  Thus, for drivers
>  which have not opted in by setting ethtool_ops.cap_rss_rxnfc_adds to
>  declare that they support the original (addition) semantics, reject in
>  ethtool_set_rxnfc any filter which combines FLOW_RSS and a nonzero ring.
> (For a ring_cookie of zero, both behaviours are equivalent.)
> Set the cap bit in sfc, as it is known to support this feature.
> 
> Signed-off-by: Edward Cree <[e-mail]>

Reviewed-by: Martin Habets <[e-mail]>

> ---
>  drivers/net/ethernet/sfc/ef100_ethtool.c | 1 +
>  drivers/net/ethernet/sfc/ethtool.c       | 1 +
>  include/linux/ethtool.h                  | 4 ++++
>  net/ethtool/ioctl.c                      | 5 +++++
>  4 files changed, 11 insertions(+)
> 
> diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c
> index 5c2551369812..6c3b74000d3b 100644
> --- a/drivers/net/ethernet/sfc/ef100_ethtool.c
> +++ b/drivers/net/ethernet/sfc/ef100_ethtool.c
> @@ -59,6 +59,7 @@ const struct ethtool_ops ef100_ethtool_ops = {
>  	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
>  	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
>  	.rxfh_per_ctx_key	= true,
> +	.cap_rss_rxnfc_adds	= true,
>  	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
>  	.get_rxfh		= efx_ethtool_get_rxfh,
>  	.set_rxfh		= efx_ethtool_set_rxfh,
> diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
> index bb1930818beb..83d715544f7f 100644
> --- a/drivers/net/ethernet/sfc/ethtool.c
> +++ b/drivers/net/ethernet/sfc/ethtool.c
> @@ -263,6 +263,7 @@ const struct ethtool_ops efx_ethtool_ops = {
>  	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
>  	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
>  	.rxfh_per_ctx_key	= true,
> +	.cap_rss_rxnfc_adds	= true,
>  	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
>  	.get_rxfh		= efx_ethtool_get_rxfh,
>  	.set_rxfh		= efx_ethtool_set_rxfh,
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 1199e308c8dd..299280c94d07 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -734,6 +734,9 @@ struct kernel_ethtool_ts_info {
>   * @rxfh_per_ctx_key: device supports setting different RSS key for each
>   *	additional context. Netlink API should report hfunc, key, and input_xfrm
>   *	for every context, not just context 0.
> + * @cap_rss_rxnfc_adds: device supports nonzero ring_cookie in filters with
> + *	%FLOW_RSS flag; the queue ID from the filter is added to the value from
> + *	the indirection table to determine the delivery queue.
>   * @rxfh_indir_space: max size of RSS indirection tables, if indirection table
>   *	size as returned by @get_rxfh_indir_size may change during lifetime
>   *	of the device. Leave as 0 if the table size is constant.
> @@ -956,6 +959,7 @@ struct ethtool_ops {
>  	u32     cap_rss_ctx_supported:1;
>  	u32	cap_rss_sym_xor_supported:1;
>  	u32	rxfh_per_ctx_key:1;
> +	u32	cap_rss_rxnfc_adds:1;
>  	u32	rxfh_indir_space;
>  	u16	rxfh_key_space;
>  	u16	rxfh_priv_size;
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 7da94e26ced6..d86399bcf223 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -992,6 +992,11 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
>  	if (rc)
>  		return rc;
>  
> +	/* Nonzero ring with RSS only makes sense if NIC adds them together */
> +	if (info.flow_type & FLOW_RSS && !ops->cap_rss_rxnfc_adds &&
> +	    ethtool_get_flow_spec_ring(info.fs.ring_cookie))
> +		return -EINVAL;
> +
>  	if (ops->get_rxfh) {
>  		struct ethtool_rxfh_param rxfh = {};
>

Gal Pressman Nov. 25, 2024, 7:11 a.m. UTC | #2

Hi Edward,

On 13/11/2024 14:13, [e-mail] wrote:
> From: Edward Cree <[e-mail]>
> 
> Ethtool ntuple filters with FLOW_RSS were originally defined as adding
>  the base queue ID (ring_cookie) to the value from the indirection table,
>  so that the same table could distribute over more than one set of queues
>  when used by different filters.

TBH, I'm not sure I understand the difference? Perhaps you can share an
example?

> However, some drivers / hardware ignore the ring_cookie, and simply use
>  the indirection table entries as queue IDs directly.  Thus, for drivers
>  which have not opted in by setting ethtool_ops.cap_rss_rxnfc_adds to
>  declare that they support the original (addition) semantics, reject in
>  ethtool_set_rxnfc any filter which combines FLOW_RSS and a nonzero ring.
> (For a ring_cookie of zero, both behaviours are equivalent.)
> Set the cap bit in sfc, as it is known to support this feature.
> 
> Signed-off-by: Edward Cree <[e-mail]>
> ---
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 7da94e26ced6..d86399bcf223 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -992,6 +992,11 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
>  	if (rc)
>  		return rc;
>  
> +	/* Nonzero ring with RSS only makes sense if NIC adds them together */
> +	if (info.flow_type & FLOW_RSS && !ops->cap_rss_rxnfc_adds &&
> +	    ethtool_get_flow_spec_ring(info.fs.ring_cookie))
> +		return -EINVAL;

I believe this check shouldn't happen when we do ETHTOOL_SRXCLSRLDEL as
flow_type is garbage, WDYT?

> +
>  	if (ops->get_rxfh) {
>  		struct ethtool_rxfh_param rxfh = {};
>  
>

Edward Cree Nov. 25, 2024, 1:21 p.m. UTC | #3

On 25/11/2024 07:11, Gal Pressman wrote:
> On 13/11/2024 14:13, [e-mail] wrote:
>> Ethtool ntuple filters with FLOW_RSS were originally defined as adding
>>  the base queue ID (ring_cookie) to the value from the indirection table,
>>  so that the same table could distribute over more than one set of queues
>>  when used by different filters.
> 
> TBH, I'm not sure I understand the difference? Perhaps you can share an
> example?

Something like this:

ethtool -X $intf context new equal 2
# creates context ID 1, table filled with 0s and 1s
ethtool -N $intf <match fields...> context 1
# filter distributes traffic to queues 0 and 1
ethtool -N $intf <match fields...> context 1 action 2
# filter distributes traffic to queues 2 and 3

See the selftest in patch 4 for a concrete example of this.
Some NICs were apparently sending the traffic from both filters to
 queues 0 and 1, and ignoring the 'action 2' on the second filter.

>> @@ -992,6 +992,11 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
>>  	if (rc)
>>  		return rc;
>>  
>> +	/* Nonzero ring with RSS only makes sense if NIC adds them together */
>> +	if (info.flow_type & FLOW_RSS && !ops->cap_rss_rxnfc_adds &&
>> +	    ethtool_get_flow_spec_ring(info.fs.ring_cookie))
>> +		return -EINVAL;
> 
> I believe this check shouldn't happen when we do ETHTOOL_SRXCLSRLDEL as
> flow_type is garbage, WDYT?

Agreed; this check should only apply to ETHTOOL_SRXCLSRLINS.  Do you want
 to send the fix or shall I?

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?

Gal Pressman Nov. 25, 2024, 2:10 p.m. UTC | #4

On 25/11/2024 15:21, Edward Cree wrote:
> On 25/11/2024 07:11, Gal Pressman wrote:
>> On 13/11/2024 14:13, [e-mail] wrote:
>>> Ethtool ntuple filters with FLOW_RSS were originally defined as adding
>>>  the base queue ID (ring_cookie) to the value from the indirection table,
>>>  so that the same table could distribute over more than one set of queues
>>>  when used by different filters.
>>
>> TBH, I'm not sure I understand the difference? Perhaps you can share an
>> example?
> 
> Something like this:
> 
> ethtool -X $intf context new equal 2
> # creates context ID 1, table filled with 0s and 1s
> ethtool -N $intf <match fields...> context 1
> # filter distributes traffic to queues 0 and 1
> ethtool -N $intf <match fields...> context 1 action 2
> # filter distributes traffic to queues 2 and 3
> 
> See the selftest in patch 4 for a concrete example of this.
> Some NICs were apparently sending the traffic from both filters to
>  queues 0 and 1, and ignoring the 'action 2' on the second filter.

Thanks, I did not know it works that way, is it actually documented
anywhere?

> 
>>> @@ -992,6 +992,11 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
>>>  	if (rc)
>>>  		return rc;
>>>  
>>> +	/* Nonzero ring with RSS only makes sense if NIC adds them together */
>>> +	if (info.flow_type & FLOW_RSS && !ops->cap_rss_rxnfc_adds &&
>>> +	    ethtool_get_flow_spec_ring(info.fs.ring_cookie))
>>> +		return -EINVAL;
>>
>> I believe this check shouldn't happen when we do ETHTOOL_SRXCLSRLDEL as
>> flow_type is garbage, WDYT?
> 
> Agreed; this check should only apply to ETHTOOL_SRXCLSRLINS.  Do you want
>  to send the fix or shall I?

I will do it.

> 
> 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:

1. Request hash on src only
2. Enable symmetric xor
= Success :(.

I've been thinking of improving the situation, but that requires
iterating over all flow types on symmetric xor enablement and that feels
quite bad..

Ahmed Zaki Nov. 25, 2024, 2:20 p.m. UTC | #5

On 2024-11-25 7:10 a.m., Gal Pressman wrote:
> On 25/11/2024 15:21, Edward Cree wrote:
>> On 25/11/2024 07:11, Gal Pressman wrote:
>>> On 13/11/2024 14:13, [e-mail] wrote:
>>>> Ethtool ntuple filters with FLOW_RSS were originally defined as adding
>>>>   the base queue ID (ring_cookie) to the value from the indirection table,
>>>>   so that the same table could distribute over more than one set of queues
>>>>   when used by different filters.
>>>
>>> TBH, I'm not sure I understand the difference? Perhaps you can share an
>>> example?
>>
>> Something like this:
>>
>> ethtool -X $intf context new equal 2
>> # creates context ID 1, table filled with 0s and 1s
>> ethtool -N $intf <match fields...> context 1
>> # filter distributes traffic to queues 0 and 1
>> ethtool -N $intf <match fields...> context 1 action 2
>> # filter distributes traffic to queues 2 and 3
>>
>> See the selftest in patch 4 for a concrete example of this.
>> Some NICs were apparently sending the traffic from both filters to
>>   queues 0 and 1, and ignoring the 'action 2' on the second filter.
> 
> Thanks, I did not know it works that way, is it actually documented
> anywhere?
> 
>>
>>>> @@ -992,6 +992,11 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
>>>>   	if (rc)
>>>>   		return rc;
>>>>   
>>>> +	/* Nonzero ring with RSS only makes sense if NIC adds them together */
>>>> +	if (info.flow_type & FLOW_RSS && !ops->cap_rss_rxnfc_adds &&
>>>> +	    ethtool_get_flow_spec_ring(info.fs.ring_cookie))
>>>> +		return -EINVAL;
>>>
>>> I believe this check shouldn't happen when we do ETHTOOL_SRXCLSRLDEL as
>>> flow_type is garbage, WDYT?
>>
>> Agreed; this check should only apply to ETHTOOL_SRXCLSRLINS.  Do you want
>>   to send the fix or shall I?
> 
> I will do it.
> 
>>
>> 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.

> 
> 1. Request hash on src only
> 2. Enable symmetric xor
> = Success :(.
> 
> I've been thinking of improving the situation, but that requires
> iterating over all flow types on symmetric xor enablement and that feels
> quite bad..

and delete/disable filters? may be just a warning to the user that some 
filters are not symmetric.

Gal Pressman Nov. 25, 2024, 2:26 p.m. UTC | #6

On 25/11/2024 16:20, Ahmed Zaki wrote:
> 
> 
> On 2024-11-25 7:10 a.m., Gal Pressman wrote:
>> On 25/11/2024 15:21, Edward Cree wrote:
>>> On 25/11/2024 07:11, Gal Pressman wrote:
>>>> On 13/11/2024 14:13, [e-mail] wrote:
>>>>> Ethtool ntuple filters with FLOW_RSS were originally defined as adding
>>>>>   the base queue ID (ring_cookie) to the value from the indirection
>>>>> table,
>>>>>   so that the same table could distribute over more than one set of
>>>>> queues
>>>>>   when used by different filters.
>>>>
>>>> TBH, I'm not sure I understand the difference? Perhaps you can share an
>>>> example?
>>>
>>> Something like this:
>>>
>>> ethtool -X $intf context new equal 2
>>> # creates context ID 1, table filled with 0s and 1s
>>> ethtool -N $intf <match fields...> context 1
>>> # filter distributes traffic to queues 0 and 1
>>> ethtool -N $intf <match fields...> context 1 action 2
>>> # filter distributes traffic to queues 2 and 3
>>>
>>> See the selftest in patch 4 for a concrete example of this.
>>> Some NICs were apparently sending the traffic from both filters to
>>>   queues 0 and 1, and ignoring the 'action 2' on the second filter.
>>
>> Thanks, I did not know it works that way, is it actually documented
>> anywhere?
>>
>>>
>>>>> @@ -992,6 +992,11 @@ static noinline_for_stack int
>>>>> ethtool_set_rxnfc(struct net_device *dev,
>>>>>       if (rc)
>>>>>           return rc;
>>>>>   +    /* Nonzero ring with RSS only makes sense if NIC adds them
>>>>> together */
>>>>> +    if (info.flow_type & FLOW_RSS && !ops->cap_rss_rxnfc_adds &&
>>>>> +        ethtool_get_flow_spec_ring(info.fs.ring_cookie))
>>>>> +        return -EINVAL;
>>>>
>>>> I believe this check shouldn't happen when we do ETHTOOL_SRXCLSRLDEL as
>>>> flow_type is garbage, WDYT?
>>>
>>> Agreed; this check should only apply to ETHTOOL_SRXCLSRLINS.  Do you
>>> want
>>>   to send the fix or shall I?
>>
>> I will do it.
>>
>>>
>>> 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.
> 
>>
>> 1. Request hash on src only
>> 2. Enable symmetric xor
>> = Success :(.
>>
>> I've been thinking of improving the situation, but that requires
>> iterating over all flow types on symmetric xor enablement and that feels
>> quite bad..
> 
> and delete/disable filters? may be just a warning to the user that some
> filters are not symmetric.

I think the right thing to do in that case is fail the symmetric xor
enablement, but do we really want to query the driver for all flow types
and check if an asymmetric filter exists?

Edward Cree Nov. 25, 2024, 2:42 p.m. UTC | #7

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.
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)`.

Edward Cree Nov. 25, 2024, 6:13 p.m. UTC | #8

On 25/11/2024 14:10, Gal Pressman wrote:
> Thanks, I did not know it works that way, is it actually documented
> anywhere?

Yes; the struct ethtool_rxnfc kerneldoc has:
 * For %ETHTOOL_SRXCLSRLINS, @fs specifies the rule to add or update.
 * @fs.@location either specifies the location to use or is a special
 * location value with %RX_CLS_LOC_SPECIAL flag set.  On return,
 * @fs.@location is the actual rule location.  If @fs.@flow_type
 * includes the %FLOW_RSS flag, @rss_context is the RSS context ID to
 * use for flow spreading traffic which matches this rule.  The value
 * from the rxfh indirection table will be added to @fs.@ring_cookie
 * to choose which ring to deliver to.

I am also preparing a patch to add this info to the ethtool man page.

Ahmed Zaki Nov. 25, 2024, 7:01 p.m. UTC | #9

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.

Ahmed Zaki Dec. 2, 2024, 3:19 p.m. UTC | #10

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.

Patch

diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c
index 5c2551369812..6c3b74000d3b 100644
--- a/drivers/net/ethernet/sfc/ef100_ethtool.c
+++ b/drivers/net/ethernet/sfc/ef100_ethtool.c
@@ -59,6 +59,7 @@  const struct ethtool_ops ef100_ethtool_ops = {
 	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
 	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
 	.rxfh_per_ctx_key	= true,
+	.cap_rss_rxnfc_adds	= true,
 	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
 	.get_rxfh		= efx_ethtool_get_rxfh,
 	.set_rxfh		= efx_ethtool_set_rxfh,
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index bb1930818beb..83d715544f7f 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -263,6 +263,7 @@  const struct ethtool_ops efx_ethtool_ops = {
 	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
 	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
 	.rxfh_per_ctx_key	= true,
+	.cap_rss_rxnfc_adds	= true,
 	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
 	.get_rxfh		= efx_ethtool_get_rxfh,
 	.set_rxfh		= efx_ethtool_set_rxfh,
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 1199e308c8dd..299280c94d07 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -734,6 +734,9 @@  struct kernel_ethtool_ts_info {
  * @rxfh_per_ctx_key: device supports setting different RSS key for each
  *	additional context. Netlink API should report hfunc, key, and input_xfrm
  *	for every context, not just context 0.
+ * @cap_rss_rxnfc_adds: device supports nonzero ring_cookie in filters with
+ *	%FLOW_RSS flag; the queue ID from the filter is added to the value from
+ *	the indirection table to determine the delivery queue.
  * @rxfh_indir_space: max size of RSS indirection tables, if indirection table
  *	size as returned by @get_rxfh_indir_size may change during lifetime
  *	of the device. Leave as 0 if the table size is constant.
@@ -956,6 +959,7 @@  struct ethtool_ops {
 	u32     cap_rss_ctx_supported:1;
 	u32	cap_rss_sym_xor_supported:1;
 	u32	rxfh_per_ctx_key:1;
+	u32	cap_rss_rxnfc_adds:1;
 	u32	rxfh_indir_space;
 	u16	rxfh_key_space;
 	u16	rxfh_priv_size;
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 7da94e26ced6..d86399bcf223 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -992,6 +992,11 @@  static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
 	if (rc)
 		return rc;
 
+	/* Nonzero ring with RSS only makes sense if NIC adds them together */
+	if (info.flow_type & FLOW_RSS && !ops->cap_rss_rxnfc_adds &&
+	    ethtool_get_flow_spec_ring(info.fs.ring_cookie))
+		return -EINVAL;
+
 	if (ops->get_rxfh) {
 		struct ethtool_rxfh_param rxfh = {};