pidfs: don't let PIDFD_GET_INFO miss a reaped task

PIDFD_GET_INFO describes the task while there is one, and with
PIDFD_INFO_EXIT its exit status once it has been reaped.  pidfd_info()
looks at the two in the wrong order and can miss both:

  pidfd_info()                          release_task()
    PIDFS_ATTR_BIT_EXIT is not set
                                          pidfs_exit()
                                          __unhash_process()
    get_pid_task() == NULL
    goto copy_out

The ioctl succeeds with neither PIDFD_INFO_PID nor PIDFD_INFO_EXIT in the
mask.  If the task is unhashed a little later, after get_pid_task(), its
pid reads as 0 and the ioctl fails with -ESRCH, although the caller asked
for the exit information and it is available by now.

The second case does not even need an exit.  When a sub-thread execs,
de_thread() hands the leader's pid over to it and releases the old leader.
A caller that got hold of the old leader reports the tid of the sub-thread
as the pid, or -ESRCH for a process that is alive.

Look up the task first.  pidfs_exit() runs before the task is unhashed, so
a caller that finds no task finds the exit information; this needs a
barrier on both sides because taking tasklist_lock does not order the
store in pidfs_exit() against the one in detach_pid().  And start over if
the task no longer owns the pid when all the fields have been read: then
there is either exit information or another task.

Go by the pid for that check and for the pid we report, not by
->thread_pid of the task.  exchange_tids() moves the pid to the exec'ing
thread before it updates ->thread_pid of either task.  A caller that
finds the new leader in between would read the old tid of the sub-thread
from ->thread_pid, and a check of ->thread_pid would restart until the
exec'ing thread gets to its store.  On PREEMPT_RT that thread can be
preempted with tasklist_lock held and can't move to another CPU.  If the
RT task that preempted it calls PIDFD_GET_INFO, the ioctl spins until
the exec'ing thread gets the CPU back, and that never happens if the
exec'ing thread is an RT task of lower priority.

Christian Brauner <brauner@kernel.org> says:

Go by @pid instead of ->thread_pid of the task for the reported pid and
for the restart check.

Fixes: 7477d7dce48a ("pidfs: allow to retrieve exit information")
Fixes: cdda1f26e74b ("pidfd: add ioctl to retrieve pid info")
Cc: stable@vger.kernel.org
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

1 files changed, 37 insertions, 6 deletions

diff --git a/fs/pidfs.c b/fs/pidfs.c
index 7f8c323ce6cb..66c1046e2929 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -362,6 +362,12 @@ static __u32 pidfs_coredump_mask(enum task_dumpable dumpable)
 			      PIDFD_INFO_COREDUMP_SIGNAL | \
 			      PIDFD_INFO_COREDUMP_CODE)
 
+static bool pidfs_task_owns_pid(struct task_struct *task, struct pid *pid)
+{
+	guard(rcu)();
+	return pid_task(pid, PIDTYPE_PID) == task;
+}
+
 static long pidfd_info(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct pidfd_info __user *uinfo = (struct pidfd_info __user *)arg;
@@ -392,6 +398,16 @@ static long pidfd_info(struct file *file, unsigned int cmd, unsigned long arg)
 		return -EREMOTE;
 
 	attr = READ_ONCE(pid->attr);
+again:
+	/*
+	 * Look for the task first. pidfs_exit() records the exit information
+	 * before the task is unhashed, so if the task is gone the information
+	 * is there.
+	 */
+	task = get_pid_task(pid, PIDTYPE_PID);
+	if (!task)
+		smp_rmb(); /* pairs with smp_mb__after_atomic() in pidfs_exit() */
+
 	if (mask & PIDFD_INFO_EXIT) {
 		if (test_bit(PIDFS_ATTR_BIT_EXIT, &attr->attr_mask)) {
 			smp_rmb();
@@ -414,7 +430,6 @@ static long pidfd_info(struct file *file, unsigned int cmd, unsigned long arg)
 		}
 	}
 
-	task = get_pid_task(pid, PIDTYPE_PID);
 	if (!task) {
 		/*
 		 * If the task has already been reaped, only exit
@@ -468,16 +483,27 @@ static long pidfd_info(struct file *file, unsigned int cmd, unsigned long arg)
 	 * task might return as soon as the copy_to_user finishes, but that's ok
 	 * and userspace expects that might happen and can act accordingly, so
 	 * this is just best-effort. What we can do however is checking that all
-	 * the fields are set correctly, or return ESRCH to avoid providing
-	 * incomplete information. */
+	 * the fields are set correctly, to avoid providing incomplete
+	 * information.
+	 */
 
 	kinfo.ppid = task_ppid_vnr(task);
 	kinfo.tgid = task_tgid_vnr(task);
-	kinfo.pid = task_pid_vnr(task);
+	kinfo.pid = pid_vnr(pid);
 	kinfo.mask |= PIDFD_INFO_PID;
 
-	if (kinfo.pid == 0 || kinfo.tgid == 0)
-		return -ESRCH;
+	/*
+	 * The task was reaped, or lost @pid to a sub-thread that called exec,
+	 * while we were looking at it. Start over: there is exit information
+	 * now, or a new task. Don't go by ->thread_pid, exchange_tids()
+	 * updates it only after it has moved @pid.
+	 */
+	if (!pidfs_task_owns_pid(task, pid) || kinfo.tgid == 0) {
+		put_task_struct(task);
+		task = NULL;
+		memset(&kinfo, 0, sizeof(kinfo));
+		goto again;
+	}
 
 copy_out:
 	if (mask & PIDFD_INFO_SUPPORTED_MASK) {
@@ -790,6 +816,11 @@ void pidfs_exit(struct task_struct *tsk)
 	/* Ensure that PIDFD_GET_INFO sees either all or nothing. */
 	smp_wmb();
 	set_bit(PIDFS_ATTR_BIT_EXIT, &attr->attr_mask);
+	/*
+	 * Whoever sees that the task is unhashed must see the bit, see
+	 * pidfd_info(). Taking tasklist_lock does not order the two stores.
+	 */
+	smp_mb__after_atomic();
 }
 
 #ifdef CONFIG_COREDUMP