linux-kernel - Re: [PATCH v6 3/3] rust: sync: Add 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: <87a5f2svid.fsf@kernel.org>
Date: Thu, 17 Oct 2024 15:34:50 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: Lyude Paul <lyude@...hat.com>
Cc: Boqun Feng <boqun.feng@...il.com>,  Thomas Gleixner
 <tglx@...utronix.de>,  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,  Benno
 Lossin <benno.lossin@...ton.me>,  Daniel Almeida
 <daniel.almeida@...labora.com>,  Gary Guo <gary@...yguo.net>,  Miguel
 Ojeda <ojeda@...nel.org>,  Alex Gaynor <alex.gaynor@...il.com>,  Wedson
 Almeida Filho <wedsonaf@...il.com>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  Andreas Hindborg <a.hindborg@...sung.com>,
  Alice Ryhl <aliceryhl@...gle.com>,  Trevor Gross <tmgross@...ch.edu>,
  Martin Rodriguez Reboredo <yakoyoku@...il.com>,  Valentin Obst
 <kernel@...entinobst.de>
Subject: Re: [PATCH v6 3/3] rust: sync: Add SpinLockIrq

Lyude Paul <lyude@...hat.com> writes:

> On Tue, 2024-10-15 at 13:21 -0700, Boqun Feng wrote:
>> On Tue, Oct 15, 2024 at 01:17:37PM -0700, Boqun Feng wrote:
>> > On Tue, Oct 15, 2024 at 02:57:11PM +0200, Andreas Hindborg wrote:
>> > > Boqun Feng <boqun.feng@...il.com> writes:
>> > > 
>> > > > On Sat, Oct 05, 2024 at 02:19:38PM -0400, Lyude Paul wrote:
>> > > > > On Fri, 2024-10-04 at 14:48 -0400, Lyude Paul wrote:
>> > > > > > 
>> > > > > > FWIW: I agree we want things to map C closely wherever we can, but part of the
>> > > > > > reason of having rust in the kernel at all is to take advantage of the
>> > > > > > features it provides us that aren't in C - so there's always going to be
>> > > > > > differences in some places. This being said though, I'm more then happy to
>> > > > > > minimize those as much as possible and explore ways to figure out how to make
>> > > > > > it so that correctly using these interfaces is as obvious and not-error prone
>> > > > > > as possible. The last thing I want is to encourage bad patterns in drivers
>> > > > > > that maintainers have to deal with the headaches of for ages to come,
>> > > > > > especially when rust should be able to help with this as opposed to harm :).
>> > > > > 
>> > > > > I was thinking about this a bit more today and I realized I might actually
>> > > > > have a better solution that I think would actually map a lot closer to the C
>> > > > > primitives and I feel a bit silly it didn't occur to me before.
>> > > > > 
>> > > > > What if instead of with_interrupts_disabled, we extended Lock so that types
>> > > > > like SpinLockIrq that require a context like IrqDisabled can require the use
>> > > > > of two new methods:
>> > > > > 
>> > > > > * first_lock<R>(&self, cb: impl for<'a> FnOnce(Guard<'a, T, B>, B::Context<'a>) -> R) -> R
>> > > > 
>> > > > I think you really want to use a `&mut T` instead of `Guard<'a, T, B>`,
>> > > > otherwise people can do:
>> > > > 
>> > > > 	let g = lock1.first_lock(|guard, _ctx| { guard });
>> > > > 	// here the lock is held, but the interrupts might be enabled.
>> > > 
>> > > Is it impossible to limit the lifetime of the guard such that it cannot
>> > > be returned from `first_lock`?
>> > > 
>> > 
>> > I was wrong saying the original doesn't work, because it has a
>> > `for<'a>`, that means `'a` is lifetime of the closure, which cannot
>> > outlive the return value `R`. So this signature might be valid.
>> > 
>> 
>> But another problem is that with this signature, `cb` can drop the lock,
>> which is not expected, because the lock dropping should be done by
>> `first_lock` itself.
>
> I thought we agreed on switching this to &mut though? In which case dropping
> the guard doesn't really matter

I think we arrived the following over on Zulip [1]:

pub fn lock_with_new<U>(&self, cb: impl FnOnce(&mut Guard<'_, T, Backend>, IrqDisabled<'a>) -> U) -> U


Best regards,
Andreas


[1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Spinlocks.20with.20IRQs.3F/near/477072424