Re: [PATCH v4 3/6] virtio/vhost-vsock: avoid potential deadlock

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, Mar 31, 2016 at 01:41:35PM +0200, Claudio Imbrenda wrote:
> In some circumstances, like when a RESET packet needs to be sent back,
> both the send and receive locks are held. This makes lockdep unhappy,
> because both locks belong to the same class. In general it is not a good
> idea to hold both the send and receive locks at the same time; this patch
> introduces a reset queue, serviced by the default workqueue. This
> guarantees that the reset packets are sent back with only one lock being
> held, which makes lockdep happy.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> ---
>  net/vmw_vsock/virtio_transport_common.c | 98 +++++++++++++++++++++++++++++++--
>  1 file changed, 93 insertions(+), 5 deletions(-)
> 
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index eab1644..fa54359 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -8,6 +8,7 @@
>   * This work is licensed under the terms of the GNU GPL, version 2.
>   */
>  #include <linux/spinlock.h>
> +#include <linux/workqueue.h>
>  #include <linux/module.h>
>  #include <linux/ctype.h>
>  #include <linux/list.h>
> @@ -22,6 +23,27 @@
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/vsock_virtio_transport_common.h>
>  
> +enum {
> +	VSOCK_RESET_NOSOCK = 0,
> +	VSOCK_RESET_WITHSOCK,
> +};
> +
> +struct reset_queue {
> +	struct list_head list;
> +	union {
> +		struct virtio_vsock_pkt *pkt;
> +		struct vsock_sock *vsk;
> +	} u;
> +	char type;
> +};
> +
> +static void virtio_transport_send_reset_work(struct work_struct *unused);
> +
> +static DEFINE_SPINLOCK(resetqueue_lock);
> +static LIST_HEAD(queued_resets);
> +static DECLARE_WORK(virtio_transport_resets_wq,
> +			virtio_transport_send_reset_work);
> +
>  static const struct virtio_transport *virtio_transport_get_ops(void)
>  {
>  	const struct vsock_transport *t = vsock_core_get_transport();
> @@ -544,19 +566,61 @@ void virtio_transport_destruct(struct vsock_sock *vsk)
>  }
>  EXPORT_SYMBOL_GPL(virtio_transport_destruct);
>  
> -static int virtio_transport_send_reset(struct vsock_sock *vsk,
> -				       struct virtio_vsock_pkt *pkt)
> +static void virtio_transport_send_reset_work(struct work_struct *unused)
>  {
> +	struct reset_queue *q;
>  	struct virtio_vsock_pkt_info info = {
>  		.op = VIRTIO_VSOCK_OP_RST,
>  		.type = VIRTIO_VSOCK_TYPE_STREAM,
>  	};
>  
> +	spin_lock(&resetqueue_lock);
> +	while (!list_empty(&queued_resets)) {
> +		q = list_first_entry(&queued_resets, struct reset_queue, list);
> +		list_del(&q->list);
> +		spin_unlock(&resetqueue_lock);
> +
> +		/* Since we are now asynchronous, we ignore the return values.
> +		 * After all we don't even know who we should report them to.

I'm a little concerned about adding kmalloc and ignoring return values
because virtio-vsock packets must never be lost.  Unlike TCP there is no
retransmission so the transport must be reliable.

Attachment: signature.asc
Description: PGP signature