All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC][PATCH v3] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups
@ 2016-10-21  1:24 ` John Stultz
  0 siblings, 0 replies; 8+ messages in thread
From: John Stultz @ 2016-10-21  1:24 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Tejun Heo, Li Zefan, Jonathan Corbet, cgroups,
	Android Kernel Team, Rom Lemarchand, Colin Cross, Dmitry Shmidt,
	Todd Kjos, Christian Poetzsch, Amit Pundir, Dmitry Torokhov,
	Kees Cook, Serge E . Hallyn, linux-api

This patch adds logic to allows a process to migrate other tasks
between cgroups if they have CAP_SYS_RESOURCE.

In Android (where this feature originated), the ActivityManager tracks
various application states (TOP_APP, FOREGROUND, BACKGROUND, SYSTEM,
etc), and then as applications change states, the SchedPolicy logic
will migrate the application tasks between different cgroups used
to control the different application states (for example, there is a
background cpuset cgroup which can limit background tasks to stay
on one low-power cpu, and the bg_non_interactive cpuctrl cgroup can
then further limit those background tasks to a small percentage of
that one cpu's cpu time).

However, for security reasons, Android doesn't want to make the
system_server (the process that runs the ActivityManager and
SchedPolicy logic), run as root. So in the Android common.git
kernel, they have some logic to allow cgroups to loosen their
permissions so CAP_SYS_NICE tasks can migrate other tasks between
cgroups.

I feel the approach taken there overloads CAP_SYS_NICE a bit much,
and is maybe more complicated then needed.

So this patch, as suggested by Michael Kerrisk, simply adds a
check for CAP_SYS_RESOURCE.

I've tested this with AOSP master, and this seems to work well
as Zygote and system_server already use CAP_SYS_RESOURCE. I've
also submitted patches against the android-4.4 kernel to change
it to use CAP_SYS_RESOURCE, and the Android developers have
seemed ok with this change.

Thouhts and feedback would be appreciated!

Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: cgroups@vger.kernel.org
Cc: Android Kernel Team <kernel-team@android.com>
Cc: Rom Lemarchand <romlem@android.com>
Cc: Colin Cross <ccross@android.com>
Cc: Dmitry Shmidt <dimitrysh@google.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Christian Poetzsch <christian.potzsch@imgtec.com>
Cc: Amit Pundir <amit.pundir@linaro.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
Cc: linux-api@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
v3: Switched to just using CAP_SYS_RESOURCE as suggested by Michael
---
 kernel/cgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 09f84d2..866059a 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2857,7 +2857,7 @@ static int cgroup_procs_write_permission(struct task_struct *task,
 	if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
 	    !uid_eq(cred->euid, tcred->uid) &&
 	    !uid_eq(cred->euid, tcred->suid) &&
-	    !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE))
+	    !ns_capable(tcred->user_ns, CAP_SYS_RESOURCE))
 		ret = -EACCES;
 
 	if (!ret && cgroup_on_dfl(dst_cgrp)) {
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [RFC][PATCH v3] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups
@ 2016-10-21  1:24 ` John Stultz
  0 siblings, 0 replies; 8+ messages in thread
From: John Stultz @ 2016-10-21  1:24 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Tejun Heo, Li Zefan, Jonathan Corbet,
	cgroups-u79uwXL29TY76Z2rM5mHXA, Android Kernel Team,
	Rom Lemarchand, Colin Cross, Dmitry Shmidt, Todd Kjos,
	Christian Poetzsch, Amit Pundir, Dmitry Torokhov, Kees Cook,
	Serge E . Hallyn, linux-api-u79uwXL29TY76Z2rM5mHXA

This patch adds logic to allows a process to migrate other tasks
between cgroups if they have CAP_SYS_RESOURCE.

In Android (where this feature originated), the ActivityManager tracks
various application states (TOP_APP, FOREGROUND, BACKGROUND, SYSTEM,
etc), and then as applications change states, the SchedPolicy logic
will migrate the application tasks between different cgroups used
to control the different application states (for example, there is a
background cpuset cgroup which can limit background tasks to stay
on one low-power cpu, and the bg_non_interactive cpuctrl cgroup can
then further limit those background tasks to a small percentage of
that one cpu's cpu time).

However, for security reasons, Android doesn't want to make the
system_server (the process that runs the ActivityManager and
SchedPolicy logic), run as root. So in the Android common.git
kernel, they have some logic to allow cgroups to loosen their
permissions so CAP_SYS_NICE tasks can migrate other tasks between
cgroups.

I feel the approach taken there overloads CAP_SYS_NICE a bit much,
and is maybe more complicated then needed.

So this patch, as suggested by Michael Kerrisk, simply adds a
check for CAP_SYS_RESOURCE.

I've tested this with AOSP master, and this seems to work well
as Zygote and system_server already use CAP_SYS_RESOURCE. I've
also submitted patches against the android-4.4 kernel to change
it to use CAP_SYS_RESOURCE, and the Android developers have
seemed ok with this change.

Thouhts and feedback would be appreciated!

Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Android Kernel Team <kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
Cc: Rom Lemarchand <romlem-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
Cc: Dmitry Shmidt <dimitrysh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Todd Kjos <tkjos-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Christian Poetzsch <christian.potzsch-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Cc: Amit Pundir <amit.pundir-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Serge E. Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
v3: Switched to just using CAP_SYS_RESOURCE as suggested by Michael
---
 kernel/cgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 09f84d2..866059a 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2857,7 +2857,7 @@ static int cgroup_procs_write_permission(struct task_struct *task,
 	if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
 	    !uid_eq(cred->euid, tcred->uid) &&
 	    !uid_eq(cred->euid, tcred->suid) &&
-	    !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE))
+	    !ns_capable(tcred->user_ns, CAP_SYS_RESOURCE))
 		ret = -EACCES;
 
 	if (!ret && cgroup_on_dfl(dst_cgrp)) {
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [RFC][PATCH v3] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups
@ 2016-10-21  6:42   ` Michael Kerrisk (man-pages)
  0 siblings, 0 replies; 8+ messages in thread
From: Michael Kerrisk (man-pages) @ 2016-10-21  6:42 UTC (permalink / raw)
  To: John Stultz, lkml
  Cc: mtk.manpages, Tejun Heo, Li Zefan, Jonathan Corbet, cgroups,
	Android Kernel Team, Rom Lemarchand, Colin Cross, Dmitry Shmidt,
	Todd Kjos, Christian Poetzsch, Amit Pundir, Dmitry Torokhov,
	Kees Cook, Serge E . Hallyn, linux-api

Hi John,

On 10/21/2016 03:24 AM, John Stultz wrote:
> This patch adds logic to allows a process to migrate other tasks
> between cgroups if they have CAP_SYS_RESOURCE.

This appears to be a patch against your previous patch, 
rather than against mainline. Was that intended?

Cheers,

Michael


> In Android (where this feature originated), the ActivityManager tracks
> various application states (TOP_APP, FOREGROUND, BACKGROUND, SYSTEM,
> etc), and then as applications change states, the SchedPolicy logic
> will migrate the application tasks between different cgroups used
> to control the different application states (for example, there is a
> background cpuset cgroup which can limit background tasks to stay
> on one low-power cpu, and the bg_non_interactive cpuctrl cgroup can
> then further limit those background tasks to a small percentage of
> that one cpu's cpu time).
> 
> However, for security reasons, Android doesn't want to make the
> system_server (the process that runs the ActivityManager and
> SchedPolicy logic), run as root. So in the Android common.git
> kernel, they have some logic to allow cgroups to loosen their
> permissions so CAP_SYS_NICE tasks can migrate other tasks between
> cgroups.
> 
> I feel the approach taken there overloads CAP_SYS_NICE a bit much,
> and is maybe more complicated then needed.
> 
> So this patch, as suggested by Michael Kerrisk, simply adds a
> check for CAP_SYS_RESOURCE.
> 
> I've tested this with AOSP master, and this seems to work well
> as Zygote and system_server already use CAP_SYS_RESOURCE. I've
> also submitted patches against the android-4.4 kernel to change
> it to use CAP_SYS_RESOURCE, and the Android developers have
> seemed ok with this change.
> 
> Thouhts and feedback would be appreciated!
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Li Zefan <lizefan@huawei.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: cgroups@vger.kernel.org
> Cc: Android Kernel Team <kernel-team@android.com>
> Cc: Rom Lemarchand <romlem@android.com>
> Cc: Colin Cross <ccross@android.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Christian Poetzsch <christian.potzsch@imgtec.com>
> Cc: Amit Pundir <amit.pundir@linaro.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Cc: linux-api@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
> v3: Switched to just using CAP_SYS_RESOURCE as suggested by Michael
> ---
>  kernel/cgroup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 09f84d2..866059a 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2857,7 +2857,7 @@ static int cgroup_procs_write_permission(struct task_struct *task,
>  	if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
>  	    !uid_eq(cred->euid, tcred->uid) &&
>  	    !uid_eq(cred->euid, tcred->suid) &&
> -	    !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE))
> +	    !ns_capable(tcred->user_ns, CAP_SYS_RESOURCE))
>  		ret = -EACCES;
>  
>  	if (!ret && cgroup_on_dfl(dst_cgrp)) {
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC][PATCH v3] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups
@ 2016-10-21  6:42   ` Michael Kerrisk (man-pages)
  0 siblings, 0 replies; 8+ messages in thread
From: Michael Kerrisk (man-pages) @ 2016-10-21  6:42 UTC (permalink / raw)
  To: John Stultz, lkml
  Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, Tejun Heo, Li Zefan,
	Jonathan Corbet, cgroups-u79uwXL29TY76Z2rM5mHXA,
	Android Kernel Team, Rom Lemarchand, Colin Cross, Dmitry Shmidt,
	Todd Kjos, Christian Poetzsch, Amit Pundir, Dmitry Torokhov,
	Kees Cook, Serge E . Hallyn, linux-api-u79uwXL29TY76Z2rM5mHXA

Hi John,

On 10/21/2016 03:24 AM, John Stultz wrote:
> This patch adds logic to allows a process to migrate other tasks
> between cgroups if they have CAP_SYS_RESOURCE.

This appears to be a patch against your previous patch, 
rather than against mainline. Was that intended?

Cheers,

Michael


> In Android (where this feature originated), the ActivityManager tracks
> various application states (TOP_APP, FOREGROUND, BACKGROUND, SYSTEM,
> etc), and then as applications change states, the SchedPolicy logic
> will migrate the application tasks between different cgroups used
> to control the different application states (for example, there is a
> background cpuset cgroup which can limit background tasks to stay
> on one low-power cpu, and the bg_non_interactive cpuctrl cgroup can
> then further limit those background tasks to a small percentage of
> that one cpu's cpu time).
> 
> However, for security reasons, Android doesn't want to make the
> system_server (the process that runs the ActivityManager and
> SchedPolicy logic), run as root. So in the Android common.git
> kernel, they have some logic to allow cgroups to loosen their
> permissions so CAP_SYS_NICE tasks can migrate other tasks between
> cgroups.
> 
> I feel the approach taken there overloads CAP_SYS_NICE a bit much,
> and is maybe more complicated then needed.
> 
> So this patch, as suggested by Michael Kerrisk, simply adds a
> check for CAP_SYS_RESOURCE.
> 
> I've tested this with AOSP master, and this seems to work well
> as Zygote and system_server already use CAP_SYS_RESOURCE. I've
> also submitted patches against the android-4.4 kernel to change
> it to use CAP_SYS_RESOURCE, and the Android developers have
> seemed ok with this change.
> 
> Thouhts and feedback would be appreciated!
> 
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Android Kernel Team <kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Cc: Rom Lemarchand <romlem-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Cc: Dmitry Shmidt <dimitrysh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Todd Kjos <tkjos-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Christian Poetzsch <christian.potzsch-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Cc: Amit Pundir <amit.pundir-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Serge E. Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
> Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
> v3: Switched to just using CAP_SYS_RESOURCE as suggested by Michael
> ---
>  kernel/cgroup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 09f84d2..866059a 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2857,7 +2857,7 @@ static int cgroup_procs_write_permission(struct task_struct *task,
>  	if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
>  	    !uid_eq(cred->euid, tcred->uid) &&
>  	    !uid_eq(cred->euid, tcred->suid) &&
> -	    !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE))
> +	    !ns_capable(tcred->user_ns, CAP_SYS_RESOURCE))
>  		ret = -EACCES;
>  
>  	if (!ret && cgroup_on_dfl(dst_cgrp)) {
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC][PATCH v3] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups
@ 2016-10-21 13:41   ` Serge E. Hallyn
  0 siblings, 0 replies; 8+ messages in thread
From: Serge E. Hallyn @ 2016-10-21 13:41 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Tejun Heo, Li Zefan, Jonathan Corbet, cgroups,
	Android Kernel Team, Rom Lemarchand, Colin Cross, Dmitry Shmidt,
	Todd Kjos, Christian Poetzsch, Amit Pundir, Dmitry Torokhov,
	Kees Cook, Serge E . Hallyn, linux-api

Quoting John Stultz (john.stultz@linaro.org):
> This patch adds logic to allows a process to migrate other tasks
> between cgroups if they have CAP_SYS_RESOURCE.

Hi,

fwiw this seems to me a reasonable choice, that is, giving a task
CAP_SYS_RESOURCE to be allow it to write to the procs file doesn't
seem like giving it too much.  +1 from me.  So long as the Android
folks don't have a reason why this won't work for them,

Acked-by: Serge Hallyn <serge@hallyn.com>

-serge

> In Android (where this feature originated), the ActivityManager tracks
> various application states (TOP_APP, FOREGROUND, BACKGROUND, SYSTEM,
> etc), and then as applications change states, the SchedPolicy logic
> will migrate the application tasks between different cgroups used
> to control the different application states (for example, there is a
> background cpuset cgroup which can limit background tasks to stay
> on one low-power cpu, and the bg_non_interactive cpuctrl cgroup can
> then further limit those background tasks to a small percentage of
> that one cpu's cpu time).
> 
> However, for security reasons, Android doesn't want to make the
> system_server (the process that runs the ActivityManager and
> SchedPolicy logic), run as root. So in the Android common.git
> kernel, they have some logic to allow cgroups to loosen their
> permissions so CAP_SYS_NICE tasks can migrate other tasks between
> cgroups.
> 
> I feel the approach taken there overloads CAP_SYS_NICE a bit much,
> and is maybe more complicated then needed.
> 
> So this patch, as suggested by Michael Kerrisk, simply adds a
> check for CAP_SYS_RESOURCE.
> 
> I've tested this with AOSP master, and this seems to work well
> as Zygote and system_server already use CAP_SYS_RESOURCE. I've
> also submitted patches against the android-4.4 kernel to change
> it to use CAP_SYS_RESOURCE, and the Android developers have
> seemed ok with this change.
> 
> Thouhts and feedback would be appreciated!
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Li Zefan <lizefan@huawei.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: cgroups@vger.kernel.org
> Cc: Android Kernel Team <kernel-team@android.com>
> Cc: Rom Lemarchand <romlem@android.com>
> Cc: Colin Cross <ccross@android.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Christian Poetzsch <christian.potzsch@imgtec.com>
> Cc: Amit Pundir <amit.pundir@linaro.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Cc: linux-api@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
> v3: Switched to just using CAP_SYS_RESOURCE as suggested by Michael
> ---
>  kernel/cgroup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 09f84d2..866059a 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2857,7 +2857,7 @@ static int cgroup_procs_write_permission(struct task_struct *task,
>  	if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
>  	    !uid_eq(cred->euid, tcred->uid) &&
>  	    !uid_eq(cred->euid, tcred->suid) &&
> -	    !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE))
> +	    !ns_capable(tcred->user_ns, CAP_SYS_RESOURCE))
>  		ret = -EACCES;
>  
>  	if (!ret && cgroup_on_dfl(dst_cgrp)) {
> -- 
> 2.7.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC][PATCH v3] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups
@ 2016-10-21 13:41   ` Serge E. Hallyn
  0 siblings, 0 replies; 8+ messages in thread
From: Serge E. Hallyn @ 2016-10-21 13:41 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Tejun Heo, Li Zefan, Jonathan Corbet,
	cgroups-u79uwXL29TY76Z2rM5mHXA, Android Kernel Team,
	Rom Lemarchand, Colin Cross, Dmitry Shmidt, Todd Kjos,
	Christian Poetzsch, Amit Pundir, Dmitry Torokhov, Kees Cook,
	Serge E . Hallyn, linux-api-u79uwXL29TY76Z2rM5mHXA

Quoting John Stultz (john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org):
> This patch adds logic to allows a process to migrate other tasks
> between cgroups if they have CAP_SYS_RESOURCE.

Hi,

fwiw this seems to me a reasonable choice, that is, giving a task
CAP_SYS_RESOURCE to be allow it to write to the procs file doesn't
seem like giving it too much.  +1 from me.  So long as the Android
folks don't have a reason why this won't work for them,

Acked-by: Serge Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>

-serge

> In Android (where this feature originated), the ActivityManager tracks
> various application states (TOP_APP, FOREGROUND, BACKGROUND, SYSTEM,
> etc), and then as applications change states, the SchedPolicy logic
> will migrate the application tasks between different cgroups used
> to control the different application states (for example, there is a
> background cpuset cgroup which can limit background tasks to stay
> on one low-power cpu, and the bg_non_interactive cpuctrl cgroup can
> then further limit those background tasks to a small percentage of
> that one cpu's cpu time).
> 
> However, for security reasons, Android doesn't want to make the
> system_server (the process that runs the ActivityManager and
> SchedPolicy logic), run as root. So in the Android common.git
> kernel, they have some logic to allow cgroups to loosen their
> permissions so CAP_SYS_NICE tasks can migrate other tasks between
> cgroups.
> 
> I feel the approach taken there overloads CAP_SYS_NICE a bit much,
> and is maybe more complicated then needed.
> 
> So this patch, as suggested by Michael Kerrisk, simply adds a
> check for CAP_SYS_RESOURCE.
> 
> I've tested this with AOSP master, and this seems to work well
> as Zygote and system_server already use CAP_SYS_RESOURCE. I've
> also submitted patches against the android-4.4 kernel to change
> it to use CAP_SYS_RESOURCE, and the Android developers have
> seemed ok with this change.
> 
> Thouhts and feedback would be appreciated!
> 
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Android Kernel Team <kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Cc: Rom Lemarchand <romlem-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Cc: Dmitry Shmidt <dimitrysh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Todd Kjos <tkjos-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Christian Poetzsch <christian.potzsch-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Cc: Amit Pundir <amit.pundir-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Serge E. Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
> Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
> v3: Switched to just using CAP_SYS_RESOURCE as suggested by Michael
> ---
>  kernel/cgroup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 09f84d2..866059a 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2857,7 +2857,7 @@ static int cgroup_procs_write_permission(struct task_struct *task,
>  	if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
>  	    !uid_eq(cred->euid, tcred->uid) &&
>  	    !uid_eq(cred->euid, tcred->suid) &&
> -	    !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE))
> +	    !ns_capable(tcred->user_ns, CAP_SYS_RESOURCE))
>  		ret = -EACCES;
>  
>  	if (!ret && cgroup_on_dfl(dst_cgrp)) {
> -- 
> 2.7.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC][PATCH v3] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups
@ 2016-10-21 18:06     ` John Stultz
  0 siblings, 0 replies; 8+ messages in thread
From: John Stultz @ 2016-10-21 18:06 UTC (permalink / raw)
  To: Michael Kerrisk (man-pages)
  Cc: lkml, Tejun Heo, Li Zefan, Jonathan Corbet,
	open list:CONTROL GROUP (CGROUP),
	Android Kernel Team, Rom Lemarchand, Colin Cross, Dmitry Shmidt,
	Todd Kjos, Christian Poetzsch, Amit Pundir, Dmitry Torokhov,
	Kees Cook, Serge E . Hallyn, Linux API

On Thu, Oct 20, 2016 at 11:42 PM, Michael Kerrisk (man-pages)
<mtk.manpages@gmail.com> wrote:
> Hi John,
>
> On 10/21/2016 03:24 AM, John Stultz wrote:
>> This patch adds logic to allows a process to migrate other tasks
>> between cgroups if they have CAP_SYS_RESOURCE.
>
> This appears to be a patch against your previous patch,
> rather than against mainline. Was that intended?

Heh. Definitely not. Apologies. I thought I had folded things down
(and the real patch is a similar looking one-liner).

I'll re-send the proper patch on monday. :)

thanks
-john

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC][PATCH v3] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups
@ 2016-10-21 18:06     ` John Stultz
  0 siblings, 0 replies; 8+ messages in thread
From: John Stultz @ 2016-10-21 18:06 UTC (permalink / raw)
  To: Michael Kerrisk (man-pages)
  Cc: lkml, Tejun Heo, Li Zefan, Jonathan Corbet,
	open list:CONTROL GROUP (CGROUP),
	Android Kernel Team, Rom Lemarchand, Colin Cross, Dmitry Shmidt,
	Todd Kjos, Christian Poetzsch, Amit Pundir, Dmitry Torokhov,
	Kees Cook, Serge E . Hallyn, Linux API

On Thu, Oct 20, 2016 at 11:42 PM, Michael Kerrisk (man-pages)
<mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi John,
>
> On 10/21/2016 03:24 AM, John Stultz wrote:
>> This patch adds logic to allows a process to migrate other tasks
>> between cgroups if they have CAP_SYS_RESOURCE.
>
> This appears to be a patch against your previous patch,
> rather than against mainline. Was that intended?

Heh. Definitely not. Apologies. I thought I had folded things down
(and the real patch is a similar looking one-liner).

I'll re-send the proper patch on monday. :)

thanks
-john

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-10-21 18:07 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-21  1:24 [RFC][PATCH v3] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups John Stultz
2016-10-21  1:24 ` John Stultz
2016-10-21  6:42 ` Michael Kerrisk (man-pages)
2016-10-21  6:42   ` Michael Kerrisk (man-pages)
2016-10-21 18:06   ` John Stultz
2016-10-21 18:06     ` John Stultz
2016-10-21 13:41 ` Serge E. Hallyn
2016-10-21 13:41   ` Serge E. Hallyn

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.