Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Missing memory barrier between setting ufence->signalled and copy_to_user() allows spurious -EBUSY errors on weakly ordered architectures. Pre-existing issues: - [High] Userspace can trigger a kernel warning (WARN_ON) by unmapping the user fence memory before it signals, creating a local Denial of Service vector. -- commit 59eae6961ce055613e8672c56bafdaedc5de05d7 Author: Srinivasan Shanmugam <[email protected]> drm/xe: Convert xe_user_fence to drm_user_fence This commit replaces the open-coded user fence implementation in xe_sync.c with the new common drm_user_fence helper. It embeds the base structure and delegates the worker logic to standard ops. > diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c > index 37866768d64c2..7549123f84869 100644 > --- a/drivers/gpu/drm/xe/xe_sync.c > +++ b/drivers/gpu/drm/xe/xe_sync.c [ ... ] > @@ -19,36 +18,51 @@ > struct xe_user_fence { > - struct xe_device *xe; > - struct kref refcount; > - struct dma_fence_cb cb; > - struct work_struct worker; > - struct mm_struct *mm; > - u64 __user *addr; > - u64 value; > - int signalled; > + struct drm_user_fence base; > + struct xe_device *xe; > + u64 __user *addr; > + u64 value; > + int signalled; > }; > > -static void user_fence_destroy(struct kref *kref) > +static void xe_ufence_worker(struct drm_user_fence *base, bool mm_ok) > { > - struct xe_user_fence *ufence = container_of(kref, struct xe_user_fence, > - refcount); > + struct xe_user_fence *ufence = container_of(base, struct xe_user_fence, > base); > > - mmdrop(ufence->mm); > - kfree(ufence); > -} > + /* > + * Mark signalled before waking waiters so UMD can safely reuse > + * the same ufence without hitting -EBUSY. > + */ > + WRITE_ONCE(ufence->signalled, 1); > > + if (mm_ok) { [Severity: High] Does this code need a memory barrier here? The xe_ufence_worker() updates the kernel state (ufence->signalled) and then writes to user memory sequentially without a memory barrier. If the CPU reorders these stores (which is possible on weakly ordered architectures like ARM64), userspace can observe the user memory change via polling, immediately issue an unmap ioctl, and the kernel's check_ufence() may read the stale ufence->signalled == 0 state. This sequence could cause userspace to receive an unexpected -EBUSY error when unmapping the VM area, causing the UMD to fail or abort. This was introduced because the removed kthread_use_mm() call previously acted as an implicit full memory barrier. Should an smp_wmb() be added here? > + if (copy_to_user(ufence->addr, &ufence->value, > sizeof(ufence->value))) > + XE_WARN_ON("Copy to user failed"); [Severity: High] This is a pre-existing issue, but can userspace trigger a kernel warning here by unmapping the user fence memory before it signals? If a user application creates a user fence pointing to valid mapped memory, and then calls munmap() or mprotect() on the address to revoke write access before the GPU signals the fence, the asynchronous copy_to_user() in xe_ufence_worker() will naturally fail. This hits the unconditional XE_WARN_ON(), which allows unprivileged userspace to spam the kernel log and cause a kernel panic if panic_on_warn is enabled. Could this XE_WARN_ON() be replaced with drm_dbg(), or removed entirely? > + } else { > + drm_dbg(&ufence->xe->drm, > + "mmget_not_zero() failed, ufence wasn't signaled\n"); > + } > + > + wake_up_all(&ufence->xe->ufence_wq); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
