[PATCH] vhost: fix deadlock with no multiqueue

[PATCH] vhost: fix deadlock with no multiqueue

[David Marchand]

This deadlock happens when a guest, that had virtio ports with multi
queues configured, does not announce the multi q feature in
SET_FEATURES.
In such a situation, all vq locks are already taken before calling
free_vq(), which itself takes the lock.

As mentioned in the code, in this situation, the virtio device is not
running yet and no datapath thread is using the vq. So we can
release the lock before calling free_vq().

Bugzilla ID: 1196
Fixes: 4b02c2673757 ("vhost: annotate async accesses")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/vhost/vhost_user.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index f5edba8548..d60e39b6bc 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -397,6 +397,9 @@ vhost_user_set_features(struct virtio_net **pdev,
 			dev->virtqueue[dev->nr_vring] = NULL;
 			cleanup_vq(vq, 1);
 			cleanup_vq_inflight(dev, vq);
+			/* vhost_user_lock_all_queue_pairs locked all qps */
+			vq_assert_lock(dev, vq);
+			rte_spinlock_unlock(&vq->access_lock);
 			free_vq(dev, vq);
 		}
 	}
-- 
2.39.2

Re: [PATCH] vhost: fix deadlock with no multiqueue

[Maxime Coquelin]

On 3/23/23 15:44, David Marchand wrote:
> This deadlock happens when a guest, that had virtio ports with multi
> queues configured, does not announce the multi q feature in
> SET_FEATURES.
> In such a situation, all vq locks are already taken before calling
> free_vq(), which itself takes the lock.
> 
> As mentioned in the code, in this situation, the virtio device is not
> running yet and no datapath thread is using the vq. So we can
> release the lock before calling free_vq().
> 
> Bugzilla ID: 1196
> Fixes: 4b02c2673757 ("vhost: annotate async accesses")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/vhost/vhost_user.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index f5edba8548..d60e39b6bc 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -397,6 +397,9 @@ vhost_user_set_features(struct virtio_net **pdev,
>   			dev->virtqueue[dev->nr_vring] = NULL;
>   			cleanup_vq(vq, 1);
>   			cleanup_vq_inflight(dev, vq);
> +			/* vhost_user_lock_all_queue_pairs locked all qps */
> +			vq_assert_lock(dev, vq);
> +			rte_spinlock_unlock(&vq->access_lock);
>   			free_vq(dev, vq);
>   		}
>   	}

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [PATCH] vhost: fix deadlock with no multiqueue

[David Marchand]

On Thu, Mar 23, 2023 at 3:48 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 3/23/23 15:44, David Marchand wrote:
> > This deadlock happens when a guest, that had virtio ports with multi
> > queues configured, does not announce the multi q feature in
> > SET_FEATURES.
> > In such a situation, all vq locks are already taken before calling
> > free_vq(), which itself takes the lock.
> >
> > As mentioned in the code, in this situation, the virtio device is not
> > running yet and no datapath thread is using the vq. So we can
> > release the lock before calling free_vq().
> >
> > Bugzilla ID: 1196
> > Fixes: 4b02c2673757 ("vhost: annotate async accesses")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Applied, thanks.


-- 
David Marchand