diff mbox series

vhost: fix deadlock with no multiqueue

Message ID 20230323144433.3104768-1-david.marchand@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: fix deadlock with no multiqueue |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Functional success Functional PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-unit-testing success Testing PASS

Commit Message

David Marchand March 23, 2023, 2:44 p.m. UTC 
  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(+)

Comments

Maxime Coquelin March 23, 2023, 2:48 p.m. UTC | #1 
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
David Marchand March 26, 2023, 3:51 p.m. UTC | #2 
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.
diff mbox series

Patch

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);
 		}
 	}