diff mbox series

vhost: new vhost_vdpa SET/GET_BACKEND_FEATURES handlers

Message ID 20200909022233.26559-1-lingshan.zhu@intel.com (mailing list archive)
State New, archived
Headers show
Series vhost: new vhost_vdpa SET/GET_BACKEND_FEATURES handlers | expand

Commit Message

Zhu, Lingshan Sept. 9, 2020, 2:22 a.m. UTC 
This commit introduced vhost_vdpa_set/get_backend_features() to
resolve these issues:
(1)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.
(2)SET_BACKEND_FEATURES was blindly added to vring ioctl instead
of vdpa device ioctl

To resolve these issues, this commit (1)removed mutex operations
in vhost_set_backend_features. (2)Handle ioctl
SET/GET_BACKEND_FEATURES in vdpa ioctl. (3)introduce a new
function vhost_net_set_backend_features() for vhost_net,
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 ++++++++-
 drivers/vhost/vdpa.c  | 47 ++++++++++++++++++++++++++++++-------------
 drivers/vhost/vhost.c |  2 --
 3 files changed, 41 insertions(+), 17 deletions(-)

Comments

Jason Wang Sept. 9, 2020, 2:38 a.m. UTC | #1 
----- Original Message -----
> This commit introduced vhost_vdpa_set/get_backend_features() to
> resolve these issues:
> (1)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.
> (2)SET_BACKEND_FEATURES was blindly added to vring ioctl instead
> of vdpa device ioctl
> 
> To resolve these issues, this commit (1)removed mutex operations
> in vhost_set_backend_features. (2)Handle ioctl
> SET/GET_BACKEND_FEATURES in vdpa ioctl. (3)introduce a new
> function vhost_net_set_backend_features() for vhost_net,
> which is a wrap of vhost_set_backend_features() with
> necessary mutex lockings.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>

So this patch touches not only vhost-vDPA.

Though the function looks correct, I'd prefer you do cleanup on top of
my patches which has been tested by Eli.

Note that, vhost generic handlers tend to hold mutex by itself.

So we probably need more thought on this.

Thanks

> ---
>  drivers/vhost/net.c   |  9 ++++++++-
>  drivers/vhost/vdpa.c  | 47 ++++++++++++++++++++++++++++++-------------
>  drivers/vhost/vhost.c |  2 --
>  3 files changed, 41 insertions(+), 17 deletions(-)
> 
> 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);
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 3fab94f88894..ade33c566a81 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -344,6 +344,33 @@ static long vhost_vdpa_set_config_call(struct vhost_vdpa
> *v, u32 __user *argp)
>  	return 0;
>  }
>  
> +
> +static long vhost_vdpa_get_backend_features(void __user *argp)
> +{
> +	u64 features = VHOST_VDPA_BACKEND_FEATURES;
> +	u64 __user *featurep = argp;
> +	long r;
> +
> +	r = copy_to_user(featurep, &features, sizeof(features));
> +
> +	return r;
> +}
> +static long vhost_vdpa_set_backend_features(struct vhost_vdpa *v, void
> __user *argp)
> +{
> +	u64 __user *featurep = argp;
> +	u64 features;
> +
> +	if (copy_from_user(&features, featurep, sizeof(features)))
> +		return -EFAULT;
> +
> +	if (features & ~VHOST_VDPA_BACKEND_FEATURES)
> +		return -EOPNOTSUPP;
> +
> +	vhost_set_backend_features(&v->vdev, features);
> +
> +	return 0;
> +}
> +
>  static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
>  				   void __user *argp)
>  {
> @@ -353,8 +380,6 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v,
> unsigned int cmd,
>  	struct vdpa_callback cb;
>  	struct vhost_virtqueue *vq;
>  	struct vhost_vring_state s;
> -	u64 __user *featurep = argp;
> -	u64 features;
>  	u32 idx;
>  	long r;
>  
> @@ -381,18 +406,6 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v,
> unsigned int cmd,
>  
>  		vq->last_avail_idx = vq_state.avail_index;
>  		break;
> -	case VHOST_GET_BACKEND_FEATURES:
> -		features = VHOST_VDPA_BACKEND_FEATURES;
> -		if (copy_to_user(featurep, &features, sizeof(features)))
> -			return -EFAULT;
> -		return 0;
> -	case VHOST_SET_BACKEND_FEATURES:
> -		if (copy_from_user(&features, featurep, sizeof(features)))
> -			return -EFAULT;
> -		if (features & ~VHOST_VDPA_BACKEND_FEATURES)
> -			return -EOPNOTSUPP;
> -		vhost_set_backend_features(&v->vdev, features);
> -		return 0;
>  	}
>  
>  	r = vhost_vring_ioctl(&v->vdev, cmd, argp);
> @@ -476,6 +489,12 @@ static long vhost_vdpa_unlocked_ioctl(struct file
> *filep,
>  	case VHOST_VDPA_SET_CONFIG_CALL:
>  		r = vhost_vdpa_set_config_call(v, argp);
>  		break;
> +	case VHOST_SET_BACKEND_FEATURES:
> +		r = vhost_vdpa_set_backend_features(v, argp);
> +		break;
> +	case VHOST_GET_BACKEND_FEATURES:
> +		r = vhost_vdpa_get_backend_features(argp);
> +		break;
>  	default:
>  		r = vhost_dev_ioctl(&v->vdev, cmd, argp);
>  		if (r == -ENOIOCTLCMD)
> 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
> 
>
Dan Carpenter Sept. 9, 2020, 5:23 p.m. UTC | #2 
Hi Zhu,

url:    https://github.com/0day-ci/linux/commits/Zhu-Lingshan/vhost-new-vhost_vdpa-SET-GET_BACKEND_FEATURES-handlers/20200909-115726
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: parisc-randconfig-m031-20200909 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
drivers/vhost/vdpa.c:356 vhost_vdpa_get_backend_features() warn: maybe return -EFAULT instead of the bytes remaining?

# https://github.com/0day-ci/linux/commit/f2da8fc35b4ef003de7d559a8c7730fedf9926f8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Zhu-Lingshan/vhost-new-vhost_vdpa-SET-GET_BACKEND_FEATURES-handlers/20200909-115726
git checkout f2da8fc35b4ef003de7d559a8c7730fedf9926f8
vim +356 drivers/vhost/vdpa.c

f2da8fc35b4ef0 Zhu Lingshan 2020-09-09  348  static long vhost_vdpa_get_backend_features(void __user *argp)
f2da8fc35b4ef0 Zhu Lingshan 2020-09-09  349  {
f2da8fc35b4ef0 Zhu Lingshan 2020-09-09  350  	u64 features = VHOST_VDPA_BACKEND_FEATURES;
f2da8fc35b4ef0 Zhu Lingshan 2020-09-09  351  	u64 __user *featurep = argp;
f2da8fc35b4ef0 Zhu Lingshan 2020-09-09  352  	long r;
f2da8fc35b4ef0 Zhu Lingshan 2020-09-09  353  
f2da8fc35b4ef0 Zhu Lingshan 2020-09-09  354  	r = copy_to_user(featurep, &features, sizeof(features));
f2da8fc35b4ef0 Zhu Lingshan 2020-09-09  355  
f2da8fc35b4ef0 Zhu Lingshan 2020-09-09 @356  	return r;

copy_to_user() returns the number of bytes remaining.  I haven't looked
at how this is called but it should probably return negative error codes
on error:

	if (copy_to_user(featurep, &features, sizeof(features)))
		return -EFAULT;

	return 0;

f2da8fc35b4ef0 Zhu Lingshan 2020-09-09  357  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

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);
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 3fab94f88894..ade33c566a81 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -344,6 +344,33 @@  static long vhost_vdpa_set_config_call(struct vhost_vdpa *v, u32 __user *argp)
 	return 0;
 }
 
+
+static long vhost_vdpa_get_backend_features(void __user *argp)
+{
+	u64 features = VHOST_VDPA_BACKEND_FEATURES;
+	u64 __user *featurep = argp;
+	long r;
+
+	r = copy_to_user(featurep, &features, sizeof(features));
+
+	return r;
+}
+static long vhost_vdpa_set_backend_features(struct vhost_vdpa *v, void __user *argp)
+{
+	u64 __user *featurep = argp;
+	u64 features;
+
+	if (copy_from_user(&features, featurep, sizeof(features)))
+		return -EFAULT;
+
+	if (features & ~VHOST_VDPA_BACKEND_FEATURES)
+		return -EOPNOTSUPP;
+
+	vhost_set_backend_features(&v->vdev, features);
+
+	return 0;
+}
+
 static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
 				   void __user *argp)
 {
@@ -353,8 +380,6 @@  static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
 	struct vdpa_callback cb;
 	struct vhost_virtqueue *vq;
 	struct vhost_vring_state s;
-	u64 __user *featurep = argp;
-	u64 features;
 	u32 idx;
 	long r;
 
@@ -381,18 +406,6 @@  static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
 
 		vq->last_avail_idx = vq_state.avail_index;
 		break;
-	case VHOST_GET_BACKEND_FEATURES:
-		features = VHOST_VDPA_BACKEND_FEATURES;
-		if (copy_to_user(featurep, &features, sizeof(features)))
-			return -EFAULT;
-		return 0;
-	case VHOST_SET_BACKEND_FEATURES:
-		if (copy_from_user(&features, featurep, sizeof(features)))
-			return -EFAULT;
-		if (features & ~VHOST_VDPA_BACKEND_FEATURES)
-			return -EOPNOTSUPP;
-		vhost_set_backend_features(&v->vdev, features);
-		return 0;
 	}
 
 	r = vhost_vring_ioctl(&v->vdev, cmd, argp);
@@ -476,6 +489,12 @@  static long vhost_vdpa_unlocked_ioctl(struct file *filep,
 	case VHOST_VDPA_SET_CONFIG_CALL:
 		r = vhost_vdpa_set_config_call(v, argp);
 		break;
+	case VHOST_SET_BACKEND_FEATURES:
+		r = vhost_vdpa_set_backend_features(v, argp);
+		break;
+	case VHOST_GET_BACKEND_FEATURES:
+		r = vhost_vdpa_get_backend_features(argp);
+		break;
 	default:
 		r = vhost_dev_ioctl(&v->vdev, cmd, argp);
 		if (r == -ENOIOCTLCMD)
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);