Re: rust: wrong SAFETY comments in group_leader() and pid() + questions

[Alice Ryhl <aliceryhl@google.com>]

Thanks a lot for your email!

+Christian Brauner

On Fri, Dec 05, 2025 at 03:08:23PM +0100, Oleg Nesterov wrote:
> From rust/kernel/task.rs:
> 
>     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
>         // not a data race.
>         unsafe { *ptr::addr_of!((*self.as_ptr()).pid) }
>     }
> 
> The comments look wrong. Unless same_thread_group(current, task) == T, task->group_leader
> and/or task->pid can change if a non-leader task's sub-thread execs. This also means that
> in general it is not safe to dereference group_leader, for example this C code is not safe:
> 
> 	rcu_read_lock();
> 	task = find_task_by_vpid(vpid);
> 	if (task)
> 		get_task_struct(task);
> 	rcu_read_unlock();
> 
> 	if (task)
> 		pid = task->group_leader->pid; // BUG! ->group_leader can be already freed
> 
> 
> Now the questions. Sorry! I don't know rust.
> 
> 1. Can I simply remove these misleading comments? Or SAFETY comment is mandatory?

If the safety comments are wrong, then the code is probably wrong too!

What is the correct way to read the pid or group_leader fields of a
struct task_struct?

> 2. I am working on the patch(es) which move ->group_leader from task_struct to
>    signal_struct, so the 1st change adds the new trivial helper in preparation:
> 
> 	struct task_struct *task_group_leader(struct task_struct *task)
> 	{
> 		return task->group_leader; // will be updated
> 	}
> 
>    Now, how can I change group_leader() to use it? I guess I need to add
> 
> 	struct task_struct *rust_helper_task_group_leader(struct task_struct *task)
> 	{
> 		return task_group_leader(task);
> 	}
> 
>    into rust/helpers/task.c, but will something like
> 
> 	pub fn group_leader(&self) -> &Task {
> 		unsafe { bindings::task_group_leader(self.as_ptr()) }
> 	}
> 
>     work? I'm afraid it won't ;)

That looks like it should work. The rust_helper_ function is only
required if task_group_leader is marked `static inline`. Otherwise
bindings:: will pick up the function straight from the C header. (As
long as the relevant header is included in bindings_helper.h)

That still does raise the question of how you correctly call this
function if the group leader could be freed at any time.

Alice