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

[Oleg Nesterov <oleg@redhat.com>]

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>
>