[PATCH] bug in setpgid()? process groups and thread groups

[PATCH] bug in setpgid()? process groups and thread groups

[Jeremy Fitzhardinge]

Hi,

I think there's a bug in setpgid().  At present, it only allows the
thread group leader to change process groups, but it doesn't change the
other threads in the thread group to the new process group.

The result is that if a thread group leader changes process groups, all
the other threads are left in the old group - and they can't switch
groups for themselves.

Assuming that all the threads in a thread groups should also be in the
same process group, I think the correct action is for sys_setpgid to
also switch the thread's process group.

Since it seems that the non-leader threads in the thread group are not
attached to the process group, all that needs to be done is for their
pgrp fields to be updated.  Patch against 2.6.0-test2-mm2 attached.

(Why does this matter?  I'm trying to do terminal I/O in threads in a
job control environment.  No, thanks, I'm perfectly sane.)

	J

 kernel/sys.c |   11 +++++++++++
 1 files changed, 11 insertions(+)

diff -puN kernel/sys.c~thread-pgrp kernel/sys.c
--- local-2.6/kernel/sys.c~thread-pgrp	2003-08-02 00:33:06.340860431 -0700
+++ local-2.6-jeremy/kernel/sys.c	2003-08-02 00:38:13.929221706 -0700
@@ -987,6 +987,18 @@ ok_pgid:
 		p->pgrp = pgid;
 		attach_pid(p, PIDTYPE_PGID, pgid);
 	}
+
+	{
+		/* update all threads in thread group 
+		   to new process group */
+		struct task_struct *p;
+		struct pid *pidp;
+		struct list_head *l;
+
+		for_each_task_pid(pid, PIDTYPE_TGID, p, l, pidp)
+			p->pgrp = pgid;
+	}
+
 	err = 0;
 out:
 	/* All paths lead to here, thus we are safe. -DaveM */

_

Re: [PATCH] bug in setpgid()? process groups and thread groups

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeremy Fitzhardinge wrote:

> I think there's a bug in setpgid().  At present, it only allows the
> thread group leader to change process groups, but it doesn't change the
> other threads in the thread group to the new process group.

The PGID is not the only value which is handled incorrectly like this.
The PID, GID, etc all need to be treated similarly.

Your approach with iterating over the threads is not acceptable (at
least to me).  It is racy (concurrent runs are not synchronized) and has
a non-constant time.  We've sketched out already a mechanism which
solves to problem.  Basically, most of the time the value from the
thread group leader is used (just follow the pointer).  Then setting the
value is an atomic operation an constant.

The problem is that Linus already said it is too late for this for 2.6.
 So we are waiting for a signal that the time is right.  The changes
will be substantial since all the different IDs should be covered.

- -- 
- --------------.                        ,-.            444 Castro Street
Ulrich Drepper \    ,-----------------'   \ Mountain View, CA 94041 USA
Red Hat         `--' drepper at redhat.com `---------------------------
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)

iD8DBQE/K3Q12ijCOnn/RHQRAonMAJ9L5s/ZH682oq3/gT6OfNcC+V+QTACdFESs
bwhXkcCynmDdtszLiE5OZn8=
=fyzs
-----END PGP SIGNATURE-----

Re: [PATCH] bug in setpgid()? process groups and thread groups

[Jeremy Fitzhardinge]

On Sat, 2003-08-02 at 01:20, Ulrich Drepper wrote:
> Your approach with iterating over the threads is not acceptable (at
> least to me). 

I've noticed that postings with patches get paid a lot more attention to
those without.  It was just the patch I used to prove to myself what the
problem is.

> It is racy (concurrent runs are not synchronized) 

Erm, it's protected by tasklist_lock, so that isn't a problem.

> and has
> a non-constant time.  We've sketched out already a mechanism which
> solves to problem.  Basically, most of the time the value from the
> thread group leader is used (just follow the pointer).  Then setting the
> value is an atomic operation an constant.

I was considering that, but it needs changes all over the place where
people look at current->pgrp.  This way makes for a clearer patch.

	J

Re: [PATCH] bug in setpgid()? process groups and thread groups

[Roland McGrath]

The problem exists with uids/gids as well, in the sense that they are
changed per-thread but POSIX semantics are that setuid et al affect the
whole process (i.e. all threads in a thread group).  I emphatically agree
that this should be changed, and I hope we can get it done in 2.6.  It's a
significant divergence from POSIX semantics, and not something that can be
worked around very robustly at user level.  (In the case of pgrp, you can
ignore SIGTTIN/SIGTTOU and progress to some degree at user level.  In the
case of uids/gids, you can change every thread individually.)

Changing each thread in the group seems clunky to me.  It also might have
atomicity issues, as there is no synchronization with the reading of these
values.  For job control changes, it probably doesn't matter--each thread
went into its read/write/ioctl or whatever call "before" the setpgid call
if you didn't get to it yet in the loop when it checks current->pgrp; I
can't off hand think of a scenario where two threads can perceivably be on
the opposite sides of the setpgid transition at the same time so as to call
the process-wide transition not atomic.  In the case of uid et al, there is
certainly a problem with the nonatomicity of one thread touching the fields
of another thread that might be running.  The set*id calls change multiple
fields at once, and the intermediate states in between these several word
stores could perhaps be combinations of ids that the user wasn't supposed
to be able to produce.  I am hesitant to hunt down all the permutations and
ways they can be used by another racing thread that might be exploited for
something or other.

It seems obvious to me that these fields should live in a separate data
structure that is shared, like signal_struct and other pieces of
"process-wide" state shared by the threads in a thread group.  This means a
one time swell foop of changing ->{pgrp,uid,euid,...} into ->ids->... or
perhaps task_...(current) macros in case the implementation might change
again.  That's trivial enough to do by just compiling everything and fixing
errors (given ability to compile on a reasonably wide set of platforms).
Making access appear atomic with respect to updates takes a bit more work,
but certainly less than if these fields are not shared among threads.



Thanks,
Roland

Re: [PATCH] bug in setpgid()? process groups and thread groups

[Nicholas Miell]

On Sat, 2003-08-02 at 12:08, Roland McGrath wrote:
> The problem exists with uids/gids as well, in the sense that they are
> changed per-thread but POSIX semantics are that setuid et al affect the
> whole process (i.e. all threads in a thread group).

Is there any particular reason why the POSIX semantics are desirable
(besides "that's the way POSIX says it should be")?

Personally, I can think of no benenfit to per-process uids/gids, and
several scenarios where per-thread uids/gids would be good. (Think of a
multi-threaded server handling connections from N different users on N
threads, or a 1 thread per CPU server handling many different user
connections, or a multi-threaded web server running perl/php/etc. stuff
as different users in different threads.)

Just wondering, Nicholas.

Re: [PATCH] bug in setpgid()? process groups and thread groups

[Alan Cox]

On Sad, 2003-08-02 at 20:08, Roland McGrath wrote:
> The problem exists with uids/gids as well, in the sense that they are
> changed per-thread but POSIX semantics are that setuid et al affect the
> whole process (i.e. all threads in a thread group).  I emphatically agree
> that this should be changed, and I hope we can get it done in 2.6. 

There are two reasons the uid/gid stuff can't change.

#1 Lots of non posix afflicted intelligent programmers use the per
thread uid stuff in daemons. Its really really useful

#2 Linux fundamentally assumes your security credentials don't change
mid syscall except in the specific calls we intend to.

#2 is not sanely soluble for 2.6, and of questionable value anyway, #1
is a very good reason for making libc fix up this obscure posix
stupidity itself.

Lets face it how many posix pthreads app have -performance critical
setuid/getid calls ?

Re: [PATCH] bug in setpgid()? process groups and thread groups

[Florian Weimer]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> #1 Lots of non posix afflicted intelligent programmers use the per
> thread uid stuff in daemons. Its really really useful

It doesn't work reliably because the threading implementation might
have to send signals which the current combination of credentials does
not allow.

IMHO, POSIX is wrong to favor process attributes so strongly.  It
wouldn't be a problem if there were other ways to pass these implicit
parameters (such as thread-specific attributes, or, even better,
syscall arguments).  But often there isn't.

Re: [PATCH] bug in setpgid()? process groups and thread groups

[Alan Cox]

On Sul, 2003-08-03 at 08:22, Florian Weimer wrote:
> Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
> 
> > #1 Lots of non posix afflicted intelligent programmers use the per
> > thread uid stuff in daemons. Its really really useful
> 
> It doesn't work reliably because the threading implementation might
> have to send signals which the current combination of credentials does
> not allow.

It works beautifully.  Its very effective for clone() using applications


> IMHO, POSIX is wrong to favor process attributes so strongly.  It
> wouldn't be a problem if there were other ways to pass these implicit
> parameters (such as thread-specific attributes, or, even better,
> syscall arguments).  But often there isn't.

The restriction in this case comes from a more fundamental thing - posix pthreads
is full of compromises so it can be done in user space. Clearly you can't split
uid's in user space very sanely.

Re: [PATCH] bug in setpgid()? process groups and thread groups

[Jeremy Fitzhardinge]

On Sat, 2003-08-02 at 12:08, Roland McGrath wrote:
> In the case of pgrp, you can
> ignore SIGTTIN/SIGTTOU and progress to some degree at user level.  In the
> case of uids/gids, you can change every thread individually.)

In my case I'm getting read() returning EIO when it tries to read from
the terminal, because the thread pgid doesn't equal the terminal
foreground pgid.  I could solve this problem if either setpgid sets the
pgid in all threads in the thread group, or if I could run setpgid in
all threads for myself (I would prefer the latter just because it is a
bit more flexible).  The current situation is just annoyingly broken
because there's no user-space way to fix it.

	J

Re: [PATCH] bug in setpgid()? process groups and thread groups

[William Lee Irwin III]

On Sat, Aug 02, 2003 at 01:20:05AM -0700, Ulrich Drepper wrote:
> The problem is that Linus already said it is too late for this for 2.6.
>  So we are waiting for a signal that the time is right.  The changes
> will be substantial since all the different IDs should be covered.

I think callers can be converted incrementally, so long as the
transition is (unfortunately) slow. I can help devise a patch breakup
plan or do other footwork (e.g. code) if you want or need helpers.


-- wli