[PATCH net] ethtool: Fix context creation with no parameters

[PATCH net] ethtool: Fix context creation with no parameters

[Gal Pressman]

The 'at least one change' requirement is not applicable for context
creation, skip the check in such case.
This allows a command such as 'ethtool -X eth0 context new' to work.

The command works by mistake when using older versions of userspace
ethtool due to an incompatibility issue where rxfh.input_xfrm is passed
as zero (unset) instead of RXH_XFRM_NO_CHANGE as done with recent
userspace. This patch does not try to solve the incompatibility issue.

Link: https://lore.kernel.org/netdev/05ae8316-d3aa-4356-98c6-55ed4253c8a7@nvidia.com/
Fixes: 84a1d9c48200 ("net: ethtool: extend RXNFC API to support RSS spreading of filter matches")
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
Signed-off-by: Gal Pressman <gal@nvidia.com>
---
 net/ethtool/ioctl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 8ca13208d240..2fdbdcfa1506 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1372,14 +1372,15 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	/* If either indir, hash key or function is valid, proceed further.
 	 * Must request at least one change: indir size, hash key, function
 	 * or input transformation.
+	 * There's no need for any of it in case of context creation.
 	 */
-	if ((rxfh.indir_size &&
+	if (!create && ((rxfh.indir_size &&
 	     rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE &&
 	     rxfh.indir_size != dev_indir_size) ||
 	    (rxfh.key_size && (rxfh.key_size != dev_key_size)) ||
 	    (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE &&
 	     rxfh.key_size == 0 && rxfh.hfunc == ETH_RSS_HASH_NO_CHANGE &&
-	     rxfh.input_xfrm == RXH_XFRM_NO_CHANGE))
+	     rxfh.input_xfrm == RXH_XFRM_NO_CHANGE)))
 		return -EINVAL;
 
 	indir_bytes = dev_indir_size * sizeof(rxfh_dev.indir[0]);
-- 
2.40.1

Re: [PATCH net] ethtool: Fix context creation with no parameters

[Jakub Kicinski]

At a glance I think you popped it into the wrong place.

On Wed, 7 Aug 2024 16:25:41 +0300 Gal Pressman wrote:
> -	if ((rxfh.indir_size &&
> +	if (!create && ((rxfh.indir_size &&
>  	     rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE &&
>  	     rxfh.indir_size != dev_indir_size) ||

This condition just checks if indir_size matches the device
expectations, is reset or is zero. Even if we're creating the
context, at present, the indir table size is fixed for the device.

>  	    (rxfh.key_size && (rxfh.key_size != dev_key_size)) ||

similarly this checks the key size

>  	    (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE &&
>  	     rxfh.key_size == 0 && rxfh.hfunc == ETH_RSS_HASH_NO_CHANGE &&
> -	     rxfh.input_xfrm == RXH_XFRM_NO_CHANGE))
> +	     rxfh.input_xfrm == RXH_XFRM_NO_CHANGE)))

only this validates the "is this a nop", so you gotta add the &&
!create here

That's why I (perhaps not very clearly) suggested that we should split
this if into two. Cause the first two conditions check "sizes" while
the last chunk checks for "nop". And readability suffers.

Feel free to send the new version tomorrow without a full 24h wait.

Re: [PATCH net] ethtool: Fix context creation with no parameters

[Gal Pressman]

On 07/08/2024 17:21, Jakub Kicinski wrote:
> At a glance I think you popped it into the wrong place.
> 
> On Wed, 7 Aug 2024 16:25:41 +0300 Gal Pressman wrote:
>> -	if ((rxfh.indir_size &&
>> +	if (!create && ((rxfh.indir_size &&
>>  	     rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE &&
>>  	     rxfh.indir_size != dev_indir_size) ||
> 
> This condition just checks if indir_size matches the device
> expectations, is reset or is zero. Even if we're creating the
> context, at present, the indir table size is fixed for the device.
> 
>>  	    (rxfh.key_size && (rxfh.key_size != dev_key_size)) ||
> 
> similarly this checks the key size
> 
>>  	    (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE &&
>>  	     rxfh.key_size == 0 && rxfh.hfunc == ETH_RSS_HASH_NO_CHANGE &&
>> -	     rxfh.input_xfrm == RXH_XFRM_NO_CHANGE))
>> +	     rxfh.input_xfrm == RXH_XFRM_NO_CHANGE)))
> 
> only this validates the "is this a nop", so you gotta add the &&
> !create here

Indeed, I was too concentrated on shutting up this check.

> 
> That's why I (perhaps not very clearly) suggested that we should split
> this if into two. Cause the first two conditions check "sizes" while
> the last chunk checks for "nop". And readability suffers.

You were clear, I was hoping to split it into steps/patches.

> 
> Feel free to send the new version tomorrow without a full 24h wait.
Thanks for the review, I'll try to send it tomorrow.

Re: [PATCH net] ethtool: Fix context creation with no parameters

[Gal Pressman]

On 07/08/2024 16:25, Gal Pressman wrote:
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 8ca13208d240..2fdbdcfa1506 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1372,14 +1372,15 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	/* If either indir, hash key or function is valid, proceed further.

This comment is wrong, the check doesn't really verify the hash function
is valid. I'll remove it in my fix unless you have any objections.

Not verifying the hash function is probably another bug, but it's too
late to fix it at this point?

>  	 * Must request at least one change: indir size, hash key, function
>  	 * or input transformation.
> +	 * There's no need for any of it in case of context creation.
>  	 */

Re: [PATCH net] ethtool: Fix context creation with no parameters

[Jakub Kicinski]

On Wed, 7 Aug 2024 19:14:57 +0300 Gal Pressman wrote:
> > @@ -1372,14 +1372,15 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
> >  	/* If either indir, hash key or function is valid, proceed further.  
> 
> This comment is wrong, the check doesn't really verify the hash function
> is valid. I'll remove it in my fix unless you have any objections.
> 
> Not verifying the hash function is probably another bug, but it's too
> late to fix it at this point?

Let's do that separately in net-next?
Old ethtool had a bit of a "forward compatibility" mindset,
so it may have been intentional.

Re: [PATCH net] ethtool: Fix context creation with no parameters

[Gal Pressman]

On 07/08/2024 19:14, Gal Pressman wrote:
> On 07/08/2024 16:25, Gal Pressman wrote:
>> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
>> index 8ca13208d240..2fdbdcfa1506 100644
>> --- a/net/ethtool/ioctl.c
>> +++ b/net/ethtool/ioctl.c
>> @@ -1372,14 +1372,15 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>>  	/* If either indir, hash key or function is valid, proceed further.
> 
> This comment is wrong, the check doesn't really verify the hash function
> is valid. I'll remove it in my fix unless you have any objections.

Either this comment is horribly wrong or it's too late for me to look at
code..
The code doesn't proceed if either indir or key are valid, it returns if
either of them are invalid, I'm removing this comment altogether..