Re: [PATCH] exit: hold a reference to thread_pid across proc_flush_pid

[Oleg Nesterov <oleg@redhat.com>]

On 08/29, Daehyeon Ko wrote:
>
> release_task() keeps a task's thread_pid across the point where it
> drops tasklist_lock and calls proc_flush_pid().  The commit named in Fixes
> removed the PID reference.  It assumed that the PID cannot go away before
> this release_task() invocation calls free_pids().
>
> That assumption does not cover references represented only by PIDTYPE
> links.  Process B can still use exiting process A's PID as its session and
> process group ID.  A is reaped with wait4(-1); PID-specific waits take
> their own PID reference and mask the bug.  After A's reaper drops
> tasklist_lock, B can call setsid(), remove the final PIDTYPE links, and
> queue the PID from B's own free_pids() call.  The RCU callback can then
> free the object before the reaper dereferences pid->inodes and pid->lock in
> proc_flush_pid().

Thanks Daehyeon. Yes 0a36bad01731 was wrong and should be reverted.

But can you improve the changelog and send V2? I simply can't parse the
explanation above.

But I guess the problem is clear. 0a36bad01731 assumed that the caller
of release_task()path has a reference to thread_pid, and free_pids()
will do put_pid(thread_pid).

However, __unhash_process()->detach_pid(pids)->__change_pid(pids) does
not necessarily add pid to pids[], pid_has_task() can still be true.

IOW, release_task() doesn't necessarily has a reference,
call_rcu(&pid->rcu, delayed_put_pid) is called by the caller of the "last"
detach_pid() which makes pid->tasks[] lists empty.

Hmm... Yes, my explanation doesn't look clear too :/ Can you ask you LLM
to turn it into something meaningful?

> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -261,8 +261,8 @@ void release_task(struct task_struct *p)
>  	pidfs_exit(p);
>  	cgroup_task_release(p);
>
> -	/* Retrieve @thread_pid before __unhash_process() may set it to NULL. */
> -	thread_pid = task_pid(p);
> +	/* Pin @thread_pid before __unhash_process() may set it to NULL. */
> +	thread_pid = get_pid(task_pid(p));

It would be nice to improve the comment a bit if possible...
"may set it to NULL." doesn't explain get_pid().

Thank you,

Oleg.