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

[Oleg Nesterov <oleg@redhat.com>]

On 12/05, Alice Ryhl wrote:
>
> 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?

Alice, let me repeat I don't know rust ;) So I'll try to answer in "C".

As long as "struct task_struct *task" pointer is stable and can't go away,
it is always safe to _read_ task->pid or even task->group_leader (but see below).

However, task->pid is not "const" in general, so "The pid of a task never changes ..."
doesn't look right. See exchange_tids() which swaps left->pid/right->pid.

So this C code

	pid1 = READ_ONCE(task->pid);
	...
	pid2 = READ_ONCE(task->pid);
	...
	BUG_ON(pid1 != pid2);

is only correct if same_thread(current, task) == T. Or tasklist_lock is held,
or another lock (say, cred_guard_mutex) which blocks de_thread().

Same for ->group_leader, it is not stable in general. Plus, unlike ->pid it
can go away if we race with mt-exec or this task exits. So, just for example,
in theory

	char c = task->group_leader->comm[0];

is only safe if same_thread(current, task) == T, or tasklist_lock is held,
or it is called under rcu_read_lock() and pid_alive(task) == T. This makes
me think that the "The lifetime of the returned task reference is tied to
the lifetime of `self`" comment is not right too.

See for example commit a15f37a40145c ("kernel/sys.c: fix the racy usage of
task_lock(tsk->group_leader) in sys_prlimit64() paths")

However, according to "git grep -E '\bgroup_leader\('" the only user of
group_leader(task) is drivers/android/binder/process.rs, and task is always
"current", so I think the current usage of group_leader() is fine.

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

Thanks Alice! (yes, I see your next email ;)

Oleg.