Re: [RFC PATCH] pidfs: ensure consistent ENOENT/ESRCH reporting

[Christian Brauner <brauner@kernel.org>]

On Thu, Apr 10, 2025 at 10:24:22PM +0200, Christian Brauner wrote:
> On Thu, Apr 10, 2025 at 10:05:58PM +0200, Christian Brauner wrote:
> > On Thu, Apr 10, 2025 at 03:10:09PM +0200, Oleg Nesterov wrote:
> > > On 04/10, Christian Brauner wrote:
> > > >
> > > > On Thu, Apr 10, 2025 at 12:18:01PM +0200, Oleg Nesterov wrote:
> > > > > On 04/09, Oleg Nesterov wrote:
> > > > > >
> > > > > > On 04/09, Christian Brauner wrote:
> > > > > > >
> > > > > > > The seqcounter might be
> > > > > > > useful independent of pidfs.
> > > > > >
> > > > > > Are you sure? ;) to me the new pid->pid_seq needs more justification...
> > > >
> > > > Yeah, pretty much. I'd make use of this in other cases where we need to
> > > > detect concurrent changes to struct pid without having to take any
> > > > locks. Multi-threaded exec in de_exec() comes to mind as well.
> > > 
> > > Perhaps you are right, but so far I am still not sure it makes sense.
> > > And we can always add it later if we have another (more convincing)
> > > use-case.
> > > 
> > > > > To remind, detach_pid(pid, PIDTYPE_PID) does wake_up_all(&pid->wait_pidfd) and
> > > > > takes pid->wait_pidfd->lock.
> > > > >
> > > > > So if pid_has_task(PIDTYPE_PID) succeeds, __unhash_process() -> detach_pid(TGID)
> > > > > is not possible until we drop pid->wait_pidfd->lock.
> > > > >
> > > > > If detach_pid(PIDTYPE_PID) was already called and have passed wake_up_all(),
> > > > > pid_has_task(PIDTYPE_PID) can't succeed.
> > > >
> > > > I know. I was trying to avoid having to take the lock and just make this
> > > > lockless. But if you think we should use this lock here instead I'm
> > > > willing to do this. I just find the sequence counter more elegant than
> > > > the spin_lock_irq().
> > > 
> > > This is subjective, and quite possibly I am wrong. But yes, I'd prefer
> > > to (ab)use pid->wait_pidfd->lock in pidfd_prepare() for now and not
> > > penalize __unhash_process(). Simply because this is simpler.
> 
> Looking close at this. Why is:
> 
>         if (type == PIDTYPE_PID) {
>                 WARN_ON_ONCE(pid_has_task(pid, PIDTYPE_PID));
>                 wake_up_all(&pid->wait_pidfd);
>         }
> 
> located in __change_pid()? The only valid call to __change_pid() with a NULL
> argument and PIDTYPE_PID is from __unhash_process(), no?

We used to perform free_pid() directly from __change_pid() so prior to
v6.15 changes it wasn't possible. Now that we free the pids separately let's
just move the notification into __unhash_process(). I have a patch ready
for this.

> 
> So why isn't this in __unhash_process() where it's immediately obvious
> that it's the only valid place this can currently be called from?
> 
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 1b51dc099f1e..d92e8bee0ab7 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -135,6 +135,7 @@ static void __unhash_process(struct release_task_post *post, struct task_struct
>  {
>         nr_threads--;
>         detach_pid(post->pids, p, PIDTYPE_PID);
> +       wake_up_all(&post->pids[PIDTYPE_PID]->wait_pidfd);
>         if (group_dead) {
>                 detach_pid(post->pids, p, PIDTYPE_TGID);
>                 detach_pid(post->pids, p, PIDTYPE_PGID);
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 4ac2ce46817f..26f1e136f017 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -359,11 +359,6 @@ static void __change_pid(struct pid **pids, struct task_struct *task,
>         hlist_del_rcu(&task->pid_links[type]);
>         *pid_ptr = new;
> 
> -       if (type == PIDTYPE_PID) {
> -               WARN_ON_ONCE(pid_has_task(pid, PIDTYPE_PID));
> -               wake_up_all(&pid->wait_pidfd);
> -       }
> -
>         for (tmp = PIDTYPE_MAX; --tmp >= 0; )
>                 if (pid_has_task(pid, tmp))
>                         return;
> 
> I'm probably missing something obvious.