Re: [RFC 08/10] Set PF_RECLAIMABLE_STACK in various places

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Safe Rust methods `set_flag_bits` and `clear_flag_bits` on `Task` allow data races when called on non-current tasks.
- [Medium] Unconditional clearing of `PF_RECLAIMABLE_STACK` in Rust binder breaks flag preservation logic.
--

commit 6f8ac02f62f897200c71ad824d4019095dc4428c
Author: David Stevens <stevensd@google.com>

Set PF_RECLAIMABLE_STACK in various places

Annotate various blocking locations with the PF_RECLAIMABLE_STACK.

> diff --git a/drivers/android/binder/thread.rs b/drivers/android/binder/thread.rs
> index bc0ef8927905c..be62f7fd43ca2 100644
> --- a/drivers/android/binder/thread.rs
> +++ b/drivers/android/binder/thread.rs
> @@ -538,13 +538,21 @@ fn get_work_local(self: &Arc<Self>, wait: bool) -> Result<Option<DLArc<dyn Deliv
[ ... ]
>              inner.looper_flags |= LOOPER_WAITING;
> +
> +            current.set_flag_bits(bindings::PF_RECLAIMABLE_STACK);
> +
>              let signal_pending = self.work_condvar.wait_interruptible_freezable(&mut inner);
> +
> +            current.clear_flag_bits(bindings::PF_RECLAIMABLE_STACK);

[Severity: Medium]
Will unconditionally clearing the flag break flag preservation logic if stack
reclaim states are nested?

The C allow_stack_reclaim guard explicitly saves and restores the flag to
support nesting. If get_work_local() is called from a context where
PF_RECLAIMABLE_STACK is already set, this will unexpectedly disable stack
reclamation for the outer scope upon exit.

> +
>              inner.looper_flags &= !LOOPER_WAITING;
>  
>              if signal_pending {
> @@ -592,15 +600,21 @@ fn get_work(self: &Arc<Self>, wait: bool) -> Result<Option<DLArc<dyn DeliverToRe
[ ... ]
> +            current.set_flag_bits(bindings::PF_RECLAIMABLE_STACK);
> +
>              inner.looper_flags |= LOOPER_WAITING | LOOPER_WAITING_PROC;
>              let signal_pending = self.work_condvar.wait_interruptible_freezable(&mut inner);
>              inner.looper_flags &= !(LOOPER_WAITING | LOOPER_WAITING_PROC);
>  
> +            current.clear_flag_bits(bindings::PF_RECLAIMABLE_STACK);

[Severity: Medium]
Similar to get_work_local(), is it safe to unconditionally clear the flag here
in get_work() instead of restoring its previous state?

> +
>              if signal_pending || inner.looper_need_return {
>                  // We need to return now. We need to pull the thread off the list of ready threads
>                  // (by dropping `reg`), then check the state again after it's off the list to
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 38273f4eedb51..6168f058343a2 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -344,6 +344,22 @@ pub fn group_leader(&self) -> &Task {
>          // only be used while `current` is still valid, thus still running.
>          unsafe { &*ptr.cast() }
>      }
> +
> +    /// Sets the given task flag bits on the current task.
> +    #[inline]
> +    pub fn set_flag_bits(&self, set: u32) {
> +            // SAFETY: The `flags` field of `current` is not modified from other threads, so
> +	    // the non-atomic update isn't a race.
> +	    unsafe { (*self.as_ptr()).flags |= set }
> +    }

[Severity: High]
Does this expose a soundness hole by allowing data races on the flags field
when called on a non-current task?

The safety comment assumes self is the current task, but the method signature
accepts any Task reference. Because Task implements Send + Sync, another thread
could obtain a reference to a non-current task (for example, via group_leader())
and safely call this method. This allows an unsafe non-atomic read-modify-write
and undefined behavior in safe Rust.

> +
> +    /// Clears the given task flag bits on the current task.
> +    #[inline]
> +    pub fn clear_flag_bits(&self, clear: u32) {
> +            // SAFETY: The `flags` field of `current` is not modified from other threads, so
> +	    // the non-atomic update isn't a race.
> +	    unsafe { (*self.as_ptr()).flags &= !clear }
> +    }
>  }

[Severity: High]
Does this method suffer from the same soundness hole as set_flag_bits() since it
is a safe method performing a non-atomic update without verifying self is the
current task?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827232948.2520558-1-stevensd@google.com?part=8