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: <fe71aefbd50babf1af7c4719f5581e46fce2b4fc.camel@redhat.com>
Date: Fri, 14 Jun 2024 13:04:06 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>, kent.overstreet@...ux.dev
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 kuni1840@...il.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().

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.

Thanks!

Paolo