Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] Duplicate check for `!bo_va` introduces dead code and changes the returned error code without justification, while the commit message incorrectly claims the list was walked unconditionally. Pre-existing issues: - [High] Memory allocation using `GFP_KERNEL` while holding `dma_resv` locks, risking AB-BA deadlock with memory reclaim. -- commit 6e50ec4c3e751796a8ad26f809bc3ef0737b1ddc Author: Yousef Alhouseen <[email protected]> drm/amdgpu: reject mapping info when BO VA is gone The commit adds a check to return -EINVAL if the BO-VA mapping is missing during AMDGPU_GEM_OP_GET_MAPPING_INFO. It claims this prevents unconditionally walking the lists after a racing close. > If a racing close drops the last handle reference in that window, > amdgpu_gem_object_close() can remove the BO-VA before > amdgpu_vm_bo_find() runs. The ioctl then walks the BO-VA mapping lists > unconditionally. Is it accurate that the ioctl walks the BO-VA mapping lists unconditionally? Looking at amdgpu_gem_op_ioctl(), there is already a pre-existing check for !bo_va that returns -ENOENT before any lists are walked. [ ... ] > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > index 28f89ae8ca745..f048bbc4c672e 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > @@ -1087,6 +1087,12 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void > *data, > struct drm_amdgpu_gem_vm_entry *vm_entries; > struct amdgpu_bo_va_mapping *mapping; > int num_mappings = 0; > + > + if (!bo_va) { > + r = -EINVAL; > + goto out_exec; > + } > + [Severity: Medium] Does adding this check introduce unreachable dead code just below it? A few lines further down in amdgpu_gem_op_ioctl(), there is another !bo_va check: if (!bo_va) { r = -ENOENT; goto out_exec; } With the newly added check returning -EINVAL, the pre-existing -ENOENT check becomes unreachable. Is there a specific reason to change the error code from -ENOENT to -EINVAL? [Severity: High] This is a pre-existing issue, but can this function trigger an AB-BA deadlock during memory reclaim? Further down in amdgpu_gem_op_ioctl() for this same op: vm_entries = kvcalloc(args->num_entries, sizeof(*vm_entries), GFP_KERNEL); This memory allocation occurs while holding the dma_resv locks after drm_exec_until_all_locked() has successfully acquired them. Allocating with GFP_KERNEL under a dma_resv lock allows direct memory reclaim, which can invoke the graphics shrinkers. Shrinkers often attempt to acquire dma_resv locks to evict buffers, causing a deadlock if the thread already holds them. > /* > * num_entries is set as an input to the size of the > user-allocated array of > * drm_amdgpu_gem_vm_entry stored at args->value. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
