Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Critical] The patch adds a duplicate check for a missing BO VA, which changes the API return code and renders the existing check as unreachable dead code. Pre-existing issues: - [High] Potential deadlock due to allocating memory with `GFP_KERNEL` while holding `dma_resv` locks. -- commit 6d1719ca92ca060fce9b1b2b5a70616a3a67fa1c Author: Yousef Alhouseen <[email protected]> drm/amdgpu: reject mapping info when BO VA is gone This commit adds a check in amdgpu_gem_op_ioctl() to reject the AMDGPU_GEM_OP_GET_MAPPING_INFO operation if the BO VA is missing. The intention is to prevent a NULL pointer dereference when a concurrent handle close removes the per-file BO VA before the mapping list is walked. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > --- 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, [ ... ] > int num_mappings = 0; > + > + if (!bo_va) { > + r = -EINVAL; > + goto out_exec; > + } > + [Severity: Critical] Does adding this check break userspace ABI by changing the error code? There is a pre-existing check for a NULL bo_va slightly lower in this function that correctly returns -ENOENT. By returning -EINVAL here, applications expecting the standard -ENOENT for a missing BO VA might fail unexpectedly. [ ... ] > /* > * num_entries is set as an input to the size of the > user-allocated array of [ ... ] > * be retried. > */ > if (!bo_va) { > r = -ENOENT; > goto out_exec; > } The newly added check above also makes this original check unreachable dead code. [ ... ] > if (args->num_entries > INT_MAX / sizeof(*vm_entries)) { > r = -EINVAL; > goto out_exec; > } > > vm_entries = kvcalloc(args->num_entries, sizeof(*vm_entries), > GFP_KERNEL); [Severity: High] This is a pre-existing issue, but can allocating memory with GFP_KERNEL here cause a deadlock? This allocation occurs while sleepable dma_resv locks are held (acquired via drm_exec_lock_obj() and amdgpu_vm_lock_pd() earlier in the function). Using GFP_KERNEL allows direct memory reclaim. If system memory pressure forces the kernel to invoke GPU shrinkers while this thread holds dma_resv locks, the shrinker may attempt to lock the same BO or VM root PD, leading to a deadlock. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
