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

[Alice Ryhl <aliceryhl@google.com>]

On Fri, Dec 05, 2025 at 04:35:13PM +0100, Oleg Nesterov wrote:
> 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".

That's perfect :)

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

Right. But if it might change in parallel with our read, then we should
ideally use READ_ONCE() or rcu_dereference() or an atomic load or
similar.

With regards to "as long as pointer is stable", you may assume that any
time you see a &T or &mut T, then the ampersand means a reference, which
is always stable. Pointers that may be dangling have to be raw pointers,
which use *mut T/*const T syntax.

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

Ok, then we should probably adjust the Rust Task::pid() method to use
READ_ONCE() to read the pid. And adjust the safety comment to match.

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

Yeah, the current code encodes the lifetime constraint that the task
always outlives its group leader. So for example this compiles:

	let leader = task.group_leader();
	do_stuff(leader); // ok

but this will fail to compile:

	let leader = task.group_leader();
	drop(task); // put refcount on task
	do_stuff(leader); // ERROR task is no longer valid

because Rust can see that `leader` is used in a context where using
`task` would be illegal.

But it sounds like the above is not sufficient, and that we have
situation where incorrect code would compile.

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

Yes, the current usage seems fine. However, one of the main goals behind
API design in Rust is to ensure that no matter how stupid the code that
uses it is, if the final code is safe, then it should not lead to UAF or
other memory unsafety.

In this case, we can fix the design by moving `Task::group_leader()` to
the `impl CurrentTask {}` block. That way, calling group_leader() on a
task will only compile if it's called on the current task.

We could also move Task::pid() to the `impl CurrentTask` block to avoid
the READ_ONCE(). Or we could add pid() in both places, in which case
task.pid() would use READ_ONCE() only when the &Task is not derived from
current.

Alice