Virtualization - Nov 2021 - [PATCH] virtio_ring: aovid reading flag from the descriptor ring

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 at 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

On Mon, Nov 8, 2021 at 4:13 PM Jason Wang <jasowang at 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 at 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
>

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 at 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