* [PATCH] virtio: packed: fix unmap leak for indirect desc table
@ 2024-02-01 9:56 Xuan Zhuo
2024-02-02 3:46 ` Jason Wang
2024-02-22 20:06 ` Michael S. Tsirkin
0 siblings, 2 replies; 4+ messages in thread
From: Xuan Zhuo @ 2024-02-01 9:56 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo
When use_dma_api and premapped are true, then the do_unmap is false.
Because the do_unmap is false, vring_unmap_extra_packed is not called by
detach_buf_packed.
if (unlikely(vq->do_unmap)) {
curr = id;
for (i = 0; i < state->num; i++) {
vring_unmap_extra_packed(vq,
&vq->packed.desc_extra[curr]);
curr = vq->packed.desc_extra[curr].next;
}
}
So the indirect desc table is not unmapped. This causes the unmap leak.
On the other side, the dma addr and the flags is not updated. That
will be used by vring_unmap_extra_packed.
For safety in this patch, the flags of desc without indirect flag is
also updated.
This bug does not occur, because no driver use the premapped with
indirect.
Fixes: b319940f83c2 ("virtio_ring: skip unmap for premapped")
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 49299b1f9ec7..097258a10819 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1340,14 +1340,15 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
sizeof(struct vring_packed_desc));
vq->packed.vring.desc[head].id = cpu_to_le16(id);
- if (vq->do_unmap) {
+ if (vq->use_dma_api) {
vq->packed.desc_extra[id].addr = addr;
vq->packed.desc_extra[id].len = total_sg *
sizeof(struct vring_packed_desc);
- vq->packed.desc_extra[id].flags = VRING_DESC_F_INDIRECT |
- vq->packed.avail_used_flags;
}
+ vq->packed.desc_extra[id].flags = VRING_DESC_F_INDIRECT |
+ vq->packed.avail_used_flags;
+
/*
* A driver MUST NOT make the first descriptor in the list
* available before all subsequent descriptors comprising
@@ -1484,9 +1485,10 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
if (unlikely(vq->do_unmap)) {
vq->packed.desc_extra[curr].addr = addr;
vq->packed.desc_extra[curr].len = sg->length;
- vq->packed.desc_extra[curr].flags =
- le16_to_cpu(flags);
}
+
+ vq->packed.desc_extra[curr].flags = le16_to_cpu(flags);
+
prev = curr;
curr = vq->packed.desc_extra[curr].next;
@@ -1615,13 +1617,11 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
vq->free_head = id;
vq->vq.num_free += state->num;
- if (unlikely(vq->do_unmap)) {
- curr = id;
- for (i = 0; i < state->num; i++) {
- vring_unmap_extra_packed(vq,
- &vq->packed.desc_extra[curr]);
- curr = vq->packed.desc_extra[curr].next;
- }
+ curr = id;
+ for (i = 0; i < state->num; i++) {
+ vring_unmap_extra_packed(vq,
+ &vq->packed.desc_extra[curr]);
+ curr = vq->packed.desc_extra[curr].next;
}
if (vq->indirect) {
--
2.32.0.3.g01195cf9f
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] virtio: packed: fix unmap leak for indirect desc table
2024-02-01 9:56 [PATCH] virtio: packed: fix unmap leak for indirect desc table Xuan Zhuo
@ 2024-02-02 3:46 ` Jason Wang
2024-02-02 5:56 ` Xuan Zhuo
2024-02-22 20:06 ` Michael S. Tsirkin
1 sibling, 1 reply; 4+ messages in thread
From: Jason Wang @ 2024-02-02 3:46 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: virtualization, Michael S. Tsirkin
On Thu, Feb 1, 2024 at 5:56 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> When use_dma_api and premapped are true, then the do_unmap is false.
>
> Because the do_unmap is false, vring_unmap_extra_packed is not called by
> detach_buf_packed.
>
> if (unlikely(vq->do_unmap)) {
> curr = id;
> for (i = 0; i < state->num; i++) {
> vring_unmap_extra_packed(vq,
> &vq->packed.desc_extra[curr]);
> curr = vq->packed.desc_extra[curr].next;
> }
> }
>
> So the indirect desc table is not unmapped. This causes the unmap leak.
>
> On the other side, the dma addr and the flags is not updated. That
> will be used by vring_unmap_extra_packed.
>
> For safety in this patch, the flags of desc without indirect flag is
> also updated.
>
> This bug does not occur, because no driver use the premapped with
> indirect.
>
> Fixes: b319940f83c2 ("virtio_ring: skip unmap for premapped")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
> drivers/virtio/virtio_ring.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 49299b1f9ec7..097258a10819 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1340,14 +1340,15 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> sizeof(struct vring_packed_desc));
> vq->packed.vring.desc[head].id = cpu_to_le16(id);
>
> - if (vq->do_unmap) {
> + if (vq->use_dma_api) {
> vq->packed.desc_extra[id].addr = addr;
> vq->packed.desc_extra[id].len = total_sg *
> sizeof(struct vring_packed_desc);
> - vq->packed.desc_extra[id].flags = VRING_DESC_F_INDIRECT |
> - vq->packed.avail_used_flags;
> }
>
> + vq->packed.desc_extra[id].flags = VRING_DESC_F_INDIRECT |
> + vq->packed.avail_used_flags;
> +
> /*
> * A driver MUST NOT make the first descriptor in the list
> * available before all subsequent descriptors comprising
> @@ -1484,9 +1485,10 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> if (unlikely(vq->do_unmap)) {
> vq->packed.desc_extra[curr].addr = addr;
> vq->packed.desc_extra[curr].len = sg->length;
> - vq->packed.desc_extra[curr].flags =
> - le16_to_cpu(flags);
> }
> +
> + vq->packed.desc_extra[curr].flags = le16_to_cpu(flags);
> +
> prev = curr;
> curr = vq->packed.desc_extra[curr].next;
>
> @@ -1615,13 +1617,11 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
> vq->free_head = id;
> vq->vq.num_free += state->num;
>
> - if (unlikely(vq->do_unmap)) {
I think we can simply change this to check vq->use_dma_api, then we
don't need to move the flags assignment to extra above?
Thanks
> - curr = id;
> - for (i = 0; i < state->num; i++) {
> - vring_unmap_extra_packed(vq,
> - &vq->packed.desc_extra[curr]);
> - curr = vq->packed.desc_extra[curr].next;
> - }
> + curr = id;
> + for (i = 0; i < state->num; i++) {
> + vring_unmap_extra_packed(vq,
> + &vq->packed.desc_extra[curr]);
> + curr = vq->packed.desc_extra[curr].next;
> }
>
> if (vq->indirect) {
> --
> 2.32.0.3.g01195cf9f
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] virtio: packed: fix unmap leak for indirect desc table
2024-02-02 3:46 ` Jason Wang
@ 2024-02-02 5:56 ` Xuan Zhuo
0 siblings, 0 replies; 4+ messages in thread
From: Xuan Zhuo @ 2024-02-02 5:56 UTC (permalink / raw)
To: Jason Wang; +Cc: virtualization, Michael S. Tsirkin
On Fri, 2 Feb 2024 11:46:59 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Thu, Feb 1, 2024 at 5:56 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > When use_dma_api and premapped are true, then the do_unmap is false.
> >
> > Because the do_unmap is false, vring_unmap_extra_packed is not called by
> > detach_buf_packed.
> >
> > if (unlikely(vq->do_unmap)) {
> > curr = id;
> > for (i = 0; i < state->num; i++) {
> > vring_unmap_extra_packed(vq,
> > &vq->packed.desc_extra[curr]);
> > curr = vq->packed.desc_extra[curr].next;
> > }
> > }
> >
> > So the indirect desc table is not unmapped. This causes the unmap leak.
> >
> > On the other side, the dma addr and the flags is not updated. That
> > will be used by vring_unmap_extra_packed.
> >
> > For safety in this patch, the flags of desc without indirect flag is
> > also updated.
> >
> > This bug does not occur, because no driver use the premapped with
> > indirect.
> >
> > Fixes: b319940f83c2 ("virtio_ring: skip unmap for premapped")
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> > drivers/virtio/virtio_ring.c | 24 ++++++++++++------------
> > 1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 49299b1f9ec7..097258a10819 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -1340,14 +1340,15 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> > sizeof(struct vring_packed_desc));
> > vq->packed.vring.desc[head].id = cpu_to_le16(id);
> >
> > - if (vq->do_unmap) {
> > + if (vq->use_dma_api) {
> > vq->packed.desc_extra[id].addr = addr;
> > vq->packed.desc_extra[id].len = total_sg *
> > sizeof(struct vring_packed_desc);
> > - vq->packed.desc_extra[id].flags = VRING_DESC_F_INDIRECT |
> > - vq->packed.avail_used_flags;
> > }
> >
> > + vq->packed.desc_extra[id].flags = VRING_DESC_F_INDIRECT |
> > + vq->packed.avail_used_flags;
> > +
> > /*
> > * A driver MUST NOT make the first descriptor in the list
> > * available before all subsequent descriptors comprising
> > @@ -1484,9 +1485,10 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > if (unlikely(vq->do_unmap)) {
> > vq->packed.desc_extra[curr].addr = addr;
> > vq->packed.desc_extra[curr].len = sg->length;
> > - vq->packed.desc_extra[curr].flags =
> > - le16_to_cpu(flags);
> > }
> > +
> > + vq->packed.desc_extra[curr].flags = le16_to_cpu(flags);
> > +
> > prev = curr;
> > curr = vq->packed.desc_extra[curr].next;
> >
> > @@ -1615,13 +1617,11 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
> > vq->free_head = id;
> > vq->vq.num_free += state->num;
> >
> > - if (unlikely(vq->do_unmap)) {
>
> I think we can simply change this to check vq->use_dma_api,
Sure.
> then we
> don't need to move the flags assignment to extra above?
In virtqueue_add_packed, we should update flags,
vring_unmap_extra_packed will check it unconditional.
Thanks
>
> Thanks
>
> > - curr = id;
> > - for (i = 0; i < state->num; i++) {
> > - vring_unmap_extra_packed(vq,
> > - &vq->packed.desc_extra[curr]);
> > - curr = vq->packed.desc_extra[curr].next;
> > - }
> > + curr = id;
> > + for (i = 0; i < state->num; i++) {
> > + vring_unmap_extra_packed(vq,
> > + &vq->packed.desc_extra[curr]);
> > + curr = vq->packed.desc_extra[curr].next;
> > }
> >
> > if (vq->indirect) {
> > --
> > 2.32.0.3.g01195cf9f
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] virtio: packed: fix unmap leak for indirect desc table
2024-02-01 9:56 [PATCH] virtio: packed: fix unmap leak for indirect desc table Xuan Zhuo
2024-02-02 3:46 ` Jason Wang
@ 2024-02-22 20:06 ` Michael S. Tsirkin
1 sibling, 0 replies; 4+ messages in thread
From: Michael S. Tsirkin @ 2024-02-22 20:06 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: virtualization, Jason Wang
On Thu, Feb 01, 2024 at 05:56:18PM +0800, Xuan Zhuo wrote:
> When use_dma_api and premapped are true, then the do_unmap is false.
>
> Because the do_unmap is false, vring_unmap_extra_packed is not called by
> detach_buf_packed.
>
> if (unlikely(vq->do_unmap)) {
> curr = id;
> for (i = 0; i < state->num; i++) {
> vring_unmap_extra_packed(vq,
> &vq->packed.desc_extra[curr]);
> curr = vq->packed.desc_extra[curr].next;
> }
> }
>
> So the indirect desc table is not unmapped. This causes the unmap leak.
>
> On the other side, the dma addr and the flags is not updated. That
> will be used by vring_unmap_extra_packed.
>
> For safety in this patch, the flags of desc without indirect flag is
> also updated.
>
> This bug does not occur, because no driver use the premapped with
> indirect.
>
> Fixes: b319940f83c2 ("virtio_ring: skip unmap for premapped")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
were you going to post v2?
> ---
> drivers/virtio/virtio_ring.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 49299b1f9ec7..097258a10819 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1340,14 +1340,15 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> sizeof(struct vring_packed_desc));
> vq->packed.vring.desc[head].id = cpu_to_le16(id);
>
> - if (vq->do_unmap) {
> + if (vq->use_dma_api) {
> vq->packed.desc_extra[id].addr = addr;
> vq->packed.desc_extra[id].len = total_sg *
> sizeof(struct vring_packed_desc);
> - vq->packed.desc_extra[id].flags = VRING_DESC_F_INDIRECT |
> - vq->packed.avail_used_flags;
> }
>
> + vq->packed.desc_extra[id].flags = VRING_DESC_F_INDIRECT |
> + vq->packed.avail_used_flags;
> +
> /*
> * A driver MUST NOT make the first descriptor in the list
> * available before all subsequent descriptors comprising
> @@ -1484,9 +1485,10 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> if (unlikely(vq->do_unmap)) {
> vq->packed.desc_extra[curr].addr = addr;
> vq->packed.desc_extra[curr].len = sg->length;
> - vq->packed.desc_extra[curr].flags =
> - le16_to_cpu(flags);
> }
> +
> + vq->packed.desc_extra[curr].flags = le16_to_cpu(flags);
> +
> prev = curr;
> curr = vq->packed.desc_extra[curr].next;
>
> @@ -1615,13 +1617,11 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
> vq->free_head = id;
> vq->vq.num_free += state->num;
>
> - if (unlikely(vq->do_unmap)) {
> - curr = id;
> - for (i = 0; i < state->num; i++) {
> - vring_unmap_extra_packed(vq,
> - &vq->packed.desc_extra[curr]);
> - curr = vq->packed.desc_extra[curr].next;
> - }
> + curr = id;
> + for (i = 0; i < state->num; i++) {
> + vring_unmap_extra_packed(vq,
> + &vq->packed.desc_extra[curr]);
> + curr = vq->packed.desc_extra[curr].next;
> }
>
> if (vq->indirect) {
> --
> 2.32.0.3.g01195cf9f
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-02-22 20:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-01 9:56 [PATCH] virtio: packed: fix unmap leak for indirect desc table Xuan Zhuo
2024-02-02 3:46 ` Jason Wang
2024-02-02 5:56 ` Xuan Zhuo
2024-02-22 20:06 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).