Re: [PATCH 2/2] drm/nouveau/uvmm: reject a second VM_INIT

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless access to `cli->uvmm.ptr` lacks memory barriers, potentially allowing readers to observe a partially initialized `uvmm` struct.
--

--- Patch [2]: [PATCH 2/2] drm/nouveau/uvmm: reject a second VM_INIT ---
commit 8aef81c8581a1b964e410ee072207a76a4098339
Author: Junrui Luo <moonafterrain@outlook.com>

drm/nouveau/uvmm: reject a second VM_INIT

nouveau_uvmm_ioctl_vm_init() sets up the GPU VA space for a drm_file and
is reachable from an unprivileged render node client. The patch prevents
DRM_IOCTL_NOUVEAU_VM_INIT from dropping the previous nouveau_uvmm and
leaking kernel memory by checking if cli->uvmm.ptr is already set.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index bced1481674e7..26d2a57b5aacc 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -1929,6 +1929,12 @@ nouveau_uvmm_ioctl_vm_init(struct drm_device *dev,
>  		goto out_unlock;
>  	}
>  
> +	/* Check that a GPU VA space isn't already set up for the client. */
> +	if (cli->uvmm.ptr) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but since we are checking cli->uvmm.ptr here,
is it safe that the writer publishes this pointer locklessly later in this
same function?

Further down in nouveau_uvmm_ioctl_vm_init(), the pointer is assigned without
memory barriers:

	uvmm->vmm.cli = cli;
	cli->uvmm.ptr = uvmm;
	mutex_unlock(&cli->mutex);

Could the compiler or CPU reorder this store before the uvmm structure is
fully initialized, causing readers to see a partially initialized struct?

For example, concurrent calls to nouveau_cli_uvmm() load cli->uvmm.ptr without
READ_ONCE() or smp_load_acquire():

drivers/gpu/drm/nouveau/nouveau_drv.h:nouveau_cli_uvmm() {
	return cli ? cli->uvmm.ptr : NULL;
}

If another thread executes DRM_IOCTL_MODE_CREATE_DUMB, it can follow a lockless
path through nouveau_gem_new() into nouveau_bo_alloc() which dereferences the
vmm structure:

drivers/gpu/drm/nouveau/nouveau_bo.c:nouveau_bo_alloc() {
	...
	struct nvif_vmm *vmm = &nouveau_cli_vmm(cli)->vmm;
	...
		for (i = 0; i < vmm->page_nr; i++) {
	...
}

Alternatively, nouveau_debugfs_gpuva() iterates clients and locklessly reads
cli->uvmm.ptr, then immediately calls nouveau_uvmm_lock() on it. Could this
trigger a crash if the mutex appears uninitialized?

Would using smp_store_release() for the assignment and smp_load_acquire()
for the readers ensure proper visibility of the initialized structure?

>  	uvmm = kzalloc_obj(*uvmm);
>  	if (!uvmm) {
>  		ret = -ENOMEM;

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