Re: [PATCH v6 4/6] rust: xarray: Add support for index reservations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Calling `XArray::reserve()` or dropping `Reservation` while holding the XArray lock (`Guard`) causes a guaranteed A-A self-deadlock.
- [High] SAFETY and INVARIANT comments in `XArray::reserve` and `Reservation::drop` are factually wrong, reference nonexistent variables, and actively promote deadlocking code.
- [Medium] The `Reservation::drop` behavior allows multiple threads to accidentally destroy each other's reservations, leading to unexpected `-ENOMEM` errors during atomic stores.
--

--- Patch [4]: [PATCH v6 4/6] rust: xarray: Add support for index reservations ---
commit b249894b0dc68234b47a8d9e38c201513786806c
Author: Philipp Stanner <phasta@kernel.org>

rust: xarray: Add support for index reservations

This commit adds support for xa_reserve() to the Rust XArray abstractions
by introducing reserve() and a Reservation drop guard.

> diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
> index 987c9c0c21989..fa5d47935f0a7 100644
> --- a/rust/kernel/xarray.rs
> +++ b/rust/kernel/xarray.rs
[ ... ]
> @@ -147,6 +150,46 @@ pub fn lock(&self) -> Guard<'_, T> {
>              _not_send: NotThreadSafe,
>          }
>      }
> +
> +    /// Reserve memory in the XArray at the appropriate index. If this call
> +    /// succeeds, later storing at the index will not require an allocation.
> +    ///
> +    /// Loading from reserved entries will return `None`.
> +    pub fn reserve(&self, index: usize, gfp: alloc::Flags) -> Result<Reservation<'_, T>> {
> +        // SAFETY:
> +        // - `self.xa.xa` is always valid by the type invariant.
> +        // - The caller holds the lock.
> +        //
> +        // INVARIANT: `new` came from `T::into_foreign`.

[Severity: High]
The safety comment in reserve() explicitly states "The caller holds the
lock." However, both xa_reserve() and xa_release() internally acquire the
xa_lock spinlock.

If the caller follows this documentation and acquires the lock via
XArray::lock() before calling reserve(), won't this result in an A-A
self-deadlock when xa_reserve() attempts to acquire the same non-reentrant
spinlock?

Also, this isn't a bug, but the INVARIANT references a nonexistent variable
new. Is this a copy-paste error from another function?

> +        let ret = unsafe { bindings::xa_reserve(self.xa.get(), index, gfp.as_raw()) };
> +        match ret {
> +            0 => Ok(Reservation { xa: self, index }),

[Severity: Medium]
In C, xa_reserve() returns 0 both when it successfully reserves an empty
slot and when the slot is already reserved (XA_ZERO_ENTRY).

If two threads concurrently call reserve() on the same index, won't both
succeed and receive a Reservation guard?

When the first thread's guard drops and unconditionally calls xa_release(),
won't it overwrite XA_ZERO_ENTRY with NULL, silently destroying the second
thread's active reservation and potentially leading to an unexpected -ENOMEM
on a later atomic store?

> +            _ => Err(Error::from_errno(ret)),
> +        }
> +    }
> +}
> +
> +/// An index reservation object.
> +///
> +/// As long as you keep this object alive, your reservation will stay valid. If
> +/// someone has stored to this index in the meantime, dropping your reservation
> +/// does nothing, which is why you can always drop this reservation object once
> +/// you performed your store operation.
> +///
> +/// Refer to the main C xarray documentation for more details.
> +pub struct Reservation<'a, T: ForeignOwnable> {
> +    xa: &'a XArray<T>,
> +    index: usize,
> +}
> +
> +impl<T: ForeignOwnable> Drop for Reservation<'_, T> {
> +    fn drop(&mut self) {
> +        // SAFETY:
> +        // - `self.xa` is always valid by the type invariant.
> +        // - If `self.id` is not used or has been stored to by a racing party,

[Severity: High]
This isn't a bug, but the safety comment here in the drop() method
references self.id which does not exist on this struct. Should this be
self.index instead?

> +        //   `xa_release()` will do nothing.
> +        unsafe { bindings::xa_release(self.xa.xa.get(), self.index) };

[Severity: High]
Since xa_release() internally acquires the xa_lock spinlock, will this
cause a deadlock if the Reservation guard is dropped in Drop for Reservation
while the caller still holds the XArray::lock() guard?

> +    }
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722144931.1070312-2-phasta@kernel.org?part=4