subject:"\[PATCH vhost v2 1\/2\] virtio_pci\: fix the common map size and add check for common size"

Re: [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size

2023-10-09 Thread Michael S. Tsirkin

On Mon, Oct 09, 2023 at 09:15:31AM +0800, Xuan Zhuo wrote:
> On Sun, 8 Oct 2023 06:42:37 -0400, "Michael S. Tsirkin"  
> wrote:
> > 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 and
> >
> > typo
> >
> > > _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 
> > > ---
> > >  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(_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.
> 
> For me, I think failing probe is best.
> 
> Then the developer of the device can find that firstly.
> And I think we should print an info message when we detect
> this error.

If you fail probe - maybe even a warning.

> If we clear the feature bits, the developer of the device may
> ignore this error.
> 
> >
> >
> > > +  */
> > > + check_feature(VIRTIO_F_NOTIFICATION_DATA, queue_notify_data);
> > > + check_feature(VIRTIO_F_RING_RESET, queue_reset);
> > > +
> > > +#undef check_feature
> >
> > this macro's too scary. just pass offset and feature bit as
> > parameters to an inline function.
> 
> I should pass the features as a parameter.
> 
> Thanks.
> 
> 
> 
> >
> > >
> > > - return vp_modern_get_features(_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),
> > > +   >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;
> > > +

Re: [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size

2023-10-08 Thread Xuan Zhuo

On Sun, 8 Oct 2023 06:42:37 -0400, "Michael S. Tsirkin"  wrote:
> 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 and
>
> typo
>
> > _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 
> > ---
> >  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(_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.

For me, I think failing probe is best.

Then the developer of the device can find that firstly.
And I think we should print an info message when we detect
this error.

If we clear the feature bits, the developer of the device may
ignore this error.

>
>
> > +*/
> > +   check_feature(VIRTIO_F_NOTIFICATION_DATA, queue_notify_data);
> > +   check_feature(VIRTIO_F_RING_RESET, queue_reset);
> > +
> > +#undef check_feature
>
> this macro's too scary. just pass offset and feature bit as
> parameters to an inline function.

I should pass the features as a parameter.

Thanks.



>
> >
> > -   return vp_modern_get_features(_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),
> > + >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
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org

Re: [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size

2023-10-08 Thread Michael S. Tsirkin

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 and

typo

> _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 
> ---
>  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(_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_feature

this macro's too scary. just pass offset and feature bit as
parameters to an inline function.

>  
> - return vp_modern_get_features(_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),
> +   >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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size

2023-10-08 Thread Xuan Zhuo

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 
---
 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(_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(_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),
+ >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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size

Re: [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size

Re: [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size

[PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size

4 matches

Site Navigation

Mail list logo

Footer information