[PATCH] rust: task: restrict Task::group_leader() to current

[PATCH] rust: task: restrict Task::group_leader() to current

[Alice Ryhl]

The Task::group_leader() method currently allows you to access the
group_leader() of any task, for example one you hold a refcount to. But
this is not safe in general since the group leader could change when a
task exits. See for example commit a15f37a40145c ("kernel/sys.c: fix the
racy usage of task_lock(tsk->group_leader) in sys_prlimit64() paths").

All existing users of Task::group_leader() call this method on current,
which is guaranteed running, so there's not an actual issue in Rust code
today. But to prevent code in the future from making this mistake,
restrict Task::group_leader() so that it can only be called on current.

There are some other cases where accessing task->group_leader is okay.
For example it can be safe if you hold tasklist_lock or rcu_read_lock().
However, only supporting current->group_leader is sufficient for all
in-tree Rust users of group_leader right now. Safe Rust functionality
for accessing it under rcu or while holding tasklist_lock may be added
in the future if required by any future Rust module.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Closes: https://lore.kernel.org/all/aTLnV-5jlgfk1aRK@redhat.com/
Fixes: 313c4281bc9d ("rust: add basic `Task`")
Cc: stable@vger.kernel.org
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
The rust/kernel/task.rs file has had changes land through a few
different trees:

* Originally task.rs landed through Christian's tree together with
  file.rs and pid_namespace.rs
* The change to add CurrentTask landed through Andrew Morton's tree
  together with mm.rs
* There was a patch to mark some methods #[inline] that landed through
  tip via Boqun.

I don't think there's a clear owner for this file, so to break ambiguity
I'm doing to declare that this patch is intended for Andrew Morton's
tree. Please let me know if you think a different tree is appropriate.
---
 rust/kernel/task.rs | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 49fad6de06740a9b9ad80b2f4b430cc28cd134fa..9440692a3a6d0d3f908d61d51dcd377a272f6957 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -204,18 +204,6 @@ pub fn as_ptr(&self) -> *mut bindings::task_struct {
         self.0.get()
     }
 
-    /// Returns the group leader of the given task.
-    pub fn group_leader(&self) -> &Task {
-        // SAFETY: The group leader of a task never changes after initialization, so reading this
-        // field is not a data race.
-        let ptr = unsafe { *ptr::addr_of!((*self.as_ptr()).group_leader) };
-
-        // SAFETY: The lifetime of the returned task reference is tied to the lifetime of `self`,
-        // and given that a task has a reference to its group leader, we know it must be valid for
-        // the lifetime of the returned task reference.
-        unsafe { &*ptr.cast() }
-    }
-
     /// Returns the PID of the given task.
     pub fn pid(&self) -> Pid {
         // SAFETY: The pid of a task never changes after initialization, so reading this field is
@@ -345,6 +333,18 @@ pub fn active_pid_ns(&self) -> Option<&PidNamespace> {
         // `release_task()` call.
         Some(unsafe { PidNamespace::from_ptr(active_ns) })
     }
+
+    /// Returns the group leader of the current task.
+    pub fn group_leader(&self) -> &Task {
+        // SAFETY: The group leader of a task never changes while the task is running, and `self`
+        // is the current task, which is guaranteed running.
+        let ptr = unsafe { (*self.as_ptr()).group_leader };
+
+        // SAFETY: `current.group_leader` stays valid for at least the duration in which `current`
+        // is running, and the signature of this function ensures that the returned `&Task` can
+        // only be used while `current` is still valid, thus still running.
+        unsafe { &*ptr.cast() }
+    }
 }
 
 // SAFETY: The type invariants guarantee that `Task` is always refcounted.

---
base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
change-id: 20251218-task-group-leader-a71931ced643

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

Re: [PATCH] rust: task: restrict Task::group_leader() to current

[Oleg Nesterov]

On 12/18, Alice Ryhl wrote:
>
> The Task::group_leader() method currently allows you to access the
> group_leader() of any task, for example one you hold a refcount to. But
> this is not safe in general since the group leader could change when a
> task exits. See for example commit a15f37a40145c ("kernel/sys.c: fix the
> racy usage of task_lock(tsk->group_leader) in sys_prlimit64() paths").
>
> All existing users of Task::group_leader() call this method on current,
> which is guaranteed running, so there's not an actual issue in Rust code
> today. But to prevent code in the future from making this mistake,
> restrict Task::group_leader() so that it can only be called on current.
>
> There are some other cases where accessing task->group_leader is okay.
> For example it can be safe if you hold tasklist_lock or rcu_read_lock().
> However, only supporting current->group_leader is sufficient for all
> in-tree Rust users of group_leader right now. Safe Rust functionality
> for accessing it under rcu or while holding tasklist_lock may be added
> in the future if required by any future Rust module.

I obviously can't ACK this patch ;) but just in case, it looks good to me.

Although I am not sure this is a stable material... Exactly because,
as you mentioned, all existing users call this method on current.

> I don't think there's a clear owner for this file, so to break ambiguity
> I'm doing to declare that this patch is intended for Andrew Morton's
> tree. Please let me know if you think a different tree is appropriate.

If Andrew agrees and nobody objects this would be nice. I am going to
send some tree-wide changes related to task_struct.group_leader usage,
it would be simpler to route them all via -mm tree.

So far I sent the trivial preparations

	[PATCH 0/7] don't abuse task_struct.group_leader
	https://lore.kernel.org/all/aTV1pbftBkH8n4kh@redhat.com/

and I am still waiting for more reviews. Alice, perhaps you can review
the (hopefully trivial) 1-2 which touch android/binder?

Oleg.

> ---
>  rust/kernel/task.rs | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 49fad6de06740a9b9ad80b2f4b430cc28cd134fa..9440692a3a6d0d3f908d61d51dcd377a272f6957 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -204,18 +204,6 @@ pub fn as_ptr(&self) -> *mut bindings::task_struct {
>          self.0.get()
>      }
>
> -    /// Returns the group leader of the given task.
> -    pub fn group_leader(&self) -> &Task {
> -        // SAFETY: The group leader of a task never changes after initialization, so reading this
> -        // field is not a data race.
> -        let ptr = unsafe { *ptr::addr_of!((*self.as_ptr()).group_leader) };
> -
> -        // SAFETY: The lifetime of the returned task reference is tied to the lifetime of `self`,
> -        // and given that a task has a reference to its group leader, we know it must be valid for
> -        // the lifetime of the returned task reference.
> -        unsafe { &*ptr.cast() }
> -    }
> -
>      /// Returns the PID of the given task.
>      pub fn pid(&self) -> Pid {
>          // SAFETY: The pid of a task never changes after initialization, so reading this field is
> @@ -345,6 +333,18 @@ pub fn active_pid_ns(&self) -> Option<&PidNamespace> {
>          // `release_task()` call.
>          Some(unsafe { PidNamespace::from_ptr(active_ns) })
>      }
> +
> +    /// Returns the group leader of the current task.
> +    pub fn group_leader(&self) -> &Task {
> +        // SAFETY: The group leader of a task never changes while the task is running, and `self`
> +        // is the current task, which is guaranteed running.
> +        let ptr = unsafe { (*self.as_ptr()).group_leader };
> +
> +        // SAFETY: `current.group_leader` stays valid for at least the duration in which `current`
> +        // is running, and the signature of this function ensures that the returned `&Task` can
> +        // only be used while `current` is still valid, thus still running.
> +        unsafe { &*ptr.cast() }
> +    }
>  }
>
>  // SAFETY: The type invariants guarantee that `Task` is always refcounted.
>
> ---
> base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
> change-id: 20251218-task-group-leader-a71931ced643
>
> Best regards,
> --
> Alice Ryhl <aliceryhl@google.com>
>

Re: [PATCH] rust: task: restrict Task::group_leader() to current

[Alice Ryhl]

On Thu, Dec 18, 2025 at 11:32 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 12/18, Alice Ryhl wrote:
> >
> > The Task::group_leader() method currently allows you to access the
> > group_leader() of any task, for example one you hold a refcount to. But
> > this is not safe in general since the group leader could change when a
> > task exits. See for example commit a15f37a40145c ("kernel/sys.c: fix the
> > racy usage of task_lock(tsk->group_leader) in sys_prlimit64() paths").
> >
> > All existing users of Task::group_leader() call this method on current,
> > which is guaranteed running, so there's not an actual issue in Rust code
> > today. But to prevent code in the future from making this mistake,
> > restrict Task::group_leader() so that it can only be called on current.
> >
> > There are some other cases where accessing task->group_leader is okay.
> > For example it can be safe if you hold tasklist_lock or rcu_read_lock().
> > However, only supporting current->group_leader is sufficient for all
> > in-tree Rust users of group_leader right now. Safe Rust functionality
> > for accessing it under rcu or while holding tasklist_lock may be added
> > in the future if required by any future Rust module.
>
> I obviously can't ACK this patch ;) but just in case, it looks good to me.
>
> Although I am not sure this is a stable material... Exactly because,
> as you mentioned, all existing users call this method on current.

Well, I suppose you are right that it isn't. I would like it to land
on Android's fork of 6.18 somehow so that nobody makes this mistake in
future Android drivers using 6.18, but I can always do that separately
of upstream Linux.

> > I don't think there's a clear owner for this file, so to break ambiguity
> > I'm doing to declare that this patch is intended for Andrew Morton's
> > tree. Please let me know if you think a different tree is appropriate.
>
> If Andrew agrees and nobody objects this would be nice. I am going to
> send some tree-wide changes related to task_struct.group_leader usage,
> it would be simpler to route them all via -mm tree.
>
> So far I sent the trivial preparations
>
>         [PATCH 0/7] don't abuse task_struct.group_leader
>         https://lore.kernel.org/all/aTV1pbftBkH8n4kh@redhat.com/
>
> and I am still waiting for more reviews. Alice, perhaps you can review
> the (hopefully trivial) 1-2 which touch android/binder?

Done.

Alice

Re: [PATCH] rust: task: restrict Task::group_leader() to current

[Boqun Feng]

On Thu, Dec 18, 2025 at 09:41:00AM +0000, Alice Ryhl wrote:
> The Task::group_leader() method currently allows you to access the
> group_leader() of any task, for example one you hold a refcount to. But
> this is not safe in general since the group leader could change when a
> task exits. See for example commit a15f37a40145c ("kernel/sys.c: fix the
> racy usage of task_lock(tsk->group_leader) in sys_prlimit64() paths").
> 
> All existing users of Task::group_leader() call this method on current,
> which is guaranteed running, so there's not an actual issue in Rust code
> today. But to prevent code in the future from making this mistake,
> restrict Task::group_leader() so that it can only be called on current.
> 
> There are some other cases where accessing task->group_leader is okay.
> For example it can be safe if you hold tasklist_lock or rcu_read_lock().
> However, only supporting current->group_leader is sufficient for all
> in-tree Rust users of group_leader right now. Safe Rust functionality
> for accessing it under rcu or while holding tasklist_lock may be added
> in the future if required by any future Rust module.
> 
> Reported-by: Oleg Nesterov <oleg@redhat.com>
> Closes: https://lore.kernel.org/all/aTLnV-5jlgfk1aRK@redhat.com/
> Fixes: 313c4281bc9d ("rust: add basic `Task`")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> The rust/kernel/task.rs file has had changes land through a few
> different trees:
> 
> * Originally task.rs landed through Christian's tree together with
>   file.rs and pid_namespace.rs
> * The change to add CurrentTask landed through Andrew Morton's tree
>   together with mm.rs
> * There was a patch to mark some methods #[inline] that landed through
>   tip via Boqun.
> 

I think I took that change through tip because it has the changes to
`current()` and `raw_current()` which belong to the scheduler part of
task.

> I don't think there's a clear owner for this file, so to break ambiguity
> I'm doing to declare that this patch is intended for Andrew Morton's
> tree. Please let me know if you think a different tree is appropriate.

Make sense to me.

> ---
>  rust/kernel/task.rs | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 49fad6de06740a9b9ad80b2f4b430cc28cd134fa..9440692a3a6d0d3f908d61d51dcd377a272f6957 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -204,18 +204,6 @@ pub fn as_ptr(&self) -> *mut bindings::task_struct {
>          self.0.get()
>      }
>  
> -    /// Returns the group leader of the given task.
> -    pub fn group_leader(&self) -> &Task {
> -        // SAFETY: The group leader of a task never changes after initialization, so reading this
> -        // field is not a data race.
> -        let ptr = unsafe { *ptr::addr_of!((*self.as_ptr()).group_leader) };
> -
> -        // SAFETY: The lifetime of the returned task reference is tied to the lifetime of `self`,
> -        // and given that a task has a reference to its group leader, we know it must be valid for
> -        // the lifetime of the returned task reference.
> -        unsafe { &*ptr.cast() }
> -    }
> -
>      /// Returns the PID of the given task.
>      pub fn pid(&self) -> Pid {
>          // SAFETY: The pid of a task never changes after initialization, so reading this field is
> @@ -345,6 +333,18 @@ pub fn active_pid_ns(&self) -> Option<&PidNamespace> {
>          // `release_task()` call.
>          Some(unsafe { PidNamespace::from_ptr(active_ns) })
>      }
> +
> +    /// Returns the group leader of the current task.
> +    pub fn group_leader(&self) -> &Task {
> +        // SAFETY: The group leader of a task never changes while the task is running, and `self`
> +        // is the current task, which is guaranteed running.
> +        let ptr = unsafe { (*self.as_ptr()).group_leader };
> +
> +        // SAFETY: `current.group_leader` stays valid for at least the duration in which `current`
> +        // is running, and the signature of this function ensures that the returned `&Task` can
> +        // only be used while `current` is still valid, thus still running.
> +        unsafe { &*ptr.cast() }
> +    }

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

>  }
>  
>  // SAFETY: The type invariants guarantee that `Task` is always refcounted.
> 
> ---
> base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
> change-id: 20251218-task-group-leader-a71931ced643
> 
> Best regards,
> -- 
> Alice Ryhl <aliceryhl@google.com>
> 

Re: [PATCH] rust: task: restrict Task::group_leader() to current

[Gary Guo]

On Thu, 18 Dec 2025 09:41:00 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> The Task::group_leader() method currently allows you to access the
> group_leader() of any task, for example one you hold a refcount to. But
> this is not safe in general since the group leader could change when a
> task exits. See for example commit a15f37a40145c ("kernel/sys.c: fix the
> racy usage of task_lock(tsk->group_leader) in sys_prlimit64() paths").
> 
> All existing users of Task::group_leader() call this method on current,
> which is guaranteed running, so there's not an actual issue in Rust code
> today. But to prevent code in the future from making this mistake,
> restrict Task::group_leader() so that it can only be called on current.
> 
> There are some other cases where accessing task->group_leader is okay.
> For example it can be safe if you hold tasklist_lock or rcu_read_lock().
> However, only supporting current->group_leader is sufficient for all
> in-tree Rust users of group_leader right now. Safe Rust functionality
> for accessing it under rcu or while holding tasklist_lock may be added
> in the future if required by any future Rust module.
> 
> Reported-by: Oleg Nesterov <oleg@redhat.com>
> Closes: https://lore.kernel.org/all/aTLnV-5jlgfk1aRK@redhat.com/
> Fixes: 313c4281bc9d ("rust: add basic `Task`")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/task.rs | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)