Re: [PATCH 2/2] drm/nouveau/uvmm: reject replace across page sizes

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Unaligned split remainders inherit a large page shift without alignment validation, causing silent hardware unmap failures and VMM state mismatch.

Pre-existing issues:
- [High] Missing rollback for `op_unmap_prepare` in `DRM_GPUVA_OP_REMAP` error paths leads to permanent loss of GPUVM mappings.
--

--- Patch [2]: [PATCH 2/2] drm/nouveau/uvmm: reject replace across page sizes ---
commit 90a0db35cc87b7654c52294b9abdb178f1a05f0b
Author: Junrui Luo <moonafterrain@outlook.com>

drm/nouveau/uvmm: reject replace across page sizes

A new mapping takes over the page tables of the mappings it replaces.
nouveau_uvmm_sm_prepare() only acquires page tables for the range no
existing mapping covers, and the map path frees the replaced mappings
without putting their references. That is only valid while all of them
use the same page size, which select_page_shift() no longer guarantees.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index f5e4756b4de4a..6404c54d097c1 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c

[ ... ]

> @@ -713,11 +728,22 @@ nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm,
>  			struct uvmm_map_args remap_args = {
>  				.kind = uvma_from_va(va)->kind,
>  				.region = uvma_from_va(va)->region,
> +				/* The remainders of the split keep the page
> +				 * tables of the mapping they are split from,
> +				 * so they must keep its page size too.
> +				 */
> +				.page_shift = uvma_from_va(va)->page_shift,

[Severity: Critical]
Does this inheritance of the parent's page shift bypass alignment checks for
unaligned split remainders?

If a process maps a BO with a 2MB-aligned address and size (creating a 2MB
page shift mapping), and then unmaps a 4K sub-region, the DRM_GPUVA_OP_REMAP
has args == NULL. The remainder mapping inherits the 2MB page shift via
remap_args.page_shift.

When passed to op_map_prepare():

	uvma->page_shift = args->page_shift ? args->page_shift :
			   select_page_shift(uvmm, op);

The logic skips select_page_shift() because args->page_shift is non-zero,
assigning the 2MB page size to the remainder mapping. Could this cause the
hardware unmap operations to silently fail (e.g., nvkm_vmm_iter computing a
0 iteration count for sizes smaller than the page size)? This seems like it
could leave the memory still mapped in hardware while the kernel considers
the hole empty.

>  			};
>  			u64 ustart = va->va.addr;
>  			u64 urange = va->va.range;
>  			u64 uend = ustart + urange;
>  
> +			if (map_page_shift &&
> +			    uvma_from_va(va)->page_shift != map_page_shift) {
> +				ret = -EINVAL;
> +				goto unwind;
> +			}
> +
>  			op_unmap_prepare(r->unmap);
>  
>  			if (r->prev) {
>  				ret = op_map_prepare(uvmm, &new->prev, r->prev,
>  						     &remap_args);
>  				if (ret)
>  					goto unwind;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path permanently
lose the mapping from the interval tree?

If op_map_prepare() fails (for example, due to an ENOMEM from
nouveau_uvma_alloc), the code jumps to unwind. However,
nouveau_uvmm_sm_prepare_unwind() starts unwinding from drm_gpuva_prev_op(op),
which skips the current operation.

This means op_unmap_prepare_unwind(r->unmap) is never called to roll back
the op_unmap_prepare(r->unmap) that executed just before this. Will this leave
the mapping permanently removed from the tracking tree while returning failure
to userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-nouveau-fixes-v1-0-f518d0c735f3@outlook.com?part=2