| Message ID | 20220629065656.54420-22-xuanzhuo@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio pci support VIRTIO_F_RING_RESET | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
在 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@linux.alibaba.com> > --- > 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? Actually, I think for resize we need 1) detach old 2) allocate new 3) if 2) succeed, attach new otherwise attach old This seems more clearer than the current logic? Thanks > + return -ENOMEM; > +} > + > > /* > * Generic functions and exported symbols.
On Fri, 1 Jul 2022 17:27:48 +0800, Jason Wang <jasowang@redhat.com> 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@linux.alibaba.com> > > --- > > 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. > > 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. Thanks. > > This seems more clearer than the current logic? > > Thanks > > > > + return -ENOMEM; > > +} > > + > > > > /* > > * Generic functions and exported symbols. >
在 2022/7/4 10:13, Xuan Zhuo 写道: > On Fri, 1 Jul 2022 17:27:48 +0800, Jason Wang <jasowang@redhat.com> 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@linux.alibaba.com> >>> --- >>> 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@redhat.com> > > Thanks. > > >> This seems more clearer than the current logic? >> >> Thanks >> >> >>> + return -ENOMEM; >>> +} >>> + >>> >>> /* >>> * Generic functions and exported symbols.
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); + return -ENOMEM; +} + /* * Generic functions and exported symbols.
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@linux.alibaba.com> --- drivers/virtio/virtio_ring.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)