Xuan Zhuo
2023-Oct-08 09:38 UTC
[PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size
Now, the function vp_modern_map_capability() takes the size parameter,
which corresponds to the size of virtio_pci_common_cfg. As a result,
this indicates the size of memory area to map.
However, if the _F_RING_RESET is negotiated, the extra items will be
used. Therefore, we need to use the size of virtio_pci_modern_common_cfg
to map more space.
Meanwhile, this patch removes the feature(_F_RING_ERSET and
_F_NOTIFICATION_DATA) when the common cfg size does not match
the corresponding feature.
Fixes: 0b50cece0b78 ("virtio_pci: introduce helper to get/set queue
reset")
Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com>
---
drivers/virtio/virtio_pci_modern.c | 20 +++++++++++++++++++-
drivers/virtio/virtio_pci_modern_dev.c | 4 ++--
include/linux/virtio_pci_modern.h | 1 +
3 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
index d6bb68ba84e5..c0b9d2363ddb 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -22,8 +22,26 @@
static u64 vp_get_features(struct virtio_device *vdev)
{
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+ u64 features = vp_modern_get_features(&vp_dev->mdev);
+
+#define check_feature(feature, field) \
+ do { \
+ if (features & BIT_ULL(feature)) { \
+ u32 offset = offsetofend(struct virtio_pci_modern_common_cfg, field); \
+ if (unlikely(vp_dev->mdev.common_len < offset)) \
+ features &= ~BIT_ULL(feature); \
+ } \
+ } while (0)
+
+ /* For buggy devices, if the common len does not match the feature, we
+ * remove the feature.
+ */
+ check_feature(VIRTIO_F_NOTIFICATION_DATA, queue_notify_data);
+ check_feature(VIRTIO_F_RING_RESET, queue_reset);
+
+#undef check_feature
- return vp_modern_get_features(&vp_dev->mdev);
+ return features;
}
static void vp_transport_features(struct virtio_device *vdev, u64 features)
diff --git a/drivers/virtio/virtio_pci_modern_dev.c
b/drivers/virtio/virtio_pci_modern_dev.c
index aad7d9296e77..33f319da1558 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -291,8 +291,8 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
err = -EINVAL;
mdev->common = vp_modern_map_capability(mdev, common,
sizeof(struct virtio_pci_common_cfg), 4,
- 0, sizeof(struct virtio_pci_common_cfg),
- NULL, NULL);
+ 0, sizeof(struct virtio_pci_modern_common_cfg),
+ &mdev->common_len, NULL);
if (!mdev->common)
goto err_map_common;
mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1,
diff --git a/include/linux/virtio_pci_modern.h
b/include/linux/virtio_pci_modern.h
index 067ac1d789bc..edf62bae0474 100644
--- a/include/linux/virtio_pci_modern.h
+++ b/include/linux/virtio_pci_modern.h
@@ -28,6 +28,7 @@ struct virtio_pci_modern_device {
/* So we can sanity-check accesses. */
size_t notify_len;
size_t device_len;
+ size_t common_len;
/* Capability for when we need to map notifications per-vq. */
int notify_map_cap;
--
2.32.0.3.g01195cf9f
Michael S. Tsirkin
2023-Oct-08 10:42 UTC
[PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size
On Sun, Oct 08, 2023 at 05:38:41PM +0800, Xuan Zhuo wrote:> Now, the function vp_modern_map_capability() takes the size parameter, > which corresponds to the size of virtio_pci_common_cfg. As a result, > this indicates the size of memory area to map. > > However, if the _F_RING_RESET is negotiated, the extra items will be > used. Therefore, we need to use the size of virtio_pci_modern_common_cfg > to map more space. > > Meanwhile, this patch removes the feature(_F_RING_ERSET andtypo> _F_NOTIFICATION_DATA) when the common cfg size does not match > the corresponding feature. > > Fixes: 0b50cece0b78 ("virtio_pci: introduce helper to get/set queue reset") > Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com> > --- > drivers/virtio/virtio_pci_modern.c | 20 +++++++++++++++++++- > drivers/virtio/virtio_pci_modern_dev.c | 4 ++-- > include/linux/virtio_pci_modern.h | 1 + > 3 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > index d6bb68ba84e5..c0b9d2363ddb 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -22,8 +22,26 @@ > static u64 vp_get_features(struct virtio_device *vdev) > { > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > + u64 features = vp_modern_get_features(&vp_dev->mdev); > + > +#define check_feature(feature, field) \ > + do { \ > + if (features & BIT_ULL(feature)) { \ > + u32 offset = offsetofend(struct virtio_pci_modern_common_cfg, field); \ > + if (unlikely(vp_dev->mdev.common_len < offset)) \ > + features &= ~BIT_ULL(feature); \ > + } \ > + } while (0) > + > + /* For buggy devices, if the common len does not match the feature, we > + * remove the feature.I don't like doing this in vp_get_features. userspace won't be able to see the actual device features at all. Also, we should print an info message at least. I am still debating with myself whether clearing feature bits or just failing finalize_features (and thus, probe) is best.> + */ > + check_feature(VIRTIO_F_NOTIFICATION_DATA, queue_notify_data); > + check_feature(VIRTIO_F_RING_RESET, queue_reset); > + > +#undef check_featurethis macro's too scary. just pass offset and feature bit as parameters to an inline function.> > - return vp_modern_get_features(&vp_dev->mdev); > + return features; > } > > static void vp_transport_features(struct virtio_device *vdev, u64 features) > diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c > index aad7d9296e77..33f319da1558 100644 > --- a/drivers/virtio/virtio_pci_modern_dev.c > +++ b/drivers/virtio/virtio_pci_modern_dev.c > @@ -291,8 +291,8 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev) > err = -EINVAL; > mdev->common = vp_modern_map_capability(mdev, common, > sizeof(struct virtio_pci_common_cfg), 4, > - 0, sizeof(struct virtio_pci_common_cfg), > - NULL, NULL); > + 0, sizeof(struct virtio_pci_modern_common_cfg), > + &mdev->common_len, NULL); > if (!mdev->common) > goto err_map_common; > mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1, > diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h > index 067ac1d789bc..edf62bae0474 100644 > --- a/include/linux/virtio_pci_modern.h > +++ b/include/linux/virtio_pci_modern.h > @@ -28,6 +28,7 @@ struct virtio_pci_modern_device { > /* So we can sanity-check accesses. */ > size_t notify_len; > size_t device_len; > + size_t common_len; > > /* Capability for when we need to map notifications per-vq. */ > int notify_map_cap; > -- > 2.32.0.3.g01195cf9f