Re: [PATCH v3] rust: alloc: add per-task memalloc scope abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] rust: alloc: safe ScopeKind::save() allows violating
  restore() safety
- [Low] rust: alloc: missing #[inline] on ScopeKind::with() and C
  forwarders
--

commit 41cec7ee242c1aa90cf2533b8574e3fdf2331dac
Author: Andreas Hindborg <a.hindborg@kernel.org>

rust: alloc: add per-task memalloc scope abstractions

This commit adds a ScopeKind trait and the tag types NoIo, NoFs, NoReclaim,
and MemallocPin to expose the per-task memalloc_*_save() and
memalloc_*_restore() helpers in Rust through a closure-based ScopeKind::with()
method.

> diff --git a/rust/kernel/alloc/scoped.rs b/rust/kernel/alloc/scoped.rs
> new file mode 100644
> index 0000000000000..6745c06d6725c
> --- /dev/null
> +++ b/rust/kernel/alloc/scoped.rs

[ ... ]

> +pub trait ScopeKind {
> +    /// Begin a scope on the current task and return the flags added to `current->flags`.
> +    ///
> +    /// Every call must be paired with a [`restore`] call on the same task, in LIFO order with
> +    /// respect to other scopes. Prefer [`with`], which arranges this automatically.
> +    ///
> +    /// [`restore`]: ScopeKind::restore
> +    /// [`with`]: ScopeKind::with
> +    fn save() -> c_uint;
> +
> +    /// End a scope on the current task.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `prev` must be the value returned by the matching [`save`] call, the call must execute on
> +    /// the same task that ran [`save`], and no scope begun after the matching [`save`] may still be
> +    /// active.
> +    ///
> +    /// [`save`]: ScopeKind::save
> +    unsafe fn restore(prev: c_uint);
> +
> +    /// Run `f` with this policy applied.
> +    ///
> +    /// The current policy is saved before `f` runs and restored when `f` returns.
> +    fn with<R>(f: impl FnOnce() -> R) -> R {
> +        let prev = Self::save();
> +        let ret = f();
> +        // SAFETY: `prev` was produced by the `save` above on this task. `f` is a synchronous
> +        // closure, so it has already returned and any scope it began has already been restored.
> +        unsafe { Self::restore(prev) };
> +        ret
> +    }
> +}

[Severity: Medium]
Should ScopeKind::save() be marked unsafe or moved to a private sealed
supertrait?

Because ScopeKind::save() is a safe public method, safe code can call
NoIo::save(), NoFs::save(), NoReclaim::save(), or MemallocPin::save() without
a matching restore(), permanently leaving PF_MEMALLOC_NOIO, PF_MEMALLOC_NOFS,
PF_MEMALLOC, or PF_MEMALLOC_PIN set in current->flags. Safe code inside the
closure passed to ScopeKind::with() can also call save() and return without
restoring it, violating the safety precondition on ScopeKind::restore() that
no scope begun after the matching save() is still active.

[ ... ]

> +pub enum NoIo {}
> +
> +impl ScopeKind for NoIo {
> +    fn save() -> c_uint {
> +        // SAFETY: Updates a per-task flag and is documented as safe from any context.
> +        unsafe { bindings::memalloc_noio_save() }
> +    }
> +
> +    unsafe fn restore(prev: c_uint) {
> +        // SAFETY: Per the trait contract, `prev` is the value returned by the matching `save`, on
> +        // the same task.
> +        unsafe { bindings::memalloc_noio_restore(prev) };
> +    }
> +}

[Severity: Low]
This isn't a bug, but ScopeKind::with() and the save() and restore()
implementations for NoIo, NoFs, NoReclaim, and MemallocPin are small kernel
crate helpers and direct C binding forwarders that are missing #[inline]
attributes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-gfp-noio-v3-1-a573e7be9c44@kernel.org?part=1