Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Moving `amdgpu_vm_bo_invalidate()` to `amdgpu_cs_parser_fini()` accesses BO structures without holding their `dma_resv` reservation locks, leading to data races, potential use-after-free vulnerabilities, and lockdep assertions. --
commit 130b94ccdb3ef241f2576b6d31544c37bb57192b Author: Pierre-Eric Pelloux-Prayer <[email protected]> drm/amdgpu: move debug_vm handling to amdgpu_cs_parser_fini The commit restarts the CS if the validation is still in progress, but when debug_vm is enabled, all BOs are invalidated, causing an infinite loop. This patch moves the BO invalidation to amdgpu_cs_parser_fini to avoid that. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > index d63fbc7c568de..7e75c536dab3d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c [ ... ] > @@ -1382,6 +1369,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, > /* Cleanup the parser structure */ > static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser) > { > + struct amdgpu_device *adev = parser->adev; > + struct amdgpu_bo_list_entry *e; > unsigned int i; > > amdgpu_sync_free(&parser->sync); [ ... ] > @@ -1397,8 +1386,21 @@ static void amdgpu_cs_parser_fini(struct > amdgpu_cs_parser *parser) > > if (parser->ctx) > amdgpu_ctx_put(parser->ctx); > - if (parser->bo_list) > + if (parser->bo_list) { > + if (adev->debug_vm) { > + /* Invalidate all BOs to test for userspace bugs */ > + amdgpu_bo_list_for_each_entry(e, parser->bo_list) { > + struct amdgpu_bo *bo = e->bo; > + > + /* ignore duplicates */ > + if (!bo) > + continue; > + > + amdgpu_vm_bo_invalidate(bo, false); [Severity: High] Does moving this invalidation here result in accessing the BO structures without their reservation locks? Earlier in amdgpu_cs_parser_fini(), drm_exec_fini() is called to drop all dma_resv locks acquired during parsing: amdgpu_cs_parser_fini() { ... amdgpu_sync_free(&parser->sync); drm_exec_fini(&parser->exec); ... Since amdgpu_vm_bo_invalidate() iterates over the bo->vm_bo list, which is protected by the BO's reservation lock, could this unlocked access lead to a data race or a use-after-free if it races with concurrent mapping operations like amdgpu_vm_bo_del()? Additionally, if any of the BOs are always-valid, this will eventually call amdgpu_vm_bo_moved(), which triggers an amdgpu_vm_assert_locked(vm) lockdep assertion since the root PD lock was already dropped. > + } > + } > amdgpu_bo_list_put(parser->bo_list); > + } > > for (i = 0; i < parser->nchunks; i++) > kvfree(parser->chunks[i].kdata); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
