diff mbox series

vhost: new vhost_vdpa SET/GET_BACKEND_FEATURES handlers

Message ID 20200909022233.26559-1-lingshan.zhu@intel.com
State New
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

Dan Carpenter Sept. 9, 2020, 5:23 p.m. UTC | #1
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);