Re: [PATCH v4 2/6] virtio-vsock: avoid sleeping where not allowed

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, Mar 31, 2016 at 01:41:34PM +0200, Claudio Imbrenda wrote:
> @@ -157,7 +158,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
>  	size_t bytes, total = 0;
>  	int err = -EFAULT;
>  
> -	mutex_lock(&vvs->rx_lock);
> +	spin_lock(&vvs->rx_lock);
>  	while (total < len &&
>  	       vvs->rx_bytes > 0 &&
>  	       !list_empty(&vvs->rx_queue)) {
> @@ -179,7 +180,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
>  			virtio_transport_free_pkt(pkt);
>  		}
>  	}
> -	mutex_unlock(&vvs->rx_lock);
> +	spin_unlock(&vvs->rx_lock);
>  
>  	/* Send a credit pkt to peer */
>  	virtio_transport_send_credit_update(vsk, VIRTIO_VSOCK_TYPE_STREAM,

I'm going through the various locking issues and now get the following after
applying your patch:

  BUG: sleeping function called from invalid context at ./arch/x86/include/asm/uaccess_64.h:177
  in_atomic(): 1, irqs_disabled(): 0, pid: 107, name: nc-vsock
  2 locks held by nc-vsock/107:
   #0:  (sk_lock-AF_VSOCK){+.+.+.}, at: [<ffffffffa001d9da>] vsock_stream_recvmsg+0x6a/0x430 [vsock]
   #1:  (&(&vvs->rx_lock)->rlock){+.+...}, at: [<ffffffffa002bcbb>] virtio_transport_stream_dequeue+0x4b/0x190 [virtio_transport_common]
  CPU: 0 PID: 107 Comm: nc-vsock Tainted: G           OE   4.5.0-rc6+ #5
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.1-0-gb3ef39f-prebuilt.qemu-project.org 04/01/2014
   0000000000000286 00000000f41db111 ffff88003f807bc8 ffffffff81432113
   ffff88003c238000 ffffffff81c607c0 ffff88003f807bf0 ffffffff810da759
   ffffffff81c607c0 00000000000000b1 0000000000000000 ffff88003f807c18
  Call Trace:
   [<ffffffff81432113>] dump_stack+0x85/0xc2
   [<ffffffff810da759>] ___might_sleep+0x179/0x230
   [<ffffffff810da859>] __might_sleep+0x49/0x80
   [<ffffffff8121a913>] __might_fault+0x43/0xa0
   [<ffffffffa002bcbb>] ? virtio_transport_stream_dequeue+0x4b/0x190 [virtio_transport_common]
   [<ffffffff814455b7>] copy_to_iter+0x87/0x2a0
   [<ffffffffa002bd35>] virtio_transport_stream_dequeue+0xc5/0x190 [virtio_transport_common]
   [<ffffffffa001db47>] vsock_stream_recvmsg+0x1d7/0x430 [vsock]
   [<ffffffff810fc3d0>] ? wake_atomic_t_function+0x70/0x70
   [<ffffffff81706dbb>] sock_recvmsg+0x3b/0x50
   [<ffffffff81706e65>] sock_read_iter+0x95/0xf0
   [<ffffffff8127b069>] __vfs_read+0xc9/0x110
   [<ffffffff8127c089>] vfs_read+0x89/0x130
   [<ffffffff8127d088>] SyS_read+0x58/0xd0
   [<ffffffff818746f2>] entry_SYSCALL_64_fastpath+0x12/0x76

in_atomic(): 1 seems to be because rx_lock is a spinlock.  We cannot
call memcpy_to_msg() since that may sleep.

I reduced the scope of rx_lock in virtio_transport_stream_do_dequeue()
to solve this.  Just wanted to share with you in case you have thoughts
on it:

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 36bb8c6..9497934 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -169,9 +167,17 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
                if (bytes > pkt->len - pkt->off)
                        bytes = pkt->len - pkt->off;
 
+               /* sk_lock is held by caller so no one else can dequeue.
+                * Unlock rx_lock since memcpy_to_msg() may sleep.
+                */
+               spin_unlock(&vvs->rx_lock);
+
                err = memcpy_to_msg(msg, pkt->buf + pkt->off, bytes);
                if (err)
                        goto out;
+
+               spin_lock(&vvs->rx_lock);
+
                total += bytes;
                pkt->off += bytes;
                if (pkt->off == pkt->len) {
@@ -189,7 +195,6 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
        return total;
 
 out:
-       spin_unlock(&vvs->rx_lock);
        if (total)
                err = total;
        return err;

Attachment: signature.asc
Description: PGP signature