LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Christian Brauner <christian.brauner@ubuntu.com>
To: linux-kernel@vger.kernel.org, libc-alpha@sourceware.org
Cc: oleg@redhat.com, alistair23@gmail.com, ebiederm@xmission.com,
	arnd@arndb.de, dalias@libc.org, torvalds@linux-foundation.org,
	adhemerval.zanella@linaro.org, fweimer@redhat.com,
	palmer@sifive.com, macro@wdc.com, zongbox@gmail.com,
	akpm@linux-foundation.org, viro@zeniv.linux.org.uk,
	hpa@zytor.com, Christian Brauner <christian.brauner@ubuntu.com>
Subject: [PATCH v2 1/1] waitid: Add support for waiting for the current process group
Date: Wed, 14 Aug 2019 15:07:32 +0200
Message-ID: <20190814130732.23572-2-christian.brauner@ubuntu.com> (raw)
In-Reply-To: <20190814130732.23572-1-christian.brauner@ubuntu.com>

From: "Eric W. Biederman" <ebiederm@xmission.com>

It was recently discovered that the linux version of waitid is not a
superset of the other wait functions because it does not include
support for waiting for the current process group.  This has two
downsides.  An extra system call is needed to get the current process
group, and a signal could come in between the system call that
retrieved the process gorup and the call to waitid that changes the
current process group.

Allow userspace to avoid both of those issues by defining
idtype == P_PGID and id == 0 to mean wait for the caller's process
group at the time of the call.

Arguments can be made for using a different choice of idtype and id
for this case but the BSDs already use this P_PGID and 0 to indicate
waiting for the current process's process group.  So be nice to user
space programmers and don't introduce an unnecessary incompatibility.

Some people have noted that the posix description is that
waitpid will wait for the current process group, and that in
the presence of pthreads that process group can change.  To get
clarity on this issue I looked at XNU, FreeBSD, and Luminos.  All of
those flavors of unix waited for the current process group at the
time of call and as written could not adapt to the process group
changing after the call.

At one point Linux did adapt to the current process group changing but
that stopped in 161550d74c07 ("pid: sys_wait... fixes").  It has been
over 11 years since Linux has that behavior, no programs that fail
with the change in behavior have been reported, and I could not
find any other unix that does this.  So I think it is safe to clarify
the definition of current process group, to current process group
at the time of the wait function.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Rich Felker <dalias@libc.org>
Cc: Alistair Francis <alistair23@gmail.com>
Cc: Zong Li <zongbox@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Florian Weimer <fweimer@redhat.com>
Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: GNU C Library <libc-alpha@sourceware.org>
---
v1:
- Christian Brauner <christian.brauner@ubuntu.com>:
  - move find_get_pid() calls into the switch statements to minimize
    merge conflicts with P_PIDFD changes and adhere to coding style
    discussions we had for P_PIDFD

v2:
- Oleg Nesterov <oleg@redhat.com>:
  - take rcu_read_lock() around task_pgrp() so that we don't race with
    another thread changing the pgrp
- Christian Brauner <christian.brauner@ubuntu.com>:
  - introduce find_get_pgrp()
---
 kernel/exit.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 5b4a5dcce8f8..d02715a39f68 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1554,6 +1554,20 @@ static long do_wait(struct wait_opts *wo)
 	return retval;
 }
 
+static struct pid *find_get_pgrp(pid_t nr)
+{
+	struct pid *pid;
+
+	if (nr)
+		return find_get_pid(nr);
+
+	rcu_read_lock();
+	pid = get_pid(task_pgrp(current));
+	rcu_read_unlock();
+
+	return pid;
+}
+
 static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
 			  int options, struct rusage *ru)
 {
@@ -1576,19 +1590,20 @@ static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
 		type = PIDTYPE_PID;
 		if (upid <= 0)
 			return -EINVAL;
+
+		pid = find_get_pid(upid);
 		break;
 	case P_PGID:
 		type = PIDTYPE_PGID;
-		if (upid <= 0)
+		if (upid < 0)
 			return -EINVAL;
+
+		pid = find_get_pgrp(upid);
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	if (type < PIDTYPE_MAX)
-		pid = find_get_pid(upid);
-
 	wo.wo_type	= type;
 	wo.wo_pid	= pid;
 	wo.wo_flags	= options;
-- 
2.22.0


  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CAKmqyKMJPQAOKn11xepzAwXOd4e9dU0Cyz=A0T-uMEgUp5yJjA@mail.gmail.com>
2019-08-14 11:38 ` [PATCH v1 0/1] waitid: process group enhancement christian.brauner
2019-08-14 11:38   ` [PATCH v1 1/1] waitid: Add support for waiting for the current process group christian.brauner
2019-08-14 12:29     ` Oleg Nesterov
2019-08-14 12:45       ` Christian Brauner
2019-08-14 12:50         ` Oleg Nesterov
2019-08-14 12:53           ` Christian Brauner
2019-08-14 13:07 ` [PATCH v2 0/1] waitid: process group enhancement Christian Brauner
2019-08-14 13:07   ` Christian Brauner [this message]
2019-08-14 14:19     ` [PATCH v2 1/1] waitid: Add support for waiting for the current process group Oleg Nesterov
2019-08-14 14:35       ` Christian Brauner
2019-08-14 15:27         ` Oleg Nesterov
2019-08-14 15:30           ` Christian Brauner
2019-08-14 15:43 ` [PATCH v3 0/1] waitid: process group enhancement Christian Brauner
2019-08-14 15:44   ` [PATCH v3 1/1] waitid: Add support for waiting for the current process group Christian Brauner
2019-08-14 16:09     ` Oleg Nesterov
2019-08-14 16:15       ` Christian Brauner
2019-08-14 16:34         ` Christian Brauner
2019-08-14 16:55           ` Rich Felker
2019-08-14 17:02             ` Christian Brauner
2019-08-14 17:06             ` Linus Torvalds
2019-08-14 18:00               ` Rich Felker
2019-08-14 20:50       ` Christian Brauner
2019-08-14 15:58   ` [PATCH v3 0/1] waitid: process group enhancement Rich Felker
2019-08-14 16:13     ` Christian Brauner

Reply instructions:

You may reply publically 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=20190814130732.23572-2-christian.brauner@ubuntu.com \
    --to=christian.brauner@ubuntu.com \
    --cc=adhemerval.zanella@linaro.org \
    --cc=akpm@linux-foundation.org \
    --cc=alistair23@gmail.com \
    --cc=arnd@arndb.de \
    --cc=dalias@libc.org \
    --cc=ebiederm@xmission.com \
    --cc=fweimer@redhat.com \
    --cc=hpa@zytor.com \
    --cc=libc-alpha@sourceware.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=macro@wdc.com \
    --cc=oleg@redhat.com \
    --cc=palmer@sifive.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=zongbox@gmail.com \
    /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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git