Re: [PATCH RFC 3/4] pidfd: improve uapi when task isn't found

[Christian Brauner <brauner@kernel.org>]

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