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

[lyude]

Changes down below:

On Mon, 2026-08-17 at 14:50 +0800, Junrui Luo via B4 Relay wrote:
> From: Junrui Luo <moonafterrain@xxxxxxxxxxx>
> 
> 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.
> 
> Rebinding a GART BO over a 2MiB VRAM BO therefore leaves the new
> mapping
> owning page tables built for a different page size, and it then maps
> at a
> size that was never referenced over that range. Since raw map does
> not
> allocate, nvkm_vmm_iter() can walk down to a NULL leaf and
> dereference
> it. The remainders of a split have the same problem: op_map_prepare()
> recomputes a page size with select_page_shift() while the remainder
> keeps
> the parent's page tables, so a parent that was itself downgraded can
> leave
> a remainder that re-aligns to a larger size. This happens on the
> unmap
> path too.
> 
> Reject the bind, and make split remainders inherit the page size of
> the
> mapping they are split from.
> 
> Fixes: c488a94e7e14 ("drm/nouveau/uvmm: Allow larger pages")
> Reported-by: Yuhao Jiang <danisjiang@xxxxxxxxx>
> Assisted-by: Claude:claude-opus-5
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Junrui Luo <moonafterrain@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/nouveau/nouveau_uvmm.c | 33
> ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index f5e4756b4de4..6404c54d097c 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -85,6 +85,8 @@ struct uvmm_map_args {
>  	u64 addr;
>  	u64 range;
>  	u8 kind;
> +	/* Page size to give the new mapping, or 0 to derive it from
> the op. */
> +	u8 page_shift;
>  };
>  
>  static int
> @@ -655,7 +657,8 @@ op_map_prepare(struct nouveau_uvmm *uvmm,
>  
>  	uvma->region = args->region;
>  	uvma->kind = args->kind;
> -	uvma->page_shift = select_page_shift(uvmm, op);
> +	uvma->page_shift = args->page_shift ? args->page_shift :
> +			   select_page_shift(uvmm, op);

This can just be:

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

>  
>  	drm_gpuva_map(&uvmm->base, &uvma->va, op);
>  
> @@ -684,8 +687,20 @@ nouveau_uvmm_sm_prepare(struct nouveau_uvmm
> *uvmm,
>  	struct drm_gpuva_op *op;
>  	u64 vmm_get_start = args ? args->addr : 0;
>  	u64 vmm_get_end = args ? args->addr + args->range : 0;
> +	u8 map_page_shift = 0;
>  	int ret;
>  
> +	/* A new mapping takes over the page tables of the mappings
> it replaces,
> +	 * so every one of them has to be using its page size. The
> new mapping
> +	 * is the last op drm_gpuvm_sm_map_ops_create() emits.
> +	 */
> +	if (args) {
> +		struct drm_gpuva_op *last = drm_gpuva_last_op(ops);
> +
> +		if (last->op == DRM_GPUVA_OP_MAP)
> +			map_page_shift = select_page_shift(uvmm,
> &last->map);
> +	}
> +
>  	drm_gpuva_for_each_op(op, ops) {
>  		switch (op->op) {
>  		case DRM_GPUVA_OP_MAP: {
> @@ -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,
>  			};
>  			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) {

Let's just take this value from remap_args instead of doing
uvma_from_va(va), it makes it a bit easier for people to understand
what's happening here.

With those nitpicks fixed:

Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>

> +				ret = -EINVAL;
> +				goto unwind;
> +			}
> +
>  			op_unmap_prepare(r->unmap);
>  
>  			if (r->prev) {
> @@ -756,6 +782,11 @@ nouveau_uvmm_sm_prepare(struct nouveau_uvmm
> *uvmm,
>  			u64 uend = ustart + urange;
>  			u8 page_shift = uvma_from_va(va)-
> >page_shift;
>  
> +			if (map_page_shift && page_shift !=
> map_page_shift) {
> +				ret = -EINVAL;
> +				goto unwind;
> +			}
> +
>  			op_unmap_prepare(u);
>  
>  			if (!args)