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

[Alice Ryhl <aliceryhl@google.com>]

On Fri, Dec 05, 2025 at 06:21:46PM +0100, Oleg Nesterov wrote:
> On 12/05, Alice Ryhl wrote:
> >
> > pub fn group_leader(&self) -> &Task {
> >     // 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 { &*bindings::task_group_leader(self.as_ptr()).cast::<Task>() }
> > }
> 
> Thanks again Alice, but the comment still looks misleading to me...
> OK, quite possibly this is because I don't understand what does the
> "lifetime of the returned task reference" actually mean in the rust code.
> Does it mean "lifetime of task_struct" of "lifetime of the process/thread" ?

To start with, it's likely that this comment is not the right choice
for this function, given our discussion. Most likely group_leader()
needs to be moved to `impl CurrentTask {}` and the safety comment needs
to explain why being the current task ensures that the returned &Task
lives for long enough. I just took the safety comment from the code we
have today.

To explain "lifetime of the returned task reference" it may be useful to
show you the long form of this function signature:

fn group_leader<'a>(&'a self) -> &'a Task;

By "lifetime of the returned task reference" I am referring to 'a, which
is some region of code in the caller. For example, 'a might be:

* The region of code until I put the task I called group_leader() on.
* The region of code between a rcu_read_lock() / rcu_read_unlock() pair.
* The region of code until I release a mutex.
* The region of code inside a scope or function.

So for example, with the code as it is today, this example:

	let leader = task.group_leader();
	drop(task); // put refcount
	do_something(leader); // COMPILE ERROR

Rust will see that `task` may no longer be valid after the second line,
so 'a becomes the region of code until drop(task), and the use of
leader after the end of 'a is a compilation error.


Now, the above example is not really valid. It works today, but once we
fix group_leader() to require the task to be the current task, then the
example might look like this:

	let leader;
	{
		let current = current!();
		leader = current.leader();
		do_something(leader); // OK
	}
	do_something(leader); // COMPILE ERROR

And ok, this example isn't great because the last line is actually OK.
However, the way that I designed current!() to work is that it gives you
an `&'a CurrentTask` where 'a is the duration of the current scope. So
any use of `leader` after the end of the current scope is a compile
error.

> Let me provide the artificial example. Suppose we have something like
> 
> 	struct task_struct *TASK = NULL;
> 
> 	void stupid_example(void)
> 	{
> 		TASK = get_task_struct(current);
> 
> 		do_exit(0);
> 	}
> 
> and a non-leader task calls stupid_example().
> 
> After that the global TASK pointer is still valid, it is safe to
> dereference it, task_struct itself can't go away.

This would not compile in Rust. The &CurrentTask obtained using
current!() can't leave the scope it is created in.

> But! Right after that TASK->group_leader can point to nowhere (to the freed memory)
> if another thread does do_group_exit() or sys_execve().
> 
> So. Perhaps the the comment should say something like
> 
> 	SAFETY: The lifetime of the returned task reference is tied to
> 	the lifetime of the THREAD represented by `self`
> 
> ?

I think the safety comment should make some reference to the fact that
we know it was obtained from the current task. After all, that's why we
know it's ok.

impl CurrentTask {
    fn group_leader(&self) -> &Task {
        // SAFETY: This is the current task, so the task must be alive.
        // Therefore the group leader cannot change, and thus it will
        // stay valid as long as self is the current task.
        unsafe { &*bindings::task_group_leader(self.as_ptr()).cast::<Task>() }
    }
}

Alice