linux-kernel - Re: [PATCH] workqueue: rust: add creation of workqueues

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH5fLgiKxDpWg=dDsTJsrB6Kmkw32GZ9WPO-SrpWm4TZDxGVtg@mail.gmail.com>
Date: Wed, 16 Apr 2025 21:41:21 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Tejun Heo <tj@...nel.org>, Danilo Krummrich <dakr@...nel.org>, Philipp Stanner <phasta@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Lai Jiangshan <jiangshanlai@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Trevor Gross <tmgross@...ch.edu>, Daniel Almeida <daniel.almeida@...labora.com>, 
	Tamir Duberstein <tamird@...il.com>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] workqueue: rust: add creation of workqueues

On Wed, Apr 16, 2025 at 2:17 PM Alice Ryhl <aliceryhl@...gle.com> wrote:
>
> On Tue, Apr 15, 2025 at 12:48:48PM +0200, Danilo Krummrich wrote:
> > On Tue, Apr 15, 2025 at 09:01:35AM +0000, Alice Ryhl wrote:
> > > On Mon, Apr 14, 2025 at 08:15:41PM +0200, Danilo Krummrich wrote:
> > > > On Fri, Apr 11, 2025 at 03:34:24PM +0000, Alice Ryhl wrote:
> > > > >
> > > > > +/// An owned kernel work queue.
> > > >
> > > > I'd suggest to document that dropping an OwnedQueue will wait for pending work.
> > > >
> > > > Additionally, given that you're about to implement delayed work as well, we
> > > > should also mention that destroy_workqueue() currently does not cover waiting
> > > > for delayed work *before* it is scheduled and hence may cause WARN() splats or
> > > > even UAF bugs.
> > >
> > > Ah, that's a problem :(
> > >
> > > Can we make destroy_workqueue() wait for delayed items too? And/or have
> > > a variant of it that does so? I'm not sure what is best to do here...
> >
> > I think the problem is that the workq is not aware of all the timers in flight
> > and simply queues the work in the timer callback. See also [1].
> >
> > I'm not sure there's an easy solution to that, without adding extra overhead,
> > such as keeping a list of timers in flight in the workqueue end. :(
> >
> > [1] https://elixir.bootlin.com/linux/v6.13.7/source/kernel/workqueue.c#L2489
>
> It looks like panthor handles this by only having a single delayed work
> item on each queue and using cancel_delayed_work_sync before calling
> destroy_workqueue.
>
> Tejun, what do you suggest? The goal of the Rust API is to make it
> impossible to accidentally trigger a UAF, so we need to design the API
> to prevent this mistake.

Ok, looks like I'm not the only one with this problem.
https://lore.kernel.org/all/20250404101543.74262-2-phasta@kernel.org/

I think the most natural behavior is that destroy_workqueue() should
also wait for delayed work items, since the workqueue does not know
how to cancel them. Anyone who wants them cancelled can manually
cancel those work items before calling destroy_workqueue(), and that
would be no different from what they have to do today.

I thought about implementation approaches. The first thought that
sprang to mind is add a list of all delayed work items, but now I
think we can do better. We can have an atomic counter tracking the
number of delayed work items, and have destroy_workqueue() do this:

retry:
drain_workqueue(wq);
if (has_delayed_work_items(wq)) {
    wait_for_delayed_to_be_scheduled(wq);
    goto retry;
}

where wait_for_delayed_to_be_scheduled() either waits for the counter
to hit zero, or waits for at least waits for one of them to be
scheduled. For example, maybe wait_for_delayed_to_be_scheduled() could
add a dummy work item *without* waking up the worker threads, and then
wait for that work item to get executed, which would effectively mean
that it sleeps until something else wakes up a worker.

Thoughts?

Alice