| Message ID | 20190814130732.23572-2-christian.brauner@ubuntu.com |
|---|---|
| State | Not applicable |
| Headers |
Received: (qmail 109259 invoked by alias); 14 Aug 2019 13:10:54 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 109241 invoked by uid 89); 14 Aug 2019 13:10:53 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SEM_URI, SEM_URIRED autolearn=ham version=3.3.1 spammy= X-HELO: youngberry.canonical.com 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> In-Reply-To: <20190814130732.23572-1-christian.brauner@ubuntu.com> References: <CAKmqyKMJPQAOKn11xepzAwXOd4e9dU0Cyz=A0T-uMEgUp5yJjA@mail.gmail.com> <20190814130732.23572-1-christian.brauner@ubuntu.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit |
Commit Message
Christian Brauner
Aug. 14, 2019, 1:07 p.m. UTC
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(-)
Comments
On 08/14, Christian Brauner wrote: > > +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; > +} I can't say I like this helper... even its name doesn't look good to me. I forgot that we already have get_task_pid() when I replied to the previous version... How about case P_PGID: if (upid) pid = find_get_pid(upid); else pid = get_task_pid(current, PIDTYPE_PGID); ? Oleg.
On Wed, Aug 14, 2019 at 04:19:57PM +0200, Oleg Nesterov wrote: > On 08/14, Christian Brauner wrote: > > > > +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; > > +} > > I can't say I like this helper... even its name doesn't look good to me. Well, naming scheme obviously stolen from find_get_pid(). Not sure if that doesn't look good as well. ;) > > I forgot that we already have get_task_pid() when I replied to the previous > version... How about > > case P_PGID: > > if (upid) > pid = find_get_pid(upid); > else > pid = get_task_pid(current, PIDTYPE_PGID); > > ? Hmyeah, that works but wouldn't it still be nicer to simply have: static struct pid *get_pgrp(pid_t nr) { if (nr) return find_get_pid(nr); return get_task_pid(current, PIDTYPE_PGID); } That allows us to have all cases equivalent with a single call to a function without any if-elses. Imho, this becomes especially nice when considering that P_PIDFD goes on top of that: switch (which) { case P_ALL: type = PIDTYPE_MAX; break; case P_PID: type = PIDTYPE_PID; if (upid <= 0) return -EINVAL; pid = find_get_pid(upid); break; case P_PGID: type = PIDTYPE_PGID; if (upid < 0) return -EINVAL; pid = find_get_pgrp(upid); break; case P_PIDFD: type = PIDTYPE_PID; if (upid < 0) return -EINVAL; pid = pidfd_get_pid(upid); if (IS_ERR(pid)) return PTR_ERR(pid); break; default: return -EINVAL; } Christian
On 08/14, Christian Brauner wrote: > > On Wed, Aug 14, 2019 at 04:19:57PM +0200, Oleg Nesterov wrote: > > On 08/14, Christian Brauner wrote: > > > > > > +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; > > > +} > > > > I can't say I like this helper... even its name doesn't look good to me. > > Well, naming scheme obviously stolen from find_get_pid(). Not sure if > that doesn't look good as well. ;) find_get_pid() actually tries to find a pid. The helper above does "find" or "use current" depending on nr != 0. > > I forgot that we already have get_task_pid() when I replied to the previous > > version... How about > > > > case P_PGID: > > > > if (upid) > > pid = find_get_pid(upid); > > else > > pid = get_task_pid(current, PIDTYPE_PGID); > > > > ? > > Hmyeah, that works but wouldn't it still be nicer to simply have: > > static struct pid *get_pgrp(pid_t nr) > { > if (nr) > return find_get_pid(nr); > > return get_task_pid(current, PIDTYPE_PGID); > } Who else can ever use it? It saves 4 lines in kernel_waitid() but adds 7 lines outside, and you need another ^] to see these lines if you try to understand what PIDTYPE_PGID actually does. IOW, I think this helper will make waitid less readable for no reason. Christian, I try to never argue when it comes to cosmetic issues, and in this case I won't insist too. Oleg.
On Wed, Aug 14, 2019 at 05:27:12PM +0200, Oleg Nesterov wrote: > On 08/14, Christian Brauner wrote: > > > > On Wed, Aug 14, 2019 at 04:19:57PM +0200, Oleg Nesterov wrote: > > > On 08/14, Christian Brauner wrote: > > > > > > > > +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; > > > > +} > > > > > > I can't say I like this helper... even its name doesn't look good to me. > > > > Well, naming scheme obviously stolen from find_get_pid(). Not sure if > > that doesn't look good as well. ;) > > find_get_pid() actually tries to find a pid. The helper above does "find" > or "use current" depending on nr != 0. > > > > I forgot that we already have get_task_pid() when I replied to the previous > > > version... How about > > > > > > case P_PGID: > > > > > > if (upid) > > > pid = find_get_pid(upid); > > > else > > > pid = get_task_pid(current, PIDTYPE_PGID); > > > > > > ? > > > > Hmyeah, that works but wouldn't it still be nicer to simply have: > > > > static struct pid *get_pgrp(pid_t nr) > > { > > if (nr) > > return find_get_pid(nr); > > > > return get_task_pid(current, PIDTYPE_PGID); > > } > > Who else can ever use it? > > It saves 4 lines in kernel_waitid() but adds 7 lines outside, and you > need another ^] to see these lines if you try to understand what > PIDTYPE_PGID actually does. IOW, I think this helper will make waitid > less readable for no reason. > > > Christian, I try to never argue when it comes to cosmetic issues, and > in this case I won't insist too. Yeah, I know. I'm not insisisting either. We can do your thing since you do after all seem to care at least a tiny bit. ;) Christian
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;