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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <3ce6dc2e-b474-49a9-992a-55a6c33a480b@nvidia.com>
Date: Wed, 7 Aug 2024 17:38:06 +0300
From: Gal Pressman <gal@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 Dragos Tatulea <dtatulea@...dia.com>, Jianbo Liu <jianbol@...dia.com>
Subject: Re: [PATCH net] ethtool: Fix context creation with no parameters

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.