Re: [PATCH V3 11/11] mshv: Mark mem regions as non-movable upfront if device passthru

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Failing `copy_to_user` during device creation bypasses the `pt_regions_pinned` enforcement while leaving the device fully accessible.
- [High] Device creation does not validate or pin pre-existing memory regions, allowing an unsafe mix of movable regions and device passthrough.
- [High] Missing synchronization between device creation and region creation allows concurrent bypass of the pinning constraint.
--

commit add8cd40c15400cf2b19c8993ce14d4b4a7adef6
Author: Mukesh R <mrathor@linux.microsoft.com>

mshv: Mark mem regions as non-movable upfront if device passthru

This commit ensures that if a VM is started with a device attached, its
memory regions are marked as non-movable upfront to allow the use of SLAT
for IOMMU. It also adds a boot parameter to disable movable pages globally.

> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index a7864463961bc..ac71534733bd8 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c

[ ... ]

> @@ -1303,6 +1307,12 @@ static void mshv_async_hvcall_handler(void *data, u64 *status)
>  	*status = partition->async_hypercall_status;
>  }
>  
> +static bool mshv_do_pt_regions_pinned(struct mshv_partition *pt)
> +{
> +	return pt->pt_regions_pinned || mshv_partition_encrypted(pt) ||
> +	       hv_no_movbl_pgs;
> +}

Is there a potential race condition here between device creation and region
creation?

pt->pt_regions_pinned is written in mshv_partition_ioctl_create_device()
without holding any locks. Concurrently, mshv_partition_create_region()
reads this flag via mshv_do_pt_regions_pinned() without synchronization.

If a region is created while a concurrent device creation thread is about to
set pt_regions_pinned to true, could the region incorrectly be instantiated
as movable while a passthrough device is active?

[ ... ]

> @@ -1808,6 +1818,9 @@ static long mshv_partition_ioctl_create_device(struct mshv_partition *partition,
>  	if (copy_to_user(uarg, &devargk, sizeof(devargk)))
>  		return -EFAULT;    /* cleanup in mshv_device_fop_release() */
>  
> +	/* For now, all regions must be pinned if there is device passthru. */
> +	partition->pt_regions_pinned = true;

If the copy_to_user() fails, the function returns early and skips setting
pt_regions_pinned to true. However, anon_inode_getfd() has already installed
the file descriptor in the process's file descriptor table, meaning the
device remains fully accessible. Can this allow userspace to bypass the
pt_regions_pinned enforcement?

Additionally, when a device is attached, this sets pt_regions_pinned to true
for future memory region creations. Does this handle the case where a
partition already has existing memory regions?

If userspace creates movable regions first and then attaches a device, is
there a risk that page migration could cause the device to DMA into incorrect
physical memory?

>  	return 0;
>  
>  undo_out:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512020259.1678627-1-mrathor@linux.microsoft.com?part=11