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 <[email protected]> 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/[email protected]?part=2
