Re: [PATCH v11 21/40] virtio_ring: packed: introduce virtqueue_resize_packed()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




在 2022/7/4 10:13, Xuan Zhuo 写道:
On Fri, 1 Jul 2022 17:27:48 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
在 2022/6/29 14:56, Xuan Zhuo 写道:
virtio ring packed supports resize.

Only after the new vring is successfully allocated based on the new num,
we will release the old vring. In any case, an error is returned,
indicating that the vring still points to the old vring.

In the case of an error, re-initialize(by virtqueue_reinit_packed()) the
virtqueue to ensure that the vring can be used.

Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
---
   drivers/virtio/virtio_ring.c | 29 +++++++++++++++++++++++++++++
   1 file changed, 29 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 650f701a5480..4860787286db 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2042,6 +2042,35 @@ static struct virtqueue *vring_create_virtqueue_packed(
   	return NULL;
   }

+static int virtqueue_resize_packed(struct virtqueue *_vq, u32 num)
+{
+	struct vring_virtqueue_packed vring = {};
+	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct virtio_device *vdev = _vq->vdev;
+	int err;
+
+	if (vring_alloc_queue_packed(&vring, vdev, num))
+		goto err_ring;
+
+	err = vring_alloc_state_extra_packed(&vring);
+	if (err)
+		goto err_state_extra;
+
+	vring_free(&vq->vq);
+
+	virtqueue_init(vq, vring.vring.num);
+	virtqueue_vring_attach_packed(vq, &vring);
+	virtqueue_vring_init_packed(vq);
+
+	return 0;
+
+err_state_extra:
+	vring_free_packed(&vring, vdev);
+err_ring:
+	virtqueue_reinit_packed(vq);

So desc_state and desc_extra has been freed vring_free_packed() when
vring_alloc_state_extra_packed() fails. We might get use-after-free here?
vring_free_packed() frees the temporary structure vring. It does not affect
desc_state and desc_extra of vq. So it is safe.


You are right.



Actually, I think for resize we need

1) detach old
2) allocate new
3) if 2) succeed, attach new otherwise attach old

The implementation is now:

1. allocate new
2. free old (detach old)
3. attach new

error:
1. free temporary
2. reinit old

Do you think this is ok? We need to add a new variable to save the old vring in
the process you mentioned, there is not much difference in other.


Yes, I think the code is fine. But I'd suggest to rename "vring" to "vring_packed", this simplify the reviewers.

Other than this, you can add:

Acked-by: Jason Wang <jasowang@xxxxxxxxxx>



Thanks.


This seems more clearer than the current logic?

Thanks


+	return -ENOMEM;
+}
+

   /*
    * Generic functions and exported symbols.




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux