linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christian Brauner <christian.brauner@ubuntu.com>
To: oleg@redhat.com, linux-kernel@vger.kernel.org
Cc: aarcange@redhat.com, akpm@linux-foundation.org,
	christian@kellner.me, cyphar@cyphar.com,
	elena.reshetova@intel.com, guro@fb.com, jannh@google.com,
	ldv@altlinux.org, linux-api@vger.kernel.org,
	linux-kselftest@vger.kernel.org, mhocko@suse.com,
	mingo@kernel.org, peterz@infradead.org, shuah@kernel.org,
	tglx@linutronix.de, viro@zeniv.linux.org.uk,
	Christian Brauner <christian.brauner@ubuntu.com>
Subject: [PATCH v2 1/5] pidfd: verify task is alive when printing fdinfo
Date: Wed, 16 Oct 2019 17:36:02 +0200	[thread overview]
Message-ID: <20191016153606.2326-1-christian.brauner@ubuntu.com> (raw)
In-Reply-To: <20191015141332.4055-1-christian.brauner@ubuntu.com>

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 by introducing the task_alive() helper which will be used by other
callers in follow-up patches.
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. So
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>
---
/* v1 */
Link: https://lore.kernel.org/r/20191015141332.4055-1-christian.brauner@ubuntu.com

/* v2 */
- Oleg Nesterov <oleg@redhat.com>:
  - simplify check whether task is still alive to hlist_empty()
  - optionally introduce generic helper to replace open coded
    hlist_emtpy() checks whether or not a task is alive
- Christian Brauner <christian.brauner@ubuntu.com>:
  - introduce task_alive() helper and use in pidfd_show_fdinfo()
---
 include/linux/pid.h |  4 ++++
 kernel/fork.c       | 17 +++++++++++------
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/include/linux/pid.h b/include/linux/pid.h
index 9645b1194c98..5f1c8ce10b71 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -85,6 +85,10 @@ static inline struct pid *get_pid(struct pid *pid)
 
 extern void put_pid(struct pid *pid);
 extern struct task_struct *pid_task(struct pid *pid, enum pid_type);
+static inline bool task_alive(struct pid *pid, enum pid_type type)
+{
+	return !hlist_empty(&pid->tasks[type]);
+}
 extern struct task_struct *get_pid_task(struct pid *pid, enum pid_type);
 
 extern struct pid *get_task_pid(struct task_struct *task, enum pid_type type);
diff --git a/kernel/fork.c b/kernel/fork.c
index 782986962d47..ef9a9d661079 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1732,15 +1732,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;
 
-	seq_put_decimal_ull(m, "Pid:\t", nr);
+	if (likely(task_alive(pid, PIDTYPE_PID))) {
+		ns = proc_pid_ns(file_inode(m->file));
+		nr = pid_nr_ns(pid, ns);
+	}
+
+	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 +1754,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


  parent reply	other threads:[~2019-10-16 15:37 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-15 14:13 [PATCH 1/2] " 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 ` Christian Brauner [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191016153606.2326-1-christian.brauner@ubuntu.com \
    --to=christian.brauner@ubuntu.com \
    --cc=aarcange@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=christian@kellner.me \
    --cc=cyphar@cyphar.com \
    --cc=elena.reshetova@intel.com \
    --cc=guro@fb.com \
    --cc=jannh@google.com \
    --cc=ldv@altlinux.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=mhocko@suse.com \
    --cc=mingo@kernel.org \
    --cc=oleg@redhat.com \
    --cc=peterz@infradead.org \
    --cc=shuah@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH v2 1/5] pidfd: verify task is alive when printing fdinfo' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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