[PATCH 1/2] pidfd: verify task is alive when printing fdinfo

[PATCH 1/2] pidfd: verify task is alive when printing fdinfo

[Christian Brauner]

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

[PATCH 2/2] test: verify fdinfo for pidfd of reaped process

[Christian Brauner]

Test that the fdinfo field of a pidfd referring to a dead process
correctly shows Pid: -1 and NSpid: -1.

Cc: Christian Kellner <christian@kellner.me>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 .../selftests/pidfd/pidfd_fdinfo_test.c       | 59 ++++++++++++++-----
 1 file changed, 45 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c b/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
index 3721be994abd..22558524f71c 100644
--- a/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
@@ -113,11 +113,15 @@ static struct child clone_newns(int (*fn)(void *), void *args,
 	return ret;
 }
 
+static inline void child_close(struct child *child)
+{
+	close(child->fd);
+}
+
 static inline int child_join(struct child *child, struct error *err)
 {
 	int r;
 
-	(void)close(child->fd);
 	r = wait_for_pid(child->pid);
 	if (r < 0)
 		error_set(err, PIDFD_ERROR, "waitpid failed (ret %d, errno %d)",
@@ -128,6 +132,12 @@ static inline int child_join(struct child *child, struct error *err)
 	return r;
 }
 
+static inline int child_join_close(struct child *child, struct error *err)
+{
+	child_close(child);
+	return child_join(child, err);
+}
+
 static inline void trim_newline(char *str)
 {
 	char *pos = strrchr(str, '\n');
@@ -136,8 +146,8 @@ static inline void trim_newline(char *str)
 		*pos = '\0';
 }
 
-static int verify_fdinfo_nspid(int pidfd, struct error *err,
-			       const char *expect, ...)
+static int verify_fdinfo(int pidfd, struct error *err, const char *prefix,
+			 size_t prefix_len, const char *expect, ...)
 {
 	char buffer[512] = {0, };
 	char path[512] = {0, };
@@ -160,17 +170,20 @@ static int verify_fdinfo_nspid(int pidfd, struct error *err,
 				 pidfd);
 
 	while (getline(&line, &n, f) != -1) {
-		if (strncmp(line, "NSpid:", 6))
+		char *val;
+
+		if (strncmp(line, prefix, prefix_len))
 			continue;
 
 		found = 1;
 
-		r = strcmp(line + 6, buffer);
+		val = line + prefix_len;
+		r = strcmp(val, buffer);
 		if (r != 0) {
 			trim_newline(line);
 			trim_newline(buffer);
-			error_set(err, PIDFD_FAIL, "NSpid: '%s' != '%s'",
-				  line + 6, buffer);
+			error_set(err, PIDFD_FAIL, "%s '%s' != '%s'",
+				  prefix, val, buffer);
 		}
 		break;
 	}
@@ -179,8 +192,8 @@ static int verify_fdinfo_nspid(int pidfd, struct error *err,
 	fclose(f);
 
 	if (found == 0)
-		return error_set(err, PIDFD_FAIL, "NSpid not found for fd %d",
-				 pidfd);
+		return error_set(err, PIDFD_FAIL, "%s not found for fd %d",
+				 prefix, pidfd);
 
 	return PIDFD_PASS;
 }
@@ -213,7 +226,7 @@ static int child_fdinfo_nspid_test(void *args)
 	}
 
 	pidfd = *(int *)args;
-	r = verify_fdinfo_nspid(pidfd, &err, "\t0\n");
+	r = verify_fdinfo(pidfd, &err, "NSpid:", 6, "\t0\n");
 
 	if (r != PIDFD_PASS)
 		ksft_print_msg("NSpid fdinfo check failed: %s\n", err.msg);
@@ -242,24 +255,42 @@ static void test_pidfd_fdinfo_nspid(void)
 	/* The children will have pid 1 in the new pid namespace,
 	 * so the line must be 'NSPid:\t<pid>\t1'.
 	 */
-	verify_fdinfo_nspid(a.fd, &err, "\t%d\t%d\n", a.pid, 1);
-	verify_fdinfo_nspid(b.fd, &err, "\t%d\t%d\n", b.pid, 1);
+	verify_fdinfo(a.fd, &err, "NSpid:", 6, "\t%d\t%d\n", a.pid, 1);
+	verify_fdinfo(b.fd, &err, "NSpid:", 6, "\t%d\t%d\n", b.pid, 1);
 
 	/* wait for the process, check the exit status and set
 	 * 'err' accordingly, if it is not already set.
 	 */
+	child_join_close(&a, &err);
+	child_join_close(&b, &err);
+
+	error_report(&err, test_name);
+}
+
+static void test_pidfd_dead_fdinfo(void)
+{
+	struct child a;
+	struct error err = {0, };
+	const char *test_name = "pidfd check fdinfo for dead process";
+
+	/* Create a new child in a new pid and mount namespace */
+	a = clone_newns(child_fdinfo_nspid_test, NULL, &err);
+	error_check(&err, test_name);
 	child_join(&a, &err);
-	child_join(&b, &err);
 
+	verify_fdinfo(a.fd, &err, "Pid:", 4, "\t-1\n");
+	verify_fdinfo(a.fd, &err, "NSpid:", 6, "\t-1\n");
+	child_close(&a);
 	error_report(&err, test_name);
 }
 
 int main(int argc, char **argv)
 {
 	ksft_print_header();
-	ksft_set_plan(1);
+	ksft_set_plan(2);
 
 	test_pidfd_fdinfo_nspid();
+	test_pidfd_dead_fdinfo();
 
 	return ksft_exit_pass();
 }
-- 
2.23.0

Re: [PATCH 2/2] test: verify fdinfo for pidfd of reaped process

[Christian Kellner]

On Tue, 2019-10-15 at 16:13 +0200, Christian Brauner wrote:
> Test that the fdinfo field of a pidfd referring to a dead process
> correctly shows Pid: -1 and NSpid: -1.
> 
> Cc: Christian Kellner <christian@kellner.me>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Looks good to me.

Reviewed-by: Christian Kellner <christian@kellner.me>

Re: [PATCH 1/2] pidfd: verify task is alive when printing fdinfo

[Oleg Nesterov]

On 10/15, Christian Brauner wrote:
>
> +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;
> +}

Well, the usage of rcu_read_lock/unlock looks confusing to me...

I mean, this helper does not need rcu lock at all. Except
rcu_dereference_check() will complain.

	static inline bool task_alive(struct pid *pid)
	{
		bool alive;

		/* shut up rcu_dereference_check() */
		rcu_lock_acquire(&rcu_lock_map);
		alive = !!pid_task(pid, PIDTYPE_PID));
		rcu_lock_release(&rcu_lock_map);

		return alive;
	}

looks more clear imo.

But in fact I'd suggest to simply use !hlist_empty(&pid->tasks[PIDTYPE_PID])
in pidfd_show_fdinfo() and do not add a new helper.

Oleg.

Re: [PATCH 1/2] pidfd: verify task is alive when printing fdinfo

[Christian Brauner]

On Tue, Oct 15, 2019 at 04:43:57PM +0200, Oleg Nesterov wrote:
> On 10/15, Christian Brauner wrote:
> >
> > +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;
> > +}
> 
> Well, the usage of rcu_read_lock/unlock looks confusing to me...
> 
> I mean, this helper does not need rcu lock at all. Except
> rcu_dereference_check() will complain.

Yep, I think we have another codepath were the rcu locks might be purely
cosmetic so I thought it's not a big deal (see below).

> 
> 	static inline bool task_alive(struct pid *pid)
> 	{
> 		bool alive;
> 
> 		/* shut up rcu_dereference_check() */
> 		rcu_lock_acquire(&rcu_lock_map);
> 		alive = !!pid_task(pid, PIDTYPE_PID));
> 		rcu_lock_release(&rcu_lock_map);
> 
> 		return alive;
> 	}
> 
> looks more clear imo.
> 
> But in fact I'd suggest to simply use !hlist_empty(&pid->tasks[PIDTYPE_PID])
> in pidfd_show_fdinfo() and do not add a new helper.

Sounds good to me. But can't we then just do something similar just with
!hlist_empty(&pid->tasks[PIDTYPE_TGID])

in v5.4-rc3:kernel/pid.c:pidfd_open():514-517 ?

or would this be problematic because of de_thread()?

Thanks!
Christian

Re: [PATCH 1/2] pidfd: verify task is alive when printing fdinfo

[Oleg Nesterov]

On 10/15, Christian Brauner wrote:
>
> On Tue, Oct 15, 2019 at 04:43:57PM +0200, Oleg Nesterov wrote:
> > But in fact I'd suggest to simply use !hlist_empty(&pid->tasks[PIDTYPE_PID])
> > in pidfd_show_fdinfo() and do not add a new helper.
>
> Sounds good to me. But can't we then just do something similar just with
> !hlist_empty(&pid->tasks[PIDTYPE_TGID])
>
> in v5.4-rc3:kernel/pid.c:pidfd_open():514-517 ?

Agreed. Actually, it seems to me I suggested to use rcu_lock_acquire() rather
than rcu_read_lock() in pidfd_open() too.

But hlist_empty(pid->tasks[type]) looks even better.

If you decide to add a new helper, you can also change do_wait() which checks
hlist_empty(&wo->wo_pid->tasks[wo->wo_type]). May be even __change_pid().

Oleg.

[PATCH v2 1/5] pidfd: verify task is alive when printing fdinfo

[Christian Brauner]

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

[PATCH v2 2/5] test: verify fdinfo for pidfd of reaped process

[Christian Brauner]

Test that the fdinfo field of a pidfd referring to a dead process
correctly shows Pid: -1 and NSpid: -1.

Cc: Christian Kellner <christian@kellner.me>
Reviewed-by: Christian Kellner <christian@kellner.me>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lore.kernel.org/r/20191015141332.4055-2-christian.brauner@ubuntu.com

/* v2 */
unchanged
---
 .../selftests/pidfd/pidfd_fdinfo_test.c       | 59 ++++++++++++++-----
 1 file changed, 45 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c b/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
index 3721be994abd..22558524f71c 100644
--- a/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
@@ -113,11 +113,15 @@ static struct child clone_newns(int (*fn)(void *), void *args,
 	return ret;
 }
 
+static inline void child_close(struct child *child)
+{
+	close(child->fd);
+}
+
 static inline int child_join(struct child *child, struct error *err)
 {
 	int r;
 
-	(void)close(child->fd);
 	r = wait_for_pid(child->pid);
 	if (r < 0)
 		error_set(err, PIDFD_ERROR, "waitpid failed (ret %d, errno %d)",
@@ -128,6 +132,12 @@ static inline int child_join(struct child *child, struct error *err)
 	return r;
 }
 
+static inline int child_join_close(struct child *child, struct error *err)
+{
+	child_close(child);
+	return child_join(child, err);
+}
+
 static inline void trim_newline(char *str)
 {
 	char *pos = strrchr(str, '\n');
@@ -136,8 +146,8 @@ static inline void trim_newline(char *str)
 		*pos = '\0';
 }
 
-static int verify_fdinfo_nspid(int pidfd, struct error *err,
-			       const char *expect, ...)
+static int verify_fdinfo(int pidfd, struct error *err, const char *prefix,
+			 size_t prefix_len, const char *expect, ...)
 {
 	char buffer[512] = {0, };
 	char path[512] = {0, };
@@ -160,17 +170,20 @@ static int verify_fdinfo_nspid(int pidfd, struct error *err,
 				 pidfd);
 
 	while (getline(&line, &n, f) != -1) {
-		if (strncmp(line, "NSpid:", 6))
+		char *val;
+
+		if (strncmp(line, prefix, prefix_len))
 			continue;
 
 		found = 1;
 
-		r = strcmp(line + 6, buffer);
+		val = line + prefix_len;
+		r = strcmp(val, buffer);
 		if (r != 0) {
 			trim_newline(line);
 			trim_newline(buffer);
-			error_set(err, PIDFD_FAIL, "NSpid: '%s' != '%s'",
-				  line + 6, buffer);
+			error_set(err, PIDFD_FAIL, "%s '%s' != '%s'",
+				  prefix, val, buffer);
 		}
 		break;
 	}
@@ -179,8 +192,8 @@ static int verify_fdinfo_nspid(int pidfd, struct error *err,
 	fclose(f);
 
 	if (found == 0)
-		return error_set(err, PIDFD_FAIL, "NSpid not found for fd %d",
-				 pidfd);
+		return error_set(err, PIDFD_FAIL, "%s not found for fd %d",
+				 prefix, pidfd);
 
 	return PIDFD_PASS;
 }
@@ -213,7 +226,7 @@ static int child_fdinfo_nspid_test(void *args)
 	}
 
 	pidfd = *(int *)args;
-	r = verify_fdinfo_nspid(pidfd, &err, "\t0\n");
+	r = verify_fdinfo(pidfd, &err, "NSpid:", 6, "\t0\n");
 
 	if (r != PIDFD_PASS)
 		ksft_print_msg("NSpid fdinfo check failed: %s\n", err.msg);
@@ -242,24 +255,42 @@ static void test_pidfd_fdinfo_nspid(void)
 	/* The children will have pid 1 in the new pid namespace,
 	 * so the line must be 'NSPid:\t<pid>\t1'.
 	 */
-	verify_fdinfo_nspid(a.fd, &err, "\t%d\t%d\n", a.pid, 1);
-	verify_fdinfo_nspid(b.fd, &err, "\t%d\t%d\n", b.pid, 1);
+	verify_fdinfo(a.fd, &err, "NSpid:", 6, "\t%d\t%d\n", a.pid, 1);
+	verify_fdinfo(b.fd, &err, "NSpid:", 6, "\t%d\t%d\n", b.pid, 1);
 
 	/* wait for the process, check the exit status and set
 	 * 'err' accordingly, if it is not already set.
 	 */
+	child_join_close(&a, &err);
+	child_join_close(&b, &err);
+
+	error_report(&err, test_name);
+}
+
+static void test_pidfd_dead_fdinfo(void)
+{
+	struct child a;
+	struct error err = {0, };
+	const char *test_name = "pidfd check fdinfo for dead process";
+
+	/* Create a new child in a new pid and mount namespace */
+	a = clone_newns(child_fdinfo_nspid_test, NULL, &err);
+	error_check(&err, test_name);
 	child_join(&a, &err);
-	child_join(&b, &err);
 
+	verify_fdinfo(a.fd, &err, "Pid:", 4, "\t-1\n");
+	verify_fdinfo(a.fd, &err, "NSpid:", 6, "\t-1\n");
+	child_close(&a);
 	error_report(&err, test_name);
 }
 
 int main(int argc, char **argv)
 {
 	ksft_print_header();
-	ksft_set_plan(1);
+	ksft_set_plan(2);
 
 	test_pidfd_fdinfo_nspid();
+	test_pidfd_dead_fdinfo();
 
 	return ksft_exit_pass();
 }
-- 
2.23.0

[PATCH v2 3/5] pid: use task_alive() in __change_pid()

[Christian Brauner]

Replace hlist_empty() with the new task_alive() helper which is more
idiomatic, easier to grep for, and unifies how callers perform this
check.

Cc: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
patch not present

/* v2 */
patch introduced
---
 kernel/pid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/pid.c b/kernel/pid.c
index 0a9f2e437217..70ad4a9f728c 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -299,7 +299,7 @@ static void __change_pid(struct task_struct *task, enum pid_type type,
 	*pid_ptr = new;
 
 	for (tmp = PIDTYPE_MAX; --tmp >= 0; )
-		if (!hlist_empty(&pid->tasks[tmp]))
+		if (task_alive(pid, tmp))
 			return;
 
 	free_pid(pid);
-- 
2.23.0

[PATCH v2 4/5] exit: use task_alive() in do_wait()

[Christian Brauner]

Replace hlist_empty() with the new task_alive() helper which is more
idiomatic, easier to grep for, and unifies how callers perform this check.

Cc: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
patch not present

/* v2 */
patch introduced
---
 kernel/exit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index a46a50d67002..2bb0cbe94bda 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1457,7 +1457,7 @@ static long do_wait(struct wait_opts *wo)
 	 */
 	wo->notask_error = -ECHILD;
 	if ((wo->wo_type < PIDTYPE_MAX) &&
-	   (!wo->wo_pid || hlist_empty(&wo->wo_pid->tasks[wo->wo_type])))
+	   (!wo->wo_pid || !task_alive(wo->wo_pid, wo->wo_type)))
 		goto notask;
 
 	set_current_state(TASK_INTERRUPTIBLE);
-- 
2.23.0

[PATCH v2 5/5] pid: use task_alive() in pidfd_open()

[Christian Brauner]

Use the new task_alive() helper in pidfd_open(). This simplifies the
code and avoids taking rcu_read_{lock,unlock}() and leads to overall
nicer code.

Cc: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
patch not present

/* v2 */
patch introduced
---
 kernel/pid.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/kernel/pid.c b/kernel/pid.c
index 70ad4a9f728c..1f425b6c4c47 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -497,7 +497,7 @@ static int pidfd_create(struct pid *pid)
  */
 SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
 {
-	int fd, ret;
+	int fd;
 	struct pid *p;
 
 	if (flags)
@@ -510,13 +510,11 @@ SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
 	if (!p)
 		return -ESRCH;
 
-	ret = 0;
-	rcu_read_lock();
-	if (!pid_task(p, PIDTYPE_TGID))
-		ret = -EINVAL;
-	rcu_read_unlock();
+	if (task_alive(p, PIDTYPE_TGID))
+		fd = pidfd_create(p);
+	else
+		fd = -EINVAL;
 
-	fd = ret ?: pidfd_create(p);
 	put_pid(p);
 	return fd;
 }
-- 
2.23.0

Re: [PATCH v2 1/5] pidfd: verify task is alive when printing fdinfo

[Oleg Nesterov]

On 10/16, Christian Brauner wrote:
>
> +static inline bool task_alive(struct pid *pid, enum pid_type type)
> +{
> +	return !hlist_empty(&pid->tasks[type]);
> +}

So you decided to add a helper ;) OK, but note that its name is very
confusing and misleading. Even more than pid_alive() we already have.

What does "alive" actually mean? Say, task_alive(pid, PIDTYPE_SID) == F
after fork(). Then it becomes T if this task does setsid().

And why task_ if it accepts pid+pid_type? May be pid_has_task() or
something like this...

OK, since I can't suggest a better name I won't really argue. Feel free
to add my reviewed-by to this series.

Oleg.

Re: [PATCH v2 1/5] pidfd: verify task is alive when printing fdinfo

[Christian Brauner]

On Wed, Oct 16, 2019 at 06:24:09PM +0200, Oleg Nesterov wrote:
> On 10/16, Christian Brauner wrote:
> >
> > +static inline bool task_alive(struct pid *pid, enum pid_type type)
> > +{
> > +	return !hlist_empty(&pid->tasks[type]);
> > +}
> 
> So you decided to add a helper ;) OK, but note that its name is very
> confusing and misleading. Even more than pid_alive() we already have.

That's why I chose that name. This is the second time I get bitten by
taking inspiration from prior naming examples. :)

> 
> What does "alive" actually mean? Say, task_alive(pid, PIDTYPE_SID) == F
> after fork(). Then it becomes T if this task does setsid().

Yes, that annoyed me to. If you think about pidfd_open() you have a
similar problem. The question we are asking in pidfd_open() is not
task_alive() but rather was-this-pid-used-as.

> 
> And why task_ if it accepts pid+pid_type? May be pid_has_task() or
> something like this...

Given what I said above that might be a decent name.

> 
> OK, since I can't suggest a better name I won't really argue. Feel free
> to add my reviewed-by to this series.

No, naming is important. Thanks for being picky about that too and I'll
happily resend. :)

Thanks!
Christian

Re: [PATCH v2 1/5] pidfd: verify task is alive when printing fdinfo

[Oleg Nesterov]

On 10/16, Christian Brauner wrote:
>
> On Wed, Oct 16, 2019 at 06:24:09PM +0200, Oleg Nesterov wrote:
> >
> > And why task_ if it accepts pid+pid_type? May be pid_has_task() or
> > something like this...
>
> Given what I said above that might be a decent name.
>
> >
> > OK, since I can't suggest a better name I won't really argue. Feel free
> > to add my reviewed-by to this series.
>
> No, naming is important. Thanks for being picky about that too and I'll
> happily resend. :)

Thanks ;) May be pid_in_use() ? Up to you, anything which starts with pid_
looks better to me than task_alive().

Oleg.

[PATCH v3 1/5] pidfd: check pid has attached task in fdinfo

[Christian Brauner]

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 pid_has_task() 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: linux-api@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
---
/* pidfd selftests */
passed

/* v1 */
Link: https://lore.kernel.org/r/20191015141332.4055-1-christian.brauner@ubuntu.com

/* v2 */
Link: https://lore.kernel.org/r/20191016153606.2326-1-christian.brauner@ubuntu.com
- 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()

/* v3 */
- Oleg Nesterov <oleg@redhat.com>:
  - s/task_alive/pid_has_task/g
- Christian Brauner <christian.brauner@ubuntu.com>:
  - reword commit message to better reflect naming switch from
    task_alive() to pid_has_task()
---
 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..034e3cd60dc0 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 pid_has_task(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..ffa314838b43 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(pid_has_task(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

[PATCH v3 2/5] test: verify fdinfo for pidfd of reaped process

[Christian Brauner]

Test that the fdinfo field of a pidfd referring to a dead process
correctly shows Pid: -1 and NSpid: -1.

Cc: Christian Kellner <christian@kellner.me>
Cc: linux-kselftest@vger.kernel.org
Reviewed-by: Christian Kellner <christian@kellner.me>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* pidfd selftests */
passed

/* v1 */
Link: https://lore.kernel.org/r/20191015141332.4055-2-christian.brauner@ubuntu.com

/* v2 */
Link: https://lore.kernel.org/r/20191016153606.2326-2-christian.brauner@ubuntu.com
unchanged

/* v3 */
unchanged
---
 .../selftests/pidfd/pidfd_fdinfo_test.c       | 59 ++++++++++++++-----
 1 file changed, 45 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c b/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
index 3721be994abd..22558524f71c 100644
--- a/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
@@ -113,11 +113,15 @@ static struct child clone_newns(int (*fn)(void *), void *args,
 	return ret;
 }
 
+static inline void child_close(struct child *child)
+{
+	close(child->fd);
+}
+
 static inline int child_join(struct child *child, struct error *err)
 {
 	int r;
 
-	(void)close(child->fd);
 	r = wait_for_pid(child->pid);
 	if (r < 0)
 		error_set(err, PIDFD_ERROR, "waitpid failed (ret %d, errno %d)",
@@ -128,6 +132,12 @@ static inline int child_join(struct child *child, struct error *err)
 	return r;
 }
 
+static inline int child_join_close(struct child *child, struct error *err)
+{
+	child_close(child);
+	return child_join(child, err);
+}
+
 static inline void trim_newline(char *str)
 {
 	char *pos = strrchr(str, '\n');
@@ -136,8 +146,8 @@ static inline void trim_newline(char *str)
 		*pos = '\0';
 }
 
-static int verify_fdinfo_nspid(int pidfd, struct error *err,
-			       const char *expect, ...)
+static int verify_fdinfo(int pidfd, struct error *err, const char *prefix,
+			 size_t prefix_len, const char *expect, ...)
 {
 	char buffer[512] = {0, };
 	char path[512] = {0, };
@@ -160,17 +170,20 @@ static int verify_fdinfo_nspid(int pidfd, struct error *err,
 				 pidfd);
 
 	while (getline(&line, &n, f) != -1) {
-		if (strncmp(line, "NSpid:", 6))
+		char *val;
+
+		if (strncmp(line, prefix, prefix_len))
 			continue;
 
 		found = 1;
 
-		r = strcmp(line + 6, buffer);
+		val = line + prefix_len;
+		r = strcmp(val, buffer);
 		if (r != 0) {
 			trim_newline(line);
 			trim_newline(buffer);
-			error_set(err, PIDFD_FAIL, "NSpid: '%s' != '%s'",
-				  line + 6, buffer);
+			error_set(err, PIDFD_FAIL, "%s '%s' != '%s'",
+				  prefix, val, buffer);
 		}
 		break;
 	}
@@ -179,8 +192,8 @@ static int verify_fdinfo_nspid(int pidfd, struct error *err,
 	fclose(f);
 
 	if (found == 0)
-		return error_set(err, PIDFD_FAIL, "NSpid not found for fd %d",
-				 pidfd);
+		return error_set(err, PIDFD_FAIL, "%s not found for fd %d",
+				 prefix, pidfd);
 
 	return PIDFD_PASS;
 }
@@ -213,7 +226,7 @@ static int child_fdinfo_nspid_test(void *args)
 	}
 
 	pidfd = *(int *)args;
-	r = verify_fdinfo_nspid(pidfd, &err, "\t0\n");
+	r = verify_fdinfo(pidfd, &err, "NSpid:", 6, "\t0\n");
 
 	if (r != PIDFD_PASS)
 		ksft_print_msg("NSpid fdinfo check failed: %s\n", err.msg);
@@ -242,24 +255,42 @@ static void test_pidfd_fdinfo_nspid(void)
 	/* The children will have pid 1 in the new pid namespace,
 	 * so the line must be 'NSPid:\t<pid>\t1'.
 	 */
-	verify_fdinfo_nspid(a.fd, &err, "\t%d\t%d\n", a.pid, 1);
-	verify_fdinfo_nspid(b.fd, &err, "\t%d\t%d\n", b.pid, 1);
+	verify_fdinfo(a.fd, &err, "NSpid:", 6, "\t%d\t%d\n", a.pid, 1);
+	verify_fdinfo(b.fd, &err, "NSpid:", 6, "\t%d\t%d\n", b.pid, 1);
 
 	/* wait for the process, check the exit status and set
 	 * 'err' accordingly, if it is not already set.
 	 */
+	child_join_close(&a, &err);
+	child_join_close(&b, &err);
+
+	error_report(&err, test_name);
+}
+
+static void test_pidfd_dead_fdinfo(void)
+{
+	struct child a;
+	struct error err = {0, };
+	const char *test_name = "pidfd check fdinfo for dead process";
+
+	/* Create a new child in a new pid and mount namespace */
+	a = clone_newns(child_fdinfo_nspid_test, NULL, &err);
+	error_check(&err, test_name);
 	child_join(&a, &err);
-	child_join(&b, &err);
 
+	verify_fdinfo(a.fd, &err, "Pid:", 4, "\t-1\n");
+	verify_fdinfo(a.fd, &err, "NSpid:", 6, "\t-1\n");
+	child_close(&a);
 	error_report(&err, test_name);
 }
 
 int main(int argc, char **argv)
 {
 	ksft_print_header();
-	ksft_set_plan(1);
+	ksft_set_plan(2);
 
 	test_pidfd_fdinfo_nspid();
+	test_pidfd_dead_fdinfo();
 
 	return ksft_exit_pass();
 }
-- 
2.23.0

[PATCH v3 3/5] pid: use pid_has_task() in __change_pid()

[Christian Brauner]

Replace hlist_empty() with the new pid_has_task() helper which is more
idiomatic, easier to grep for, and unifies how callers perform this
check.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
---
/* pidfd selftests */
passed

/* v1 */
patch not present

/* v2 */
Link: https://lore.kernel.org/r/20191016153606.2326-3-christian.brauner@ubuntu.com
patch introduced

/* v2 */
- Oleg Nesterov <oleg@redhat.com>:
  - s/task_alive/pid_has_task/g
---
 kernel/pid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/pid.c b/kernel/pid.c
index 0a9f2e437217..124d40b239b1 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -299,7 +299,7 @@ static void __change_pid(struct task_struct *task, enum pid_type type,
 	*pid_ptr = new;
 
 	for (tmp = PIDTYPE_MAX; --tmp >= 0; )
-		if (!hlist_empty(&pid->tasks[tmp]))
+		if (pid_has_task(pid, tmp))
 			return;
 
 	free_pid(pid);
-- 
2.23.0

[PATCH v3 4/5] exit: use pid_has_task() in do_wait()

[Christian Brauner]

Replace hlist_empty() with the new pid_has_task() helper which is more
idiomatic, easier to grep for, and unifies how callers perform this check.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
---
/* pidfd selftests */
passed

/* v1 */
patch not present

/* v2 */
Link: https://lore.kernel.org/r/20191016153606.2326-4-christian.brauner@ubuntu.com
patch introduced

/* v3 */
- Oleg Nesterov <oleg@redhat.com>:
  - s/task_alive/pid_has_task/
---
 kernel/exit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index a46a50d67002..f2d20ab74422 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1457,7 +1457,7 @@ static long do_wait(struct wait_opts *wo)
 	 */
 	wo->notask_error = -ECHILD;
 	if ((wo->wo_type < PIDTYPE_MAX) &&
-	   (!wo->wo_pid || hlist_empty(&wo->wo_pid->tasks[wo->wo_type])))
+	   (!wo->wo_pid || !pid_has_task(wo->wo_pid, wo->wo_type)))
 		goto notask;
 
 	set_current_state(TASK_INTERRUPTIBLE);
-- 
2.23.0

[PATCH v3 5/5] pid: use pid_has_task() in pidfd_open()

[Christian Brauner]

Use the new pid_has_task() helper in pidfd_open(). This simplifies the
code and avoids taking rcu_read_{lock,unlock}() and leads to overall
nicer code.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
---
/* pidfd selftests */
passed

/* v1 */
patch not present

/* v2 */
Link: https://lore.kernel.org/r/20191016153606.2326-5-christian.brauner@ubuntu.com
patch introduced

/* v3 */
- Oleg Nesterov <oleg@redhat.com>:
  - s/task_alive/pid_has_task/
---
 kernel/pid.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/kernel/pid.c b/kernel/pid.c
index 124d40b239b1..7b5f6c963d72 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -497,7 +497,7 @@ static int pidfd_create(struct pid *pid)
  */
 SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
 {
-	int fd, ret;
+	int fd;
 	struct pid *p;
 
 	if (flags)
@@ -510,13 +510,11 @@ SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
 	if (!p)
 		return -ESRCH;
 
-	ret = 0;
-	rcu_read_lock();
-	if (!pid_task(p, PIDTYPE_TGID))
-		ret = -EINVAL;
-	rcu_read_unlock();
+	if (pid_has_task(p, PIDTYPE_TGID))
+		fd = pidfd_create(p);
+	else
+		fd = -EINVAL;
 
-	fd = ret ?: pidfd_create(p);
 	put_pid(p);
 	return fd;
 }
-- 
2.23.0

Re: [PATCH v3 1/5] pidfd: check pid has attached task in fdinfo

[Christian Brauner]

On Thu, Oct 17, 2019 at 12:18:28PM +0200, Christian Brauner wrote:
> 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 pid_has_task() 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: linux-api@vger.kernel.org
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Applied this series to:
https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=pidfd

Thanks!
Christian