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