On Thu, Feb 1, 2024 at 5:56 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
> ---
> 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
>
