Re: [PATCH] nbd: reclassify u->iolock of AF_UNIX sockets

[Christian Borntraeger]

> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 8f10762e90ef..a811e431b47a 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -32,6 +32,7 @@
>   #include <linux/err.h>
>   #include <linux/kernel.h>
>   #include <linux/slab.h>
> +#include <net/af_unix.h>
>   #include <net/sock.h>
>   #include <linux/net.h>
>   #include <linux/kthread.h>
> @@ -1241,6 +1242,7 @@ static struct socket *nbd_get_socket(struct nbd_device *nbd, unsigned long fd,
>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>   static struct lock_class_key nbd_key[3];
>   static struct lock_class_key nbd_slock_key[3];
> +static struct lock_class_key nbd_unix_iolock_key;
>   
>   static void nbd_reclassify_socket(struct socket *sock)
>   {
> @@ -1267,6 +1269,17 @@ static void nbd_reclassify_socket(struct socket *sock)
>   					      &nbd_slock_key[2],
>   					      "sk_lock-AF_UNIX-NBD",
>   					      &nbd_key[2]);
> +		/*
> +		 * The AF_UNIX stream recvmsg/sendmsg paths serialize on
> +		 * u->iolock, not sk_lock, so it must be reclassified as
> +		 * well.  A held mutex cannot be reclassified; skip it in
> +		 * that case, as sock_allow_reclassification() does for
> +		 * sk_lock.
> +		 */
> +		if (!mutex_is_locked(&unix_sk(sk)->iolock))
> +			lockdep_set_class_and_name(&unix_sk(sk)->iolock,
> +						   &nbd_unix_iolock_key,
> +						   "&u->iolock-NBD");
>   		break;
>   	}
>   }


FWIW, as sashiko pointed out, the mutex_lock check is racy;
it narrows the window but does not close it. In mirrors the existing
sk_lock path, which has the same property.

AFAIK, the consequences are confined to lockdep bookkeeping.
In practice the window is one-shot and unreachable for a functioning
client: the reclassification runs once at socket hand-over, and a
thread concurrently doing recvmsg() on the socket it just handed to
nbd would destroy the NBD protocol framing anyway.

So the patch is basically best effort to avoid lockdep being turned
off - which is my concern since this happens early during our CI runs.