Re: [RFC PATCHv2 net-next 3/3] ipv4/udp: Add 4-tuple hash for connected socket

[Philo Lu]

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