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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250409-sesshaft-absurd-35d97607142c@brauner>
Date: Wed, 9 Apr 2025 17:38:06 +0200
From: Christian Brauner <brauner@...nel.org>
To: Oleg Nesterov <oleg@...hat.com>
Cc: linux-fsdevel@...r.kernel.org, Jeff Layton <jlayton@...nel.org>, 
	Lennart Poettering <lennart@...ttering.net>, Daan De Meyer <daan.j.demeyer@...il.com>, 
	Mike Yuan <me@...dnzj.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 3/4] pidfd: improve uapi when task isn't found

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