linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] pidfd: verify task is alive when printing fdinfo
@ 2019-10-15 14:13 Christian Brauner
  2019-10-15 14:13 ` [PATCH 2/2] test: verify fdinfo for pidfd of reaped process Christian Brauner
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Christian Brauner @ 2019-10-15 14:13 UTC (permalink / raw)
  To: linux-kernel
  Cc: Shuah Khan, Andrew Morton, Peter Zijlstra (Intel),
	Ingo Molnar, Michal Hocko, Thomas Gleixner, Elena Reshetova,
	Christian Kellner, Roman Gushchin, Andrea Arcangeli, Al Viro,
	Aleksa Sarai, Dmitry V. Levin, linux-kselftest,
	Christian Brauner, Jann Horn, Oleg Nesterov, linux-api

Currently, when a task is dead we still print the pid it used to use in
the fdinfo files of its pidfds. This doesn't make much sense since the
pid may have already been reused. So verify that the task is still
alive. If the task is not alive anymore, we will print -1. This allows
us to differentiate between a task not being present in a given pid
namespace - in which case we already print 0 - and a task having been
reaped.

Note that this uses PIDTYPE_PID for the check. Technically, we could've
checked PIDTYPE_TGID since pidfds currently only refer to thread-group
leaders but if they won't anymore in the future then this check becomes
problematic without it being immediately obvious to non-experts imho. If
a thread is created via clone(CLONE_THREAD) than struct pid has a single
non-empty list pid->tasks[PIDTYPE_PID] and this pid can't be used as a
PIDTYPE_TGID meaning pid->tasks[PIDTYPE_TGID] will return NULL even
though the thread-group leader might still be very much alive. We could
be more complicated and do something like:

bool alive = false;
rcu_read_lock();
struct task_struct *tsk = pid_task(pid, PIDTYPE_PID);
if (tsk && task_tgid(tsk))
	alive = true;
rcu_read_unlock();

but it's really not worth it. We already have created a pidfd and we
thus know it refers to a thread-group leader. Checking PIDTYPE_PID is
fine and is easier to maintain should we ever allow pidfds to refer to
threads.

Cc: Jann Horn <jannh@google.com>
Cc: Christian Kellner <christian@kellner.me>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: linux-api@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 kernel/fork.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 782986962d47..a67944a5e542 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1695,6 +1695,18 @@ static int pidfd_release(struct inode *inode, struct file *file)
 }
 
 #ifdef CONFIG_PROC_FS
+static inline bool task_alive(struct pid *pid)
+{
+	bool alive = true;
+
+	rcu_read_lock();
+	if (!pid_task(pid, PIDTYPE_PID))
+		alive = false;
+	rcu_read_unlock();
+
+	return alive;
+}
+
 /**
  * pidfd_show_fdinfo - print information about a pidfd
  * @m: proc fdinfo file
@@ -1732,15 +1744,20 @@ static int pidfd_release(struct inode *inode, struct file *file)
  */
 static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
 {
-	struct pid_namespace *ns = proc_pid_ns(file_inode(m->file));
 	struct pid *pid = f->private_data;
-	pid_t nr = pid_nr_ns(pid, ns);
+	struct pid_namespace *ns;
+	pid_t nr = -1;
+
+	if (likely(task_alive(pid))) {
+		ns = proc_pid_ns(file_inode(m->file));
+		nr = pid_nr_ns(pid, ns);
+	}
 
-	seq_put_decimal_ull(m, "Pid:\t", nr);
+	seq_put_decimal_ll(m, "Pid:\t", nr);
 
 #ifdef CONFIG_PID_NS
-	seq_put_decimal_ull(m, "\nNSpid:\t", nr);
-	if (nr) {
+	seq_put_decimal_ll(m, "\nNSpid:\t", nr);
+	if (nr > 0) {
 		int i;
 
 		/* If nr is non-zero it means that 'pid' is valid and that
@@ -1749,7 +1766,7 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
 		 * Start at one below the already printed level.
 		 */
 		for (i = ns->level + 1; i <= pid->level; i++)
-			seq_put_decimal_ull(m, "\t", pid->numbers[i].nr);
+			seq_put_decimal_ll(m, "\t", pid->numbers[i].nr);
 	}
 #endif
 	seq_putc(m, '\n');
-- 
2.23.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2019-10-18 15:05 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-15 14:13 [PATCH 1/2] pidfd: verify task is alive when printing fdinfo Christian Brauner
2019-10-15 14:13 ` [PATCH 2/2] test: verify fdinfo for pidfd of reaped process Christian Brauner
2019-10-16 12:07   ` Christian Kellner
2019-10-15 14:43 ` [PATCH 1/2] pidfd: verify task is alive when printing fdinfo Oleg Nesterov
2019-10-15 14:56   ` Christian Brauner
2019-10-15 15:43     ` Oleg Nesterov
2019-10-16 15:36 ` [PATCH v2 1/5] " Christian Brauner
2019-10-16 15:36   ` [PATCH v2 2/5] test: verify fdinfo for pidfd of reaped process Christian Brauner
2019-10-16 15:36   ` [PATCH v2 3/5] pid: use task_alive() in __change_pid() Christian Brauner
2019-10-16 15:36   ` [PATCH v2 4/5] exit: use task_alive() in do_wait() Christian Brauner
2019-10-16 15:36   ` [PATCH v2 5/5] pid: use task_alive() in pidfd_open() Christian Brauner
2019-10-16 16:24   ` [PATCH v2 1/5] pidfd: verify task is alive when printing fdinfo Oleg Nesterov
2019-10-16 16:31     ` Christian Brauner
2019-10-17  8:54       ` Oleg Nesterov
2019-10-17 10:18   ` [PATCH v3 1/5] pidfd: check pid has attached task in fdinfo Christian Brauner
2019-10-17 10:18     ` [PATCH v3 2/5] test: verify fdinfo for pidfd of reaped process Christian Brauner
2019-10-17 10:18     ` [PATCH v3 3/5] pid: use pid_has_task() in __change_pid() Christian Brauner
2019-10-17 10:18     ` [PATCH v3 4/5] exit: use pid_has_task() in do_wait() Christian Brauner
2019-10-17 10:18     ` [PATCH v3 5/5] pid: use pid_has_task() in pidfd_open() Christian Brauner
2019-10-18 15:05     ` [PATCH v3 1/5] pidfd: check pid has attached task in fdinfo Christian Brauner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).