| 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
| ||
|
Message-ID: <20251021162002.3f29cd48@nimda.home>
Date: Tue, 21 Oct 2025 16:20:02 +0300
From: Onur Özkan <work@...rozkan.dev>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
lossin@...nel.org, lyude@...hat.com, ojeda@...nel.org,
alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net,
a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu,
dakr@...nel.org, peterz@...radead.org, mingo@...hat.com, will@...nel.org,
longman@...hat.com, felipe_life@...e.com, daniel@...lak.dev,
bjorn3_gh@...tonmail.com
Subject: Re: [PATCH v6 5/7] rust: ww_mutex: add context-free locking
functions
On Fri, 5 Sep 2025 16:14:59 -0300
Daniel Almeida <daniel.almeida@...labora.com> wrote:
> Hi Onur
>
> > On 3 Sep 2025, at 10:13, Onur Özkan <work@...rozkan.dev> wrote:
> >
> > Adds new `WwMutex` functions (`lock`, `lock_interruptible`,
> > `lock_slow`, `lock_slow_interruptible` and `try_lock`) that
> > can be used without `WwAcquireCtx`. This provides simpler API
> > when deadlock avoidance grouping is not needed.
> >
> > Signed-off-by: Onur Özkan <work@...rozkan.dev>
> > ---
> > rust/kernel/sync/lock/ww_mutex.rs | 162
> > ++++++++++++++++++++++-------- 1 file changed, 122 insertions(+),
> > 40 deletions(-)
> >
> > diff --git a/rust/kernel/sync/lock/ww_mutex.rs
> > b/rust/kernel/sync/lock/ww_mutex.rs index
> > d289718d2c98..b415d6deae9b 100644 ---
> > a/rust/kernel/sync/lock/ww_mutex.rs +++
> > b/rust/kernel/sync/lock/ww_mutex.rs @@ -138,6 +138,75 @@ pub fn
> > new_wound_wait(name: &'static CStr) -> impl PinInit<Self> { }
> > }
> >
> > +/// Locking kinds used by [`lock_common`] to unify internal FFI
> > locking logic.
>
> Can you please mention how this is not exposed to the outside world?
>
> It should be obvious from its private visibility, but still.
>
> > +#[derive(Copy, Clone, Debug)]
> > +enum LockKind {
> > + /// Blocks until lock is acquired.
> > + Regular,
> > + /// Blocks but can be interrupted by signals.
> > + Interruptible,
> > + /// Used in slow path after deadlock detection.
> > + Slow,
> > + /// Slow path but interruptible.
> > + SlowInterruptible,
> > + /// Does not block, returns immediately if busy.
> > + Try,
> > +}
> > +
> > +/// Internal helper that unifies the different locking kinds.
> > +fn lock_common<'a, T: ?Sized>(
> > + ww_mutex: &'a WwMutex<'a, T>,
> > + ctx: Option<&WwAcquireCtx<'_>>,
> > + kind: LockKind,
> > +) -> Result<WwMutexGuard<'a, T>> {
> > + let ctx_ptr = ctx.map_or(core::ptr::null_mut(), |c|
> > c.inner.get()); +
> > + match kind {
> > + LockKind::Regular => {
> > + // SAFETY: `WwMutex` is always pinned. If
> > `WwAcquireCtx` is `Some`, it is pinned,
> > + // if `None`, it is set to `core::ptr::null_mut()`.
> > Both cases are safe.
> > + let ret = unsafe {
> > bindings::ww_mutex_lock(ww_mutex.mutex.get(), ctx_ptr) };
>
> Use an intermediary variable to refer to the actual function. Then
> call this only once below the match statement.
>
> This will consolidate all the safety comments and "to_result()" calls
> into a single place, considerably reducing the clutter here.
>
I tried to consolidate them but it became very unreadable. Not all of
the functions return Result<i32> and the Try arm needs special handling
on the result. I can wrap the whole match in an unsafe block to write
the SAFETY note once, but that wouldn't be good I guess.
-Onur
> > +
> > + to_result(ret)?;
> > + }
> > + LockKind::Interruptible => {
> > + // SAFETY: `WwMutex` is always pinned. If
> > `WwAcquireCtx` is `Some`, it is pinned,
> > + // if `None`, it is set to `core::ptr::null_mut()`.
> > Both cases are safe.
> > + let ret =
> > + unsafe {
> > bindings::ww_mutex_lock_interruptible(ww_mutex.mutex.get(),
> > ctx_ptr) }; +
> > + to_result(ret)?;
> > + }
> > + LockKind::Slow => {
> > + // SAFETY: `WwMutex` is always pinned. If
> > `WwAcquireCtx` is `Some`, it is pinned,
> > + // if `None`, it is set to `core::ptr::null_mut()`.
> > Both cases are safe.
> > + unsafe {
> > bindings::ww_mutex_lock_slow(ww_mutex.mutex.get(), ctx_ptr) };
> > + }
> > + LockKind::SlowInterruptible => {
> > + // SAFETY: `WwMutex` is always pinned. If
> > `WwAcquireCtx` is `Some`, it is pinned,
> > + // if `None`, it is set to `core::ptr::null_mut()`.
> > Both cases are safe.
> > + let ret = unsafe {
> > +
> > bindings::ww_mutex_lock_slow_interruptible(ww_mutex.mutex.get(),
> > ctx_ptr)
> > + };
> > +
> > + to_result(ret)?;
> > + }
> > + LockKind::Try => {
> > + // SAFETY: `WwMutex` is always pinned. If
> > `WwAcquireCtx` is `Some`, it is pinned,
> > + // if `None`, it is set to `core::ptr::null_mut()`.
> > Both cases are safe.
> > + let ret = unsafe {
> > bindings::ww_mutex_trylock(ww_mutex.mutex.get(), ctx_ptr) }; +
> > + if ret == 0 {
> > + return Err(EBUSY);
> > + } else {
> > + to_result(ret)?;
> > + }
> > + }
> > + };
> > +
> > + Ok(WwMutexGuard::new(ww_mutex))
> > +}
> > +
> > /// Groups multiple mutex acquisitions together for deadlock
> > avoidance. ///
> > /// Must be used when acquiring multiple mutexes of the same class.
> > @@ -196,14 +265,9 @@ pub fn done(&self) {
> > unsafe { bindings::ww_acquire_done(self.inner.get()) };
> > }
> >
> > - /// Locks the given mutex.
> > + /// Locks the given mutex on this acquire context
> > ([`WwAcquireCtx`]). pub fn lock<'a, T>(&'a self, ww_mutex: &'a
> > WwMutex<'a, T>) -> Result<WwMutexGuard<'a, T>> {
> > - // SAFETY: The mutex is pinned and valid.
> > - let ret = unsafe {
> > bindings::ww_mutex_lock(ww_mutex.mutex.get(), self.inner.get()) }; -
> > - to_result(ret)?;
> > -
> > - Ok(WwMutexGuard::new(ww_mutex))
> > + lock_common(ww_mutex, Some(self), LockKind::Regular)
> > }
> >
> > /// Similar to `lock`, but can be interrupted by signals.
> > @@ -211,24 +275,14 @@ pub fn lock_interruptible<'a, T>(
> > &'a self,
> > ww_mutex: &'a WwMutex<'a, T>,
> > ) -> Result<WwMutexGuard<'a, T>> {
> > - // SAFETY: The mutex is pinned and valid.
> > - let ret = unsafe {
> > -
> > bindings::ww_mutex_lock_interruptible(ww_mutex.mutex.get(),
> > self.inner.get())
> > - };
> > -
> > - to_result(ret)?;
> > -
> > - Ok(WwMutexGuard::new(ww_mutex))
> > + lock_common(ww_mutex, Some(self), LockKind::Interruptible)
> > }
> >
> > - /// Locks the given mutex using the slow path.
> > + /// Locks the given mutex on this acquire context
> > ([`WwAcquireCtx`]) using the slow path. ///
> > /// This function should be used when `lock` fails (typically
> > due to a potential deadlock). pub fn lock_slow<'a, T>(&'a self,
> > ww_mutex: &'a WwMutex<'a, T>) -> Result<WwMutexGuard<'a, T>> {
> > - // SAFETY: The mutex is pinned and valid, and we're in the
> > slow path.
> > - unsafe {
> > bindings::ww_mutex_lock_slow(ww_mutex.mutex.get(),
> > self.inner.get()) }; -
> > - Ok(WwMutexGuard::new(ww_mutex))
> > + lock_common(ww_mutex, Some(self), LockKind::Slow)
> > }
> >
> > /// Similar to `lock_slow`, but can be interrupted by signals.
> > @@ -236,30 +290,14 @@ pub fn lock_slow_interruptible<'a, T>(
> > &'a self,
> > ww_mutex: &'a WwMutex<'a, T>,
> > ) -> Result<WwMutexGuard<'a, T>> {
> > - // SAFETY: The mutex is pinned and valid, and we are in
> > the slow path.
> > - let ret = unsafe {
> > -
> > bindings::ww_mutex_lock_slow_interruptible(ww_mutex.mutex.get(),
> > self.inner.get())
> > - };
> > -
> > - to_result(ret)?;
> > -
> > - Ok(WwMutexGuard::new(ww_mutex))
> > + lock_common(ww_mutex, Some(self),
> > LockKind::SlowInterruptible) }
> >
> > - /// Tries to lock the mutex without blocking.
> > + /// Tries to lock the mutex on this acquire context
> > ([`WwAcquireCtx`]) without blocking. ///
> > /// Unlike `lock`, no deadlock handling is performed.
> > pub fn try_lock<'a, T>(&'a self, ww_mutex: &'a WwMutex<'a, T>)
> > -> Result<WwMutexGuard<'a, T>> {
> > - // SAFETY: The mutex is pinned and valid.
> > - let ret = unsafe {
> > bindings::ww_mutex_trylock(ww_mutex.mutex.get(), self.inner.get())
> > }; -
> > - if ret == 0 {
> > - return Err(EBUSY);
> > - } else {
> > - to_result(ret)?;
> > - }
> > -
> > - Ok(WwMutexGuard::new(ww_mutex))
> > + lock_common(ww_mutex, Some(self), LockKind::Try)
> > }
> > }
> >
> > @@ -355,7 +393,7 @@ pub fn new(t: T, ww_class: &'ww_class WwClass)
> > -> impl PinInit<Self> { }
> > }
> >
> > -impl<T: ?Sized> WwMutex<'_, T> {
> > +impl<'ww_class, T: ?Sized> WwMutex<'ww_class, T> {
> > /// Returns a raw pointer to the inner mutex.
> > fn as_ptr(&self) -> *mut bindings::ww_mutex {
> > self.mutex.get()
> > @@ -370,6 +408,35 @@ fn is_locked(&self) -> bool {
> > // SAFETY: The mutex is pinned and valid.
> > unsafe { bindings::ww_mutex_is_locked(self.mutex.get()) }
> > }
> > +
> > + /// Locks the given mutex without acquire context
> > ([`WwAcquireCtx`]).
> > + pub fn lock<'a>(&'a self) -> Result<WwMutexGuard<'a, T>> {
> > + lock_common(self, None, LockKind::Regular)
> > + }
> > +
> > + /// Similar to `lock`, but can be interrupted by signals.
> > + pub fn lock_interruptible<'a>(&'a self) ->
> > Result<WwMutexGuard<'a, T>> {
> > + lock_common(self, None, LockKind::Interruptible)
> > + }
> > +
> > + /// Locks the given mutex without acquire context
> > ([`WwAcquireCtx`]) using the slow path.
> > + ///
> > + /// This function should be used when `lock` fails (typically
> > due to a potential deadlock).
> > + pub fn lock_slow<'a>(&'a self) -> Result<WwMutexGuard<'a, T>> {
> > + lock_common(self, None, LockKind::Slow)
> > + }
> > +
> > + /// Similar to `lock_slow`, but can be interrupted by signals.
> > + pub fn lock_slow_interruptible<'a>(&'a self) ->
> > Result<WwMutexGuard<'a, T>> {
> > + lock_common(self, None, LockKind::SlowInterruptible)
> > + }
> > +
> > + /// Tries to lock the mutex without acquire context
> > ([`WwAcquireCtx`]) and without blocking.
>
> “With no acquire context”.
>
> > + ///
> > + /// Unlike `lock`, no deadlock handling is performed.
> > + pub fn try_lock<'a>(&'a self) -> Result<WwMutexGuard<'a, T>> {
> > + lock_common(self, None, LockKind::Try)
> > + }
> > }
> >
> > /// A guard that provides exclusive access to the data protected
> > @@ -547,4 +614,19 @@ fn test_with_global_classes() -> Result {
> >
> > Ok(())
> > }
> > +
> > + #[test]
> > + fn test_mutex_without_ctx() -> Result {
> > + let mutex = Arc::pin_init(WwMutex::new(100,
> > &TEST_WOUND_WAIT_CLASS), GFP_KERNEL)?;
> > + let guard = mutex.lock()?;
> > +
> > + assert_eq!(*guard, 100);
> > + assert!(mutex.is_locked());
> > +
> > + drop(guard);
> > +
> > + assert!(!mutex.is_locked());
> > +
> > + Ok(())
> > + }
> > }
> > --
> > 2.50.0
> >
> >
>
Powered by blists - more mailing lists