Jason Wang
2022-Feb-23 07:34 UTC
[PATCH] virtio_ring: aovid reading flag from the descriptor ring
On Wed, Feb 23, 2022 at 3:08 PM Michael S. Tsirkin <mst at 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 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 > > 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 > > > >
Jason Wang
2022-Feb-23 07:50 UTC
[PATCH] virtio_ring: aovid reading flag from the descriptor ring
On Wed, Feb 23, 2022 at 3:34 PM Jason Wang <jasowang at redhat.com> wrote:> > On Wed, Feb 23, 2022 at 3:08 PM Michael S. Tsirkin <mst at 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 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 > > > > 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 APII 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 > > > > > >