thr3ads.net - Virtualization - [RFC PATCH 00/10] vdpa: get/set

If this information is useful, please help other people find it:
Share via:

Stefano Garzarella

2021-Feb-16 09:44 UTC

[RFC PATCH 00/10] vdpa: get/set_config() rework

Following the discussion with Michael and Jason [1], I reworked a bit
get/set_config() in vdpa.

I changed vdpa_get_config() to check the boundaries and added vdpa_set_config().
When 'offset' or 'len' parameters exceed boundaries, we limit
the reading to
the available configuration space in the device, and we return the amount of
bytes read/written.

In this way the user space can pass buffers bigger than config space.
I also returned the amount of bytes read and written to user space.

Patches also available here:
https://github.com/stefano-garzarella/linux/tree/vdpa-get-set-config-refactoring

Thanks for your comments,
Stefano

[1] https://lkml.org/lkml/2021/2/10/350

Stefano Garzarella (10):
  vdpa: add get_config_size callback in vdpa_config_ops
  vdpa: check vdpa_get_config() parameters and return bytes read
  vdpa: add vdpa_set_config() helper
  vdpa: remove param checks in the get/set_config callbacks
  vdpa: remove WARN_ON() in the get/set_config callbacks
  virtio_vdpa: use vdpa_set_config()
  vhost/vdpa: use vdpa_set_config()
  vhost/vdpa: allow user space to pass buffers bigger than config space
  vhost/vdpa: use get_config_size callback in
    vhost_vdpa_config_validate()
  vhost/vdpa: return configuration bytes read and written to user space

 include/linux/vdpa.h              | 22 ++++-------
 drivers/vdpa/ifcvf/ifcvf_base.c   |  3 +-
 drivers/vdpa/ifcvf/ifcvf_main.c   |  8 +++-
 drivers/vdpa/mlx5/net/mlx5_vnet.c |  9 ++++-
 drivers/vdpa/vdpa.c               | 51 ++++++++++++++++++++++++
 drivers/vdpa/vdpa_sim/vdpa_sim.c  | 15 +++++---
 drivers/vhost/vdpa.c              | 64 ++++++++++++++++---------------
 drivers/virtio/virtio_vdpa.c      |  3 +-
 8 files changed, 116 insertions(+), 59 deletions(-)

-- 
2.29.2

Stefano Garzarella

2021-Feb-16 09:44 UTC

head link

[RFC PATCH 01/10] vdpa: add get_config_size callback in vdpa_config_ops

This new callback is used to get the size of the configuration space
of vDPA devices.

Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
---
 include/linux/vdpa.h              | 4 ++++
 drivers/vdpa/ifcvf/ifcvf_main.c   | 6 ++++++
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 6 ++++++
 drivers/vdpa/vdpa_sim/vdpa_sim.c  | 9 +++++++++
 4 files changed, 25 insertions(+)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 4ab5494503a8..fddf42b17573 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -150,6 +150,9 @@ struct vdpa_iova_range {
  * @set_status:			Set the device status
  *				@vdev: vdpa device
  *				@status: virtio device status
+ * @get_config_size:		Get the size of the configuration space
+ *				@vdev: vdpa device
+ *				Returns size_t: configuration size
  * @get_config:			Read from device specific configuration space
  *				@vdev: vdpa device
  *				@offset: offset from the beginning of
@@ -231,6 +234,7 @@ struct vdpa_config_ops {
 	u32 (*get_vendor_id)(struct vdpa_device *vdev);
 	u8 (*get_status)(struct vdpa_device *vdev);
 	void (*set_status)(struct vdpa_device *vdev, u8 status);
+	size_t (*get_config_size)(struct vdpa_device *vdev);
 	void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
 			   void *buf, unsigned int len);
 	void (*set_config)(struct vdpa_device *vdev, unsigned int offset,
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 7c8bbfcf6c3e..2443271e17d2 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -332,6 +332,11 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device
*vdpa_dev)
 	return IFCVF_QUEUE_ALIGNMENT;
 }
 
+static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
+{
+	return sizeof(struct virtio_net_config);
+}
+
 static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev,
 				  unsigned int offset,
 				  void *buf, unsigned int len)
@@ -392,6 +397,7 @@ static const struct vdpa_config_ops ifc_vdpa_ops = {
 	.get_device_id	= ifcvf_vdpa_get_device_id,
 	.get_vendor_id	= ifcvf_vdpa_get_vendor_id,
 	.get_vq_align	= ifcvf_vdpa_get_vq_align,
+	.get_config_size	= ifcvf_vdpa_get_config_size,
 	.get_config	= ifcvf_vdpa_get_config,
 	.set_config	= ifcvf_vdpa_set_config,
 	.set_config_cb  = ifcvf_vdpa_set_config_cb,
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 10e9b09932eb..78043ee567b6 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1814,6 +1814,11 @@ static void mlx5_vdpa_set_status(struct vdpa_device
*vdev, u8 status)
 	ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
 }
 
+static size_t mlx5_vdpa_get_config_size(struct vdpa_device *vdev)
+{
+	return sizeof(struct virtio_net_config);
+}
+
 static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
void *buf,
 				 unsigned int len)
 {
@@ -1900,6 +1905,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
 	.get_vendor_id = mlx5_vdpa_get_vendor_id,
 	.get_status = mlx5_vdpa_get_status,
 	.set_status = mlx5_vdpa_set_status,
+	.get_config_size = mlx5_vdpa_get_config_size,
 	.get_config = mlx5_vdpa_get_config,
 	.set_config = mlx5_vdpa_set_config,
 	.get_generation = mlx5_vdpa_get_generation,
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index d5942842432d..779ae6c144d7 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -439,6 +439,13 @@ static void vdpasim_set_status(struct vdpa_device *vdpa, u8
status)
 	spin_unlock(&vdpasim->lock);
 }
 
+static size_t vdpasim_get_config_size(struct vdpa_device *vdpa)
+{
+	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+
+	return vdpasim->dev_attr.config_size;
+}
+
 static void vdpasim_get_config(struct vdpa_device *vdpa, unsigned int offset,
 			     void *buf, unsigned int len)
 {
@@ -566,6 +573,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
 	.get_vendor_id          = vdpasim_get_vendor_id,
 	.get_status             = vdpasim_get_status,
 	.set_status             = vdpasim_set_status,
+	.get_config_size        = vdpasim_get_config_size,
 	.get_config             = vdpasim_get_config,
 	.set_config             = vdpasim_set_config,
 	.get_generation         = vdpasim_get_generation,
@@ -593,6 +601,7 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops
= {
 	.get_vendor_id          = vdpasim_get_vendor_id,
 	.get_status             = vdpasim_get_status,
 	.set_status             = vdpasim_set_status,
+	.get_config_size        = vdpasim_get_config_size,
 	.get_config             = vdpasim_get_config,
 	.set_config             = vdpasim_set_config,
 	.get_generation         = vdpasim_get_generation,
-- 
2.29.2

Stefano Garzarella

2021-Feb-16 09:44 UTC

head link

[RFC PATCH 02/10] vdpa: check vdpa_get_config() parameters and return bytes read

Now we have the 'get_config_size()' callback available, so we can
check that 'offset' and 'len' parameters are valid.

When these exceed boundaries, we limit the reading to the available
configuration space in the device, and we return the amount of bytes
read.

We also move vdpa_get_config() implementation in drivers/vdpa/vdpa.c,
since the function are growing.

Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
---
 include/linux/vdpa.h | 16 ++--------------
 drivers/vdpa/vdpa.c  | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index fddf42b17573..8a679c98f8b1 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -332,20 +332,8 @@ static inline int vdpa_set_features(struct vdpa_device
*vdev, u64 features)
         return ops->set_features(vdev, features);
 }
 
-
-static inline void vdpa_get_config(struct vdpa_device *vdev, unsigned offset,
-				   void *buf, unsigned int len)
-{
-        const struct vdpa_config_ops *ops = vdev->config;
-
-	/*
-	 * Config accesses aren't supposed to trigger before features are set.
-	 * If it does happen we assume a legacy guest.
-	 */
-	if (!vdev->features_valid)
-		vdpa_set_features(vdev, 0);
-	ops->get_config(vdev, offset, buf, len);
-}
+int vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
+		    void *buf, unsigned int len);
 
 /**
  * vdpa_mgmtdev_ops - vdpa device ops
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 3d997b389345..9ed6c779c63c 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -51,6 +51,41 @@ static struct bus_type vdpa_bus = {
 	.remove = vdpa_dev_remove,
 };
 
+static int vdpa_config_size_wrap(struct vdpa_device *vdev, unsigned int offset,
+				 unsigned int len)
+{
+	const struct vdpa_config_ops *ops = vdev->config;
+	unsigned int config_size = ops->get_config_size(vdev);
+
+	if (offset > config_size || len > config_size)
+		return -1;
+
+	return min(len, config_size - offset);
+}
+
+int vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
+		    void *buf, unsigned int len)
+{
+	const struct vdpa_config_ops *ops = vdev->config;
+	int bytes_get;
+
+	bytes_get = vdpa_config_size_wrap(vdev, offset, len);
+	if (bytes_get <= 0)
+		return bytes_get;
+
+	/*
+	 * Config accesses aren't supposed to trigger before features are set.
+	 * If it does happen we assume a legacy guest.
+	 */
+	if (!vdev->features_valid)
+		vdpa_set_features(vdev, 0);
+
+	ops->get_config(vdev, offset, buf, bytes_get);
+
+	return bytes_get;
+}
+EXPORT_SYMBOL_GPL(vdpa_get_config);
+
 static void vdpa_release_dev(struct device *d)
 {
 	struct vdpa_device *vdev = dev_to_vdpa(d);
-- 
2.29.2

Stefano Garzarella

2021-Feb-16 09:44 UTC

head link

[RFC PATCH 03/10] vdpa: add vdpa_set_config() helper

Let's add a function similar to vpda_get_config() to check the
'offset' and 'len' parameters, call the set_config() device
callback,
and return the amount of bytes written.

Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
---
 include/linux/vdpa.h |  2 ++
 drivers/vdpa/vdpa.c  | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 8a679c98f8b1..562fcd14f4b5 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -334,6 +334,8 @@ static inline int vdpa_set_features(struct vdpa_device
*vdev, u64 features)
 
 int vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
 		    void *buf, unsigned int len);
+int vdpa_set_config(struct vdpa_device *vdev, unsigned int offset,
+		    const void *buf, unsigned int len);
 
 /**
  * vdpa_mgmtdev_ops - vdpa device ops
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 9ed6c779c63c..825afc690a7e 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -86,6 +86,22 @@ int vdpa_get_config(struct vdpa_device *vdev, unsigned int
offset,
 }
 EXPORT_SYMBOL_GPL(vdpa_get_config);
 
+int vdpa_set_config(struct vdpa_device *vdev, unsigned int offset,
+		    const void *buf, unsigned int len)
+{
+	const struct vdpa_config_ops *ops = vdev->config;
+	int bytes_set;
+
+	bytes_set = vdpa_config_size_wrap(vdev, offset, len);
+	if (bytes_set <= 0)
+		return bytes_set;
+
+	ops->set_config(vdev, offset, buf, bytes_set);
+
+	return bytes_set;
+}
+EXPORT_SYMBOL_GPL(vdpa_set_config);
+
 static void vdpa_release_dev(struct device *d)
 {
 	struct vdpa_device *vdev = dev_to_vdpa(d);
-- 
2.29.2

Stefano Garzarella

2021-Feb-16 09:44 UTC

head link

[RFC PATCH 04/10] vdpa: remove param checks in the get/set_config callbacks

vdpa_get_config() and vdpa_set_config() now check parameters before
calling callbacks, so we can remove these redundant checks.

Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +--
 drivers/vdpa/vdpa_sim/vdpa_sim.c  | 6 ------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 78043ee567b6..ab63dc9b8432 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1825,8 +1825,7 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev,
unsigned int offset,
 	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
 	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
 
-	if (offset + len <= sizeof(struct virtio_net_config))
-		memcpy(buf, (u8 *)&ndev->config + offset, len);
+	memcpy(buf, (u8 *)&ndev->config + offset, len);
 }
 
 static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset,
const void *buf,
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 779ae6c144d7..392180c6f2cf 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -451,9 +451,6 @@ static void vdpasim_get_config(struct vdpa_device *vdpa,
unsigned int offset,
 {
 	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
 
-	if (offset + len > vdpasim->dev_attr.config_size)
-		return;
-
 	if (vdpasim->dev_attr.get_config)
 		vdpasim->dev_attr.get_config(vdpasim, vdpasim->config);
 
@@ -465,9 +462,6 @@ static void vdpasim_set_config(struct vdpa_device *vdpa,
unsigned int offset,
 {
 	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
 
-	if (offset + len > vdpasim->dev_attr.config_size)
-		return;
-
 	memcpy(vdpasim->config + offset, buf, len);
 
 	if (vdpasim->dev_attr.set_config)
-- 
2.29.2

Stefano Garzarella

2021-Feb-16 09:44 UTC

head link

[RFC PATCH 05/10] vdpa: remove WARN_ON() in the get/set_config callbacks

vdpa_get_config() and vdpa_set_config() now check parameters before
calling callbacks, so we can remove these warnings.

Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
---
Maybe we can skip this patch and leave the WARN_ONs in place.
What do you recommend?
---
 drivers/vdpa/ifcvf/ifcvf_base.c | 3 +--
 drivers/vdpa/ifcvf/ifcvf_main.c | 2 --
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index f2a128e56de5..5941ecf934d0 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -222,7 +222,6 @@ void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset,
 	u8 old_gen, new_gen, *p;
 	int i;
 
-	WARN_ON(offset + length > sizeof(struct virtio_net_config));
 	do {
 		old_gen = ifc_ioread8(&hw->common_cfg->config_generation);
 		p = dst;
@@ -240,7 +239,7 @@ void ifcvf_write_net_config(struct ifcvf_hw *hw, u64 offset,
 	int i;
 
 	p = src;
-	WARN_ON(offset + length > sizeof(struct virtio_net_config));
+
 	for (i = 0; i < length; i++)
 		ifc_iowrite8(*p++, hw->net_cfg + offset + i);
 }
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 2443271e17d2..e55f88c57461 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -343,7 +343,6 @@ static void ifcvf_vdpa_get_config(struct vdpa_device
*vdpa_dev,
 {
 	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
 
-	WARN_ON(offset + len > sizeof(struct virtio_net_config));
 	ifcvf_read_net_config(vf, offset, buf, len);
 }
 
@@ -353,7 +352,6 @@ static void ifcvf_vdpa_set_config(struct vdpa_device
*vdpa_dev,
 {
 	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
 
-	WARN_ON(offset + len > sizeof(struct virtio_net_config));
 	ifcvf_write_net_config(vf, offset, buf, len);
 }
 
-- 
2.29.2

Stefano Garzarella

2021-Feb-16 09:44 UTC

head link

[RFC PATCH 06/10] virtio_vdpa: use vdpa_set_config()

Instead of calling the 'set_config' callback directly, we call the
new vdpa_set_config() helper which also checks the parameters.

Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
---
 drivers/virtio/virtio_vdpa.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index e28acf482e0c..2f1c4a2dd241 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -65,9 +65,8 @@ static void virtio_vdpa_set(struct virtio_device *vdev,
unsigned offset,
 			    const void *buf, unsigned len)
 {
 	struct vdpa_device *vdpa = vd_get_vdpa(vdev);
-	const struct vdpa_config_ops *ops = vdpa->config;
 
-	ops->set_config(vdpa, offset, buf, len);
+	vdpa_set_config(vdpa, offset, buf, len);
 }
 
 static u32 virtio_vdpa_generation(struct virtio_device *vdev)
-- 
2.29.2

Stefano Garzarella

2021-Feb-16 09:44 UTC

head link

[RFC PATCH 07/10] vhost/vdpa: use vdpa_set_config()

Instead of calling the 'set_config' callback directly, we call the
new vdpa_set_config() helper which also checks the parameters.

Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
---
 drivers/vhost/vdpa.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index ef688c8c0e0e..cdd8f24168b2 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -236,7 +236,6 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
 				  struct vhost_vdpa_config __user *c)
 {
 	struct vdpa_device *vdpa = v->vdpa;
-	const struct vdpa_config_ops *ops = vdpa->config;
 	struct vhost_vdpa_config config;
 	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
 	u8 *buf;
@@ -250,7 +249,7 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
 	if (IS_ERR(buf))
 		return PTR_ERR(buf);
 
-	ops->set_config(vdpa, config.off, buf, config.len);
+	vdpa_set_config(vdpa, config.off, buf, config.len);
 
 	kvfree(buf);
 	return 0;
-- 
2.29.2

Stefano Garzarella

2021-Feb-16 09:44 UTC

head link

[RFC PATCH 08/10] vhost/vdpa: allow user space to pass buffers bigger than config space

vdpa_get_config() and vdpa_set_config() now are able to read/write
only the bytes available in the device configuration space, also if
the buffer provided is bigger than that.

Let's use this feature to allow the user space application to pass any
buffer. We limit the size of the internal bounce buffer allocated with
the device config size.

Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
---
 drivers/vhost/vdpa.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index cdd8f24168b2..544f8582a42b 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -185,10 +185,10 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8
__user *statusp)
 	return 0;
 }
 
-static int vhost_vdpa_config_validate(struct vhost_vdpa *v,
-				      struct vhost_vdpa_config *c)
+static ssize_t vhost_vdpa_config_validate(struct vhost_vdpa *v,
+					  struct vhost_vdpa_config *c)
 {
-	long size = 0;
+	u32 size = 0;
 
 	switch (v->virtio_id) {
 	case VIRTIO_ID_NET:
@@ -199,10 +199,7 @@ static int vhost_vdpa_config_validate(struct vhost_vdpa *v,
 	if (c->len == 0)
 		return -EINVAL;
 
-	if (c->len > size - c->off)
-		return -E2BIG;
-
-	return 0;
+	return min(c->len, size);
 }
 
 static long vhost_vdpa_get_config(struct vhost_vdpa *v,
@@ -211,19 +208,23 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,
 	struct vdpa_device *vdpa = v->vdpa;
 	struct vhost_vdpa_config config;
 	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
+	ssize_t config_size;
 	u8 *buf;
 
 	if (copy_from_user(&config, c, size))
 		return -EFAULT;
-	if (vhost_vdpa_config_validate(v, &config))
-		return -EINVAL;
-	buf = kvzalloc(config.len, GFP_KERNEL);
+
+	config_size = vhost_vdpa_config_validate(v, &config);
+	if (config_size <= 0)
+		return config_size;
+
+	buf = kvzalloc(config_size, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	vdpa_get_config(vdpa, config.off, buf, config.len);
+	vdpa_get_config(vdpa, config.off, buf, config_size);
 
-	if (copy_to_user(c->buf, buf, config.len)) {
+	if (copy_to_user(c->buf, buf, config_size)) {
 		kvfree(buf);
 		return -EFAULT;
 	}
@@ -238,18 +239,21 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
 	struct vdpa_device *vdpa = v->vdpa;
 	struct vhost_vdpa_config config;
 	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
+	ssize_t config_size;
 	u8 *buf;
 
 	if (copy_from_user(&config, c, size))
 		return -EFAULT;
-	if (vhost_vdpa_config_validate(v, &config))
-		return -EINVAL;
 
-	buf = vmemdup_user(c->buf, config.len);
+	config_size = vhost_vdpa_config_validate(v, &config);
+	if (config_size <= 0)
+		return config_size;
+
+	buf = vmemdup_user(c->buf, config_size);
 	if (IS_ERR(buf))
 		return PTR_ERR(buf);
 
-	vdpa_set_config(vdpa, config.off, buf, config.len);
+	vdpa_set_config(vdpa, config.off, buf, config_size);
 
 	kvfree(buf);
 	return 0;
-- 
2.29.2

Stefano Garzarella

2021-Feb-16 09:44 UTC

head link

[RFC PATCH 09/10] vhost/vdpa: use get_config_size callback in vhost_vdpa_config_validate()

Let's use the new 'get_config_size()' callback available instead of
using the 'virtio_id' to get the size of the device config space.

Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
---
 drivers/vhost/vdpa.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 544f8582a42b..21eea2be5afa 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -188,13 +188,8 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8
__user *statusp)
 static ssize_t vhost_vdpa_config_validate(struct vhost_vdpa *v,
 					  struct vhost_vdpa_config *c)
 {
-	u32 size = 0;
-
-	switch (v->virtio_id) {
-	case VIRTIO_ID_NET:
-		size = sizeof(struct virtio_net_config);
-		break;
-	}
+	struct vdpa_device *vdpa = v->vdpa;
+	u32 size = vdpa->config->get_config_size(vdpa);
 
 	if (c->len == 0)
 		return -EINVAL;
-- 
2.29.2

Stefano Garzarella

2021-Feb-16 09:44 UTC

head link

[RFC PATCH 10/10] vhost/vdpa: return configuration bytes read and written to user space

vdpa_get_config() and vdpa_set_config() now return the amount
of bytes read and written, so let's return them to the user space.

We also modify vhost_vdpa_config_validate() to return 0 (bytes read
or written) instead of an error, when the buffer length is 0.

Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
---
 drivers/vhost/vdpa.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 21eea2be5afa..b754c53171a7 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -191,9 +191,6 @@ static ssize_t vhost_vdpa_config_validate(struct vhost_vdpa
*v,
 	struct vdpa_device *vdpa = v->vdpa;
 	u32 size = vdpa->config->get_config_size(vdpa);
 
-	if (c->len == 0)
-		return -EINVAL;
-
 	return min(c->len, size);
 }
 
@@ -204,6 +201,7 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,
 	struct vhost_vdpa_config config;
 	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
 	ssize_t config_size;
+	long ret;
 	u8 *buf;
 
 	if (copy_from_user(&config, c, size))
@@ -217,15 +215,18 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,
 	if (!buf)
 		return -ENOMEM;
 
-	vdpa_get_config(vdpa, config.off, buf, config_size);
-
-	if (copy_to_user(c->buf, buf, config_size)) {
-		kvfree(buf);
-		return -EFAULT;
+	ret = vdpa_get_config(vdpa, config.off, buf, config_size);
+	if (ret < 0) {
+		ret = -EFAULT;
+		goto out;
 	}
 
+	if (copy_to_user(c->buf, buf, config_size))
+		ret = -EFAULT;
+
+out:
 	kvfree(buf);
-	return 0;
+	return ret;
 }
 
 static long vhost_vdpa_set_config(struct vhost_vdpa *v,
@@ -235,6 +236,7 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
 	struct vhost_vdpa_config config;
 	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
 	ssize_t config_size;
+	long ret;
 	u8 *buf;
 
 	if (copy_from_user(&config, c, size))
@@ -248,10 +250,12 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
 	if (IS_ERR(buf))
 		return PTR_ERR(buf);
 
-	vdpa_set_config(vdpa, config.off, buf, config_size);
+	ret = vdpa_set_config(vdpa, config.off, buf, config_size);
+	if (ret < 0)
+		ret = -EFAULT;
 
 	kvfree(buf);
-	return 0;
+	return ret;
 }
 
 static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
-- 
2.29.2

Stefano Garzarella

2021-Mar-01 08:17 UTC

head link

[RFC PATCH 00/10] vdpa: get/set_config() rework

nice ping :-)

On Tue, Feb 16, 2021 at 10:44:44AM +0100, Stefano Garzarella
wrote:>Following the discussion with Michael and Jason [1], I reworked a bit
>get/set_config() in vdpa.
>
>I changed vdpa_get_config() to check the boundaries and added
vdpa_set_config().
>When 'offset' or 'len' parameters exceed boundaries, we
limit the reading to
>the available configuration space in the device, and we return the amount of
>bytes read/written.
>
>In this way the user space can pass buffers bigger than config space.
>I also returned the amount of bytes read and written to user space.
>
>Patches also available here:
>https://github.com/stefano-garzarella/linux/tree/vdpa-get-set-config-refactoring
>
>Thanks for your comments,
>Stefano
>
>[1] https://lkml.org/lkml/2021/2/10/350
>
>Stefano Garzarella (10):
>  vdpa: add get_config_size callback in vdpa_config_ops
>  vdpa: check vdpa_get_config() parameters and return bytes read
>  vdpa: add vdpa_set_config() helper
>  vdpa: remove param checks in the get/set_config callbacks
>  vdpa: remove WARN_ON() in the get/set_config callbacks
>  virtio_vdpa: use vdpa_set_config()
>  vhost/vdpa: use vdpa_set_config()
>  vhost/vdpa: allow user space to pass buffers bigger than config space
>  vhost/vdpa: use get_config_size callback in
>    vhost_vdpa_config_validate()
>  vhost/vdpa: return configuration bytes read and written to user space
>
> include/linux/vdpa.h              | 22 ++++-------
> drivers/vdpa/ifcvf/ifcvf_base.c   |  3 +-
> drivers/vdpa/ifcvf/ifcvf_main.c   |  8 +++-
> drivers/vdpa/mlx5/net/mlx5_vnet.c |  9 ++++-
> drivers/vdpa/vdpa.c               | 51 ++++++++++++++++++++++++
> drivers/vdpa/vdpa_sim/vdpa_sim.c  | 15 +++++---
> drivers/vhost/vdpa.c              | 64 ++++++++++++++++---------------
> drivers/virtio/virtio_vdpa.c      |  3 +-
> 8 files changed, 116 insertions(+), 59 deletions(-)
>
>-- 
>2.29.2
>
>_______________________________________________
>Virtualization mailing list
>Virtualization at lists.linux-foundation.org
>https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>

Jason Wang

2021-Mar-02 04:05 UTC

head link

[RFC PATCH 10/10] vhost/vdpa: return configuration bytes read and written to user space

On 2021/2/16 5:44 ??, Stefano Garzarella wrote:> vdpa_get_config() and vdpa_set_config() now return the amount
> of bytes read and written, so let's return them to the user space.
>
> We also modify vhost_vdpa_config_validate() to return 0 (bytes read
> or written) instead of an error, when the buffer length is 0.
>
> Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
> ---
>   drivers/vhost/vdpa.c | 26 +++++++++++++++-----------
>   1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 21eea2be5afa..b754c53171a7 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -191,9 +191,6 @@ static ssize_t vhost_vdpa_config_validate(struct
vhost_vdpa *v,
>   	struct vdpa_device *vdpa = v->vdpa;
>   	u32 size = vdpa->config->get_config_size(vdpa);
>   
> -	if (c->len == 0)
> -		return -EINVAL;
> -
>   	return min(c->len, size);
>   }
>   
> @@ -204,6 +201,7 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,
>   	struct vhost_vdpa_config config;
>   	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
>   	ssize_t config_size;
> +	long ret;
>   	u8 *buf;
>   
>   	if (copy_from_user(&config, c, size))
> @@ -217,15 +215,18 @@ static long vhost_vdpa_get_config(struct vhost_vdpa
*v,
>   	if (!buf)
>   		return -ENOMEM;
>   
> -	vdpa_get_config(vdpa, config.off, buf, config_size);
> -
> -	if (copy_to_user(c->buf, buf, config_size)) {
> -		kvfree(buf);
> -		return -EFAULT;
> +	ret = vdpa_get_config(vdpa, config.off, buf, config_size);
> +	if (ret < 0) {
> +		ret = -EFAULT;
> +		goto out;
>   	}
>   
> +	if (copy_to_user(c->buf, buf, config_size))
> +		ret = -EFAULT;
> +
> +out:
>   	kvfree(buf);
> -	return 0;
> +	return ret;
>   }
>   
>   static long vhost_vdpa_set_config(struct vhost_vdpa *v,
> @@ -235,6 +236,7 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
>   	struct vhost_vdpa_config config;
>   	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
>   	ssize_t config_size;
> +	long ret;
>   	u8 *buf;
>   
>   	if (copy_from_user(&config, c, size))
> @@ -248,10 +250,12 @@ static long vhost_vdpa_set_config(struct vhost_vdpa
*v,
>   	if (IS_ERR(buf))
>   		return PTR_ERR(buf);
>   
> -	vdpa_set_config(vdpa, config.off, buf, config_size);
> +	ret = vdpa_set_config(vdpa, config.off, buf, config_size);
> +	if (ret < 0)
> +		ret = -EFAULT;
>   
>   	kvfree(buf);
> -	return 0;
> +	return ret;
>   }

So I wonder whether it's worth to return the number of bytes since we 
can't propogate the result to driver or driver doesn't care about that.

Thanks

>   
>   static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user
*featurep)

Jason Wang

2021-Mar-02 04:14 UTC

head link

[RFC PATCH 01/10] vdpa: add get_config_size callback in vdpa_config_ops

On 2021/2/16 5:44 ??, Stefano Garzarella wrote:> This new callback is used to get the size of the configuration space
> of vDPA devices.
>
> Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
> ---
>   include/linux/vdpa.h              | 4 ++++
>   drivers/vdpa/ifcvf/ifcvf_main.c   | 6 ++++++
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 6 ++++++
>   drivers/vdpa/vdpa_sim/vdpa_sim.c  | 9 +++++++++
>   4 files changed, 25 insertions(+)
>
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 4ab5494503a8..fddf42b17573 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -150,6 +150,9 @@ struct vdpa_iova_range {
>    * @set_status:			Set the device status
>    *				@vdev: vdpa device
>    *				@status: virtio device status
> + * @get_config_size:		Get the size of the configuration space
> + *				@vdev: vdpa device
> + *				Returns size_t: configuration size

Rethink about this, how much we could gain by introducing a dedicated 
ops here? E.g would it be simpler if we simply introduce a 
max_config_size to vdpa device?

Thanks

>    * @get_config:			Read from device specific configuration space
>    *				@vdev: vdpa device
>    *				@offset: offset from the beginning of
> @@ -231,6 +234,7 @@ struct vdpa_config_ops {
>   	u32 (*get_vendor_id)(struct vdpa_device *vdev);
>   	u8 (*get_status)(struct vdpa_device *vdev);
>   	void (*set_status)(struct vdpa_device *vdev, u8 status);
> +	size_t (*get_config_size)(struct vdpa_device *vdev);
>   	void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
>   			   void *buf, unsigned int len);
>   	void (*set_config)(struct vdpa_device *vdev, unsigned int offset,
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 7c8bbfcf6c3e..2443271e17d2 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -332,6 +332,11 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device
*vdpa_dev)
>   	return IFCVF_QUEUE_ALIGNMENT;
>   }
>   
> +static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
> +{
> +	return sizeof(struct virtio_net_config);
> +}
> +
>   static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev,
>   				  unsigned int offset,
>   				  void *buf, unsigned int len)
> @@ -392,6 +397,7 @@ static const struct vdpa_config_ops ifc_vdpa_ops = {
>   	.get_device_id	= ifcvf_vdpa_get_device_id,
>   	.get_vendor_id	= ifcvf_vdpa_get_vendor_id,
>   	.get_vq_align	= ifcvf_vdpa_get_vq_align,
> +	.get_config_size	= ifcvf_vdpa_get_config_size,
>   	.get_config	= ifcvf_vdpa_get_config,
>   	.set_config	= ifcvf_vdpa_set_config,
>   	.set_config_cb  = ifcvf_vdpa_set_config_cb,
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 10e9b09932eb..78043ee567b6 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1814,6 +1814,11 @@ static void mlx5_vdpa_set_status(struct vdpa_device
*vdev, u8 status)
>   	ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
>   }
>   
> +static size_t mlx5_vdpa_get_config_size(struct vdpa_device *vdev)
> +{
> +	return sizeof(struct virtio_net_config);
> +}
> +
>   static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int
offset, void *buf,
>   				 unsigned int len)
>   {
> @@ -1900,6 +1905,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
>   	.get_vendor_id = mlx5_vdpa_get_vendor_id,
>   	.get_status = mlx5_vdpa_get_status,
>   	.set_status = mlx5_vdpa_set_status,
> +	.get_config_size = mlx5_vdpa_get_config_size,
>   	.get_config = mlx5_vdpa_get_config,
>   	.set_config = mlx5_vdpa_set_config,
>   	.get_generation = mlx5_vdpa_get_generation,
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c
b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index d5942842432d..779ae6c144d7 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -439,6 +439,13 @@ static void vdpasim_set_status(struct vdpa_device
*vdpa, u8 status)
>   	spin_unlock(&vdpasim->lock);
>   }
>   
> +static size_t vdpasim_get_config_size(struct vdpa_device *vdpa)
> +{
> +	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> +
> +	return vdpasim->dev_attr.config_size;
> +}
> +
>   static void vdpasim_get_config(struct vdpa_device *vdpa, unsigned int
offset,
>   			     void *buf, unsigned int len)
>   {
> @@ -566,6 +573,7 @@ static const struct vdpa_config_ops vdpasim_config_ops
= {
>   	.get_vendor_id          = vdpasim_get_vendor_id,
>   	.get_status             = vdpasim_get_status,
>   	.set_status             = vdpasim_set_status,
> +	.get_config_size        = vdpasim_get_config_size,
>   	.get_config             = vdpasim_get_config,
>   	.set_config             = vdpasim_set_config,
>   	.get_generation         = vdpasim_get_generation,
> @@ -593,6 +601,7 @@ static const struct vdpa_config_ops
vdpasim_batch_config_ops = {
>   	.get_vendor_id          = vdpasim_get_vendor_id,
>   	.get_status             = vdpasim_get_status,
>   	.set_status             = vdpasim_set_status,
> +	.get_config_size        = vdpasim_get_config_size,
>   	.get_config             = vdpasim_get_config,
>   	.set_config             = vdpasim_set_config,
>   	.get_generation         = vdpasim_get_generation,

Virtualization - Feb 2021 - [RFC PATCH 00/10] vdpa: get/set_config() rework

[RFC PATCH 00/10] vdpa: get/set_config() rework

[RFC PATCH 01/10] vdpa: add get_config_size callback in vdpa_config_ops

[RFC PATCH 02/10] vdpa: check vdpa_get_config() parameters and return bytes read

[RFC PATCH 03/10] vdpa: add vdpa_set_config() helper

[RFC PATCH 04/10] vdpa: remove param checks in the get/set_config callbacks

[RFC PATCH 05/10] vdpa: remove WARN_ON() in the get/set_config callbacks

[RFC PATCH 06/10] virtio_vdpa: use vdpa_set_config()

[RFC PATCH 07/10] vhost/vdpa: use vdpa_set_config()

[RFC PATCH 08/10] vhost/vdpa: allow user space to pass buffers bigger than config space

[RFC PATCH 09/10] vhost/vdpa: use get_config_size callback in vhost_vdpa_config_validate()

[RFC PATCH 10/10] vhost/vdpa: return configuration bytes read and written to user space

[RFC PATCH 00/10] vdpa: get/set_config() rework

[RFC PATCH 10/10] vhost/vdpa: return configuration bytes read and written to user space

[RFC PATCH 01/10] vdpa: add get_config_size callback in vdpa_config_ops