Quoting Christian König (2026-09-09 16:03:30) > On 9/9/26 13:24, Joonas Lahtinen wrote: > > (+ userfaultfd maintainers) > > > > Quoting Christian König (2026-09-09 14:02:21) > >> On 9/9/26 12:21, Simona Vetter wrote: > >>> On Wed, Sep 09, 2026 at 01:00:16PM +0300, Joonas Lahtinen wrote: > >>>> (Adding bunch of FUSE folks and DRM folks in the hopes of coming to a > >>>> resolution here.) > >>>> > >>>> Quoting Joonas Lahtinen (2026-09-07 16:24:40) > >>>>> Quoting [email protected] (2026-09-03 18:22:23) > >>>>>> Thank you for your contribution! Sashiko AI review found 2 potential > >>>>>> issue(s) to consider: > >>>>>> - [High] Global ordered workqueue stall via copy_from_user block while > >>>>>> holding ioctl_lock. > >>>>>> - [Medium] Event sequence numbers (seqno) can be queued out-of-order > >>>>>> in the event stream due to a race between seqno generation and event > >>>>>> queuing. > >>>>>> -- > >>>>>> > >>>>>> commit e2061e72d8e954a17bde35e9c970ee41bc2db0ad > >>>>>> Author: Mika Kuoppala <[email protected]> > >>>>>> > >>>>>> drm/xe/eudebug: Introduce discovery for resources > >>>>>> > >>>>>> This commit introduces a resource discovery mechanism for debugger > >>>>>> connections. > >>>>>> When a debugger connects, all existing resources for the target client > >>>>>> are > >>>>>> relayed as events via an ordered workqueue, and DRM ioctls are blocked > >>>>>> using > >>>>>> an rw_semaphore until discovery is complete. > >>>>>> > >>>>>>> diff --git a/drivers/gpu/drm/xe/xe_device.c > >>>>>>> b/drivers/gpu/drm/xe/xe_device.c > >>>>>>> index 032a7ec5fbc8d..f10959b898297 100644 > >>>>>>> --- a/drivers/gpu/drm/xe/xe_device.c > >>>>>>> +++ b/drivers/gpu/drm/xe/xe_device.c > >>>>>> [ ... ] > >>>>>>> @@ -238,8 +239,12 @@ static long xe_drm_ioctl(struct file *file, > >>>>>>> unsigned int cmd, unsigned long arg) > >>>>>>> > >>>>>>> ACQUIRE(xe_pm_runtime_ioctl, pm)(xe); > >>>>>>> ret = ACQUIRE_ERR(xe_pm_runtime_ioctl, &pm); > >>>>>>> - if (ret >= 0) > >>>>>>> + if (ret >= 0) { > >>>>>>> + bool lock = xe_eudebug_discovery_lock(file, cmd); > >>>>>>> ret = drm_ioctl(file, cmd, arg); > >>>>>> > >>>>>> [Severity: High] > >>>>>> Does holding the discovery lock across drm_ioctl() create a vector for > >>>>>> a > >>>>>> workqueue stall? > >>>>>> > >>>>>> If an unprivileged user maps the ioctl argument pointer to a blocking > >>>>>> FUSE > >>>>>> or userfaultfd region, drm_ioctl() can be stalled indefinitely during > >>>>>> copy_from_user() while holding xef->eudebug.ioctl_lock for read. > >>>>> > >>>>> Don't think this is a very realistic vector to address, as it would > >>>>> also extend to every other copy_from_user() and also to userptr across > >>>>> all drivers. > >> > >> Yeah I don't think that this is a major problem. > >> > >> Using copy_from_user() while holding a lock is usually fundamentally > >> broken in the first place. > > > > Well, ultimately also applies to userptr. How would we go about > > implementing that without any outer locks? > > > > Grab locks, figure out what pages are needed, release locks, pre-fault, > > re-grab locks and do best attempt to resolve? And that's going to be a > > live-lock at best again. > > Yeah and that is exactly what you *must* do for userptrs. > > There is no really alternative to that because you can't grab the same lock > outside a page fault you do inside an MMU notfier.
Sure, not same locks. But essentially any device or HW unit level locks held while dealing with userptr or using copy_from_user() would now become potential deadlocks automatically. No need to take any nested locks, it's just that whichever shared lock is taken will immediately become a potential deadlock for completely unrelated DRM client and thus also different process. > So all drivers who use userptrs basically implement the following sequence: > > 1. Take a copy of page table update sequence number. > 2. Walk page tables, extract PFNs. > 3. Prepare your DMA operations. > 4. Grab the MMU notifier lock. > 5. Compare your sequence number, if it doesn't match revert everything and > try again. > 6. Submit your DMA operation to the HW > 7. Drop the MMU notifier lock. > > What you describe above for eudebug sounds a lot like it won't work > correctly, you can't work with outside locks in the userptr handling. For the eudebug scenario here it's not to do with userptr. The access is now done in patch 13 with access_process_vm() and hopefully in future with access_remote_vm(). This lock we actually can easily refactor out. It's just a lock to easily allow generating a coherent snapshot of DRM client resources and debug metadata. I'm just raising to concern generically for any userspace memory access path be it copy_from_user() or userptr. Any lock taken on the outside of such handling will become poisoned for all other DRM clients. > > That'd be a massive undertaking for each driver, I think. > > > >> But there are other issues which are much more problematic. > >> > >>>>> > >>>>> Having a malfunctioning FUSE driver and getting a malfunctioning system > >>>>> as a result is probably somewhat expected. > >>>> > >>>> Based on further chatting on this with Sima, I was volunteered to pull > >>>> together the discussion here. > >>>> > >>>> We seem to have Sashiko picking up on patterns about accessing userspace > >>>> memory with locks held and potential for copy_from_user() (or userptr) to > >>>> then take indefinitely long to resolve. And that spreads to deadlocks > >>>> everywhere situation very fast. > >>>> > >>>> Based on reading of [1] and [2], it seems pretty much expected FUSE > >>>> drivers can trivially deadlock and ultimately in worst case the situation > >>>> can only be solved by manually aborting those connections by sysadmin. > >> > >> The real problem comes with userfaultfd and the combination with HMM. > >> > >> Drivers implementing HMM usually use a background workers to resolve > >> recoverable page faults using the function hmm_range_fault(). > >> > >> If userfaultfd together with hmm_range_fault() can block those background > >> workers indefinitely it can block other applications from using the HW > >> without any sysadmin having any chance to figure out what is going on. > >> > >> That is a classic local deny of service attack and I fear > >> hmm_range_fault() needs something like a timeout to handle that. > > > > From my position, if userspace is injected to the dependency path for > > resolving page-faults, with potentially unbounded execution times, the > > timeouts would better be asserted at the FUSE/UFFD side and then simply > > failing the operations with a bang if not met. > > That won't work like that. Both FUSE and UFFD can take any time they want for > an operation. Wouldn't that then directly mean that anything to do with page-faults would immediately exclude everything related to dma-bufs? > >>>> It also seems (from the Sashiko comments) that by design, there's no > >>>> upper bound for how long an operation can take, so a bad FUSE driver > >>>> may stall for however long it sees fit to serve page-fault or in the > >>>> case of [3] it may decide to not actually populate the PTEs (or maybe > >>>> invalidate them immediately). > >>>> > >>>> Should we really be refactoring the whole kernel for the sake of > >>>> knowingly allowing potentially malicious userspace driver to idefinitely > >>>> stall or incorrectly resolve page faults? That'll be quite a lot of > >>>> complexity added to all the other drivers. > >> > >> +1 > >> > >> Regards, > >> Christian. > > > > I was also meaning to convey that I don't think indefinitely blocking > > copy_from_user() is acceptable, either. However earlier you seemed to > > indicate towards that being something we should expect? > > Correct, that copy_from_user() can take any amount of time is perfectly > expected. > > The problem is that most GPU HW currently can't deal with that because it > can't take the work of the HW in case of a fault. Doesn't that then raise the question if indefinitely stalling page fault responses or improperly handling page fault responses is actually reasonable if we want to support most GPU HW? Which I kinda hope we do want to support. Regards, Joonas > > Regards, > Christian. > > > > > Regards, Joonas
