Re: [PATCH] sched/task: always defer 'struct task_struct' destruction via RCU

[Andrea Righi <arighi@nvidia.com>]

Hi Alice,

On Fri, May 08, 2026 at 02:02:45PM +0000, Alice Ryhl wrote:
> The sched/task.h header file currently exposes a tryget_task_struct()
> function, but it is very risky to use it: If the last refcount of the
> task is dropped using put_task_struct_many(), then the task is freed
> right away without an RCU grace period.
> 
> This means that if the kernel contains a code path anywhere such that
> the last refcount of a task may be dropped with put_task_struct_many(),
> and it also contains a code path anywhere that tries to stash a task
> pointer under rcu and use tryget_task_struct() on it, then if they ever
> execute on the same 'struct task_struct', it results in a
> use-after-free.
> 
> The above applies even if the RCU user drops its own task reference with
> put_task_struct(), because if that is not the last reference, then it's
> possible for another thread to invoke put_task_struct_many() and free
> the task less than a grace period after the RCU user called
> put_task_struct().
> 
> There does not appear to be an actual problem in the kernel tree right
> now because there are no in-tree users of put_task_struct_many() where
> refcount_sub_and_test() might return 'true'. Io-uring invokes the
> function from task work while the task is still running, so it will not
> decrement it all the way to zero. (Note that if I'm wrong about this,
> then it's probably possible to trigger UAF by combining this codepath in
> io-uring with the tryget_task_struct() call in sched-ext.)
> 
> However, the current situation is fragile and error-prone.
> - If you look at put_task_struct_many() in isolation, it looks like it
>   would be okay to call it in a situation where refcount_sub_and_test()
>   might return 'true'.
> - Similarly, if you look at tryget_task_struct(), you would assume that
>   you are allowed to call this method for a grace period after 'users'
>   hitting zero. (If not, why does it exist?)
> But if two different kernel developers anywhere in the kernel make these
> conflicting assumptions at any point in the future, then the combination
> of their code may lead to a use-after-free if there is any way for them
> to interact via the same 'struct task_struct'.
> 
> Thus, as a defensive measure, we should either make
> put_task_struct_many() use call_rcu(), or we should delete
> tryget_task_struct(). This patch suggests the former because it does not
> change anything for any callers that exist today. (As argued previously,
> the body of the 'if' statement is dead code in the kernel today.)
> 
> The comment in put_task_struct() is also updated so that nobody changes
> its implementation to only use call_rcu() under PREEMPT_RT in the
> future. The current comment suggests that would be a legal change, but
> it is similarly incompatible with anyone using tryget_task_struct().
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> Including sched-ext and io-uring in the cc list as they are the only
> users of tryget_task_struct() and put_task_struct_many() respectively.

For sched_ext I think we should be already protected by scx_tasks_lock.

From kernel/sched/core.c:

  finish_task_switch():
      if (prev_state == TASK_DEAD) {
          prev->sched_class->task_dead(prev);
          sched_ext_dead(prev);
          cgroup_task_dead(prev);
          put_task_stack(prev);
          ...
          put_task_struct_rcu_user(prev);
      }

 And sched_ext_dead() in kernel/sched/ext.c:

  scoped_guard(raw_spinlock_irqsave, &scx_tasks_lock) {
      list_del_init(&p->scx.tasks_node);
      ...
  }

Now on the sched_ext iter side:

  scx_task_iter_start();                     /* takes scx_tasks_lock */
  while ((p = scx_task_iter_next_locked()))
      if (!tryget_task_struct(p))            /* still under scx_tasks_lock */
         ...

So, the locking gives us the invariant: while the iter holds scx_tasks_lock and
observes p on the list, sched_ext_dead(p) cannot have completed.

And the css_task_iter paths have the analogous ordering.

That said, I think this patch still makes sense to provide a consistent
semantics between put_task_struct() and put_task_struct_many(), as mentioned by
Sebastian. So, maybe reword the message around consistency rather than UAF?

Thanks,
-Andrea

> ---
>  include/linux/sched/task.h | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
> index 41ed884cffc9..da2fbd17b676 100644
> --- a/include/linux/sched/task.h
> +++ b/include/linux/sched/task.h
> @@ -131,19 +131,25 @@ static inline void put_task_struct(struct task_struct *t)
>  		return;
>  
>  	/*
> -	 * Under PREEMPT_RT, we can't call __put_task_struct
> -	 * in atomic context because it will indirectly
> -	 * acquire sleeping locks. The same is true if the
> -	 * current process has a mutex enqueued (blocked on
> -	 * a PI chain).
> +	 * Delay __put_task_struct() for one grace period so
> +	 * that tryget_task_struct() may be used for one
> +	 * grace period after any call to put_task_struct().
>  	 *
> -	 * In !RT, it is always safe to call __put_task_struct().
> -	 * Though, in order to simplify the code, resort to the
> -	 * deferred call too.
> +	 * This also has the benefit of making it legal to
> +	 * call put_task_struct() in atomic context. We
> +	 * can't do that under PREEMPT_RT because it will
> +	 * indirectly acquire sleeping locks. The same is
> +	 * true if the current process has a mutex enqueued
> +	 * (blocked on a PI chain).
>  	 *
>  	 * call_rcu() will schedule __put_task_struct_rcu_cb()
>  	 * to be called in process context.
>  	 *
> +	 * In !RT, it is safe to call __put_task_struct()
> +	 * from atomic context, but we still need to delay
> +	 * cleanup for a grace period to accommodate
> +	 * tryget_task_struct() callers.
> +	 *
>  	 * __put_task_struct() is called when
>  	 * refcount_dec_and_test(&t->usage) succeeds.
>  	 *
> @@ -164,7 +170,7 @@ DEFINE_FREE(put_task, struct task_struct *, if (_T) put_task_struct(_T))
>  static inline void put_task_struct_many(struct task_struct *t, int nr)
>  {
>  	if (refcount_sub_and_test(nr, &t->usage))
> -		__put_task_struct(t);
> +		call_rcu(&t->rcu, __put_task_struct_rcu_cb);
>  }
>  
>  void put_task_struct_rcu_user(struct task_struct *task);
> 
> ---
> base-commit: 7fd2df204f342fc17d1a0bfcd474b24232fb0f32
> change-id: 20260508-put-task-struct-many-5b5b2f4ae174
> 
> Best regards,
> -- 
> Alice Ryhl <aliceryhl@google.com>
>