2007-11-25 14:44:17

by Oleg Nesterov

Subject: [PATCH] sys_setpgid: simplify pid/ns interaction

sys_setpgid() does unneeded conversions from pid_t to "struct pid" and vice
versa. Use "struct pid" more consistently. Saves one find_vpid() and eliminates
the explicit usage of ->nsproxy->pid_ns. Imho, cleanups the code.

Also use the same_thread_group() helper.

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

--- PT/kernel/sys.c~ 2007-11-20 17:16:10.000000000 +0300
+++ PT/kernel/sys.c 2007-11-25 17:16:59.000000000 +0300
@@ -916,8 +916,8 @@ asmlinkage long sys_setpgid(pid_t pid, p
{
struct task_struct *p;
struct task_struct *group_leader = current->group_leader;
- int err = -EINVAL;
- struct pid_namespace *ns;
+ struct pid *pgrp;
+ int err;

if (!pid)
pid = task_pid_vnr(group_leader);
@@ -929,12 +929,10 @@ asmlinkage long sys_setpgid(pid_t pid, p
/* From this point forward we keep holding onto the tasklist lock
* so that our parent does not change from under us. -DaveM
*/
- ns = current->nsproxy->pid_ns;
-
write_lock_irq(&tasklist_lock);

err = -ESRCH;
- p = find_task_by_pid_ns(pid, ns);
+ p = find_task_by_vpid(pid);
if (!p)
goto out;

@@ -942,7 +940,7 @@ asmlinkage long sys_setpgid(pid_t pid, p
if (!thread_group_leader(p))
goto out;

- if (p->real_parent->tgid == group_leader->tgid) {
+ if (same_thread_group(p->real_parent, group_leader)) {
err = -EPERM;
if (task_session(p) != task_session(group_leader))
goto out;
@@ -959,10 +957,12 @@ asmlinkage long sys_setpgid(pid_t pid, p
if (p->signal->leader)
goto out;

+ pgrp = task_pid(p);
if (pgid != pid) {
struct task_struct *g;

- g = find_task_by_pid_type_ns(PIDTYPE_PGID, pgid, ns);
+ pgrp = find_vpid(pgid);
+ g = pid_task(pgrp, PIDTYPE_PGID);
if (!g || task_session(g) != task_session(group_leader))
goto out;
}
@@ -971,13 +971,10 @@ asmlinkage long sys_setpgid(pid_t pid, p
if (err)
goto out;

- if (task_pgrp_nr_ns(p, ns) != pgid) {
- struct pid *pid;
-
+ if (task_pgrp(p) != pgrp) {
detach_pid(p, PIDTYPE_PGID);
- pid = find_vpid(pgid);
- attach_pid(p, PIDTYPE_PGID, pid);
- set_task_pgrp(p, pid_nr(pid));
+ attach_pid(p, PIDTYPE_PGID, pgrp);
+ set_task_pgrp(p, pid_nr(pgrp));
}

err = 0;

2007-11-26 08:56:46

by Pavel Emelyanov

Subject: Re: [PATCH] sys_setpgid: simplify pid/ns interaction

Oleg Nesterov wrote:
> sys_setpgid() does unneeded conversions from pid_t to "struct pid" and vice
> versa. Use "struct pid" more consistently. Saves one find_vpid() and eliminates
> the explicit usage of ->nsproxy->pid_ns. Imho, cleanups the code.
>
> Also use the same_thread_group() helper.
>
> Signed-off-by: Oleg Nesterov <[email protected]>

Looks good :)

Acked-by: Pavel Emelyanov <[email protected]>

> --- PT/kernel/sys.c~ 2007-11-20 17:16:10.000000000 +0300
> +++ PT/kernel/sys.c 2007-11-25 17:16:59.000000000 +0300
> @@ -916,8 +916,8 @@ asmlinkage long sys_setpgid(pid_t pid, p
> {
> struct task_struct *p;
> struct task_struct *group_leader = current->group_leader;
> - int err = -EINVAL;
> - struct pid_namespace *ns;
> + struct pid *pgrp;
> + int err;
>
> if (!pid)
> pid = task_pid_vnr(group_leader);
> @@ -929,12 +929,10 @@ asmlinkage long sys_setpgid(pid_t pid, p
> /* From this point forward we keep holding onto the tasklist lock
> * so that our parent does not change from under us. -DaveM
> */
> - ns = current->nsproxy->pid_ns;
> -
> write_lock_irq(&tasklist_lock);
>
> err = -ESRCH;
> - p = find_task_by_pid_ns(pid, ns);
> + p = find_task_by_vpid(pid);
> if (!p)
> goto out;
>
> @@ -942,7 +940,7 @@ asmlinkage long sys_setpgid(pid_t pid, p
> if (!thread_group_leader(p))
> goto out;
>
> - if (p->real_parent->tgid == group_leader->tgid) {
> + if (same_thread_group(p->real_parent, group_leader)) {
> err = -EPERM;
> if (task_session(p) != task_session(group_leader))
> goto out;
> @@ -959,10 +957,12 @@ asmlinkage long sys_setpgid(pid_t pid, p
> if (p->signal->leader)
> goto out;
>
> + pgrp = task_pid(p);
> if (pgid != pid) {
> struct task_struct *g;
>
> - g = find_task_by_pid_type_ns(PIDTYPE_PGID, pgid, ns);
> + pgrp = find_vpid(pgid);
> + g = pid_task(pgrp, PIDTYPE_PGID);
> if (!g || task_session(g) != task_session(group_leader))
> goto out;
> }
> @@ -971,13 +971,10 @@ asmlinkage long sys_setpgid(pid_t pid, p
> if (err)
> goto out;
>
> - if (task_pgrp_nr_ns(p, ns) != pgid) {
> - struct pid *pid;
> -
> + if (task_pgrp(p) != pgrp) {
> detach_pid(p, PIDTYPE_PGID);
> - pid = find_vpid(pgid);
> - attach_pid(p, PIDTYPE_PGID, pid);
> - set_task_pgrp(p, pid_nr(pid));
> + attach_pid(p, PIDTYPE_PGID, pgrp);
> + set_task_pgrp(p, pid_nr(pgrp));
> }
>
> err = 0;
>
>