netdev - Re: [RFC PATCHv2 net-next 1/3] net/udp: Add a new struct for hash2 slot

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: <6c50d750-5a7c-4ddb-9415-17d0a963dbc9@linux.alibaba.com>
Date: Wed, 25 Sep 2024 10:43:10 +0800
From: Philo Lu <lulie@...ux.alibaba.com>
To: Gur Stavi <gur.stavi@...wei.com>
Cc: antony.antony@...unet.com, davem@...emloft.net, dsahern@...nel.org,
 dust.li@...ux.alibaba.com, edumazet@...gle.com, fred.cc@...baba-inc.com,
 jakub@...udflare.com, kuba@...nel.org, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org, pabeni@...hat.com, steffen.klassert@...unet.com,
 willemdebruijn.kernel@...il.com, yubing.qiuyubing@...baba-inc.com
Subject: Re: [RFC PATCHv2 net-next 1/3] net/udp: Add a new struct for hash2
 slot


On 2024/9/24 20:30, Gur Stavi wrote:
>> + *	@hslot:	basic hash slot
>> + *	@hash4_cnt: number of sockets in hslot4 of the same (local port, local address)
>> + */
>> +struct udp_hslot_main {
>> +	struct udp_hslot	hslot; /* must be the first member */
>> +	u32			hash4_cnt;
>> +} __aligned(2 * sizeof(long));
>> +#define UDP_HSLOT_MAIN(__hslot) ((struct udp_hslot_main *)(__hslot))
> 
> Wouldn't container_of be more suitable than brutal cast?

I think struct udp_hslot_main is like a subclass of struct udp_hslot, so 
casting is reasonable here.

> 
>> @@ -91,7 +110,7 @@ static inline struct udp_hslot *udp_hashslot(struct udp_table *table,
>>   static inline struct udp_hslot *udp_hashslot2(struct udp_table *table,
>>   					      unsigned int hash)
>>   {
>> -	return &table->hash2[hash & table->mask];
>> +	return (struct udp_hslot *)udp_hashslot2_main(table, hash);
> 
> Why cast and not use ->hslot. Preferably with a local variable?

Got it. Will fix.

> 
>> @@ -3438,16 +3436,17 @@ void __init udp_table_init(struct udp_table *table, const char *name)
>>   					      UDP_HTABLE_SIZE_MIN,
>>   					      UDP_HTABLE_SIZE_MAX);
>>
>> -	table->hash2 = table->hash + (table->mask + 1);
>> +	table->hash2 = UDP_HSLOT_MAIN(table->hash + (table->mask + 1));
> 
> Wouldn't it be simpler to just cast to void? It is pure pointer
> arithmetic where type checking is meaningless.
> (void *)(table->hash + (table->mask + 1))
> I realize now why UDP_HSLOT_MAIN couldn't use container_of but it
> just demonstrates how convoluted this is.

Will fix. I agree that (void *) is better here.

Thank you for your review, Gur.

-- 
Philo