Re: [PATCH v2 06/15] pidfs: allow to retrieve exit information

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2025-03-04 at 10:41 +0100, Christian Brauner wrote:
> Some tools like systemd's jounral need to retrieve the exit and cgroup
> information after a process has already been reaped. This can e.g.,
> happen when retrieving a pidfd via SCM_PIDFD or SCM_PEERPIDFD.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/pidfs.c                 | 88 ++++++++++++++++++++++++++++++++++++----------
>  include/uapi/linux/pidfd.h |  3 +-
>  kernel/exit.c              |  2 +-
>  3 files changed, 73 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index 258e1c13ee56..11744d7fe177 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -36,7 +36,8 @@ struct pidfs_exit_info {
>  };
>  
>  struct pidfs_inode {
> -	struct pidfs_exit_info exit_info;
> +	struct pidfs_exit_info __pei;
> +	struct pidfs_exit_info *exit_info;
>  	struct inode vfs_inode;
>  };
>  
> @@ -228,17 +229,28 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
>  	return poll_flags;
>  }
>  
> -static long pidfd_info(struct task_struct *task, unsigned int cmd, unsigned long arg)
> +static inline bool current_in_pidns(struct pid *pid)
> +{
> +	const struct pid_namespace *ns = task_active_pid_ns(current);
> +
> +	if (ns->level <= pid->level)
> +		return pid->numbers[ns->level].ns == ns;
> +
> +	return false;
> +}
> +
> +static long pidfd_info(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	struct pidfd_info __user *uinfo = (struct pidfd_info __user *)arg;
> +	struct pid *pid = pidfd_pid(file);
>  	size_t usize = _IOC_SIZE(cmd);
>  	struct pidfd_info kinfo = {};
> +	struct pidfs_exit_info *exit_info;
> +	struct inode *inode = file_inode(file);
>  	struct user_namespace *user_ns;
> +	struct task_struct *task;
>  	const struct cred *c;
>  	__u64 mask;
> -#ifdef CONFIG_CGROUPS
> -	struct cgroup *cgrp;
> -#endif
>  
>  	if (!uinfo)
>  		return -EINVAL;
> @@ -248,6 +260,37 @@ static long pidfd_info(struct task_struct *task, unsigned int cmd, unsigned long
>  	if (copy_from_user(&mask, &uinfo->mask, sizeof(mask)))
>  		return -EFAULT;
>  
> +	task = get_pid_task(pid, PIDTYPE_PID);
> +	if (!task) {
> +		if (!(mask & PIDFD_INFO_EXIT))
> +			return -ESRCH;
> +
> +		if (!current_in_pidns(pid))
> +			return -ESRCH;
> +	}
> +
> +	if (mask & PIDFD_INFO_EXIT) {
> +		exit_info = READ_ONCE(pidfs_i(inode)->exit_info);
> +		if (exit_info) {
> +#ifdef CONFIG_CGROUPS
> +			kinfo.cgroupid = exit_info->cgroupid;
> +			kinfo.mask |= PIDFD_INFO_EXIT | PIDFD_INFO_CGROUPID;
> +#endif
> +			kinfo.exit_code = exit_info->exit_code;
> +		}
> +	}
> +
> +	/*
> +	 * If the task has already been reaped only exit information
> +	 * can be provided. It's entirely possible that the task has
> +	 * already been reaped but we managed to grab a reference to it
> +	 * before that. So a full set of information about @task doesn't
> +	 * mean it hasn't been waited upon. Similarly, a full set of
> +	 * information doesn't mean that the task hasn't already exited.
> +	 */
> +	if (!task)
> +		goto copy_out;
> +
>  	c = get_task_cred(task);
>  	if (!c)
>  		return -ESRCH;
> @@ -267,11 +310,15 @@ static long pidfd_info(struct task_struct *task, unsigned int cmd, unsigned long
>  	put_cred(c);
>  
>  #ifdef CONFIG_CGROUPS
> -	rcu_read_lock();
> -	cgrp = task_dfl_cgroup(task);
> -	kinfo.cgroupid = cgroup_id(cgrp);
> -	kinfo.mask |= PIDFD_INFO_CGROUPID;
> -	rcu_read_unlock();
> +	if (!kinfo.cgroupid) {
> +		struct cgroup *cgrp;
> +
> +		rcu_read_lock();
> +		cgrp = task_dfl_cgroup(task);
> +		kinfo.cgroupid = cgroup_id(cgrp);
> +		kinfo.mask |= PIDFD_INFO_CGROUPID;
> +		rcu_read_unlock();
> +	}
>  #endif
>  
>  	/*
> @@ -291,6 +338,7 @@ static long pidfd_info(struct task_struct *task, unsigned int cmd, unsigned long
>  	if (kinfo.pid == 0 || kinfo.tgid == 0 || (kinfo.ppid == 0 && kinfo.pid != 1))
>  		return -ESRCH;
>  
> +copy_out:
>  	/*
>  	 * If userspace and the kernel have the same struct size it can just
>  	 * be copied. If userspace provides an older struct, only the bits that
> @@ -325,7 +373,6 @@ static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	struct task_struct *task __free(put_task) = NULL;
>  	struct nsproxy *nsp __free(put_nsproxy) = NULL;
> -	struct pid *pid = pidfd_pid(file);
>  	struct ns_common *ns_common = NULL;
>  	struct pid_namespace *pid_ns;
>  
> @@ -340,13 +387,13 @@ static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		return put_user(file_inode(file)->i_generation, argp);
>  	}
>  
> -	task = get_pid_task(pid, PIDTYPE_PID);
> -	if (!task)
> -		return -ESRCH;
> -
>  	/* Extensible IOCTL that does not open namespace FDs, take a shortcut */
>  	if (_IOC_NR(cmd) == _IOC_NR(PIDFD_GET_INFO))
> -		return pidfd_info(task, cmd, arg);
> +		return pidfd_info(file, cmd, arg);
> +
> +	task = get_pid_task(pidfd_pid(file), PIDTYPE_PID);
> +	if (!task)
> +		return -ESRCH;
>  
>  	if (arg)
>  		return -EINVAL;
> @@ -479,10 +526,12 @@ void pidfs_exit(struct task_struct *tsk)
>  {
>  	struct dentry *dentry;
>  
> +	might_sleep();
> +
>  	dentry = stashed_dentry_get(&task_pid(tsk)->stashed);
>  	if (dentry) {
>  		struct inode *inode = d_inode(dentry);
> -		struct pidfs_exit_info *exit_info = &pidfs_i(inode)->exit_info;
> +		struct pidfs_exit_info *exit_info = &pidfs_i(inode)->__pei;
>  #ifdef CONFIG_CGROUPS
>  		struct cgroup *cgrp;
>  
> @@ -493,6 +542,8 @@ void pidfs_exit(struct task_struct *tsk)
>  #endif
>  		exit_info->exit_code = tsk->exit_code;
>  
> +		/* Ensure that PIDFD_GET_INFO sees either all or nothing. */
> +		smp_store_release(&pidfs_i(inode)->exit_info, &pidfs_i(inode)->__pei);
>  		dput(dentry);
>  	}
>  }
> @@ -560,7 +611,8 @@ static struct inode *pidfs_alloc_inode(struct super_block *sb)
>  	if (!pi)
>  		return NULL;
>  
> -	memset(&pi->exit_info, 0, sizeof(pi->exit_info));
> +	memset(&pi->__pei, 0, sizeof(pi->__pei));
> +	pi->exit_info = NULL;
>  
>  	return &pi->vfs_inode;
>  }
> diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
> index e0abd0b18841..e5966f1a7743 100644
> --- a/include/uapi/linux/pidfd.h
> +++ b/include/uapi/linux/pidfd.h
> @@ -20,6 +20,7 @@
>  #define PIDFD_INFO_PID			(1UL << 0) /* Always returned, even if not requested */
>  #define PIDFD_INFO_CREDS		(1UL << 1) /* Always returned, even if not requested */
>  #define PIDFD_INFO_CGROUPID		(1UL << 2) /* Always returned if available, even if not requested */
> +#define PIDFD_INFO_EXIT			(1UL << 3) /* Always returned if available, even if not requested */
>  
>  #define PIDFD_INFO_SIZE_VER0		64 /* sizeof first published struct */
>  
> @@ -86,7 +87,7 @@ struct pidfd_info {
>  	__u32 sgid;
>  	__u32 fsuid;
>  	__u32 fsgid;
> -	__u32 spare0[1];
> +	__s32 exit_code;
>  };
>  
>  #define PIDFS_IOCTL_MAGIC 0xFF
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 98d292120296..9916305e34d3 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -250,12 +250,12 @@ void release_task(struct task_struct *p)
>  	dec_rlimit_ucounts(task_ucounts(p), UCOUNT_RLIMIT_NPROC, 1);
>  	rcu_read_unlock();
>  
> +	pidfs_exit(p);
>  	cgroup_release(p);
>  
>  	write_lock_irq(&tasklist_lock);
>  	ptrace_release_task(p);
>  	thread_pid = get_pid(p->thread_pid);
> -	pidfs_exit(p);

Why move this after you just added it?

>  	__exit_signal(p);
>  
>  	/*
> 

Everything else looks fine though. Assuming you fix the nit above, you
can add:

Reviewed-by: Jeff Layton <jlayton@kernel.org>