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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 10 Dec 2021 13:22:39 -0800
From:   Martin KaFai Lau <kafai@...com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
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 Fri, Dec 10, 2021 at 05:21:13PM +0100, Sebastian Andrzej Siewior wrote:
> 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…
Yes, they are different locks.  ilb2->lock is also taken
in the inet_listen() path.  ilb->lock is not even taken
in the listening_get_next() side.