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

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

[Oleg Nesterov]

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?

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

Oleg.

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

[Alice Ryhl]

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

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

[Alice Ryhl]

On Fri, Dec 05, 2025 at 02:19:27PM +0000, Alice Ryhl wrote:
> Thanks a lot for your email!
> 
> +Christian Brauner
> 
> On Fri, Dec 05, 2025 at 03:08:23PM +0100, Oleg Nesterov wrote:
> > 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)

Ah, actually you need to convert from *mut bindings::task_struct to
&Task. And you need a safety comment. E.g.:

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

Alice

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

[Oleg Nesterov]

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

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.

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`

?

Oleg.

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

[Alice Ryhl]

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

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

[Oleg Nesterov]

Alice,

Thanks again for your explanations. Not that I fully understand
them, though ;)

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

I obviously can't comment this proposal,

> and the safety comment needs
> to explain why being the current task ensures that the returned &Task
> lives for long enough.

This is simple. task->group_leader can't change or go away until
this task exits or execs. The "current" task can't exit/exec.

(This also covers the race with mt-exec from current's subthread,
 the execing thread will kill all the threads and wait until they
 all pass release_task(). Only then it will change ->group_leader).

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

Yes, the comment looks good to me.

But we don't have the task_group_leader() helper yet, so far I
only sent the trivial initial preparations, see
https://lore.kernel.org/all/aTV1KYdcDGvjXHos@redhat.com/

So if you are going to move Task::group_leader to the
CurrentTask block, please use .group_leader directly, like
the current code does.

Oleg.

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

[Alice Ryhl]

On Mon, Dec 08, 2025 at 03:30:00PM +0100, Oleg Nesterov wrote:
> Alice,
> 
> Thanks again for your explanations. Not that I fully understand
> them, though ;)
> 
> On 12/05, Alice Ryhl wrote:
> >
> > 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 {}`
> 
> I obviously can't comment this proposal,
> 
> > and the safety comment needs
> > to explain why being the current task ensures that the returned &Task
> > lives for long enough.
> 
> This is simple. task->group_leader can't change or go away until
> this task exits or execs. The "current" task can't exit/exec.
> 
> (This also covers the race with mt-exec from current's subthread,
>  the execing thread will kill all the threads and wait until they
>  all pass release_task(). Only then it will change ->group_leader).
> 
> > 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>() }
> >     }
> > }
> 
> Yes, the comment looks good to me.
> 
> But we don't have the task_group_leader() helper yet, so far I
> only sent the trivial initial preparations, see
> https://lore.kernel.org/all/aTV1KYdcDGvjXHos@redhat.com/
> 
> So if you are going to move Task::group_leader to the
> CurrentTask block, please use .group_leader directly, like
> the current code does.

Yeah I'll be moving it soon (after conferences). I'll add a Reported-by
tag for you if that's okay :)

Alice

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

[Gary Guo]

On Fri, 5 Dec 2025 18:17:09 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

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

This indeed sounds like the right approach to take.

If `Task::pid` or `Task::group_leader` just gives the pid or group
leader at the time of invocation and doesn't have any stability
guarantee, then the user of the functions will likely be misusing these
functions.

It's better to just have them on `CurrentTask` for now. When an user
arises that need to retrieve them for another task, then we can come
back and think about a proper solution taking the scenario into account.

Best,
Gary

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

[Alice Ryhl]

On Mon, Dec 08, 2025 at 03:43:07PM +0000, Gary Guo wrote:
> On Fri, 5 Dec 2025 18:17:09 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
> > 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.
> 
> This indeed sounds like the right approach to take.
> 
> If `Task::pid` or `Task::group_leader` just gives the pid or group
> leader at the time of invocation and doesn't have any stability
> guarantee, then the user of the functions will likely be misusing these
> functions.
> 
> It's better to just have them on `CurrentTask` for now. When an user
> arises that need to retrieve them for another task, then we can come
> back and think about a proper solution taking the scenario into account.

I believe Binder just uses them for including the pid when it prints to
the console. But I'll look into this and figure out the best approach.

Alice

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

[Oleg Nesterov]

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.

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

[Alice Ryhl]

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