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

[Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>]

On 11/05/16 14:07, Stefan Hajnoczi wrote:
> 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 seems to be because rx_lock is a spinlock.  We cannot
> call memcpy_to_msg() since that may sleep.

Oops. For some reason I never hit that codepath and never noticed the 
issue :(

> 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.

So this means that this one function is never reentrant, and the lock
only protects the consistency of the values?

> +                */
> +               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;

I don't find it terribly good style to have a "hole" in the scope of the 
lock, but if it works and avoids clumsier/more complex solutions, I'm OK 
totally with it.

basically the assumption here is that nobody else is dequeueing or 
otherwise messing with the head, so we are the only ones touching the 
head of the list.