Re: [PATCH v2 1/1] rust: add Work::disable_sync

[Boris Brezillon <boris.brezillon@collabora.com>]

On Tue,  5 May 2026 12:16:12 +0300
Onur Özkan <work@onurozkan.dev> wrote:

> On Tue, 05 May 2026 08:47:45 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
> > On Tue, May 05, 2026 at 09:07:19AM +0300, Onur Özkan wrote:  
> > > On Mon, 04 May 2026 07:54:54 +0000
> > > Alice Ryhl <aliceryhl@google.com> wrote:
> > >   
> > > > On Fri, May 01, 2026 at 10:11:22PM +0300, Onur Özkan wrote:  
> > > > > Adds Work::disable_sync() as a safe wrapper for disable_work_sync().
> > > > > 
> > > > > Drivers can use this during teardown to stop new queueing and wait for
> > > > > queued or running work to finish before dropping related resources.
> > > > > 
> > > > > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > > > > ---
> > > > >  rust/kernel/workqueue.rs | 121 ++++++++++++++++++++++++++-------------
> > > > >  1 file changed, 81 insertions(+), 40 deletions(-)
> > > > > 
> > > > > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> > > > > index 7e253b6f299c..d0f9b4ba7f27 100644
> > > > > --- a/rust/kernel/workqueue.rs
> > > > > +++ b/rust/kernel/workqueue.rs
> > > > > @@ -267,7 +267,7 @@ pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue
> > > > >  
> > > > >      /// Enqueues a work item.
> > > > >      ///
> > > > > -    /// This may fail if the work item is already enqueued in a workqueue.
> > > > > +    /// This may fail if the work item is already enqueued in a workqueue or disabled.
> > > > >      ///
> > > > >      /// The work item will be submitted using `WORK_CPU_UNBOUND`.
> > > > >      pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput  
> > > > 
> > > > Can you elaborate on the case where disable leads to failure here? Can
> > > > you not enqueue a work item again after disabling it? Is there a doc
> > > > test illustrating this case that I can run for myself to see the
> > > > behavior in action?  
> > > 
> > > As we discussed on yesterday's call, I looked into cancel_work_sync and
> > > it seems we can make this work in the tyr reset implementation. We already
> > > store an atomic reset state, it can be used to prevent future enqueues in
> > > reset scheduling.
> > > 
> > > I will send a patch for the cancel_sync function soon.  
> > 
> > I did hear from Boris that Panthor actually does make use of the disable
> > feature.   
> 
> I don't have any idea what Boris said, perhaps he should write it here as well.
> Maybe Boris said something that isn't covered on the tyr reset yet in my series,
> I don't know.

So, I checked where those disable_[delayed_]work[_sync]() are, and they
seem to cover mostly the unplug path. There's a couple non-unplug
related use, which both cover the per-queue watchdog[3][4].

As for why we ended up using disable_work instead of cancel_work, it's
all explained in [1] and [2]. Yes, it could have been done differently,
but disable_work was the most convenient way of solving these UAFs in C.

> 
> What I do know is that I can achieve essentially the same behavior using
> cancel_work_sync instead of disable_work_sync on tyr. Like i said there's an
> atomic reset state we can use to prevent future work from being enqueued.

If the atomic is already there to prevent queuing more works, or
checking if a reset is pending, sure. It might just be more custom
checks to add, and if we think we'll need to support work items that can
be disabled further down the line anyway, maybe it makes sense to work
on it now, dunno.

Actually, looking back at panthor to write this reply, I'm now
considering replacing the custom checks we have in sched_queue_work()
by disable_[delayed_]work() calls in the
panthor_device_schedule_reset() path.

Of course, none of this has to drive how it's done in rust/Tyr, and if
you think it's better handled with a separate atomic and custom checks,
feel free to go for this alternative.

> The
> only real difference is that supporting disable_work_sync in the Rust workqueue
> would introduce more complexity than supporting cancel_work_sync. There is also
> the corresponding enable part we have to support if we want to go with
> disable_work_sync path.

Yep, if you use it in the reset path, you'll have to support
enable_work as well. For unplug, it's not needed, because the device is
gone after that.

[1]https://lore.kernel.org/all/20251027140217.121274-1-ketil.johnsen@arm.com/
[2]https://lore.kernel.org/all/20251029111412.924104-1-ketil.johnsen@arm.com/
[3]https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/panthor/panthor_sched.c#L2728
[4]https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/panthor/panthor_sched.c#L919