linux-kernel - Re: [PATCH v6 0/3] rust: Add irq abstraction, SpinLockIrq

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Zww-7DO8jeQfnItV@Boquns-Mac-mini.local>
Date: Sun, 13 Oct 2024 14:43:08 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Dirk Behme <dirk.behme@...il.com>, Lyude Paul <lyude@...hat.com>,
	rust-for-linux@...r.kernel.org, Danilo Krummrich <dakr@...hat.com>,
	airlied@...hat.com, Ingo Molnar <mingo@...hat.com>,
	Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>, linux-kernel@...r.kernel.org,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...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@...sung.com>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>
Subject: Re: [PATCH v6 0/3] rust: Add irq abstraction, SpinLockIrq

On Sun, Oct 13, 2024 at 09:06:01PM +0200, Thomas Gleixner wrote:
> On Sat, Oct 12 2024 at 07:29, Dirk Behme wrote:
> 
> > Hi Lyude,
> >
> > On 16.09.24 23:28, Lyude Paul wrote:
> >> This adds a simple interface for disabling and enabling CPUs, along with
> >> the ability to mark a function as expecting interrupts be disabled -
> >> along with adding bindings for spin_lock_irqsave/spin_lock_irqrestore().
> >> 
> >> Current example usecase (very much WIP driver) in rvkms:
> >> 
> >> https://gitlab.freedesktop.org/lyudess/linux/-/commits/rvkms-example-08012024
> >> 
> >> specifically drivers/gpu/drm/rvkms/crtc.rs
> >> 
> >> This series depends on
> >> https://lore.kernel.org/rust-for-linux/ZuKNszXSw-LbgW1e@boqun-archlinux/
> >> 
> >> Lyude Paul (3):
> >>    rust: Introduce irq module
> >>    rust: sync: Introduce lock::Backend::Context
> >>    rust: sync: Add SpinLockIrq
> >
> >
> > To have it in this thread as well I just want to mention the discussion in
> >
> > https://lore.kernel.org/rust-for-linux/87a5falmjy.fsf@kernel.org/
> >
> > which results in the impression that this patch series needs to update 
> > `CondVar::wait` to support waiting with irq disabled.
> 
> What means waiting with interrupts disabled?
> 

`CondVar` wraps a wait queue, and `CondVar::wait` accepts a `Guard` so
that it will 1) prepare the wait 2) drop lock and schedule 3) regrab the
lock. The usage of it loosk like:

    let cv: CondVar = ...;
    let l: Lock<...> = ...;

    let mut guard = l.lock(); // or the `guard` can be generated by the
                              // lock_with_new() function.

    while *guard != 1 {
        cv.wait(guard); // here we drop the lock and wait.
	// lock is re-grabbed.
    }

2) is implemented by the `unlock()` function of a lock backend (plus a
schedule()), and 3) is implemented by the `relock()` function a lock
backend. Currently SpinLockIrqBackend (the backend of SpinLockIrq)
doesn't re-enable interrupts in `unlock()`, and of course it doesn't
disable interrupts in `relock()`, and this is actually correct, because
SpinLockIrq expects the caller to provide `IrqDisabled` token, so it
doesn't handle the interrupt state itself, therefore `unlock()` cannot
enable interrupts.

But that makes `cv.wait()` not working, because interrtups would be
still disabled when schedule() is called.

I'm waiting for Lyude's new version (with lock_first(), and
unlock_last()) to see how we can resolve this. We may need to redesign
`CondVar::wait`.

> Spinning? Why would you want to do that in the first place?
> 
> There are not a lot of use cases to do so, except for core code.
> 

The use case currently is that a timer callback need to modify something
inside a lock, that makes the lock irq-unsafe, if a task is waiting for
this modification, it would need to use `CondVar` to do the wait.

Regards,
Boqun

> Thanks,
> 
>         tglx