On Fri, Apr 04, 2025 at 04:53:24PM +0200, Oleg Nesterov wrote:
> On 04/04, Christian Brauner wrote:
> >
> > On Fri, Apr 04, 2025 at 02:37:38PM +0200, Oleg Nesterov wrote:
> > > And... the code looks a bit overcomplicated to me, why not simply
> > >
> > > int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret)
> > > {
> > > if (!pid_has_task(pid, PIDTYPE_PID))
> > > return -ESRCH;
> > >
> > > if (!(flags & PIDFD_THREAD) && !pid_has_task(pid, PIDTYPE_TGID))
> > > return -ENOENT;
> >
> > I thought that checking PIDTYPE_PID first could cause misleading results
> > where we report ENOENT where we should report ESRCH: If the task was
> > released after the successful PIDTYPE_PID check for a pid that was never
> > a thread-group leader we report ENOENT.
>
> Hmm... but the code above can only return ENOENT if !(flags & PIDFD_THREAD),
> so in this case -ENOENT is correct?
>
> I guess -ENOENT would be wrong if this pid _was_ a leader pid and we
> race with __unhash_process() which does
>
> detach_pid(post->pids, p, PIDTYPE_PID);
> if (group_dead)
> detach_pid(post->pids, p, PIDTYPE_TGID);
Yes, exactly.
>
> but without tasklist_lock (or additional barries in both pidfd_prepare() and
> __unhash_process() pidfd_prepare() can see the result of these 2 detach_pid()'s
> in any order anyway. So I don't think the code above is "more" racy.
Right... Hm, I don't like the inherent raciness of this. I think we
should fix this. I'm playing with something. I'll try to get it out
today.
>
> Although perhaps we can rely on the fact the the 1st detach_pid(PIDTYPE_PID)
> does wake_up(pid->wait_pidfd) and use pid->wait_pidfd->lock to avoid the
> races, not sure...
>
> But,
>
> > But I can adapt that to you scheme.
>
> Again, up to you, whatever you prefer.
>
> Oleg.
>
