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

[lyude]

Looks good to me, will push to drm-misc-fixes in a moment.

On Fri, 2026-10-09 at 16:52 +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: LLM
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Junrui Luo <moonafterrain@xxxxxxxxxxx>
> Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
> ---
> Changes in v2:
> - Drop patch 1, which has been merged.
> - Use the abbreviated conditional operator for page_shift (Lyude
> Paul).
> - Compare against remap_args.page_shift (Lyude Paul).
> - Use Assisted-by: LLM (Balbir Singh).
> - Add Lyude's Reviewed-by.
> - Link to v1:
> https://lore.kernel.org/r/20260817-nouveau-fixes-v1-0-f518d0c735f3@xxxxxxxxxxx
> ---
>  drivers/gpu/drm/nouveau/nouveau_uvmm.c | 32
> +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index f5e4756b4de4..aa5b93808f41 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,7 @@ 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 ?:
> select_page_shift(uvmm, op);
>  
>  	drm_gpuva_map(&uvmm->base, &uvma->va, op);
>  
> @@ -684,8 +686,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 +727,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 &&
> +			    remap_args.page_shift != map_page_shift)
> {
> +				ret = -EINVAL;
> +				goto unwind;
> +			}
> +
>  			op_unmap_prepare(r->unmap);
>  
>  			if (r->prev) {
> @@ -756,6 +781,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)
> 
> ---
> base-commit: f5bbbfec59b4e2fb7520a91de3df8a6174325d6a
> change-id: 20260817-nouveau-fixes-23877845c3ab
> 
> Best regards,