* [PATCH vhost v2 0/2] strictly check the acccess to the common cfg @ 2023-10-08 9:38 Xuan Zhuo 2023-10-08 9:38 ` [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size Xuan Zhuo 2023-10-08 9:38 ` [PATCH vhost v2 2/2] virtio_pci: add build offset check for the new common cfg items Xuan Zhuo 0 siblings, 2 replies; 6+ messages in thread From: Xuan Zhuo @ 2023-10-08 9:38 UTC (permalink / raw) To: virtualization; +Cc: Xuan Zhuo, Michael S. Tsirkin 1. fix the length of the pci_iomap_range() to the common cfg 2. add size check for the vq-reset 3. add build size check to the new common cfg items Xuan Zhuo (2): virtio_pci: fix the common map size and add check for common size virtio_pci: add build offset check for the new common cfg items drivers/virtio/virtio_pci_modern.c | 20 +++++++++++++++++++- drivers/virtio/virtio_pci_modern_dev.c | 8 ++++++-- include/linux/virtio_pci_modern.h | 1 + 3 files changed, 26 insertions(+), 3 deletions(-) -- 2.32.0.3.g01195cf9f _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size 2023-10-08 9:38 [PATCH vhost v2 0/2] strictly check the acccess to the common cfg Xuan Zhuo @ 2023-10-08 9:38 ` Xuan Zhuo 2023-10-08 10:42 ` Michael S. Tsirkin 2023-10-08 9:38 ` [PATCH vhost v2 2/2] virtio_pci: add build offset check for the new common cfg items Xuan Zhuo 1 sibling, 1 reply; 6+ messages in thread From: Xuan Zhuo @ 2023-10-08 9:38 UTC (permalink / raw) To: virtualization; +Cc: Xuan Zhuo, Michael S. Tsirkin 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@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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size 2023-10-08 9:38 ` [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size Xuan Zhuo @ 2023-10-08 10:42 ` Michael S. Tsirkin 2023-10-09 1:15 ` Xuan Zhuo 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2023-10-08 10:42 UTC (permalink / raw) To: Xuan Zhuo; +Cc: virtualization 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 <xuanzhuo@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_feature this 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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size 2023-10-08 10:42 ` Michael S. Tsirkin @ 2023-10-09 1:15 ` Xuan Zhuo 2023-10-09 10:37 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: Xuan Zhuo @ 2023-10-09 1:15 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: virtualization On Sun, 8 Oct 2023 06:42:37 -0400, "Michael S. Tsirkin" <mst@redhat.com> 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 <xuanzhuo@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. 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(&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 > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size 2023-10-09 1:15 ` Xuan Zhuo @ 2023-10-09 10:37 ` Michael S. Tsirkin 0 siblings, 0 replies; 6+ messages in thread From: Michael S. Tsirkin @ 2023-10-09 10:37 UTC (permalink / raw) To: Xuan Zhuo; +Cc: virtualization 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" <mst@redhat.com> 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 <xuanzhuo@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. > > 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(&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 > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH vhost v2 2/2] virtio_pci: add build offset check for the new common cfg items 2023-10-08 9:38 [PATCH vhost v2 0/2] strictly check the acccess to the common cfg Xuan Zhuo 2023-10-08 9:38 ` [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size Xuan Zhuo @ 2023-10-08 9:38 ` Xuan Zhuo 1 sibling, 0 replies; 6+ messages in thread From: Xuan Zhuo @ 2023-10-08 9:38 UTC (permalink / raw) To: virtualization; +Cc: Xuan Zhuo, Michael S. Tsirkin Add checks to the check_offsets(void) for queue_notify_data and queue_reset. Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Acked-by: Jason Wang <jasowang@redhat.com> --- drivers/virtio/virtio_pci_modern_dev.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c index 33f319da1558..e2a1fe7bb66c 100644 --- a/drivers/virtio/virtio_pci_modern_dev.c +++ b/drivers/virtio/virtio_pci_modern_dev.c @@ -203,6 +203,10 @@ static inline void check_offsets(void) offsetof(struct virtio_pci_common_cfg, queue_used_lo)); BUILD_BUG_ON(VIRTIO_PCI_COMMON_Q_USEDHI != offsetof(struct virtio_pci_common_cfg, queue_used_hi)); + BUILD_BUG_ON(VIRTIO_PCI_COMMON_Q_NDATA != + offsetof(struct virtio_pci_modern_common_cfg, queue_notify_data)); + BUILD_BUG_ON(VIRTIO_PCI_COMMON_Q_RESET != + offsetof(struct virtio_pci_modern_common_cfg, queue_reset)); } /* -- 2.32.0.3.g01195cf9f _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-10-09 10:37 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-08 9:38 [PATCH vhost v2 0/2] strictly check the acccess to the common cfg Xuan Zhuo 2023-10-08 9:38 ` [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size Xuan Zhuo 2023-10-08 10:42 ` Michael S. Tsirkin 2023-10-09 1:15 ` Xuan Zhuo 2023-10-09 10:37 ` Michael S. Tsirkin 2023-10-08 9:38 ` [PATCH vhost v2 2/2] virtio_pci: add build offset check for the new common cfg items Xuan Zhuo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.