Jason Wang
2023-Aug-08 05:43 UTC
[PATCH] virtio_ring: fix avail_wrap_counter in virtqueue_add_packed
On Tue, Aug 8, 2023 at 1:11?PM Yuan Yao <yuanyaogoog at chromium.org> wrote:> > In current packed virtqueue implementation, the avail_wrap_counter won't > flip, in the case when the driver supplies a descriptor chain with a > length equals to the queue size; total_sg == vq->packed.vring.num. > > Let?s assume the following situation: > vq->packed.vring.num=4 > vq->packed.next_avail_idx: 1 > vq->packed.avail_wrap_counter: 0 > > Then the driver adds a descriptor chain containing 4 descriptors. > > We expect the following result with avail_wrap_counter flipped: > vq->packed.next_avail_idx: 1 > vq->packed.avail_wrap_counter: 1 > > But, the current implementation gives the following result: > vq->packed.next_avail_idx: 1 > vq->packed.avail_wrap_counter: 0 > > To reproduce the bug, you can set a packed queue size as small as > possible, so that the driver is more likely to provide a descriptor > chain with a length equal to the packed queue size. For example, in > qemu run following commands: > sudo qemu-system-x86_64 \ > -enable-kvm \ > -nographic \ > -kernel "path/to/kernel_image" \ > -m 1G \ > -drive file="path/to/rootfs",if=none,id=disk \ > -device virtio-blk,drive=disk \ > -drive file="path/to/disk_image",if=none,id=rwdisk \ > -device virtio-blk,drive=rwdisk,packed=on,queue-size=4,\ > indirect_desc=off \ > -append "console=ttyS0 root=/dev/vda rw init=/bin/bash" > > Inside the VM, create a directory and mount the rwdisk device on it. The > rwdisk will hang and mount operation will not complete. > > This commit fixes the wrap counter error by flipping the > packed.avail_wrap_counter, when start of descriptor chain equals to the > end of descriptor chain (head == i). > > Fixes: 1ce9e6055fa0 ("virtio_ring: introduce packed ring support") > Signed-off-by: Yuan Yao <yuanyaogoog at chromium.org> > --- > > drivers/virtio/virtio_ring.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index c5310eaf8b46..da1150d127c2 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -1461,7 +1461,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > } > } > > - if (i < head) > + if (i <= head) > vq->packed.avail_wrap_counter ^= 1;Would it be better to move the flipping to the place where we flip avail_used_flags? if ((unlikely(++i >= vq->packed.vring.num))) { i = 0; vq->packed.avail_used_flags ^ 1 << VRING_PACKED_DESC_F_AVAIL | 1 << VRING_PACKED_DESC_F_USED; } Thanks> > /* We're using some buffers from the free list. */ > -- > 2.41.0.640.ga95def55d0-goog >
Michael S. Tsirkin
2023-Aug-08 05:59 UTC
[PATCH] virtio_ring: fix avail_wrap_counter in virtqueue_add_packed
On Tue, Aug 08, 2023 at 01:43:02PM +0800, Jason Wang wrote:> On Tue, Aug 8, 2023 at 1:11?PM Yuan Yao <yuanyaogoog at chromium.org> wrote: > > > > In current packed virtqueue implementation, the avail_wrap_counter won't > > flip, in the case when the driver supplies a descriptor chain with a > > length equals to the queue size; total_sg == vq->packed.vring.num. > > > > Let?s assume the following situation: > > vq->packed.vring.num=4 > > vq->packed.next_avail_idx: 1 > > vq->packed.avail_wrap_counter: 0 > > > > Then the driver adds a descriptor chain containing 4 descriptors. > > > > We expect the following result with avail_wrap_counter flipped: > > vq->packed.next_avail_idx: 1 > > vq->packed.avail_wrap_counter: 1 > > > > But, the current implementation gives the following result: > > vq->packed.next_avail_idx: 1 > > vq->packed.avail_wrap_counter: 0 > > > > To reproduce the bug, you can set a packed queue size as small as > > possible, so that the driver is more likely to provide a descriptor > > chain with a length equal to the packed queue size. For example, in > > qemu run following commands: > > sudo qemu-system-x86_64 \ > > -enable-kvm \ > > -nographic \ > > -kernel "path/to/kernel_image" \ > > -m 1G \ > > -drive file="path/to/rootfs",if=none,id=disk \ > > -device virtio-blk,drive=disk \ > > -drive file="path/to/disk_image",if=none,id=rwdisk \ > > -device virtio-blk,drive=rwdisk,packed=on,queue-size=4,\ > > indirect_desc=off \ > > -append "console=ttyS0 root=/dev/vda rw init=/bin/bash" > > > > Inside the VM, create a directory and mount the rwdisk device on it. The > > rwdisk will hang and mount operation will not complete. > > > > This commit fixes the wrap counter error by flipping the > > packed.avail_wrap_counter, when start of descriptor chain equals to the > > end of descriptor chain (head == i). > > > > Fixes: 1ce9e6055fa0 ("virtio_ring: introduce packed ring support") > > Signed-off-by: Yuan Yao <yuanyaogoog at chromium.org> > > --- > > > > drivers/virtio/virtio_ring.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index c5310eaf8b46..da1150d127c2 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -1461,7 +1461,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > } > > } > > > > - if (i < head) > > + if (i <= head) > > vq->packed.avail_wrap_counter ^= 1; > > Would it be better to move the flipping to the place where we flip > avail_used_flags?I think I prefer this patch for stable, refactoring can be done on top.> if ((unlikely(++i >= vq->packed.vring.num))) { > i = 0; > vq->packed.avail_used_flags ^> 1 << VRING_PACKED_DESC_F_AVAIL | > 1 << VRING_PACKED_DESC_F_USED; > } > > Thanks > > > > > /* We're using some buffers from the free list. */ > > -- > > 2.41.0.640.ga95def55d0-goog > >