From: Kent Overstreet <kent.overstreet@xxxxxxxxx>
Date: Fri, 26 Jan 2024 21:08:31 -0500
> Kill
> - unix_state_lock_nested
> - _nested usage for net->unx.table.locks[].
>
> replace both with lock_set_cmp_fn_ptr_order(&u->lock).
>
> The lock ordering in sk_diag_dump_icons() looks suspicious; this may
> turn up a real issue.
Yes, you cannot use lock_cmp_fn() for unix_state_lock_nested().
The lock order in sk_diag_dump_icons() is
listening socket -> child socket in the listener's queue
, and the inverse order never happens. ptr comparison does not make
sense in this case, and lockdep will complain about false positive.
>
> Cc: netdev@xxxxxxxxxxxxxxx
> Signed-off-by: Kent Overstreet <kent.overstreet@xxxxxxxxx>
> ---
> include/net/af_unix.h | 3 ---
> net/unix/af_unix.c | 20 ++++++++------------
> net/unix/diag.c | 2 +-
> 3 files changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index 49c4640027d8..4eff0a089640 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -48,9 +48,6 @@ struct scm_stat {
>
> #define unix_state_lock(s) spin_lock(&unix_sk(s)->lock)
> #define unix_state_unlock(s) spin_unlock(&unix_sk(s)->lock)
> -#define unix_state_lock_nested(s) \
> - spin_lock_nested(&unix_sk(s)->lock, \
> - SINGLE_DEPTH_NESTING)
>
> /* The AF_UNIX socket */
> struct unix_sock {
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index d013de3c5490..1a0d273799c1 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -170,7 +170,7 @@ static void unix_table_double_lock(struct net *net,
> swap(hash1, hash2);
>
> spin_lock(&net->unx.table.locks[hash1]);
> - spin_lock_nested(&net->unx.table.locks[hash2], SINGLE_DEPTH_NESTING);
> + spin_lock(&net->unx.table.locks[hash2]);
> }
>
> static void unix_table_double_unlock(struct net *net,
> @@ -997,6 +997,7 @@ static struct sock *unix_create1(struct net *net, struct socket *sock, int kern,
> u->path.dentry = NULL;
> u->path.mnt = NULL;
> spin_lock_init(&u->lock);
> + lock_set_cmp_fn_ptr_order(&u->lock);
> atomic_long_set(&u->inflight, 0);
> INIT_LIST_HEAD(&u->link);
> mutex_init(&u->iolock); /* single task reading lock */
> @@ -1340,17 +1341,11 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>
> static void unix_state_double_lock(struct sock *sk1, struct sock *sk2)
> {
> - if (unlikely(sk1 == sk2) || !sk2) {
> - unix_state_lock(sk1);
> - return;
> - }
> - if (sk1 < sk2) {
> + if (sk1 > sk2)
> + swap(sk1, sk2);
> + if (sk1 && sk1 != sk2)
> unix_state_lock(sk1);
> - unix_state_lock_nested(sk2);
> - } else {
> - unix_state_lock(sk2);
> - unix_state_lock_nested(sk1);
> - }
> + unix_state_lock(sk2);
> }
>
> static void unix_state_double_unlock(struct sock *sk1, struct sock *sk2)
> @@ -1591,7 +1586,7 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> goto out_unlock;
> }
>
> - unix_state_lock_nested(sk);
> + unix_state_lock(sk);
>
> if (sk->sk_state != st) {
> unix_state_unlock(sk);
> @@ -3575,6 +3570,7 @@ static int __net_init unix_net_init(struct net *net)
>
> for (i = 0; i < UNIX_HASH_SIZE; i++) {
> spin_lock_init(&net->unx.table.locks[i]);
> + lock_set_cmp_fn_ptr_order(&net->unx.table.locks[i]);
> INIT_HLIST_HEAD(&net->unx.table.buckets[i]);
> }
>
> diff --git a/net/unix/diag.c b/net/unix/diag.c
> index bec09a3a1d44..8ab5e2217e4c 100644
> --- a/net/unix/diag.c
> +++ b/net/unix/diag.c
> @@ -84,7 +84,7 @@ static int sk_diag_dump_icons(struct sock *sk, struct sk_buff *nlskb)
> * queue lock. With the other's queue locked it's
> * OK to lock the state.
> */
> - unix_state_lock_nested(req);
> + unix_state_lock(req);
> peer = unix_sk(req)->peer;
> buf[i++] = (peer ? sock_i_ino(peer) : 0);
> unix_state_unlock(req);
> --
> 2.43.0
