netdev - Re: [PATCH v2 net-next 08/11] af_unix: Define locking order for U_RECVQ_LOCK_EMBRYO in unix_collect

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] [day] [month] [year] [list]

Message-ID: <20240614185554.86292-1-kuniyu@amazon.com>
Date: Fri, 14 Jun 2024 11:55:54 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <pabeni@...hat.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kent.overstreet@...ux.dev>,
	<kuba@...nel.org>, <kuni1840@...il.com>, <kuniyu@...zon.com>,
	<netdev@...r.kernel.org>
Subject: Re: [PATCH v2 net-next 08/11] af_unix: Define locking order for U_RECVQ_LOCK_EMBRYO in unix_collect_skb().

From: Paolo Abeni <pabeni@...hat.com>
Date: Fri, 14 Jun 2024 13:04:06 +0200
> On Tue, 2024-06-11 at 16:23 -0700, Kuniyuki Iwashima wrote:
> > From: Kent Overstreet <kent.overstreet@...ux.dev>
> > Date: Tue, 11 Jun 2024 19:17:53 -0400
> > > On Tue, Jun 11, 2024 at 03:29:02PM GMT, Kuniyuki Iwashima wrote:
> > > > While GC is cleaning up cyclic references by SCM_RIGHTS,
> > > > unix_collect_skb() collects skb in the socket's recvq.
> > > > 
> > > > If the socket is TCP_LISTEN, we need to collect skb in the
> > > > embryo's queue.  Then, both the listener's recvq lock and
> > > > the embroy's one are held.
> > > > 
> > > > The locking is always done in the listener -> embryo order.
> > > > 
> > > > Let's define it as unix_recvq_lock_cmp_fn() instead of using
> > > > spin_lock_nested().
> > > > 
> > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> > > > ---
> > > >  net/unix/af_unix.c | 17 +++++++++++++++++
> > > >  net/unix/garbage.c |  8 +-------
> > > >  2 files changed, 18 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> > > > index 8d03c5ef61df..8959ee8753d1 100644
> > > > --- a/net/unix/af_unix.c
> > > > +++ b/net/unix/af_unix.c
> > > > @@ -170,6 +170,21 @@ static int unix_state_lock_cmp_fn(const struct lockdep_map *_a,
> > > >  	/* unix_state_double_lock(): ascending address order. */
> > > >  	return cmp_ptr(a, b);
> > > >  }
> > > > +
> > > > +static int unix_recvq_lock_cmp_fn(const struct lockdep_map *_a,
> > > > +				  const struct lockdep_map *_b)
> > > > +{
> > > > +	const struct sock *a, *b;
> > > > +
> > > > +	a = container_of(_a, struct sock, sk_receive_queue.lock.dep_map);
> > > > +	b = container_of(_b, struct sock, sk_receive_queue.lock.dep_map);
> > > > +
> > > > +	/* unix_collect_skb(): listener -> embryo order. */
> > > > +	if (a->sk_state == TCP_LISTEN && unix_sk(b)->listener == a)
> > > > +		return -1;
> > > > +
> > > > +	return 0;
> > > > +}
> > > >  #endif
> > > 
> > > That's not symmetric.
> > 
> > I think we agreed this is allowed, no ?
> > 
> > https://lore.kernel.org/netdev/thzkgbuwuo3knevpipu4rzsh5qgmwhklihypdgziiruabvh46f@uwdkpcfxgloo/
> 
> My understanding of such thread is that you should return 1 for the
> embryo -> listener order (for consistency). You can keep returning 0
> for all the other 'undefined' cases.

Ah, I understood.  Will do so in v3.

Thanks!