2005-12-11 16:07:28

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH ? 2/2] setpgid: should work for sub-threads

setpgid(0, pgid) or setpgid(forked_child_pid, pgid) does not work
unless the calling process is a thread_group_leader().

'man setpgid' does not tell anything about that, so I consider
this behaviour is a bug.

Signed-off-by: Oleg Nesterov <[email protected]>

--- 2.6.15-rc5/kernel/sys.c~ 2005-12-11 22:40:33.000000000 +0300
+++ 2.6.15-rc5/kernel/sys.c 2005-12-11 22:50:39.000000000 +0300
@@ -1083,10 +1083,11 @@ asmlinkage long sys_times(struct tms __u
asmlinkage long sys_setpgid(pid_t pid, pid_t pgid)
{
struct task_struct *p;
+ struct task_struct *group_leader = current->group_leader;
int err = -EINVAL;

if (!pid)
- pid = current->pid;
+ pid = group_leader->pid;
if (!pgid)
pgid = pid;
if (pgid < 0)
@@ -1106,16 +1107,16 @@ asmlinkage long sys_setpgid(pid_t pid, p
if (!thread_group_leader(p))
goto out;

- if (p->real_parent == current) {
+ if (p->real_parent == group_leader) {
err = -EPERM;
- if (p->signal->session != current->signal->session)
+ if (p->signal->session != group_leader->signal->session)
goto out;
err = -EACCES;
if (p->did_exec)
goto out;
} else {
err = -ESRCH;
- if (p != current)
+ if (p != group_leader)
goto out;
}

@@ -1127,7 +1128,7 @@ asmlinkage long sys_setpgid(pid_t pid, p
struct task_struct *p;

do_each_task_pid(pgid, PIDTYPE_PGID, p) {
- if (p->signal->session == current->signal->session)
+ if (p->signal->session == group_leader->signal->session)
goto ok_pgid;
} while_each_task_pid(pgid, PIDTYPE_PGID, p);
goto out;


2005-12-11 21:11:51

by Oren Laadan

[permalink] [raw]
Subject: Re: [PATCH ? 2/2] setpgid: should work for sub-threads


[PATCH 1/1] setsid: should work for sub-threads

setsid() does not work unless the calling process is a
thread_group_leader().

'man setpgid' does not tell anything about that, so I consider
this behaviour is a bug.

Signed-off-by: Oren Laadan <[email protected]>

---

(see similar patch for setpgid by Oleg Nesterov). This one also required
modifying kernel/exit.c:__set_special_pids() (and it's prototype in
sched.h).

Please review...

diff --git a/include/linux/sched.h b/include/linux/sched.h
index b0ad6f3..9ff6e79 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1003,7 +1003,7 @@ extern struct mm_struct init_mm;
#define find_task_by_pid(nr) find_task_by_pid_type(PIDTYPE_PID, nr)
extern struct task_struct *find_task_by_pid_type(int type, int pid);
extern void set_special_pids(pid_t session, pid_t pgrp);
-extern void __set_special_pids(pid_t session, pid_t pgrp);
+extern void __set_special_pids(struct task_struct *tsk, pid_t session, pid_t pgrp);

/* per-UID process charging. */
extern struct user_struct * alloc_uid(uid_t);
diff --git a/kernel/exit.c b/kernel/exit.c
index ee51568..d47931f 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -256,26 +256,24 @@ static inline void reparent_to_init(void
switch_uid(INIT_USER);
}

-void __set_special_pids(pid_t session, pid_t pgrp)
+void __set_special_pids(struct task_struct *tsk, pid_t session, pid_t pgrp)
{
- struct task_struct *curr = current;
-
- if (curr->signal->session != session) {
- detach_pid(curr, PIDTYPE_SID);
- curr->signal->session = session;
- attach_pid(curr, PIDTYPE_SID, session);
- }
- if (process_group(curr) != pgrp) {
- detach_pid(curr, PIDTYPE_PGID);
- curr->signal->pgrp = pgrp;
- attach_pid(curr, PIDTYPE_PGID, pgrp);
+ if (tsk->signal->session != session) {
+ detach_pid(tsk, PIDTYPE_SID);
+ tsk->signal->session = session;
+ attach_pid(tsk, PIDTYPE_SID, session);
+ }
+ if (process_group(tsk) != pgrp) {
+ detach_pid(tsk, PIDTYPE_PGID);
+ tsk->signal->pgrp = pgrp;
+ attach_pid(tsk, PIDTYPE_PGID, pgrp);
}
}

void set_special_pids(pid_t session, pid_t pgrp)
{
write_lock_irq(&tasklist_lock);
- __set_special_pids(session, pgrp);
+ __set_special_pids(current, session, pgrp);
write_unlock_irq(&tasklist_lock);
}

diff --git a/kernel/sys.c b/kernel/sys.c
index bce933e..c907f88 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1207,24 +1207,22 @@ asmlinkage long sys_getsid(pid_t pid)

asmlinkage long sys_setsid(void)
{
+ struct task_struct *leader = current->group_leader;
struct pid *pid;
int err = -EPERM;

- if (!thread_group_leader(current))
- return -EINVAL;
-
down(&tty_sem);
write_lock_irq(&tasklist_lock);

- pid = find_pid(PIDTYPE_PGID, current->pid);
+ pid = find_pid(PIDTYPE_PGID, leader->pid);
if (pid)
goto out;

- current->signal->leader = 1;
- __set_special_pids(current->pid, current->pid);
- current->signal->tty = NULL;
- current->signal->tty_old_pgrp = 0;
- err = process_group(current);
+ leader->signal->leader = 1;
+ __set_special_pids(leader, leader->pid, leader->pid);
+ leader->signal->tty = NULL;
+ leader->signal->tty_old_pgrp = 0;
+ err = process_group(leader);
out:
write_unlock_irq(&tasklist_lock);
up(&tty_sem);



2005-12-12 12:03:40

by Oleg Nesterov

[permalink] [raw]
Subject: Re: [PATCH ? 2/2] setpgid: should work for sub-threads

Oren Laadan wrote:
>
> -void __set_special_pids(pid_t session, pid_t pgrp)
> +void __set_special_pids(struct task_struct *tsk, pid_t session, pid_t pgrp)
> {
> - struct task_struct *curr = current;

Do we need to add the new paramater? I think we can just do:

- struct task_struct *curr = current;
+ struct task_struct *curr = current->group_leader;

and that's all. This function is called from daemonize() also,
but the kernel thread is a thread group leader always.

Oleg.