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

2020-09-21 Thread 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 
> 
> 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(>mutex);
> for (i = 0; i < dev->nvqs; ++i) {
> vq = dev->vqs[i];
> mutex_lock(>mutex);
> vq->acked_backend_features = features;
> mutex_unlock(>mutex);
> }
> -   mutex_unlock(>mutex);
>  }
>  EXPORT_SYMBOL_GPL(vhost_set_backend_features);
> 
> --
> 2.18.4
> 

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


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

2020-09-08 Thread 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 

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(>mutex);
>   for (i = 0; i < dev->nvqs; ++i) {
>   vq = dev->vqs[i];
>   mutex_lock(>mutex);
>   vq->acked_backend_features = features;
>   mutex_unlock(>mutex);
>   }
> - mutex_unlock(>mutex);
>  }
>  EXPORT_SYMBOL_GPL(vhost_set_backend_features);
>  
> -- 
> 2.18.4

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization