Re: [PATCH v1 1/1] waitid: Add support for waiting for the current process group

2019-08-14 Thread Christian Brauner
On Wed, Aug 14, 2019 at 02:50:12PM +0200, Oleg Nesterov wrote:
> On 08/14, Christian Brauner wrote:
> >
> > On Wed, Aug 14, 2019 at 02:29:10PM +0200, Oleg Nesterov wrote:
> > > On 08/14, christian.brau...@ubuntu.com wrote:
> > > >
> > > > case P_PGID:
> > > > type = PIDTYPE_PGID;
> > > > -   if (upid <= 0)
> > > > +   if (upid < 0)
> > > > return -EINVAL;
> > > > +
> > > > +   if (upid == 0)
> > > > +   pid = get_pid(task_pgrp(current));
> > >
> > > this needs rcu lock or tasklist_lock, this can race with another thread
> > > doing sys_setpgid/setsid (see change_pid(PIDTYPE_PGID)).
> >
> > Oh, I naively assumed task_pgrp() would take an rcu lock...
> 
> but it would not help ;)

Yeah, it doesn't do a get. :)


Re: [PATCH v1 1/1] waitid: Add support for waiting for the current process group

2019-08-14 Thread Oleg Nesterov
On 08/14, Christian Brauner wrote:
>
> On Wed, Aug 14, 2019 at 02:29:10PM +0200, Oleg Nesterov wrote:
> > On 08/14, christian.brau...@ubuntu.com wrote:
> > >
> > >   case P_PGID:
> > >   type = PIDTYPE_PGID;
> > > - if (upid <= 0)
> > > + if (upid < 0)
> > >   return -EINVAL;
> > > +
> > > + if (upid == 0)
> > > + pid = get_pid(task_pgrp(current));
> >
> > this needs rcu lock or tasklist_lock, this can race with another thread
> > doing sys_setpgid/setsid (see change_pid(PIDTYPE_PGID)).
>
> Oh, I naively assumed task_pgrp() would take an rcu lock...

but it would not help ;)

> though I think we should be fine with just rcu_read_lock().

Yes,

Oleg.



Re: [PATCH v1 1/1] waitid: Add support for waiting for the current process group

2019-08-14 Thread Christian Brauner
On Wed, Aug 14, 2019 at 02:29:10PM +0200, Oleg Nesterov wrote:
> On 08/14, christian.brau...@ubuntu.com wrote:
> >
> > case P_PGID:
> > type = PIDTYPE_PGID;
> > -   if (upid <= 0)
> > +   if (upid < 0)
> > return -EINVAL;
> > +
> > +   if (upid == 0)
> > +   pid = get_pid(task_pgrp(current));
> 
> this needs rcu lock or tasklist_lock, this can race with another thread
> doing sys_setpgid/setsid (see change_pid(PIDTYPE_PGID)).

Oh, I naively assumed task_pgrp() would take an rcu lock...

kernel/sys.c takes both, i.e.

rcu_read_lock();
write_lock_irq(_lock);

though I think we should be fine with just rcu_read_lock(). setpgid()
indicates that it wants to check real_parent and needs the
write_lock_irq() because it might change behind its back which we don't
care about since we're not changing the pgrp.

Christian


Re: [PATCH v1 1/1] waitid: Add support for waiting for the current process group

2019-08-14 Thread Oleg Nesterov
On 08/14, christian.brau...@ubuntu.com wrote:
>
>   case P_PGID:
>   type = PIDTYPE_PGID;
> - if (upid <= 0)
> + if (upid < 0)
>   return -EINVAL;
> +
> + if (upid == 0)
> + pid = get_pid(task_pgrp(current));

this needs rcu lock or tasklist_lock, this can race with another thread
doing sys_setpgid/setsid (see change_pid(PIDTYPE_PGID)).

Oleg.



[PATCH v1 1/1] waitid: Add support for waiting for the current process group

2019-08-14 Thread christian . brauner
From: "Eric W. Biederman" 

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" 
Signed-off-by: Christian Brauner 
Cc: "H. Peter Anvin" 
Cc: Arnd Bergmann 
Cc: Palmer Dabbelt 
Cc: Rich Felker 
Cc: Alistair Francis 
Cc: Zong Li 
Cc: Andrew Morton 
Cc: Oleg Nesterov 
Cc: Linus Torvalds 
Cc: Al Viro 
Cc: Florian Weimer 
Cc: Adhemerval Zanella 
Cc: GNU C Library 
---
v1:
- Christian Brauner :
  - 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
---
 kernel/exit.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 5b4a5dcce8f8..e70083b14f31 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1576,19 +1576,23 @@ 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;
+
+   if (upid == 0)
+   pid = get_pid(task_pgrp(current));
+   else
+   pid = find_get_pid(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