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]

Date: Tue, 11 Jun 2024 19:17:53 -0400
From: Kent Overstreet <kent.overstreet@...ux.dev>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Kuniyuki Iwashima <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, 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.