[PATCH 0/2] fix dead lock issues in vhost_vdpa

[PATCH 0/2] fix dead lock issues in vhost_vdpa

[Zhu Lingshan]

This series fixed a dead lock issue in vhost_vdpa.
In current vhost_vdpa ioctl code path, it will lock
vhost_dev mutex once, then try to re-lock it in
vhost_set_backend_features, which is buggy.

These commits will remove mutex locks operations
in vhost_set_backend_features, then as a compensation,
a new function vhost_net_set_backend_features() in vhost_net
is introduced to do proper mutex locking and call
vhost_set_backend_features()

Please help review. Thanks!

Zhu Lingshan (2):
  vhost: remove mutex ops in vhost_set_backend_features
  vhost_net: introduce vhost_net_set_backend_features()

 drivers/vhost/net.c   | 9 ++++++++-
 drivers/vhost/vhost.c | 2 --
 2 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.18.4

[PATCH 1/2] vhost: remove mutex ops in vhost_set_backend_features

[Zhu Lingshan]

In vhost_vdpa ioctl SET_BACKEND_FEATURES path, currect code
would try to acquire vhost dev mutex twice
(first shown in vhost_vdpa_unlocked_ioctl), which can lead
to a dead lock issue.
This commit removed mutex operations in vhost_set_backend_features.
As a compensation for vhost_net, a followinig commit will add
needed mutex lock/unlock operations in a new function
vhost_net_set_backend_features() which is a wrap of
vhost_set_backend_features().

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vhost/vhost.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index b45519ca66a7..e03c9e6f058f 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2591,14 +2591,12 @@ void vhost_set_backend_features(struct vhost_dev *dev, u64 features)
 	struct vhost_virtqueue *vq;
 	int i;
 
-	mutex_lock(&dev->mutex);
 	for (i = 0; i < dev->nvqs; ++i) {
 		vq = dev->vqs[i];
 		mutex_lock(&vq->mutex);
 		vq->acked_backend_features = features;
 		mutex_unlock(&vq->mutex);
 	}
-	mutex_unlock(&dev->mutex);
 }
 EXPORT_SYMBOL_GPL(vhost_set_backend_features);
 
-- 
2.18.4

Re: [PATCH 1/2] vhost: remove mutex ops in vhost_set_backend_features

[Michael S. Tsirkin]

On Mon, Sep 07, 2020 at 06:52:19PM +0800, Zhu Lingshan wrote:
> In vhost_vdpa ioctl SET_BACKEND_FEATURES path, currect code
> would try to acquire vhost dev mutex twice
> (first shown in vhost_vdpa_unlocked_ioctl), which can lead
> to a dead lock issue.
> This commit removed mutex operations in vhost_set_backend_features.
> As a compensation for vhost_net, a followinig commit will add
> needed mutex lock/unlock operations in a new function
> vhost_net_set_backend_features() which is a wrap of
> vhost_set_backend_features().
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>

I think you need to squash these two or reorder, we can't first
make code racy then fix it up.

> ---
>  drivers/vhost/vhost.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index b45519ca66a7..e03c9e6f058f 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2591,14 +2591,12 @@ void vhost_set_backend_features(struct vhost_dev *dev, u64 features)
>  	struct vhost_virtqueue *vq;
>  	int i;
>  
> -	mutex_lock(&dev->mutex);
>  	for (i = 0; i < dev->nvqs; ++i) {
>  		vq = dev->vqs[i];
>  		mutex_lock(&vq->mutex);
>  		vq->acked_backend_features = features;
>  		mutex_unlock(&vq->mutex);
>  	}
> -	mutex_unlock(&dev->mutex);
>  }
>  EXPORT_SYMBOL_GPL(vhost_set_backend_features);
>  
> -- 
> 2.18.4

Re: [PATCH 1/2] vhost: remove mutex ops in vhost_set_backend_features

[Michael S. Tsirkin]

On Tue, Sep 08, 2020 at 09:00:19PM +0800, Zhu, Lingshan wrote:
> 
> On 9/8/2020 8:05 PM, Michael S. Tsirkin wrote:
> 
>     On Mon, Sep 07, 2020 at 06:52:19PM +0800, Zhu Lingshan wrote:
> 
>         In vhost_vdpa ioctl SET_BACKEND_FEATURES path, currect code
>         would try to acquire vhost dev mutex twice
>         (first shown in vhost_vdpa_unlocked_ioctl), which can lead
>         to a dead lock issue.
>         This commit removed mutex operations in vhost_set_backend_features.
>         As a compensation for vhost_net, a followinig commit will add
>         needed mutex lock/unlock operations in a new function
>         vhost_net_set_backend_features() which is a wrap of
>         vhost_set_backend_features().
> 
>         Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> 
>     I think you need to squash these two or reorder, we can't first
>     make code racy then fix it up.
> 
> OK, I will send a V2 series with Jason's fixes tomorrow (handle SET/GET_BACKEND_FEATURES in vhost_vdpa ioctl than vring ioctl).
> 
> Thanks,
> BR
> Zhu Lingshan


this never materialized ...

> 
> 
>         ---
>          drivers/vhost/vhost.c | 2 --
>          1 file changed, 2 deletions(-)
> 
>         diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>         index b45519ca66a7..e03c9e6f058f 100644
>         --- a/drivers/vhost/vhost.c
>         +++ b/drivers/vhost/vhost.c
>         @@ -2591,14 +2591,12 @@ void vhost_set_backend_features(struct vhost_dev *dev, u64 features)
>                 struct vhost_virtqueue *vq;
>                 int i;
> 
>         -       mutex_lock(&dev->mutex);
>                 for (i = 0; i < dev->nvqs; ++i) {
>                         vq = dev->vqs[i];
>                         mutex_lock(&vq->mutex);
>                         vq->acked_backend_features = features;
>                         mutex_unlock(&vq->mutex);
>                 }
>         -       mutex_unlock(&dev->mutex);
>          }
>          EXPORT_SYMBOL_GPL(vhost_set_backend_features);
> 
>         --
>         2.18.4
> 

[PATCH 2/2] vhost_net: introduce vhost_net_set_backend_features()

[Zhu Lingshan]

This commit introduced a new function
vhost_net_set_backend_features() which is a wrap of
vhost_set_backend_features() with necessary mutex lockings.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vhost/net.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 531a00d703cd..e01da77538c8 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -1679,6 +1679,13 @@ static long vhost_net_set_owner(struct vhost_net *n)
 	return r;
 }
 
+static void vhost_net_set_backend_features(struct vhost_dev *dev, u64 features)
+{
+	mutex_lock(&dev->mutex);
+	vhost_set_backend_features(dev, features);
+	mutex_unlock(&dev->mutex);
+}
+
 static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
 			    unsigned long arg)
 {
@@ -1715,7 +1722,7 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
 			return -EFAULT;
 		if (features & ~VHOST_NET_BACKEND_FEATURES)
 			return -EOPNOTSUPP;
-		vhost_set_backend_features(&n->dev, features);
+		vhost_net_set_backend_features(&n->dev, features);
 		return 0;
 	case VHOST_RESET_OWNER:
 		return vhost_net_reset_owner(n);
-- 
2.18.4