Re: [PATCH net v2] tcp: Don't acquire inet_listen_hashbucket::lock with disabled BH.

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2021-12-09 12:06:32 [-0800], Martin KaFai Lau wrote:
> > local_bh_disable() + spin_lock(&ilb->lock):
> >   inet_listen()
> >     inet_csk_listen_start()
> >       sk->sk_prot->hash() := inet_hash()
> > 	local_bh_disable()
> > 	__inet_hash()
> > 	  spin_lock(&ilb->lock);
> > 	    acquire(&ilb->lock);
> > 
> > Reverse order: spin_lock(&ilb->lock) + local_bh_disable():
> >   tcp_seq_next()
> >     listening_get_next()
> >       spin_lock(&ilb->lock);
> The net tree has already been using ilb2 instead of ilb.
> It does not change the problem though but updating
> the commit log will be useful to avoid future confusion.

You think so? But having ilb2 and ilb might suggest that these two are
different locks while they are the same. I could repost it early next
week if you this actually confuses more…

> iiuc, established_get_next() does not hit this because
> it calls spin_lock_bh() which then keeps the
> local_bh_disable() => spin_lock() ordering?

Yes. BH was disabled before the spin_lock was acquired so it is the same
ordering. The important part is not to disable BH after the spin_lock
was acquired and in the reverse order.

> The patch lgtm.
> Acked-by: Martin KaFai Lau <kafai@fb.com>

Sebastian