[PATCH] virtio_ring: aovid reading flag from the descriptor ring

[PATCH] virtio_ring: aovid reading flag from the descriptor ring

[Jason Wang]

Commit 72b5e8958738 ("virtio-ring: store DMA metadata in desc_extra
for split virtqueue") tries to make it possible for the driver to not
read from the descriptor ring to prevent the device from corrupting
the descriptor ring. But it still read the descriptor flag from the
descriptor ring during buffer detach.

This patch fixes by always store the descriptor flag no matter whether
DMA API is used and then we can avoid reading descriptor flag from the
descriptor ring. This eliminates the possibly of unexpected next
descriptor caused by the wrong flag (e.g the next flag).

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_ring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 00f64f2f8b72..28734f4e57d3 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -583,7 +583,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 	}
 	/* Last one doesn't continue. */
 	desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
-	if (!indirect && vq->use_dma_api)
+	if (!indirect)
 		vq->split.desc_extra[prev & (vq->split.vring.num - 1)].flags &=
 			~VRING_DESC_F_NEXT;
 
@@ -713,7 +713,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
 	/* Put back on free list: unmap first-level descriptors and find end */
 	i = head;
 
-	while (vq->split.vring.desc[i].flags & nextflag) {
+	while (vq->split.desc_extra[i].flags & nextflag) {
 		vring_unmap_one_split(vq, i);
 		i = vq->split.desc_extra[i].next;
 		vq->vq.num_free++;
-- 
2.25.1

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

Re: [PATCH] virtio_ring: aovid reading flag from the descriptor ring

[Jason Wang]

On Mon, Nov 8, 2021 at 4:13 PM Jason Wang <jasowang@redhat.com> wrote:
>
> Commit 72b5e8958738 ("virtio-ring: store DMA metadata in desc_extra
> for split virtqueue") tries to make it possible for the driver to not
> read from the descriptor ring to prevent the device from corrupting
> the descriptor ring. But it still read the descriptor flag from the
> descriptor ring during buffer detach.
>
> This patch fixes by always store the descriptor flag no matter whether
> DMA API is used and then we can avoid reading descriptor flag from the
> descriptor ring. This eliminates the possibly of unexpected next
> descriptor caused by the wrong flag (e.g the next flag).
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Michael, any comment for this?

Thanks

> ---
>  drivers/virtio/virtio_ring.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 00f64f2f8b72..28734f4e57d3 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -583,7 +583,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>         }
>         /* Last one doesn't continue. */
>         desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
> -       if (!indirect && vq->use_dma_api)
> +       if (!indirect)
>                 vq->split.desc_extra[prev & (vq->split.vring.num - 1)].flags &=
>                         ~VRING_DESC_F_NEXT;
>
> @@ -713,7 +713,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
>         /* Put back on free list: unmap first-level descriptors and find end */
>         i = head;
>
> -       while (vq->split.vring.desc[i].flags & nextflag) {
> +       while (vq->split.desc_extra[i].flags & nextflag) {
>                 vring_unmap_one_split(vq, i);
>                 i = vq->split.desc_extra[i].next;
>                 vq->vq.num_free++;
> --
> 2.25.1
>

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

Re: [PATCH] virtio_ring: aovid reading flag from the descriptor ring

[Michael S. Tsirkin]

On Wed, Feb 23, 2022 at 11:19:03AM +0800, Jason Wang wrote:
> On Mon, Nov 8, 2021 at 4:13 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > Commit 72b5e8958738 ("virtio-ring: store DMA metadata in desc_extra
> > for split virtqueue") tries to make it possible for the driver to not
> > read from the descriptor ring to prevent the device from corrupting
> > the descriptor ring. But it still read the descriptor flag from the
> > descriptor ring during buffer detach.
> >
> > This patch fixes by always store the descriptor flag no matter whether
> > DMA API is used and then we can avoid reading descriptor flag from the
> > descriptor ring. This eliminates the possibly of unexpected next
> > descriptor caused by the wrong flag (e.g the next flag).
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> 
> Michael, any comment for this?
> 
> Thanks

I don't exactly see why we should care without DMA API, it seems
cleaner not to poke at the array one extra time.

> > ---
> >  drivers/virtio/virtio_ring.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 00f64f2f8b72..28734f4e57d3 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -583,7 +583,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> >         }
> >         /* Last one doesn't continue. */
> >         desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
> > -       if (!indirect && vq->use_dma_api)
> > +       if (!indirect)
> >                 vq->split.desc_extra[prev & (vq->split.vring.num - 1)].flags &=
> >                         ~VRING_DESC_F_NEXT;
> >
> > @@ -713,7 +713,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> >         /* Put back on free list: unmap first-level descriptors and find end */
> >         i = head;
> >
> > -       while (vq->split.vring.desc[i].flags & nextflag) {
> > +       while (vq->split.desc_extra[i].flags & nextflag) {
> >                 vring_unmap_one_split(vq, i);
> >                 i = vq->split.desc_extra[i].next;
> >                 vq->vq.num_free++;
> > --
> > 2.25.1
> >

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

Re: [PATCH] virtio_ring: aovid reading flag from the descriptor ring

[Jason Wang]

On Wed, Feb 23, 2022 at 3:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Feb 23, 2022 at 11:19:03AM +0800, Jason Wang wrote:
> > On Mon, Nov 8, 2021 at 4:13 PM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > Commit 72b5e8958738 ("virtio-ring: store DMA metadata in desc_extra
> > > for split virtqueue") tries to make it possible for the driver to not
> > > read from the descriptor ring to prevent the device from corrupting
> > > the descriptor ring. But it still read the descriptor flag from the
> > > descriptor ring during buffer detach.
> > >
> > > This patch fixes by always store the descriptor flag no matter whether
> > > DMA API is used and then we can avoid reading descriptor flag from the
> > > descriptor ring. This eliminates the possibly of unexpected next
> > > descriptor caused by the wrong flag (e.g the next flag).
> > >
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> > Michael, any comment for this?
> >
> > Thanks
>
> I don't exactly see why we should care without DMA API, it seems
> cleaner not to poke at the array one extra time.

I think the answer is that we have any special care about the DMA API
for all other places that are using desc_extra.

Thanks


>
> > > ---
> > >  drivers/virtio/virtio_ring.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 00f64f2f8b72..28734f4e57d3 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -583,7 +583,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > >         }
> > >         /* Last one doesn't continue. */
> > >         desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
> > > -       if (!indirect && vq->use_dma_api)
> > > +       if (!indirect)
> > >                 vq->split.desc_extra[prev & (vq->split.vring.num - 1)].flags &=
> > >                         ~VRING_DESC_F_NEXT;
> > >
> > > @@ -713,7 +713,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> > >         /* Put back on free list: unmap first-level descriptors and find end */
> > >         i = head;
> > >
> > > -       while (vq->split.vring.desc[i].flags & nextflag) {
> > > +       while (vq->split.desc_extra[i].flags & nextflag) {
> > >                 vring_unmap_one_split(vq, i);
> > >                 i = vq->split.desc_extra[i].next;
> > >                 vq->vq.num_free++;
> > > --
> > > 2.25.1
> > >
>

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

Re: [PATCH] virtio_ring: aovid reading flag from the descriptor ring

[Jason Wang]

On Wed, Feb 23, 2022 at 3:34 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Feb 23, 2022 at 3:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Feb 23, 2022 at 11:19:03AM +0800, Jason Wang wrote:
> > > On Mon, Nov 8, 2021 at 4:13 PM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > Commit 72b5e8958738 ("virtio-ring: store DMA metadata in desc_extra
> > > > for split virtqueue") tries to make it possible for the driver to not
> > > > read from the descriptor ring to prevent the device from corrupting
> > > > the descriptor ring. But it still read the descriptor flag from the
> > > > descriptor ring during buffer detach.
> > > >
> > > > This patch fixes by always store the descriptor flag no matter whether
> > > > DMA API is used and then we can avoid reading descriptor flag from the
> > > > descriptor ring. This eliminates the possibly of unexpected next
> > > > descriptor caused by the wrong flag (e.g the next flag).
> > > >
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >
> > > Michael, any comment for this?
> > >
> > > Thanks
> >
> > I don't exactly see why we should care without DMA API, it seems
> > cleaner not to poke at the array one extra time.
>
> I think the answer is that we have any special care about the DMA API

I meant "we haven't had" actually.

Thanks

> for all other places that are using desc_extra.
>
> Thanks
>
>
> >
> > > > ---
> > > >  drivers/virtio/virtio_ring.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 00f64f2f8b72..28734f4e57d3 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -583,7 +583,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > > >         }
> > > >         /* Last one doesn't continue. */
> > > >         desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
> > > > -       if (!indirect && vq->use_dma_api)
> > > > +       if (!indirect)
> > > >                 vq->split.desc_extra[prev & (vq->split.vring.num - 1)].flags &=
> > > >                         ~VRING_DESC_F_NEXT;
> > > >
> > > > @@ -713,7 +713,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> > > >         /* Put back on free list: unmap first-level descriptors and find end */
> > > >         i = head;
> > > >
> > > > -       while (vq->split.vring.desc[i].flags & nextflag) {
> > > > +       while (vq->split.desc_extra[i].flags & nextflag) {
> > > >                 vring_unmap_one_split(vq, i);
> > > >                 i = vq->split.desc_extra[i].next;
> > > >                 vq->vq.num_free++;
> > > > --
> > > > 2.25.1
> > > >
> >

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

Re: [PATCH] virtio_ring: aovid reading flag from the descriptor ring

[Michael S. Tsirkin]

On Wed, Feb 23, 2022 at 03:50:07PM +0800, Jason Wang wrote:
> On Wed, Feb 23, 2022 at 3:34 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Feb 23, 2022 at 3:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Wed, Feb 23, 2022 at 11:19:03AM +0800, Jason Wang wrote:
> > > > On Mon, Nov 8, 2021 at 4:13 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > Commit 72b5e8958738 ("virtio-ring: store DMA metadata in desc_extra
> > > > > for split virtqueue") tries to make it possible for the driver to not
> > > > > read from the descriptor ring to prevent the device from corrupting
> > > > > the descriptor ring. But it still read the descriptor flag from the
> > > > > descriptor ring during buffer detach.
> > > > >
> > > > > This patch fixes by always store the descriptor flag no matter whether
> > > > > DMA API is used and then we can avoid reading descriptor flag from the
> > > > > descriptor ring. This eliminates the possibly of unexpected next
> > > > > descriptor caused by the wrong flag (e.g the next flag).
> > > > >
> > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > >
> > > > Michael, any comment for this?
> > > >
> > > > Thanks
> > >
> > > I don't exactly see why we should care without DMA API, it seems
> > > cleaner not to poke at the array one extra time.
> >
> > I think the answer is that we have any special care about the DMA API
> 
> I meant "we haven't had" actually.
> 
> Thanks

I'm just asking what's better for performance. An extra write in the
first chunk has a cost. Want to test and see?

> > for all other places that are using desc_extra.
> >
> > Thanks
> >
> >
> > >
> > > > > ---
> > > > >  drivers/virtio/virtio_ring.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > index 00f64f2f8b72..28734f4e57d3 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -583,7 +583,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > > > >         }
> > > > >         /* Last one doesn't continue. */
> > > > >         desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
> > > > > -       if (!indirect && vq->use_dma_api)
> > > > > +       if (!indirect)
> > > > >                 vq->split.desc_extra[prev & (vq->split.vring.num - 1)].flags &=
> > > > >                         ~VRING_DESC_F_NEXT;
> > > > >
> > > > > @@ -713,7 +713,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> > > > >         /* Put back on free list: unmap first-level descriptors and find end */
> > > > >         i = head;
> > > > >
> > > > > -       while (vq->split.vring.desc[i].flags & nextflag) {
> > > > > +       while (vq->split.desc_extra[i].flags & nextflag) {
> > > > >                 vring_unmap_one_split(vq, i);
> > > > >                 i = vq->split.desc_extra[i].next;
> > > > >                 vq->vq.num_free++;
> > > > > --
> > > > > 2.25.1
> > > > >
> > >

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

Re: [PATCH] virtio_ring: aovid reading flag from the descriptor ring

[Jason Wang]

On Fri, Feb 25, 2022 at 1:26 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Feb 23, 2022 at 03:50:07PM +0800, Jason Wang wrote:
> > On Wed, Feb 23, 2022 at 3:34 PM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Feb 23, 2022 at 3:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Wed, Feb 23, 2022 at 11:19:03AM +0800, Jason Wang wrote:
> > > > > On Mon, Nov 8, 2021 at 4:13 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > Commit 72b5e8958738 ("virtio-ring: store DMA metadata in desc_extra
> > > > > > for split virtqueue") tries to make it possible for the driver to not
> > > > > > read from the descriptor ring to prevent the device from corrupting
> > > > > > the descriptor ring. But it still read the descriptor flag from the
> > > > > > descriptor ring during buffer detach.
> > > > > >
> > > > > > This patch fixes by always store the descriptor flag no matter whether
> > > > > > DMA API is used and then we can avoid reading descriptor flag from the
> > > > > > descriptor ring. This eliminates the possibly of unexpected next
> > > > > > descriptor caused by the wrong flag (e.g the next flag).
> > > > > >
> > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > >
> > > > > Michael, any comment for this?
> > > > >
> > > > > Thanks
> > > >
> > > > I don't exactly see why we should care without DMA API, it seems
> > > > cleaner not to poke at the array one extra time.
> > >
> > > I think the answer is that we have any special care about the DMA API
> >
> > I meant "we haven't had" actually.
> >
> > Thanks
>
> I'm just asking what's better for performance. An extra write in the
> first chunk has a cost. Want to test and see?

I will do it.

Thanks

>
> > > for all other places that are using desc_extra.
> > >
> > > Thanks
> > >
> > >
> > > >
> > > > > > ---
> > > > > >  drivers/virtio/virtio_ring.c | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > > index 00f64f2f8b72..28734f4e57d3 100644
> > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > @@ -583,7 +583,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > > > > >         }
> > > > > >         /* Last one doesn't continue. */
> > > > > >         desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
> > > > > > -       if (!indirect && vq->use_dma_api)
> > > > > > +       if (!indirect)
> > > > > >                 vq->split.desc_extra[prev & (vq->split.vring.num - 1)].flags &=
> > > > > >                         ~VRING_DESC_F_NEXT;
> > > > > >
> > > > > > @@ -713,7 +713,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> > > > > >         /* Put back on free list: unmap first-level descriptors and find end */
> > > > > >         i = head;
> > > > > >
> > > > > > -       while (vq->split.vring.desc[i].flags & nextflag) {
> > > > > > +       while (vq->split.desc_extra[i].flags & nextflag) {
> > > > > >                 vring_unmap_one_split(vq, i);
> > > > > >                 i = vq->split.desc_extra[i].next;
> > > > > >                 vq->vq.num_free++;
> > > > > > --
> > > > > > 2.25.1
> > > > > >
> > > >
>

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

Re: [PATCH] virtio_ring: aovid reading flag from the descriptor ring

[Michael S. Tsirkin]

Typo in the subject btw.

minor tweaks to commit log below

On Mon, Nov 08, 2021 at 04:13:24PM +0800, Jason Wang wrote:
> Commit 72b5e8958738 ("virtio-ring: store DMA metadata in desc_extra
> for split virtqueue") tries to make it possible for the driver to not
> read from the descriptor ring to prevent the device from corrupting
> the descriptor ring. But it still read 

reads

>the descriptor flag from the
> descriptor ring during buffer detach.
> 
> This patch fixes 

fixes this

>by always store 

storing

>the descriptor flag no matter whether
> DMA API is used and then we can avoid reading descriptor flag from the
> descriptor ring. This eliminates the possibly of unexpected next
> descriptor caused by the wrong flag (e.g the next flag).
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>


I'd also like the commit log to document what the issue is in a bit more depth.
I think the main reason we are checking the dma API is this


static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
                                          unsigned int i)
{
        struct vring_desc_extra *extra = vq->split.desc_extra;
        u16 flags;

        if (!vq->use_dma_api)
                goto out;

	...
}


so I guess with a bad flag, what will happen is num_free will become too
big is that right?




> ---
>  drivers/virtio/virtio_ring.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 00f64f2f8b72..28734f4e57d3 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -583,7 +583,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>  	}
>  	/* Last one doesn't continue. */
>  	desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
> -	if (!indirect && vq->use_dma_api)
> +	if (!indirect)
>  		vq->split.desc_extra[prev & (vq->split.vring.num - 1)].flags &=
>  			~VRING_DESC_F_NEXT;
>  

BTW I'm a bit confused why we need the & (vq->split.vring.num - 1) logic.
Maybe it's time we stopped writing out descriptor then overwriting it -
e.g. return the desc_extra pointer from virtqueue_add_desc_split
instead of an index. Worth checking what this does to performance.


> @@ -713,7 +713,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
>  	/* Put back on free list: unmap first-level descriptors and find end */
>  	i = head;
>  
> -	while (vq->split.vring.desc[i].flags & nextflag) {
> +	while (vq->split.desc_extra[i].flags & nextflag) {
>  		vring_unmap_one_split(vq, i);
>  		i = vq->split.desc_extra[i].next;
>  		vq->vq.num_free++;
> -- 
> 2.25.1

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

Re: [PATCH] virtio_ring: aovid reading flag from the descriptor ring

[Jason Wang]

On Fri, Feb 25, 2022 at 1:55 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> Typo in the subject btw.
>
> minor tweaks to commit log below
>
> On Mon, Nov 08, 2021 at 04:13:24PM +0800, Jason Wang wrote:
> > Commit 72b5e8958738 ("virtio-ring: store DMA metadata in desc_extra
> > for split virtqueue") tries to make it possible for the driver to not
> > read from the descriptor ring to prevent the device from corrupting
> > the descriptor ring. But it still read
>
> reads
>
> >the descriptor flag from the
> > descriptor ring during buffer detach.
> >
> > This patch fixes
>
> fixes this
>
> >by always store
>
> storing
>

Will fix.

> >the descriptor flag no matter whether
> > DMA API is used and then we can avoid reading descriptor flag from the
> > descriptor ring. This eliminates the possibly of unexpected next
> > descriptor caused by the wrong flag (e.g the next flag).
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
>
> I'd also like the commit log to document what the issue is in a bit more depth.
> I think the main reason we are checking the dma API is this
>
>
> static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
>                                           unsigned int i)
> {
>         struct vring_desc_extra *extra = vq->split.desc_extra;
>         u16 flags;
>
>         if (!vq->use_dma_api)
>                 goto out;
>
>         ...
> }
>
>
> so I guess with a bad flag, what will happen is num_free will become too
> big is that right?

Yes, and it may have other implications and this is not easy to answer
since with the current code, next/flag is the only one that can be
controlled by the device. From what I've seen so far, it can cause an
early unmap for the descriptors in the free list. This may cause
several issues when using DMA API (e.g IOTLB for bouncing). Indeed
software IOMMU/IOTLB has done a lot of checks for this but we can't
solely depend on IOMMU/IOTLB to handle malicious inputs.

So my understanding is that, instead of trying to figuring out what
may happens if some flag or descriptor is modified by the malicious
device/hypervisor, we can simply eliminate all those possibilities
with minimal efforts and this is why we try not read anything from
shared memory area (e.g the descriptor ring etc) if possible.

>
>
>
>
> > ---
> >  drivers/virtio/virtio_ring.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 00f64f2f8b72..28734f4e57d3 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -583,7 +583,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> >       }
> >       /* Last one doesn't continue. */
> >       desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
> > -     if (!indirect && vq->use_dma_api)
> > +     if (!indirect)
> >               vq->split.desc_extra[prev & (vq->split.vring.num - 1)].flags &=
> >                       ~VRING_DESC_F_NEXT;
> >
>
> BTW I'm a bit confused why we need the & (vq->split.vring.num - 1) logic.
> Maybe it's time we stopped writing out descriptor then overwriting it -

Right since the next can be read from the descriptor ring directly (as
you said below, this needs to be fixed as well).

> e.g. return the desc_extra pointer from virtqueue_add_desc_split
> instead of an index. Worth checking what this does to performance.

Right, let me try to do that in the next version.

Thanks

>
>
> > @@ -713,7 +713,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> >       /* Put back on free list: unmap first-level descriptors and find end */
> >       i = head;
> >
> > -     while (vq->split.vring.desc[i].flags & nextflag) {
> > +     while (vq->split.desc_extra[i].flags & nextflag) {
> >               vring_unmap_one_split(vq, i);
> >               i = vq->split.desc_extra[i].next;
> >               vq->vq.num_free++;
> > --
> > 2.25.1
>

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