Jason Wang
2023-Aug-08 06:05 UTC
[PATCH] virtio_ring: fix avail_wrap_counter in virtqueue_add_packed
On Tue, Aug 8, 2023 at 1:59?PM Yuan Yao <yuanyaogoog at chromium.org> wrote:> > Since there is a check for 0-length chain in code;BUG_ON(total_sg == 0), we won?t get a 0-length chain in practice. So, I think use (i <= head) makes the commit as small as possible.Ok, offered ack in other mail. Thanks> > > Best regards > > > > On Tue, Aug 8, 2023 at 2:43?PM Jason Wang <jasowang at redhat.com> 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? >> >> 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 >> > >>