netdev - Re: [PATCH net-next 02/10] eth: fbnic: support querying RSS config

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: <51452dd7-a574-4bf7-9b5f-4939f7f189e4@intel.com>
Date: Fri, 20 Dec 2024 15:23:30 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: Alexander Duyck <alexanderduyck@...com>, <netdev@...r.kernel.org>,
	<edumazet@...gle.com>, <davem@...emloft.net>, <pabeni@...hat.com>
Subject: Re: [PATCH net-next 02/10] eth: fbnic: support querying RSS config

On 12/20/24 15:08, Jakub Kicinski wrote:
> On Fri, 20 Dec 2024 12:42:42 +0100 Przemek Kitszel wrote:

with [1] this patch looks good to me, thank you for explanation:
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>

the rest of the series is also fine for me, but I didn't spend that
much time to inflate my RB count

>>> +static int
>>> +fbnic_get_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rxfh)
>>> +{
>>> +	struct fbnic_net *fbn = netdev_priv(netdev);
>>> +	unsigned int i;
>>
>> AFAIK index type should be spelled as u32
> 
> Does it matter? I have a weak preference for not using explicitly sized
> types unless the bit width is itself meaningful.

me too, unless it is "unsiged int", but I', not gonna fight for it

I would say that plain int is fine for iterator unless you want to
explicitly get the non-UB wrap-around (or count more than 31 bit wide).
But again, does not matter

> 
>> And will be best declared in the first clause of the for()
> 
> I don't see woohaii.

with such long to type types, it would be inconvenient, but the usual
argument about the proper scope of the variable

> 
>>> +
>>> +	rxfh->hfunc = ETH_RSS_HASH_TOP;
>>> +
>>> +	if (rxfh->key) {
>>> +		for (i = 0; i < FBNIC_RPC_RSS_KEY_BYTE_LEN; i++) {
>>> +			u32 rss_key = fbn->rss_key[i / 4] << ((i % 4) * 8);
>>
>> are you dropping 75% of entropy provided in fbn->rss_key?
> 
> Nope, it's shifting out the unused bits. And below we shift back
> down to the lowest byte.
> We store the key as u32 (register width) while the uAPI is in u8.

[1]
OK, I've double checked and the fbn->rss_key array has indeed
FBNIC_RPC_RSS_KEY_BYTE_LEN/4 entries


> 
>>> +
>>> +			rxfh->key[i] = rss_key >> 24;
>>> +		}
>>> +	}
>>> +
>>> +	if (rxfh->indir) {
>>> +		for (i = 0; i < FBNIC_RPC_RSS_TBL_SIZE; i++)
>>> +			rxfh->indir[i] = fbn->indir_tbl[0][i];
>>> +	}
>>> +
>>> +	return 0;