[PATCH v2 0/2] rust: task: clean up safety issues wrt de_thread()

[PATCH v2 0/2] rust: task: clean up safety issues wrt de_thread()

[Jann Horn]

Task::pid() and Task::group_leader() wrongly assume that task::pid and
task::group_leader remain constant until the task refcount drops to zero,
but those members can actually change due to concurrent de_thread().
Fix those assumptions.

Signed-off-by: Jann Horn <jannh@google.com>
---
Changes in v2:
- split into two patches (Boqun)
- use relaxed atomic load instead of volatile load (Boqun)
- Link to v1: https://lore.kernel.org/r/20260212-rust-de_thread-v1-1-948f5b992624@google.com

---
Jann Horn (2):
      rust: task: limit group_leader() to current
      rust: task: use atomic read for pid()

 rust/kernel/task.rs | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)
---
base-commit: 192c0159402e6bfbe13de6f8379546943297783d
change-id: 20260212-rust-de_thread-0ad9154aedb0

--  
Jann Horn <jannh@google.com>

[PATCH v2 1/2] rust: task: limit group_leader() to current

[Jann Horn]

(Note: This is not a bugfix, it just cleans up an incorrect assumption.)

Task::group_leader() assumes that task::group_leader remains constant until
the task refcount drops to zero.

However, Linux has a special quirk where, when execve() is called by a
thread other than the thread group leader (the main thread), de_thread()
swaps the current thread's identity with the thread group leader's,
making the current thread the new thread group leader.
This means task::group_leader can't be assumed to be immutable for
non-current tasks.

For reference, you can see that accessing the ->group_leader of some random
task requires extra caution in the prlimit64() syscall, which grabs the
tasklist_lock and has a comment explaining that this is done to prevent
races with de_thread().

Signed-off-by: Jann Horn <jannh@google.com>
---
 rust/kernel/task.rs | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 49fad6de0674..91ad88cdfd3b 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -103,7 +103,7 @@ macro_rules! current {
 unsafe impl Send for Task {}
 
 // SAFETY: It's OK to access `Task` through shared references from other threads because we're
-// either accessing properties that don't change (e.g., `pid`, `group_leader`) or that are properly
+// either accessing properties that don't change or that are properly
 // synchronised by C code (e.g., `signal_pending`).
 unsafe impl Sync for Task {}
 
@@ -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 the current task never changes in syscall
+        // context (except in the implementation of execve()).
+        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() }
+    }
 }
 
 // SAFETY: The type invariants guarantee that `Task` is always refcounted.

-- 
2.53.0.273.g2a3d683680-goog

Re: [PATCH v2 1/2] rust: task: limit group_leader() to current

[Alice Ryhl]

On Thu, Feb 12, 2026 at 11:12 PM Jann Horn <jannh@google.com> wrote:
>
> (Note: This is not a bugfix, it just cleans up an incorrect assumption.)
>
> Task::group_leader() assumes that task::group_leader remains constant until
> the task refcount drops to zero.
>
> However, Linux has a special quirk where, when execve() is called by a
> thread other than the thread group leader (the main thread), de_thread()
> swaps the current thread's identity with the thread group leader's,
> making the current thread the new thread group leader.
> This means task::group_leader can't be assumed to be immutable for
> non-current tasks.
>
> For reference, you can see that accessing the ->group_leader of some random
> task requires extra caution in the prlimit64() syscall, which grabs the
> tasklist_lock and has a comment explaining that this is done to prevent
> races with de_thread().
>
> Signed-off-by: Jann Horn <jannh@google.com>

This has already been fixed:
https://lore.kernel.org/all/20260107-task-group-leader-v2-1-8fbf816f2a2f@google.com/

Re: [PATCH v2 1/2] rust: task: limit group_leader() to current

[Jann Horn]

On Fri, Feb 13, 2026 at 8:52 AM Alice Ryhl <aliceryhl@google.com> wrote:
> On Thu, Feb 12, 2026 at 11:12 PM Jann Horn <jannh@google.com> wrote:
> >
> > (Note: This is not a bugfix, it just cleans up an incorrect assumption.)
> >
> > Task::group_leader() assumes that task::group_leader remains constant until
> > the task refcount drops to zero.
> >
> > However, Linux has a special quirk where, when execve() is called by a
> > thread other than the thread group leader (the main thread), de_thread()
> > swaps the current thread's identity with the thread group leader's,
> > making the current thread the new thread group leader.
> > This means task::group_leader can't be assumed to be immutable for
> > non-current tasks.
> >
> > For reference, you can see that accessing the ->group_leader of some random
> > task requires extra caution in the prlimit64() syscall, which grabs the
> > tasklist_lock and has a comment explaining that this is done to prevent
> > races with de_thread().
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
>
> This has already been fixed:
> https://lore.kernel.org/all/20260107-task-group-leader-v2-1-8fbf816f2a2f@google.com/

Ah, and I didn't notice it because it was in the mm tree while I was
looking at the rust tree. Sorry for the duplicate patch.

Re: [PATCH v2 1/2] rust: task: limit group_leader() to current

[Alice Ryhl]

On Fri, Feb 13, 2026 at 2:38 PM Jann Horn <jannh@google.com> wrote:
>
> On Fri, Feb 13, 2026 at 8:52 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > On Thu, Feb 12, 2026 at 11:12 PM Jann Horn <jannh@google.com> wrote:
> > >
> > > (Note: This is not a bugfix, it just cleans up an incorrect assumption.)
> > >
> > > Task::group_leader() assumes that task::group_leader remains constant until
> > > the task refcount drops to zero.
> > >
> > > However, Linux has a special quirk where, when execve() is called by a
> > > thread other than the thread group leader (the main thread), de_thread()
> > > swaps the current thread's identity with the thread group leader's,
> > > making the current thread the new thread group leader.
> > > This means task::group_leader can't be assumed to be immutable for
> > > non-current tasks.
> > >
> > > For reference, you can see that accessing the ->group_leader of some random
> > > task requires extra caution in the prlimit64() syscall, which grabs the
> > > tasklist_lock and has a comment explaining that this is done to prevent
> > > races with de_thread().
> > >
> > > Signed-off-by: Jann Horn <jannh@google.com>
> >
> > This has already been fixed:
> > https://lore.kernel.org/all/20260107-task-group-leader-v2-1-8fbf816f2a2f@google.com/
>
> Ah, and I didn't notice it because it was in the mm tree while I was
> looking at the rust tree. Sorry for the duplicate patch.

No worries! I did not get around to fixing the pid() issue, which was
also pointed out previously, so thanks for the patch on that one!

Alice

[PATCH v2 2/2] rust: task: use atomic read for pid()

[Jann Horn]

(Note: This is not a bugfix, it just cleans up an incorrect assumption.)

Task::pid() wrongly assumes that task::pid remains constant until the task
refcount drops to zero.

However, Linux has a special quirk where, when execve() is called by a
thread other than the thread group leader (the main thread), the thread
calling execve() swaps its identity with the thread group leader's,
becoming the new thread group leader. This means task::pid can't be assumed
to be immutable for non-current tasks.
(The actual swapping of PIDs is implemented in exchange_tids().)

Signed-off-by: Jann Horn <jannh@google.com>
---
 rust/kernel/task.rs | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 91ad88cdfd3b..de0d90b47862 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -10,6 +10,8 @@
     mm::MmWithUser,
     pid_namespace::PidNamespace,
     sync::aref::ARef,
+    sync::atomic::ordering::Relaxed,
+    sync::atomic::Atomic,
     types::{NotThreadSafe, Opaque},
 };
 use core::{
@@ -206,9 +208,13 @@ pub fn as_ptr(&self) -> *mut bindings::task_struct {
 
     /// 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
-        // not a data race.
-        unsafe { *ptr::addr_of!((*self.as_ptr()).pid) }
+        // SAFETY: The pid of a task almost never changes after initialization,
+        // so reading this field is usually not a data race.
+        // The exception is a race where the task is part of a process that
+        // goes through execve(), see exchange_tids().
+        // A temporary mutable pointer is created, but only actually used for
+        // a load.
+        unsafe { Atomic::from_ptr(&raw mut (*self.as_ptr()).pid).load(Relaxed) }
     }
 
     /// Returns the UID of the given task.

-- 
2.53.0.273.g2a3d683680-goog

Re: [PATCH v2 2/2] rust: task: use atomic read for pid()

[Alice Ryhl]

On Thu, Feb 12, 2026 at 11:12:07PM +0100, Jann Horn wrote:
> (Note: This is not a bugfix, it just cleans up an incorrect assumption.)
> 
> Task::pid() wrongly assumes that task::pid remains constant until the task
> refcount drops to zero.
> 
> However, Linux has a special quirk where, when execve() is called by a
> thread other than the thread group leader (the main thread), the thread
> calling execve() swaps its identity with the thread group leader's,
> becoming the new thread group leader. This means task::pid can't be assumed
> to be immutable for non-current tasks.
> (The actual swapping of PIDs is implemented in exchange_tids().)
> 
> Signed-off-by: Jann Horn <jannh@google.com>

Thanks.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> ---
>  rust/kernel/task.rs | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 91ad88cdfd3b..de0d90b47862 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -10,6 +10,8 @@
>      mm::MmWithUser,
>      pid_namespace::PidNamespace,
>      sync::aref::ARef,
> +    sync::atomic::ordering::Relaxed,
> +    sync::atomic::Atomic,
>      types::{NotThreadSafe, Opaque},
>  };
>  use core::{
> @@ -206,9 +208,13 @@ pub fn as_ptr(&self) -> *mut bindings::task_struct {
>  
>      /// 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
> -        // not a data race.
> -        unsafe { *ptr::addr_of!((*self.as_ptr()).pid) }
> +        // SAFETY: The pid of a task almost never changes after initialization,
> +        // so reading this field is usually not a data race.
> +        // The exception is a race where the task is part of a process that
> +        // goes through execve(), see exchange_tids().
> +        // A temporary mutable pointer is created, but only actually used for
> +        // a load.
> +        unsafe { Atomic::from_ptr(&raw mut (*self.as_ptr()).pid).load(Relaxed) }
>      }
>  
>      /// Returns the UID of the given task.
> 
> -- 
> 2.53.0.273.g2a3d683680-goog
> 

Re: [PATCH v2 2/2] rust: task: use atomic read for pid()

[Boqun Feng]

On Thu, Feb 12, 2026 at 11:12:07PM +0100, Jann Horn wrote:
> (Note: This is not a bugfix, it just cleans up an incorrect assumption.)
> 
> Task::pid() wrongly assumes that task::pid remains constant until the task
> refcount drops to zero.
> 
> However, Linux has a special quirk where, when execve() is called by a
> thread other than the thread group leader (the main thread), the thread
> calling execve() swaps its identity with the thread group leader's,
> becoming the new thread group leader. This means task::pid can't be assumed
> to be immutable for non-current tasks.
> (The actual swapping of PIDs is implemented in exchange_tids().)
> 
> Signed-off-by: Jann Horn <jannh@google.com>

Reviewed-by: Boqun Feng <boqun@kernel.org>

Regards,
Boqun

> ---
>  rust/kernel/task.rs | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 91ad88cdfd3b..de0d90b47862 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -10,6 +10,8 @@
>      mm::MmWithUser,
>      pid_namespace::PidNamespace,
>      sync::aref::ARef,
> +    sync::atomic::ordering::Relaxed,
> +    sync::atomic::Atomic,
>      types::{NotThreadSafe, Opaque},
>  };
>  use core::{
> @@ -206,9 +208,13 @@ pub fn as_ptr(&self) -> *mut bindings::task_struct {
>  
>      /// 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
> -        // not a data race.
> -        unsafe { *ptr::addr_of!((*self.as_ptr()).pid) }
> +        // SAFETY: The pid of a task almost never changes after initialization,
> +        // so reading this field is usually not a data race.
> +        // The exception is a race where the task is part of a process that
> +        // goes through execve(), see exchange_tids().
> +        // A temporary mutable pointer is created, but only actually used for
> +        // a load.
> +        unsafe { Atomic::from_ptr(&raw mut (*self.as_ptr()).pid).load(Relaxed) }
>      }
>  
>      /// Returns the UID of the given task.
> 
> -- 
> 2.53.0.273.g2a3d683680-goog
>