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