Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] State Desynchronization / Broken Retry Logic for hardware GPU mapping updates - [High] Type Confusion / Struct Layout Mismatch on 32-bit platforms during BO overlap detection - [High] Kernel Resource Exhaustion (OOM) due to lack of interval tree merging - [Medium] Unprivileged users can flood the kernel log by triggering BO overlap errors - [Medium] Silent failure to apply attribute changes to virtual page 0 --
commit 5cf3077146707f9870d0ea122388797eb6226731 Author: Honglei Huang <[email protected]> Subject: drm/amdgpu: implement SVM attribute set/get/clear operations This commit completes the attribute subsystem with validation, tree modification, and a public API. It introduces attribute validation, flag application, VMA range validation, VM BO overlap detection, tree modification with gap/overlap handling, and public APIs. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_attr.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_attr.c > index ba09aa3c3fe4d..26e33defb8b9d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_attr.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_attr.c [ ... ] > +int > +amdgpu_svm_attr_check_vm_bo(struct amdgpu_svm_attr_tree *attr_tree, > + unsigned long start_page, > + unsigned long last_page, > + unsigned long *bo_start, unsigned long *bo_last) > +{ [ ... ] > + node = interval_tree_iter_first(&vm->va, start_page, last_page); [Severity: High] Could this cause a struct layout mismatch on 32-bit platforms? This passes the AMDGPU-specific vm->va tree to the generic interval_tree_iter_first() macro, which expects the start and last fields to be unsigned long. The vm->va tree uses uint64_t bounds. On 32-bit architectures, this will read the incorrect layout and potentially corrupt the interval bounds during the traversal. > + if (node) { > + AMDGPU_SVM_ERR("SVM range [0x%lx 0x%lx] overlaps with BO > mapping [0x%lx 0x%lx]\n", > + start_page, last_page, node->start, node->last); [Severity: Medium] Is there a risk of unprivileged userspace applications flooding the kernel log? If a requested SVM range overlaps with an existing BO mapping, this unconditionally logs an error using AMDGPU_SVM_ERR(). Since the start_page and last_page parameters are controlled by user input, a malicious application could repeatedly request overlapping ranges and cause massive dmesg spam without any ratelimiting. [ ... ] > +static int > +amdgpu_svm_attr_set_hole(struct amdgpu_svm_attr_tree *attr_tree, > + const struct amdgpu_svm_attrs *default_attrs, > + unsigned long start_page, unsigned long last_page, > + uint32_t nattr, > + const struct drm_amdgpu_svm_attribute *attrs, > + struct attr_set_ctx *change) > +{ [ ... ] > + /* Always create a range entry even when attrs equal defaults */ > + range = amdgpu_svm_attr_range_alloc(start_page, last_page, &new_attrs); > + if (!range) > + return -ENOMEM; > + > + amdgpu_svm_attr_range_insert_locked(attr_tree, range); [Severity: High] Can this lead to unbounded memory allocation and a potential kernel OOM? New range nodes are unconditionally allocated and inserted into the tree here without checking if they can be merged with adjacent identical ranges. Because there is no max_map_count limit, a user application repeatedly calling the ioctl on alternating pages could indefinitely fragment the interval tree and exhaust kmem_cache. [ ... ] > +static int > +amdgpu_svm_attr_set_existing(struct amdgpu_svm_attr_tree *attr_tree, > + struct amdgpu_svm_attr_range *range, > + unsigned long start_page, unsigned long last_page, > + uint32_t nattr, > + const struct drm_amdgpu_svm_attribute *attrs, > + struct attr_set_ctx *change) > +{ [ ... ] > + if (attr_same_attrs(range, nattr, attrs)) { > + /* Report old==new so apply_attr_change can decide */ > + attr_change_ctx_set(change, &old_attrs, > + &old_attrs, > + start_page, last_page); > + return 0; > + } [Severity: High] Does this logic cause state desynchronization during a retry? During a retry attempt after a hardware mapping failure, the software interval tree has already been updated. This check will see that the requested attributes perfectly match the tree, returning old==new and fatally short-circuiting the hardware update. [ ... ] > + /* split head */ > + if (start_page > range_start) { > + left = amdgpu_svm_attr_range_alloc(range_start, start_page - 1, > &old_attrs); > + if (!left) > + return -ENOMEM; > + } [Severity: High] Can this split logic also contribute to kernel resource exhaustion? Existing ranges are permanently split into smaller segments here with no subsequent defragmentation or merging logic. This enables unbounded fragmentation of the tree. [ ... ] > +static int > +amdgpu_svm_attr_set_range(struct amdgpu_svm_attr_tree *attr_tree, > + const struct amdgpu_svm_attrs *default_attrs, > + unsigned long start_page, unsigned long last_page, > + uint32_t nattr, > + const struct drm_amdgpu_svm_attribute *attrs) > +{ [ ... ] > + ret = amdgpu_svm_apply_attr_change(svm, > + &change.old_attrs, > + &change.new_attrs, > + change.start_page, > + change.last_page); [Severity: Medium] Does this skip legitimate attribute changes for virtual page 0? The amdgpu_svm_apply_attr_change() function contains a fast-return check: drivers/gpu/drm/amd/amdgpu/amdgpu_svm.c:amdgpu_svm_apply_attr_change() { ... if (!start_page && !last_page) return 0; ... } If a request is made to map memory exactly at virtual address 0 (start_page=0, last_page=0), it appears it would silently exit with success without actually applying the hardware attributes. > + amdgpu_svm_unlock(svm); > + > + if (ret == -EAGAIN) { > + need_retry = true; > + ret = 0; > + } [Severity: High] Is the software interval tree rolled back on failure here? The software tree is updated prior to this hardware update attempt. If amdgpu_svm_apply_attr_change() returns -EAGAIN, this merely sets a retry flag without rolling back the tree. As noted earlier, this guarantees the subsequent retry will skip the hardware update entirely. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
