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

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

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/