On Tue, Mar 18, 2025 at 03:46:54PM +0100, Oleg Nesterov wrote:
> I'll try to actually read this patch (and pidfs: improve multi-threaded
> exec and premature thread-group leader exit polling) tomorrow, but I am
> a bit confused after the quick glance...
>
>
> On 03/18, Christian Brauner wrote:
> >
> > +static inline bool pidfs_pid_valid(struct pid *pid, const struct path *path,
> > + unsigned int flags)
> > +{
> > + enum pid_type type;
> > +
> > + if (flags & CLONE_PIDFD)
> > + return true;
>
> OK, this is clear.
>
> > + if (flags & PIDFD_THREAD)
> > + type = PIDTYPE_PID;
> > + else
> > + type = PIDTYPE_TGID;
> > +
> > + /*
> > + * Since pidfs_exit() is called before struct pid's task linkage
> > + * is removed the case where the task got reaped but a dentry
> > + * was already attached to struct pid and exit information was
> > + * recorded and published can be handled correctly.
> > + */
> > + if (unlikely(!pid_has_task(pid, type))) {
> > + struct inode *inode = d_inode(path->dentry);
> > + return !!READ_ONCE(pidfs_i(inode)->exit_info);
> > + }
>
> Why pidfs_pid_valid() can't simply return false if !pid_has_task(pid,type) ?
>
> pidfd_open() paths check pid_has_task() too and fail if it returns NULL.
> If this task is already reaped when pidfs_pid_valid() is called, we can
> pretend it was reaped before sys_pidfd_open() was called?
We could for sure but why would we. If we know that exit information is
available then returning a pidfd can still be valuable for userspace as
they can retrieve exit information via PIDFD_INFO_EXIT and it really
doesn't hurt to do this.
