| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACGkMEvoyi1Bhs9q5ZmO-xvcYeU+ZVJ6EcM_sUtJW-F_349Hqw@mail.gmail.com>
Date: Thu, 5 Sep 2024 10:25:55 +0800
From: Jason Wang <jasowang@...hat.com>
To: Carlos Bilbao <carlos.bilbao.osdev@...il.com>
Cc: dtatulea@...dia.com, mst@...hat.com, shannon.nelson@....com,
sashal@...nel.org, alvaro.karsz@...id-run.com, christophe.jaillet@...adoo.fr,
steven.sistare@...cle.com, bilbao@...edu, xuanzhuo@...ux.alibaba.com,
johnah.palmer@...cle.com, eperezma@...hat.com, cratiu@...dia.com,
virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org,
Carlos Bilbao <cbilbao@...italocean.com>
Subject: Re: [PATCH v3 2/2] vdpa: Remove ioctl VHOST_VDPA_SET_CONFIG per spec compliance
On Wed, Sep 4, 2024 at 11:18 PM Carlos Bilbao
<carlos.bilbao.osdev@...il.com> wrote:
>
> From: Carlos Bilbao <cbilbao@...italocean.com>
>
> Remove invalid ioctl VHOST_VDPA_SET_CONFIG and all its implementations with
> vdpa_config_ops->set_config(). This is needed per virtio spec requirements:
> virtio-spec v1.3 Sec 5.1.4 states that "All of the device configuration
> fields are read-only for the driver.".
Just to make sure we are on the same page.
That part is specific to the virtio-net device. Other types of device
may have a field that could be written. For example the writeback
field of the virtio-blk device when WCE is enabled.
Even for the networking device, it doesn't mean we can't have a
writable field in the future.
So I tend to leave the code as-is.
Thanks
> This excludes legacy Alibaba's ENI
> so make it use vda_config_ops->set_config_legacy() to avoid future
> confusion.
>
> Signed-off-by: Carlos Bilbao <cbilbao@...italocean.com>
> ---
> drivers/vdpa/alibaba/eni_vdpa.c | 2 +-
> drivers/vdpa/ifcvf/ifcvf_main.c | 10 ----------
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 7 -------
> drivers/vdpa/pds/vdpa_dev.c | 16 ----------------
> drivers/vdpa/solidrun/snet_main.c | 18 ------------------
> drivers/vdpa/vdpa.c | 16 ----------------
> drivers/vdpa/vdpa_sim/vdpa_sim.c | 16 ----------------
> drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 -
> drivers/vdpa/vdpa_user/vduse_dev.c | 7 -------
> drivers/vdpa/virtio_pci/vp_vdpa.c | 14 --------------
> drivers/vhost/vdpa.c | 26 --------------------------
> drivers/virtio/virtio_vdpa.c | 9 ---------
> include/linux/vdpa.h | 7 ++++---
> include/uapi/linux/vhost.h | 8 ++++----
> 14 files changed, 9 insertions(+), 148 deletions(-)
>
> diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c
> index cce3d1837104..64b0c0be89ae 100644
> --- a/drivers/vdpa/alibaba/eni_vdpa.c
> +++ b/drivers/vdpa/alibaba/eni_vdpa.c
> @@ -429,7 +429,7 @@ static const struct vdpa_config_ops eni_vdpa_ops = {
> .get_vq_align = eni_vdpa_get_vq_align,
> .get_config_size = eni_vdpa_get_config_size,
> .get_config = eni_vdpa_get_config,
> - .set_config = eni_vdpa_set_config,
> + .set_config_legacy = eni_vdpa_set_config,
> .set_config_cb = eni_vdpa_set_config_cb,
> .get_vq_irq = eni_vdpa_get_vq_irq,
> };
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index e98fa8100f3c..7cbac787ad5f 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -568,15 +568,6 @@ static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev,
> ifcvf_read_dev_config(vf, offset, buf, len);
> }
>
> -static void ifcvf_vdpa_set_config(struct vdpa_device *vdpa_dev,
> - unsigned int offset, const void *buf,
> - unsigned int len)
> -{
> - struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> -
> - ifcvf_write_dev_config(vf, offset, buf, len);
> -}
> -
> static void ifcvf_vdpa_set_config_cb(struct vdpa_device *vdpa_dev,
> struct vdpa_callback *cb)
> {
> @@ -640,7 +631,6 @@ static const struct vdpa_config_ops ifc_vdpa_ops = {
> .get_vq_group = ifcvf_vdpa_get_vq_group,
> .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,
> .get_vq_notification = ifcvf_get_vq_notification,
> };
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 41ca268d43ff..35ed46c65b4d 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2918,12 +2918,6 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
> memcpy(buf, (u8 *)&ndev->config + offset, len);
> }
>
> -static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
> - unsigned int len)
> -{
> - /* not supported */
> -}
> -
> static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
> {
> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> @@ -3218,7 +3212,6 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> .reset = mlx5_vdpa_reset,
> .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,
> .set_map = mlx5_vdpa_set_map,
> .set_group_asid = mlx5_set_group_asid,
> diff --git a/drivers/vdpa/pds/vdpa_dev.c b/drivers/vdpa/pds/vdpa_dev.c
> index 25c0fe5ec3d5..553dcd2aa065 100644
> --- a/drivers/vdpa/pds/vdpa_dev.c
> +++ b/drivers/vdpa/pds/vdpa_dev.c
> @@ -553,21 +553,6 @@ static void pds_vdpa_get_config(struct vdpa_device *vdpa_dev,
> memcpy_fromio(buf, device + offset, len);
> }
>
> -static void pds_vdpa_set_config(struct vdpa_device *vdpa_dev,
> - unsigned int offset, const void *buf,
> - unsigned int len)
> -{
> - struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> - void __iomem *device;
> -
> - if (offset + len > sizeof(struct virtio_net_config)) {
> - WARN(true, "%s: bad read, offset %d len %d\n", __func__, offset, len);
> - return;
> - }
> -
> - device = pdsv->vdpa_aux->vd_mdev.device;
> - memcpy_toio(device + offset, buf, len);
> -}
>
> static const struct vdpa_config_ops pds_vdpa_ops = {
> .set_vq_address = pds_vdpa_set_vq_address,
> @@ -595,7 +580,6 @@ static const struct vdpa_config_ops pds_vdpa_ops = {
> .reset = pds_vdpa_reset,
> .get_config_size = pds_vdpa_get_config_size,
> .get_config = pds_vdpa_get_config,
> - .set_config = pds_vdpa_set_config,
> };
> static struct virtio_device_id pds_vdpa_id_table[] = {
> {VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID},
> diff --git a/drivers/vdpa/solidrun/snet_main.c b/drivers/vdpa/solidrun/snet_main.c
> index 99428a04068d..141740269b6c 100644
> --- a/drivers/vdpa/solidrun/snet_main.c
> +++ b/drivers/vdpa/solidrun/snet_main.c
> @@ -478,23 +478,6 @@ static void snet_get_config(struct vdpa_device *vdev, unsigned int offset,
> *buf_ptr++ = ioread8(cfg_ptr + i);
> }
>
> -static void snet_set_config(struct vdpa_device *vdev, unsigned int offset,
> - const void *buf, unsigned int len)
> -{
> - struct snet *snet = vdpa_to_snet(vdev);
> - void __iomem *cfg_ptr = snet->cfg->virtio_cfg + offset;
> - const u8 *buf_ptr = buf;
> - u32 i;
> -
> - /* check for offset error */
> - if (offset + len > snet->cfg->cfg_size)
> - return;
> -
> - /* Write into PCI BAR */
> - for (i = 0; i < len; i++)
> - iowrite8(*buf_ptr++, cfg_ptr + i);
> -}
> -
> static int snet_suspend(struct vdpa_device *vdev)
> {
> struct snet *snet = vdpa_to_snet(vdev);
> @@ -548,7 +531,6 @@ static const struct vdpa_config_ops snet_config_ops = {
> .get_status = snet_get_status,
> .set_status = snet_set_status,
> .get_config = snet_get_config,
> - .set_config = snet_set_config,
> .suspend = snet_suspend,
> .resume = snet_resume,
> };
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index a7612e0783b3..a9eac31f3757 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -401,22 +401,6 @@ void vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
> }
> EXPORT_SYMBOL_GPL(vdpa_get_config);
>
> -/**
> - * vdpa_set_config - Set one or more device configuration fields.
> - * @vdev: vdpa device to operate on
> - * @offset: starting byte offset of the field
> - * @buf: buffer pointer to read from
> - * @length: length of the configuration fields in bytes
> - */
> -void vdpa_set_config(struct vdpa_device *vdev, unsigned int offset,
> - const void *buf, unsigned int length)
> -{
> - down_write(&vdev->cf_lock);
> - vdev->config->set_config(vdev, offset, buf, length);
> - up_write(&vdev->cf_lock);
> -}
> -EXPORT_SYMBOL_GPL(vdpa_set_config);
> -
> static bool mgmtdev_handle_match(const struct vdpa_mgmt_dev *mdev,
> const char *busname, const char *devname)
> {
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index 421ab01ef06b..c2e14bcc01f6 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -546,20 +546,6 @@ static void vdpasim_get_config(struct vdpa_device *vdpa, unsigned int offset,
> memcpy(buf, vdpasim->config + offset, len);
> }
>
> -static void vdpasim_set_config(struct vdpa_device *vdpa, unsigned int offset,
> - const void *buf, unsigned int len)
> -{
> - 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)
> - vdpasim->dev_attr.set_config(vdpasim, vdpasim->config);
> -}
> -
> static u32 vdpasim_get_generation(struct vdpa_device *vdpa)
> {
> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> @@ -754,7 +740,6 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
> .resume = vdpasim_resume,
> .get_config_size = vdpasim_get_config_size,
> .get_config = vdpasim_get_config,
> - .set_config = vdpasim_set_config,
> .get_generation = vdpasim_get_generation,
> .get_iova_range = vdpasim_get_iova_range,
> .set_group_asid = vdpasim_set_group_asid,
> @@ -792,7 +777,6 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
> .resume = vdpasim_resume,
> .get_config_size = vdpasim_get_config_size,
> .get_config = vdpasim_get_config,
> - .set_config = vdpasim_set_config,
> .get_generation = vdpasim_get_generation,
> .get_iova_range = vdpasim_get_iova_range,
> .set_group_asid = vdpasim_set_group_asid,
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> index bb137e479763..b48bf954a3bb 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> @@ -46,7 +46,6 @@ struct vdpasim_dev_attr {
>
> void (*work_fn)(struct vdpasim *vdpasim);
> void (*get_config)(struct vdpasim *vdpasim, void *config);
> - void (*set_config)(struct vdpasim *vdpasim, const void *config);
> int (*get_stats)(struct vdpasim *vdpasim, u16 idx,
> struct sk_buff *msg,
> struct netlink_ext_ack *extack);
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index df7869537ef1..4fe69cb5b156 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -698,12 +698,6 @@ static void vduse_vdpa_get_config(struct vdpa_device *vdpa, unsigned int offset,
> memcpy(buf, dev->config + offset, len);
> }
>
> -static void vduse_vdpa_set_config(struct vdpa_device *vdpa, unsigned int offset,
> - const void *buf, unsigned int len)
> -{
> - /* Now we only support read-only configuration space */
> -}
> -
> static int vduse_vdpa_reset(struct vdpa_device *vdpa)
> {
> struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> @@ -790,7 +784,6 @@ static const struct vdpa_config_ops vduse_vdpa_config_ops = {
> .set_status = vduse_vdpa_set_status,
> .get_config_size = vduse_vdpa_get_config_size,
> .get_config = vduse_vdpa_get_config,
> - .set_config = vduse_vdpa_set_config,
> .get_generation = vduse_vdpa_get_generation,
> .set_vq_affinity = vduse_vdpa_set_vq_affinity,
> .get_vq_affinity = vduse_vdpa_get_vq_affinity,
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index 281287fae89f..5e8ff91475e3 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -400,19 +400,6 @@ static void vp_vdpa_get_config(struct vdpa_device *vdpa,
> } while (old != new);
> }
>
> -static void vp_vdpa_set_config(struct vdpa_device *vdpa,
> - unsigned int offset, const void *buf,
> - unsigned int len)
> -{
> - struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa);
> - struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa);
> - const u8 *p = buf;
> - int i;
> -
> - for (i = 0; i < len; i++)
> - vp_iowrite8(*p++, mdev->device + offset + i);
> -}
> -
> static void vp_vdpa_set_config_cb(struct vdpa_device *vdpa,
> struct vdpa_callback *cb)
> {
> @@ -457,7 +444,6 @@ static const struct vdpa_config_ops vp_vdpa_ops = {
> .get_vq_align = vp_vdpa_get_vq_align,
> .get_config_size = vp_vdpa_get_config_size,
> .get_config = vp_vdpa_get_config,
> - .set_config = vp_vdpa_set_config,
> .set_config_cb = vp_vdpa_set_config_cb,
> .get_vq_irq = vp_vdpa_get_vq_irq,
> };
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index fb590e346e43..c6fcd54f59be 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -350,29 +350,6 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,
> return 0;
> }
>
> -static long vhost_vdpa_set_config(struct vhost_vdpa *v,
> - struct vhost_vdpa_config __user *c)
> -{
> - struct vdpa_device *vdpa = v->vdpa;
> - struct vhost_vdpa_config config;
> - unsigned long size = offsetof(struct vhost_vdpa_config, buf);
> - 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);
> - if (IS_ERR(buf))
> - return PTR_ERR(buf);
> -
> - vdpa_set_config(vdpa, config.off, buf, config.len);
> -
> - kvfree(buf);
> - return 0;
> -}
> -
> static bool vhost_vdpa_can_suspend(const struct vhost_vdpa *v)
> {
> struct vdpa_device *vdpa = v->vdpa;
> @@ -719,9 +696,6 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> case VHOST_VDPA_GET_CONFIG:
> r = vhost_vdpa_get_config(v, argp);
> break;
> - case VHOST_VDPA_SET_CONFIG:
> - r = vhost_vdpa_set_config(v, argp);
> - break;
> case VHOST_GET_FEATURES:
> r = vhost_vdpa_get_features(v, argp);
> break;
> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> index 06ce6d8c2e00..481ded50c916 100644
> --- a/drivers/virtio/virtio_vdpa.c
> +++ b/drivers/virtio/virtio_vdpa.c
> @@ -62,14 +62,6 @@ static void virtio_vdpa_get(struct virtio_device *vdev, unsigned int offset,
> vdpa_get_config(vdpa, offset, buf, len);
> }
>
> -static void virtio_vdpa_set(struct virtio_device *vdev, unsigned int offset,
> - const void *buf, unsigned int len)
> -{
> - struct vdpa_device *vdpa = vd_get_vdpa(vdev);
> -
> - vdpa_set_config(vdpa, offset, buf, len);
> -}
> -
> static u32 virtio_vdpa_generation(struct virtio_device *vdev)
> {
> struct vdpa_device *vdpa = vd_get_vdpa(vdev);
> @@ -462,7 +454,6 @@ virtio_vdpa_get_vq_affinity(struct virtio_device *vdev, int index)
>
> static const struct virtio_config_ops virtio_vdpa_config_ops = {
> .get = virtio_vdpa_get,
> - .set = virtio_vdpa_set,
> .generation = virtio_vdpa_generation,
> .get_status = virtio_vdpa_get_status,
> .set_status = virtio_vdpa_set_status,
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 0e652026b776..9346fa9e3d97 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -259,7 +259,8 @@ struct vdpa_map_file {
> * @buf: buffer used to read to
> * @len: the length to read from
> * configuration space
> - * @set_config: Write to device specific configuration space
> + * @set_config_legacy: Write to device specific configuration space
> + * Legacy functionality for virtio-spec < v1.3
> * @vdev: vdpa device
> * @offset: offset from the beginning of
> * configuration space
> @@ -378,8 +379,8 @@ struct vdpa_config_ops {
> 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,
> - const void *buf, unsigned int len);
> + void (*set_config_legacy)(struct vdpa_device *vdev,
> + unsigned int offset, const void *buf, unsigned int len);
> u32 (*get_generation)(struct vdpa_device *vdev);
> struct vdpa_iova_range (*get_iova_range)(struct vdpa_device *vdev);
> int (*set_vq_affinity)(struct vdpa_device *vdev, u16 idx,
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index f5c48b61ab62..b7977f9ae596 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -157,13 +157,13 @@
> */
> #define VHOST_VDPA_GET_STATUS _IOR(VHOST_VIRTIO, 0x71, __u8)
> #define VHOST_VDPA_SET_STATUS _IOW(VHOST_VIRTIO, 0x72, __u8)
> -/* Get and set the device config. The device config follows the same
> - * definition of the device config defined in virtio-spec.
> +/* Get the device config. The device config follows the same
> + * definition of the device config defined in virtio-spec. According to
> + * virtio-spec v1.3, all device configuration fields are read-only for the
> + * driver, and thus we do not have VHOST_VDPA_SET_CONFIG.
> */
> #define VHOST_VDPA_GET_CONFIG _IOR(VHOST_VIRTIO, 0x73, \
> struct vhost_vdpa_config)
> -#define VHOST_VDPA_SET_CONFIG _IOW(VHOST_VIRTIO, 0x74, \
> - struct vhost_vdpa_config)
> /* Enable/disable the ring. */
> #define VHOST_VDPA_SET_VRING_ENABLE _IOW(VHOST_VIRTIO, 0x75, \
> struct vhost_vring_state)
> --
> 2.34.1
>
Powered by blists - more mailing lists