Re: [PATCH v10 5/7] rust: ww_mutex: add Mutex, AcquireCtx and MutexGuard

[Daniel Almeida <daniel.almeida@collabora.com>]

Hi Onur,

> +impl<'class, T: ?Sized> Mutex<'class, T> {
> +    /// Checks if this [`Mutex`] is currently locked.
> +    ///
> +    /// The returned value is racy as another thread can acquire
> +    /// or release the lock immediately after this call returns.
> +    pub fn is_locked(&self) -> bool {
> +        // SAFETY: It's safe to call `ww_mutex_is_locked` on
> +        // a valid mutex.
> +        unsafe { bindings::ww_mutex_is_locked(self.inner.get()) }
> +    }
> +
> +    /// Locks this [`Mutex`] without [`AcquireCtx`].
> +    pub fn lock(&self) -> Result<MutexGuard<'_, T>> {
> +        lock_common(self, None, LockKind::Regular)
> +    }
> +
> +    /// Similar to [`Self::lock`], but can be interrupted by signals.
> +    pub fn lock_interruptible(&self) -> Result<MutexGuard<'_, T>> {
> +        lock_common(self, None, LockKind::Interruptible)
> +    }
> +
> +    /// Locks this [`Mutex`] without [`AcquireCtx`] using the slow path.
> +    ///
> +    /// This function should be used when [`Self::lock`] fails (typically due
> +    /// to a potential deadlock).
> +    pub fn lock_slow(&self) -> Result<MutexGuard<'_, T>> {
> +        lock_common(self, None, LockKind::Slow)
> +    }
> +
> +    /// Similar to [`Self::lock_slow`], but can be interrupted by signals.
> +    pub fn lock_slow_interruptible(&self) -> Result<MutexGuard<'_, T>> {
> +        lock_common(self, None, LockKind::SlowInterruptible)
> +    }

^ Let's remove the slow path, this is equivalent to a normal lock(),
except that it also contains this dereference:

static inline void
ww_mutex_lock_slow(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
{
    int ret;
#ifdef DEBUG_WW_MUTEXES
    DEBUG_LOCKS_WARN_ON(!ctx->contending_lock); <-----
#endif
    ret = ww_mutex_lock(lock, ctx);
    (void)ret;
}

But we (and most of the C API) allow null ctxs:

    let ctx_ptr = match ctx {
        Some(acquire_ctx) => {
            let ctx_ptr = acquire_ctx.inner.get();

            // SAFETY: `ctx_ptr` is a valid pointer for the entire
            // lifetime of `ctx`.
            let ctx_class = unsafe { (*ctx_ptr).ww_class };

            // SAFETY: `mutex_ptr` is a valid pointer for the entire
            // lifetime of `mutex`.
            let mutex_class = unsafe { (*mutex_ptr).ww_class };

            // `ctx` and `mutex` must use the same class.
            if ctx_class != mutex_class {
                return Err(EINVAL);
            }

            ctx_ptr
        }
        None => core::ptr::null_mut(), <----
    };

IOW, to call the slow path correctly, the Rust side would already have
to know the thing the slow path checks, and then the slow path adds
nothing.

Even the docs say:

 * Note that the slowpath lock acquiring can also be done by calling
 * ww_mutex_lock directly. This function here is simply to help w/w mutex
 * locking code readability by clearly denoting the slowpath.

By the way, LockSet itself does not use it, so let's drop that. It also
solves some problems in the other patches too.

> // SAFETY: `Mutex` can be shared across threads if the protected
> // data `T` can be.
> unsafe impl<T: ?Sized + Send + Sync> Sync for Mutex<'_, T> {}

I don't exactly remember why this has to be different than sync::Lock?
i.e.:

// SAFETY: `Lock` serialises the interior mutability it provides, so it is `Sync` as long as the
// data it protects is `Send`.
unsafe impl<T: ?Sized + Send, B: Backend> Sync for Lock<T, B> {}

Why does one require Send + Sync and the other just Send?

> +impl<'a> MutexGuard<'a, ()> {
> +    /// Creates a [`MutexGuard`] from a raw pointer.
> +    ///
> +    /// If the given pointer refers to a mutex that is not locked,
> +    /// returns [`EINVAL`].
> +    ///
> +    /// This function is intended for interoperability with C code.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that:
> +    ///
> +    /// - `ptr` is a valid pointer to a `ww_mutex`.
> +    /// - `ptr` must remain valid for the lifetime `'b`.
> +    /// - The `ww_class` associated with the `ww_mutex` must be valid for the lifetime `'b`.
> +    pub unsafe fn from_raw<'b>(ptr: *mut bindings::ww_mutex) -> Result<MutexGuard<'b, ()>> {
> +        // SAFETY: By this function's safety contract, the caller guarantees that `ptr` points to a
> +        // valid `ww_mutex` which is the `inner` field of a `Mutex`. The caller also guarantees
> +        // that both `ptr` and the associated `ww_class` are valid for the lifetime `'b`.
> +        let mutex = unsafe { Mutex::from_raw(ptr) };
> +
> +        if !mutex.is_locked() {
> +            return Err(EINVAL);
> +        }
> +
> +        Ok(MutexGuard::new(mutex))
> +    }
> +}

The caller must also guarantee that the current task holds this lock,
and that it won't unlock it itself afterwards. Otherwise we may release
someone else's lock, or release it twice.

> +        LockKind::Slow => {
> +            // SAFETY: `Mutex` is always pinned. If `AcquireCtx` 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(mutex_ptr, ctx_ptr) };
> +        }
> +        LockKind::SlowInterruptible => {
> +            // SAFETY: `Mutex` is always pinned. If `AcquireCtx` 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(mutex_ptr, ctx_ptr) };
> +
> +            to_result(ret)?;
> +        }

Also remove the slowpath in AcquireCtx, but for a different reason:
ww_mutex_lock_slow() throws away the return value of ww_mutex_lock().
That is only fine in C because they "require" that the caller not hold
any other lock of the context. Here nothing enforces that, so
ctx.lock(&m) followed by ctx.lock_slow(&m) returns Ok with a second
guard for m.


> +    /// Marks the end of the acquire phase.
> +    ///
> +    /// Calling this function is optional. It is just useful to document
> +    /// the code and clearly designated the acquire phase from actually
> +    /// using the locked data structures.
> +    ///
> +    /// After calling this function, no more mutexes can be acquired with
> +    /// this context.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that this function is called only once
> +    /// and after calling it, no further mutexes are acquired using
> +    /// this context.
> +    pub unsafe fn done(&self) {
> +        // SAFETY: By the safety contract, the caller guarantees that this
> +        // function is called only once.
> +        unsafe { bindings::ww_acquire_done(self.inner.get()) };
> +    }

^ Are we sure that this needs to be unsafe? The function itself merely
sets a flag:

/**
 * ww_acquire_done - marks the end of the acquire phase
 * @ctx: the acquire context
 *
 * Marks the end of the acquire phase, any further w/w mutex lock calls using
 * this context are forbidden.
 *
 * Calling this function is optional, it is just useful to document w/w mutex
 * code and clearly designated the acquire phase from actually using the locked
 * data structures.
 */
static inline void ww_acquire_done(struct ww_acquire_ctx *ctx)
{
#ifdef DEBUG_WW_MUTEXES
    lockdep_assert_held(ctx);

    DEBUG_LOCKS_WARN_ON(ctx->done_acquire);
    ctx->done_acquire = 1;
#endif
}

This is even a no-op if DEBUG_WW_MUTEXES is not set.

> +    /// Locks the given [`Mutex`] on this [`AcquireCtx`].
> +    pub fn lock<'a, T>(&'a self, mutex: &'a Mutex<'a, T>) -> Result<MutexGuard<'a, T>> {
> +        lock_common(mutex, Some(self), LockKind::Regular)
> +    }
> +
> +    /// Similar to [`Self::lock`], but can be interrupted by signals.
> +    pub fn lock_interruptible<'a, T>(
> +        &'a self,
> +        mutex: &'a Mutex<'a, T>,
> +    ) -> Result<MutexGuard<'a, T>> {
> +        lock_common(mutex, Some(self), LockKind::Interruptible)
> +    }
> +
> +    /// Locks the given [`Mutex`] on this [`AcquireCtx`] using the slow path.
> +    ///
> +    /// This function should be used when [`Self::lock`] fails (typically due
> +    /// to a potential deadlock).
> +    pub fn lock_slow<'a, T>(&'a self, mutex: &'a Mutex<'a, T>) -> Result<MutexGuard<'a, T>> {
> +        lock_common(mutex, Some(self), LockKind::Slow)
> +    }
> +
> +    /// Similar to [`Self::lock_slow`], but can be interrupted by signals.
> +    pub fn lock_slow_interruptible<'a, T>(
> +        &'a self,
> +        mutex: &'a Mutex<'a, T>,
> +    ) -> Result<MutexGuard<'a, T>> {
> +        lock_common(mutex, Some(self), LockKind::SlowInterruptible)
> +    }
> +
> +    /// Tries to lock the [`Mutex`] on this [`AcquireCtx`] without blocking.
> +    ///
> +    /// Unlike [`Self::lock`], no deadlock handling is performed.
> +    pub fn try_lock<'a, T>(&'a self, mutex: &'a Mutex<'a, T>) -> Result<MutexGuard<'a, T>> {
> +        lock_common(mutex, Some(self), LockKind::Try)
> +    }
> +}

These  suffer from the same mem::forget() issue that plagued a similar
patch recently.

When you lock, the C side will remember the ctx in a field. If you
mem::forget() the Guard, the borrow on AcquireCtx is gone, and ctx can
drop, and lock->ctx dangles.

My preferred solution is to make the locking functions unsafe fn if they
take a context, with the requirement that the lock is released before
the context goes away.

-- Daniel