pid: improved namespaced iteration over processes list (v2)

pid: improved namespaced iteration over processes list (v2)

[Gowrishankar M]

Earlier version of this patch, attempted to change general macros 
for_each_process and do_each_thread which is not safe as suggested
by most of reviewers.

Hence, I go with adding renamed versions of them, so that we can use
them only whereever needed.

I will include code optimisation that Dave suggested if this patch 
gets approved mostly.

Thanks.
Gowrishankar

[PATCH 1/5] pid: add new iterative macros to list processes in a namespace

[Gowrishankar M]

From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Below patch addresses a common solution for any place where a process
should be checked if it is associated to caller namespace. At present,
we use 'task_pid_vnr(t) > 0' to further proceed with task 't' in current
namespace.

To avoid applying this check in every userspace code related to PID namespace,
this patch provides two new iterative macros;for_each_process_in_ns and
do_each_thread_in_ns.

Signed-off-by: Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 include/linux/pid.h   |    1 +
 include/linux/sched.h |   12 ++++++++++++
 kernel/pid.c          |   17 +++++++++++++++++
 3 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/include/linux/pid.h b/include/linux/pid.h
index d7e98ff..1c6b24f 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -117,6 +117,7 @@ extern struct pid *find_vpid(int nr);
  */
 extern struct pid *find_get_pid(int nr);
 extern struct pid *find_ge_pid(int nr, struct pid_namespace *);
+extern struct pid *find_ge_tgid(int nr, struct pid_namespace *);
 int next_pidmap(struct pid_namespace *pid_ns, int last);
 
 extern struct pid *alloc_pid(struct pid_namespace *ns);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2e46189..bdd9f0d 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1919,16 +1919,28 @@ static inline unsigned long wait_task_inactive(struct task_struct *p,
 
 #define next_task(p)	list_entry(rcu_dereference((p)->tasks.next), struct task_struct, tasks)
 
+#define next_task_in_ns(p, ns) \
+	pid_task(find_ge_tgid(task_pid_nr_ns(p, ns) + 1, ns), PIDTYPE_PID)
+
 #define for_each_process(p) \
 	for (p = &init_task ; (p = next_task(p)) != &init_task ; )
 
+#define for_each_process_in_ns(p, ns) \
+	for (p = find_task_by_pid_ns(1, ns) ; p != NULL ; \
+	     p = next_task_in_ns(p, ns))
+
 /*
  * Careful: do_each_thread/while_each_thread is a double loop so
  *          'break' will not work as expected - use goto instead.
+ *          same applies to do_each_thread_in_ns.
  */
 #define do_each_thread(g, t) \
 	for (g = t = &init_task ; (g = t = next_task(g)) != &init_task ; ) do
 
+#define do_each_thread_in_ns(g, t, ns) \
+	for (g = t = find_task_by_pid_ns(1, ns) ; g != NULL ; \
+	    (g = t = next_task_in_ns(g, ns))) do
+
 #define while_each_thread(g, t) \
 	while ((t = next_thread(t)) != g)
 
diff --git a/kernel/pid.c b/kernel/pid.c
index 064e76a..15f707a 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -493,6 +493,23 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
 	return pid;
 }
 
+struct pid *find_ge_tgid(int nr,  struct pid_namespace *ns)
+{
+	struct pid *pid;
+	struct task_struct *task;
+
+retry:
+	pid = find_ge_pid(nr, ns);
+	if (pid) {
+		task = pid_task(pid, PIDTYPE_PID);
+		if (!task || !has_group_leader_pid(task)) {
+			nr += 1;
+			goto retry;
+		}
+	}
+	return pid;
+}
+
 /*
  * The pid hash table is scaled according to the amount of memory in the
  * machine.  From a minimum of 16 slots up to 4096 slots at one gigabyte or
-- 
1.5.5.1

[PATCH 2/5] pid: use namespaced iteration on processes while using sysrq

[Gowrishankar M]

From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

At present, while signalling processes using sysrq, process iteration
goes beyond current PID namespace.

Below patch uses one of the proposed namespace iteration macros to fix
the boundary.

Signed-off-by: Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/sysrq.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/char/sysrq.c b/drivers/char/sysrq.c
index ce0d9da..adca5da 100644
--- a/drivers/char/sysrq.c
+++ b/drivers/char/sysrq.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/sched.h>
+#include <linux/pid_namespace.h>
 #include <linux/interrupt.h>
 #include <linux/mm.h>
 #include <linux/fs.h>
@@ -293,8 +294,8 @@ static void send_sig_all(int sig)
 {
 	struct task_struct *p;
 
-	for_each_process(p) {
-		if (p->mm && !is_global_init(p))
+	for_each_process_in_ns(p, current->nsproxy->pid_ns) {
+		if (p->mm && !is_container_init(p))
 			/* Not swapper, init nor kernel thread */
 			force_sig(sig, p);
 	}
-- 
1.5.5.1

[PATCH 3/5] pid: use namespaced iteration on processes while setting capability

[Gowrishankar M]

From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

In piece of dead code, cap_set_all() propogates through processes outside
PID namespace, as iteration is always in init PID namespace.

Below patch adjusts macro controller to use do_each_thread_in_ns() so that
only processes in current namespace are scanned

Signed-off-by: Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 kernel/capability.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/capability.c b/kernel/capability.c
index 33e51e7..e3e3765 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -201,7 +201,7 @@ static inline int cap_set_all(kernel_cap_t *effective,
 	spin_lock(&task_capability_lock);
 	read_lock(&tasklist_lock);
 
-	do_each_thread(g, target) {
+	do_each_thread_in_ns(g, target, current->nsproxy->pid_ns) {
 		if (target == current
 		    || is_container_init(target->group_leader))
 			continue;
-- 
1.5.5.1

[PATCH 4/5] pid: use namespaced iteration on processes while sending signal to all

[Gowrishankar M]

From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

At present we scan all processes in init namespace, whether in new namespace
or not, to send signal to all processes for container. Also we filter out
processes belonging to same namespace using task_pid_vnr().

Below patch proposes to use new macro controller to save time using pidmap.
In init namespace, this saving can be more or less achieved, as we check to
take every process with task_pid_vnr() otherwise.

Signed-off-by: Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 kernel/signal.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 4530fc6..a2651bc 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1143,9 +1143,8 @@ static int kill_something_info(int sig, struct siginfo *info, pid_t pid)
 		int retval = 0, count = 0;
 		struct task_struct * p;
 
-		for_each_process(p) {
-			if (task_pid_vnr(p) > 1 &&
-					!same_thread_group(p, current)) {
+		for_each_process_in_ns(p, current->nsproxy->pid_ns) {
+			if (!same_thread_group(p, current)) {
 				int err = group_send_sig_info(sig, info, p);
 				++count;
 				if (err != -EPERM)
-- 
1.5.5.1

[PATCH 5/5] pid: use namespaced iteration on processes while managing priority

[Gowrishankar M]

From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

At present we scan all processes in init namespace, while getting or setting
process priorities for a user. Incase of PID namespace, it leads to leak
priority to processes in other namespace.

Below patch proposes to use new macro controller to fix the boundary of
processes list in current namespace.

Signed-off-by: Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 kernel/sys.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 31deba8..50973de 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -181,7 +181,7 @@ asmlinkage long sys_setpriority(int which, int who, int niceval)
 				if ((who != current->uid) && !(user = find_user(who)))
 					goto out_unlock;	/* No processes for this user */
 
-			do_each_thread(g, p)
+			do_each_thread_in_ns(g, p, current->nsproxy->pid_ns)
 				if (p->uid == who)
 					error = set_one_prio(p, niceval, error);
 			while_each_thread(g, p);
@@ -243,7 +243,7 @@ asmlinkage long sys_getpriority(int which, int who)
 				if ((who != current->uid) && !(user = find_user(who)))
 					goto out_unlock;	/* No processes for this user */
 
-			do_each_thread(g, p)
+			do_each_thread_in_ns(g, p, current->nsproxy->pid_ns)
 				if (p->uid == who) {
 					niceval = 20 - task_nice(p);
 					if (niceval > retval)
-- 
1.5.5.1

Re: [PATCH 2/5] pid: use namespaced iteration on processes while using sysrq

[Dave Hansen]

On Thu, 2008-12-18 at 22:12 +0530, Gowrishankar M wrote:
> From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
> At present, while signalling processes using sysrq, process iteration
> goes beyond current PID namespace.
> 
> Below patch uses one of the proposed namespace iteration macros to fix
> the boundary.

I think this is a really bad idea.  Maybe not for 'echo t
> /proc/sysrq-trigger', but definitely for the console triggers.

The sysrq code can get called via the serial console code.  That may
even get processed during softirqs so the current process may not have
anything to do with what the user is typing into.  That's just one
example.  I bet there are a bunch more.

What problem does this patch solve anyway?

-- Dave

Re: [PATCH 3/5] pid: use namespaced iteration on processes while setting capability

[Serge E. Hallyn]

Quoting Gowrishankar M (gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
> In piece of dead code, cap_set_all() propogates through processes outside
> PID namespace, as iteration is always in init PID namespace.
> 
> Below patch adjusts macro controller to use do_each_thread_in_ns() so that
> only processes in current namespace are scanned
> 
> Signed-off-by: Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

> ---
>  kernel/capability.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 33e51e7..e3e3765 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -201,7 +201,7 @@ static inline int cap_set_all(kernel_cap_t *effective,
>  	spin_lock(&task_capability_lock);
>  	read_lock(&tasklist_lock);
> 
> -	do_each_thread(g, target) {
> +	do_each_thread_in_ns(g, target, current->nsproxy->pid_ns) {
>  		if (target == current
>  		    || is_container_init(target->group_leader))
>  			continue;
> -- 
> 1.5.5.1

Re: [PATCH 4/5] pid: use namespaced iteration on processes while sending signal to all

[Serge E. Hallyn]

Quoting Gowrishankar M (gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
> At present we scan all processes in init namespace, whether in new namespace
> or not, to send signal to all processes for container. Also we filter out
> processes belonging to same namespace using task_pid_vnr().
> 
> Below patch proposes to use new macro controller to save time using pidmap.
> In init namespace, this saving can be more or less achieved, as we check to
> take every process with task_pid_vnr() otherwise.
> 
> Signed-off-by: Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

> ---
>  kernel/signal.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 4530fc6..a2651bc 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1143,9 +1143,8 @@ static int kill_something_info(int sig, struct siginfo *info, pid_t pid)
>  		int retval = 0, count = 0;
>  		struct task_struct * p;
> 
> -		for_each_process(p) {
> -			if (task_pid_vnr(p) > 1 &&
> -					!same_thread_group(p, current)) {
> +		for_each_process_in_ns(p, current->nsproxy->pid_ns) {
> +			if (!same_thread_group(p, current)) {
>  				int err = group_send_sig_info(sig, info, p);
>  				++count;
>  				if (err != -EPERM)
> -- 
> 1.5.5.1

Re: [PATCH 5/5] pid: use namespaced iteration on processes while managing priority

[Serge E. Hallyn]

Quoting Gowrishankar M (gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
> At present we scan all processes in init namespace, while getting or setting
> process priorities for a user. Incase of PID namespace, it leads to leak
> priority to processes in other namespace.
> 
> Below patch proposes to use new macro controller to fix the boundary of
> processes list in current namespace.
> 
> Signed-off-by: Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

> ---
>  kernel/sys.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 31deba8..50973de 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -181,7 +181,7 @@ asmlinkage long sys_setpriority(int which, int who, int niceval)
>  				if ((who != current->uid) && !(user = find_user(who)))
>  					goto out_unlock;	/* No processes for this user */
> 
> -			do_each_thread(g, p)
> +			do_each_thread_in_ns(g, p, current->nsproxy->pid_ns)
>  				if (p->uid == who)
>  					error = set_one_prio(p, niceval, error);
>  			while_each_thread(g, p);
> @@ -243,7 +243,7 @@ asmlinkage long sys_getpriority(int which, int who)
>  				if ((who != current->uid) && !(user = find_user(who)))
>  					goto out_unlock;	/* No processes for this user */
> 
> -			do_each_thread(g, p)
> +			do_each_thread_in_ns(g, p, current->nsproxy->pid_ns)
>  				if (p->uid == who) {
>  					niceval = 20 - task_nice(p);
>  					if (niceval > retval)
> -- 
> 1.5.5.1

Re: [PATCH 4/5] pid: use namespaced iteration on processes while sending signal to all

[Dave Hansen]

On Thu, 2008-12-18 at 22:12 +0530, Gowrishankar M wrote:
> At present we scan all processes in init namespace, whether in new namespace
> or not, to send signal to all processes for container. Also we filter out
> processes belonging to same namespace using task_pid_vnr().
> 
> Below patch proposes to use new macro controller to save time using pidmap.
> In init namespace, this saving can be more or less achieved, as we check to
> take every process with task_pid_vnr() otherwise.

> diff --git a/kernel/signal.c b/kernel/signal.c
> index 4530fc6..a2651bc 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1143,9 +1143,8 @@ static int kill_something_info(int sig, struct siginfo *info, pid_t pid)
>  		int retval = 0, count = 0;
>  		struct task_struct * p;
> 
> -		for_each_process(p) {
> -			if (task_pid_vnr(p) > 1 &&
> -					!same_thread_group(p, current)) {
> +		for_each_process_in_ns(p, current->nsproxy->pid_ns) {
> +			if (!same_thread_group(p, current)) {
>  				int err = group_send_sig_info(sig, info, p);
>  				++count;
>  				if (err != -EPERM)

So this is a performance optimization?

Isn't that task_pid_vnr() basically an is_container_init() check?  Why
did it go away?

This patch implies that ever process in another's thread group is also
in the same pid namespace.  That seems like a sane assumption, but I'd
probably hesitate without Oleg or Eric taking a good look.

-- Dave

Re: [PATCH 2/5] pid: use namespaced iteration on processes while using sysrq

[Serge E. Hallyn]

Quoting Gowrishankar M (gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
> At present, while signalling processes using sysrq, process iteration
> goes beyond current PID namespace.
> 
> Below patch uses one of the proposed namespace iteration macros to fix
> the boundary.
> 
> Signed-off-by: Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Nack, like Dave said.

For hitting alt-sysrq-t on console, you want to hit the whole
system, not just one ns.

For echo t > /sys/sysrq-trigger, you do though.  Fortunately, you
can tell the difference using check_mask in __hande_sysrq().  So
do that, and either always pass that on to op->handler and check it in
send_sig_all(), or just filter on key there and call a custom handler.

-serge

Re: [PATCH 2/5] pid: use namespaced iteration on processes while using sysrq

[Eric W. Biederman]

Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> writes:

> From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>
> At present, while signalling processes using sysrq, process iteration
> goes beyond current PID namespace.
>
> Below patch uses one of the proposed namespace iteration macros to fix
> the boundary.
>
> Signed-off-by: Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

sysrq is a debugging helper and send_sig_all should apply to
ever process in the system.  It should only apply to the initial
pid namespace.

Attempting to make it namespace aware is wrong.


Eric

Re: [PATCH 4/5] pid: use namespaced iteration on processes while sending signal to all

[Serge E. Hallyn]

Quoting Dave Hansen (dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> On Thu, 2008-12-18 at 22:12 +0530, Gowrishankar M wrote:
> > At present we scan all processes in init namespace, whether in new namespace
> > or not, to send signal to all processes for container. Also we filter out
> > processes belonging to same namespace using task_pid_vnr().
> > 
> > Below patch proposes to use new macro controller to save time using pidmap.
> > In init namespace, this saving can be more or less achieved, as we check to
> > take every process with task_pid_vnr() otherwise.
> 
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index 4530fc6..a2651bc 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1143,9 +1143,8 @@ static int kill_something_info(int sig, struct siginfo *info, pid_t pid)
> >  		int retval = 0, count = 0;
> >  		struct task_struct * p;
> > 
> > -		for_each_process(p) {
> > -			if (task_pid_vnr(p) > 1 &&
> > -					!same_thread_group(p, current)) {
> > +		for_each_process_in_ns(p, current->nsproxy->pid_ns) {
> > +			if (!same_thread_group(p, current)) {
> >  				int err = group_send_sig_info(sig, info, p);
> >  				++count;
> >  				if (err != -EPERM)
> 
> So this is a performance optimization?
> 
> Isn't that task_pid_vnr() basically an is_container_init() check?  Why
> did it go away?

Oh, good catch - the container init should still be skipped, no?

> This patch implies that ever process in another's thread group is also
> in the same pid namespace.  That seems like a sane assumption, but I'd
> probably hesitate without Oleg or Eric taking a good look.

copy_pid_ns() enforced that CLONE_THREAD and CLONE_NEWPID cannot
be specified together.

-serge

Re: [PATCH 3/5] pid: use namespaced iteration on processes while setting capability

[Eric W. Biederman]

Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> writes:

> From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>
> In piece of dead code, cap_set_all() propogates through processes outside
> PID namespace, as iteration is always in init PID namespace.
>
> Below patch adjusts macro controller to use do_each_thread_in_ns() so that
> only processes in current namespace are scanned

Yes.  This case in capability.c needs to be fixed.

Acked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>


> Signed-off-by: Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  kernel/capability.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 33e51e7..e3e3765 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -201,7 +201,7 @@ static inline int cap_set_all(kernel_cap_t *effective,
>  	spin_lock(&task_capability_lock);
>  	read_lock(&tasklist_lock);
>  
> -	do_each_thread(g, target) {
> +	do_each_thread_in_ns(g, target, current->nsproxy->pid_ns) {
>  		if (target == current
>  		    || is_container_init(target->group_leader))
>  			continue;
> -- 
> 1.5.5.1

Re: [PATCH 5/5] pid: use namespaced iteration on processes while managing priority

[Eric W. Biederman]

Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> writes:

> From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>
> At present we scan all processes in init namespace, while getting or setting
> process priorities for a user. Incase of PID namespace, it leads to leak
> priority to processes in other namespace.
>
> Below patch proposes to use new macro controller to fix the boundary of
> processes list in current namespace.

Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

This has nothing to do with pids.  The command is to set the
iopriority for a given user.  This is a problem of the user namespace
not the pid namespace.


Eric

Re: [PATCH 1/5] pid: add new iterative macros to list processes in a namespace

[Eric W. Biederman]

Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> writes:

> From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>
> Below patch addresses a common solution for any place where a process
> should be checked if it is associated to caller namespace. At present,
> we use 'task_pid_vnr(t) > 0' to further proceed with task 't' in current
> namespace.
>
> To avoid applying this check in every userspace code related to PID namespace,
> this patch provides two new iterative macros;for_each_process_in_ns and
> do_each_thread_in_ns.

There may be a point to this, as a cleanup.

I am not convinced at the moment that has_group_leader_pid() does
the right thing in this context.

Can you please send the fix for cap_set_all (adding task_pid_vnr(X) > 1) 
and then we can revisit possible cleanups.

Thanks,

Eric

Re: [PATCH 5/5] pid: use namespaced iteration on processes while managing priority

[Eric W. Biederman]

"Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:

> Quoting Gowrishankar M (gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
>> From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>> 
>> At present we scan all processes in init namespace, while getting or setting
>> process priorities for a user. Incase of PID namespace, it leads to leak
>> priority to processes in other namespace.
>> 
>> Below patch proposes to use new macro controller to fix the boundary of
>> processes list in current namespace.
>> 
>> Signed-off-by: Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>
> Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Serge please look again. This has absolutely nothing to do with pids.
This is a problem to be solved with the user namespace.

Eric

Re: [PATCH 5/5] pid: use namespaced iteration on processes while managing priority

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
> Gowrishankar M <gowrishankar.m-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> writes:
> 
> > From: Gowrishankar M <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> >
> > At present we scan all processes in init namespace, while getting or setting
> > process priorities for a user. Incase of PID namespace, it leads to leak
> > priority to processes in other namespace.
> >
> > Below patch proposes to use new macro controller to fix the boundary of
> > processes list in current namespace.
> 
> Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> 
> This has nothing to do with pids.  The command is to set the
> iopriority for a given user.  This is a problem of the user namespace
> not the pid namespace.

The uid check needs to be fixed for user namespaces, agreed.  I could
go either way though on whether we should also restrict to the same
pidns.

(note to fix the userns part of this added to my userns queue - first
I want to finish with keys; then maybe this should be done before
handling capabilities)

So if you want to nack this, I'll go along with that, but I think it's
useful.

thanks,
-serge

Re: [PATCH 5/5] pid: use namespaced iteration on processes while managing priority

[Eric W. Biederman]

"Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:


> The uid check needs to be fixed for user namespaces, agreed.  I could
> go either way though on whether we should also restrict to the same
> pidns.

It would be a subtle unexpected semantic change, that we would need
to copy linux-abi and document etc.  I'm not convinced it is that
useful.

I'm inclined to keep the semantics pure until there is some real
experience from the field on issues like this.

> (note to fix the userns part of this added to my userns queue - first
> I want to finish with keys; then maybe this should be done before
> handling capabilities)

Sounds good.  Mentioning the user namespace was just to make it clear
where it should be fixed.

Eric

Re: [PATCH 5/5] pid: use namespaced iteration on processes while managing priority

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
> "Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:
> 
> 
> > The uid check needs to be fixed for user namespaces, agreed.  I could
> > go either way though on whether we should also restrict to the same
> > pidns.
> 
> It would be a subtle unexpected semantic change, that we would need
> to copy linux-abi and document etc.  I'm not convinced it is that
> useful.
> 
> I'm inclined to keep the semantics pure until there is some real
> experience from the field on issues like this.

Ok.

Gowrishankar, please drop this patch.

> > (note to fix the userns part of this added to my userns queue - first
> > I want to finish with keys; then maybe this should be done before
> > handling capabilities)
> 
> Sounds good.  Mentioning the user namespace was just to make it clear
> where it should be fixed.

(And I might not have caught it if you hadn't mentioned it)

thanks,
-serge

Re: [PATCH 5/5] pid: use namespaced iteration on processes while managing priority

[Matt Helsley]

On Thu, 2008-12-18 at 10:54 -0800, Eric W. Biederman wrote:
> "Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:
> 
> 
> > The uid check needs to be fixed for user namespaces, agreed.  I could
> > go either way though on whether we should also restrict to the same
> > pidns.
> 
> It would be a subtle unexpected semantic change, that we would need
> to copy linux-abi and document etc.  I'm not convinced it is that
> useful.
> 
> I'm inclined to keep the semantics pure until there is some real
> experience from the field on issues like this.

Well the man page talks about PRIO_PROCESS and PRIO_PGRP and in those
cases it looks like "who" is really a pid or pgrp id:

>        The  value  which  is one of PRIO_PROCESS, PRIO_PGRP, or PRIO_USER, and
>        who  is  interpreted  relative  to  which  (a  process  identifier  for
>        PRIO_PROCESS, process group identifier for PRIO_PGRP, and a user ID for
>        PRIO_USER). 

It looks to me like restricting by pidns is required if "which" is
PRIO_PROCESS or PRIO_PGRP. If "which" is PRIO_USER then yes, it sounds
like a user ns issue.

Cheers,
	-Matt Helsley

Re: [PATCH 5/5] pid: use namespaced iteration on processes while managing priority

[Matt Helsley]

On Thu, 2008-12-18 at 20:30 -0800, Matt Helsley wrote:
> On Thu, 2008-12-18 at 10:54 -0800, Eric W. Biederman wrote:
> > "Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:
> > 
> > 
> > > The uid check needs to be fixed for user namespaces, agreed.  I could
> > > go either way though on whether we should also restrict to the same
> > > pidns.
> > 
> > It would be a subtle unexpected semantic change, that we would need
> > to copy linux-abi and document etc.  I'm not convinced it is that
> > useful.
> > 
> > I'm inclined to keep the semantics pure until there is some real
> > experience from the field on issues like this.
> 
> Well the man page talks about PRIO_PROCESS and PRIO_PGRP and in those
> cases it looks like "who" is really a pid or pgrp id:
> 
> >        The  value  which  is one of PRIO_PROCESS, PRIO_PGRP, or PRIO_USER, and
> >        who  is  interpreted  relative  to  which  (a  process  identifier  for
> >        PRIO_PROCESS, process group identifier for PRIO_PGRP, and a user ID for
> >        PRIO_USER). 
> 
> It looks to me like restricting by pidns is required if "which" is
> PRIO_PROCESS or PRIO_PGRP. If "which" is PRIO_USER then yes, it sounds
> like a user ns issue.

Eh, ignore me. Looks like this is already the case in the code.

> Cheers,
> 	-Matt Helsley