Re: (no subject)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Pages are unpinned before the hardware IOMMU is updated, creating a Use-After-Free race condition where the device can DMA into reallocated physical memory.
- [High] Hardware unmap failures are silently ignored, leaving stale active mappings in the hardware while the OS considers the memory freed.
- [High] Pinned physical pages are permanently leaked when `vhost_vdpa_map` or `pin_user_pages` fails after a successfully mapped discontinuity in `vhost_vdpa_pa_map`.
--

--- Patch [1]: (no subject) ---
commit 9d5471c1144ba5ee59ae8bddf13fc8fca006ee54
Author: xiongweimin <xiongweimin@kylinos.cn>

vhost-vdpa: propagate set_map error to caller

The return value of ops->set_map() is currently ignored when handling
VHOST_IOTLB_BATCH_END. If the backend fails to program the IOTLB,
the VMM incorrectly believes the operation succeeded and may continue
with stale or incorrect mappings. Propagate the error from ops->set_map()
to the caller.

> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index ac55275fa0d0a..89af89af26ca9 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -1293,7 +1293,7 @@ static int vhost_vdpa_process_iotlb_msg(struct vhost_dev *dev, u32 asid,
>  		break;
>  	case VHOST_IOTLB_BATCH_END:
>  		if (v->in_batch && ops->set_map)
> -			ops->set_map(vdpa, asid, iotlb);
> +			r = ops->set_map(vdpa, asid, iotlb);

[Severity: Critical]
This isn't a bug introduced by this patch, but does the unmap sequence
create a race condition where the device can DMA into reallocated memory?

Looking at vhost_vdpa_pa_unmap(), pages are unpinned before the mapping is
removed from the hardware IOMMU:

drivers/vhost/vdpa.c:vhost_vdpa_pa_unmap() {
    ...
            unpin_user_page(page);
        }
        atomic64_sub(PFN_DOWN(map->size), &dev->mm->pinned_vm);
        vhost_vdpa_general_unmap(v, map, asid);
}

For devices using set_map, the hardware is updated much later in
vhost_vdpa_unmap():

drivers/vhost/vdpa.c:vhost_vdpa_unmap() {
    ...
    vhost_vdpa_iotlb_unmap(v, iotlb, iova, iova + size - 1, asid);

    if (ops->set_map) {
        if (!v->in_batch)
            ops->set_map(vdpa, asid, iotlb);
    }
}

Could an unprivileged device execute asynchronous DMA into these newly freed
pages during the window between unpin_user_page() and the hardware update?

[Severity: High]
This is a pre-existing issue, but are we silently masking hardware unmap
failures elsewhere?

In vhost_vdpa_unmap(), the return value of ops->set_map() is discarded:

drivers/vhost/vdpa.c:vhost_vdpa_unmap() {
    ...
    if (ops->set_map) {
        if (!v->in_batch)
            ops->set_map(vdpa, asid, iotlb);
    }
}

Similarly, vhost_vdpa_general_unmap() ignores the return values of
ops->dma_unmap() and iommu_unmap():

drivers/vhost/vdpa.c:vhost_vdpa_general_unmap() {
    ...
    if (ops->dma_map) {
        ops->dma_unmap(vdpa, asid, map->start, map->size);
    } else if (ops->set_map == NULL) {
        iommu_unmap(v->domain, map->start, map->size);
    }
}

If the hardware backend encounters an error and fails to remove the mapping,
could this leave stale active mappings in the hardware while the OS
considers the memory freed?

[Severity: High]
This is also a pre-existing issue, but does the map function leak pinned
physical pages if an error occurs?

In vhost_vdpa_pa_map(), when a chunk is mapped, nchunks is reset to 0:

drivers/vhost/vdpa.c:vhost_vdpa_pa_map() {
    ...
            map_pfn = this_pfn;
            iova += csize;
            nchunks = 0;
        }
}

If a subsequent discontinuity causes a failure, the error cleanup path
skips unpinning the currently pending chunk because nchunks evaluates
to false:

drivers/vhost/vdpa.c:vhost_vdpa_pa_map() {
    ...
out:
    if (ret) {
        if (nchunks) {
            unsigned long pfn;

            WARN_ON(!last_pfn);
            for (pfn = map_pfn; pfn <= last_pfn; pfn++)
                unpin_user_page(pfn_to_page(pfn));
        }
    }
}

Will this permanently leak the pinned memory if a failure happens after a
successfully mapped discontinuity?

>  		v->in_batch = false;
>  		break;
>  	default:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717021305.327441-1-xiongwm2026@163.com?part=1