[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <0fb425e0-5482-4cdf-9dc1-3906751f8f81@linux.alibaba.com>
Date: Wed, 25 Sep 2024 11:16:53 +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 3/3] ipv4/udp: Add 4-tuple hash for
connected socket
On 2024/9/24 20:31, Gur Stavi wrote:
>> +/* In hash4, rehash can also happen in connect(), where hash4_cnt keeps unchanged. */
>> +static void udp4_rehash4(struct udp_table *udptable, struct sock *sk, u16 newhash4)
>> +{
>> + struct udp_hslot *hslot4, *nhslot4;
>> +
>> + hslot4 = udp_hashslot4(udptable, udp_sk(sk)->udp_lrpa_hash);
>> + nhslot4 = udp_hashslot4(udptable, newhash4);
>> + udp_sk(sk)->udp_lrpa_hash = newhash4;
>> +
>> + if (hslot4 != nhslot4) {
>> + spin_lock_bh(&hslot4->lock);
>> + hlist_del_init_rcu(&udp_sk(sk)->udp_lrpa_node);
>> + hslot4->count--;
>> + spin_unlock_bh(&hslot4->lock);
>
> I realize this is copied from udp_lib_rehash, but isn't it an RCU bug?
> Once a node is removed from a list, shouldn't synchronize_rcu be called
> before it is reused for a new list? A reader that was traversing the
> old list may find itself on the new list.
>
>> +
>> + spin_lock_bh(&nhslot4->lock);
>> + hlist_add_head_rcu(&udp_sk(sk)->udp_lrpa_node, &nhslot4->head);
>> + nhslot4->count++;
>> + spin_unlock_bh(&nhslot4->lock);
>> + }
>> +}
>> +
Good catch! IIUC, synchronize_rcu() is needed here, or otherwise, this
could happen:
Reader(lookup) Writer(rehash)
----------------- ---------------
1. rcu_read_lock()
2. pos = sk;
3. hlist_del_init_rcu(sk, old_slot)
4. hlist_add_head_rcu(sk, new_slot)
5. pos = pos->next; <=
6. rcu_read_unlock()
In step 5, we wrongly moved from old_slot to new_slot.
Perhaps the similar codes in udp_lib_rehash() for hslot2 also need a fix.
Thanks.
--
Philo
Powered by blists - more mailing lists