Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] copy_from_user_nofault() disables page faults, causing legitimate completion events to be permanently dropped if the CPU PTE is not populated. - [Medium] drm_user_fence_set_compare() returns void and silently ignores invalid configuration arguments. --
commit ff537705b8c71e85d2a5b3d68ffe3a36096823b1 Author: Srinivasan Shanmugam <[email protected]> drm: Add per-signal compare functionality to drm_user_fence This patch adds optional compare fields to drm_user_fence to support per-signal filtering, as used in the EOP eventfd path. It introduces drm_user_fence_set_compare to configure the comparison and modifies the worker to read the userspace value using copy_from_user_nofault before signaling. > diff --git a/drivers/gpu/drm/drm_user_fence.c > b/drivers/gpu/drm/drm_user_fence.c > index 0f229b7210a9b..06a5683db10d9 100644 > --- a/drivers/gpu/drm/drm_user_fence.c > +++ b/drivers/gpu/drm/drm_user_fence.c [ ... ] > @@ -30,13 +39,46 @@ static void drm_user_fence_do_work(struct drm_work_fence > *wfence) [ ... ] > + if (ufence->cmp_op != DRM_USER_FENCE_CMP_NONE && > + !(wfence->fence && wfence->fence->error)) { > + if (!mm) { > + call_worker = false; > + } else { > + __le64 raw; > + > + /* > + * Use copy_from_user_nofault() to prevent a > + * userfaultfd-registered page from blocking this > + * workqueue thread indefinitely (DoS). > + */ > + if (copy_from_user_nofault(&raw, ufence->cmp_addr, > + sizeof(raw))) { > + call_worker = false; [Severity: High] Because copy_from_user_nofault disables page faults in drm_user_fence_do_work, will this return -EFAULT and permanently drop legitimate completion events if the CPU PTE is not populated? This can happen if the page is not resident, such as when NUMA balancing sets PROT_NONE, or if the memory uses lazy mapping and the CPU hasn't touched it since the GPU wrote to it. > + } else { > + /* GPU writes LE; convert before comparing. */ > + u64 cur_val = le64_to_cpu(raw); [ ... ] > @@ -63,8 +105,47 @@ void drm_user_fence_init(struct drm_user_fence *ufence, [ ... ] > +void drm_user_fence_set_compare(struct drm_user_fence *ufence, > + u64 __user *addr, u64 value, > + enum drm_user_fence_cmp op) > +{ > + /* > + * get_user() of u64 is not atomic on 32-bit — caller should not > + * reach here on non-64-bit kernels. > + */ > + if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_64BIT))) > + return; [Severity: Medium] Because this function returns void, does this early return on unsupported architectures silently fail without the caller knowing? If a driver attempts to configure a comparison here, it appears it would silently leave cmp_op disabled as DRM_USER_FENCE_CMP_NONE, treating the fence as an unconditional signal. Could this function return an error code instead so the driver can propagate the failure to userspace? > + > + if (op != DRM_USER_FENCE_CMP_NONE) { > + if (!addr || !IS_ALIGNED((unsigned long)addr, sizeof(u64))) > + return; [Severity: Medium] Similarly, if a driver passes an unaligned userspace address, does this return silently bypass the intended logic and leave the fence as an unconditional signal? > + } > + > + ufence->cmp_addr = addr; > + ufence->cmp_value = value; > + ufence->cmp_op = op; > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
