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

[Christian Brauner]

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?

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.