netdev - Re: [PATCH net v2] tcp: Don't acquire inet_listen

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: <YbN+edCO3DGMDPmj@linutronix.de>
Date:   Fri, 10 Dec 2021 17:21:13 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Martin KaFai Lau <kafai@...com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Kuniyuki Iwashima <kuniyu@...zon.co.jp>,
        eric.dumazet@...il.com, davem@...emloft.net, dsahern@...nel.org,
        efault@....de, netdev@...r.kernel.org, tglx@...utronix.de,
        yoshfuji@...ux-ipv6.org
Subject: Re: [PATCH net v2] tcp: Don't acquire inet_listen_hashbucket::lock
 with disabled BH.

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@...com>

Sebastian