[RFC][PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[RFC][PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[John Stultz]

This patch adds CAP_GROUP_MIGRATE_TASK and logic to allows a process
to migrate other tasks between cgroups.

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.

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

So this patch, as suggested by Tejun,  simply adds a new process
capability flag (CAP_CGROUP_MIGRATE_TASK), and uses it when checking
if a task can migrate other tasks between cgroups.

I've tested this with AOSP master (though its a bit hacked in as I
still need to properly get the selinux bits aware of the new
capability bit) with selinux set to permissive and it seems to be
working well.

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: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 include/uapi/linux/capability.h | 5 ++++-
 kernel/cgroup.c                 | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 49bc062..e199ea0 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -349,8 +349,11 @@ struct vfs_cap_data {
 
 #define CAP_AUDIT_READ		37
 
+/* Allow migrating tasks between cgroups */
 
-#define CAP_LAST_CAP         CAP_AUDIT_READ
+#define CAP_CGROUP_MIGRATE_TASK	38
+
+#define CAP_LAST_CAP         CAP_CGROUP_MIGRATE_TASK
 
 #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
 
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 9ba28310..a318956 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2847,7 +2847,8 @@ 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))
+	    !uid_eq(cred->euid, tcred->suid) &&
+	    !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE_TASK))
 		ret = -EACCES;
 
 	if (!ret && cgroup_on_dfl(dst_cgrp)) {
-- 
1.9.1

Re: [RFC][PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[Serge E. Hallyn]

Quoting John Stultz (john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org):
> This patch adds CAP_GROUP_MIGRATE_TASK and logic to allows a process
> to migrate other tasks between cgroups.
> 
> 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.
> 
> The approach taken there overloads CAP_SYS_NICE a bit much, and
> is maybe more complicated then needed.
> 
> So this patch, as suggested by Tejun,  simply adds a new process
> capability flag (CAP_CGROUP_MIGRATE_TASK), and uses it when checking

So realistically, what all can this mean?  Freezing tasks, changing
cpu/memory limits, changing network and disk throughput, forbid forking,
and (most importantly) forbid access to certain devices.

I think that's all ok.  (And we still separately check for inode write
perms.)

If anything I'd say the GLOBAL_ROOT_UID check could be taken out since
otherwise a host-root task effectively cannot drop this capability.

> if a task can migrate other tasks between cgroups.
> 
> I've tested this with AOSP master (though its a bit hacked in as I
> still need to properly get the selinux bits aware of the new
> capability bit) with selinux set to permissive and it seems to be
> working well.
> 
> 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: Serge E. Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>

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

> Signed-off-by: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  include/uapi/linux/capability.h | 5 ++++-
>  kernel/cgroup.c                 | 3 ++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 49bc062..e199ea0 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -349,8 +349,11 @@ struct vfs_cap_data {
>  
>  #define CAP_AUDIT_READ		37
>  
> +/* Allow migrating tasks between cgroups */
>  
> -#define CAP_LAST_CAP         CAP_AUDIT_READ
> +#define CAP_CGROUP_MIGRATE_TASK	38
> +
> +#define CAP_LAST_CAP         CAP_CGROUP_MIGRATE_TASK
>  
>  #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>  
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 9ba28310..a318956 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2847,7 +2847,8 @@ 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))
> +	    !uid_eq(cred->euid, tcred->suid) &&
> +	    !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE_TASK))
>  		ret = -EACCES;
>  
>  	if (!ret && cgroup_on_dfl(dst_cgrp)) {
> -- 
> 1.9.1

Re: [RFC][PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[Serge E. Hallyn]

Quoting John Stultz (john.stultz@linaro.org):
> This patch adds CAP_GROUP_MIGRATE_TASK and logic to allows a process
> to migrate other tasks between cgroups.
> 
> 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.
> 
> The approach taken there overloads CAP_SYS_NICE a bit much, and
> is maybe more complicated then needed.
> 
> So this patch, as suggested by Tejun,  simply adds a new process
> capability flag (CAP_CGROUP_MIGRATE_TASK), and uses it when checking

So realistically, what all can this mean?  Freezing tasks, changing
cpu/memory limits, changing network and disk throughput, forbid forking,
and (most importantly) forbid access to certain devices.

I think that's all ok.  (And we still separately check for inode write
perms.)

If anything I'd say the GLOBAL_ROOT_UID check could be taken out since
otherwise a host-root task effectively cannot drop this capability.

> if a task can migrate other tasks between cgroups.
> 
> I've tested this with AOSP master (though its a bit hacked in as I
> still need to properly get the selinux bits aware of the new
> capability bit) with selinux set to permissive and it seems to be
> working well.
> 
> 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: Serge E. Hallyn <serge@hallyn.com>

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

> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  include/uapi/linux/capability.h | 5 ++++-
>  kernel/cgroup.c                 | 3 ++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 49bc062..e199ea0 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -349,8 +349,11 @@ struct vfs_cap_data {
>  
>  #define CAP_AUDIT_READ		37
>  
> +/* Allow migrating tasks between cgroups */
>  
> -#define CAP_LAST_CAP         CAP_AUDIT_READ
> +#define CAP_CGROUP_MIGRATE_TASK	38
> +
> +#define CAP_LAST_CAP         CAP_CGROUP_MIGRATE_TASK
>  
>  #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>  
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 9ba28310..a318956 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2847,7 +2847,8 @@ 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))
> +	    !uid_eq(cred->euid, tcred->suid) &&
> +	    !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE_TASK))
>  		ret = -EACCES;
>  
>  	if (!ret && cgroup_on_dfl(dst_cgrp)) {
> -- 
> 1.9.1

Re: [RFC][PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[John Stultz]

On Tue, Oct 4, 2016 at 5:38 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> Quoting John Stultz (john.stultz@linaro.org):
>> So this patch, as suggested by Tejun,  simply adds a new process
>> capability flag (CAP_CGROUP_MIGRATE_TASK), and uses it when checking
>
> So realistically, what all can this mean?  Freezing tasks, changing
> cpu/memory limits, changing network and disk throughput, forbid forking,
> and (most importantly) forbid access to certain devices.
>
> I think that's all ok.  (And we still separately check for inode write
> perms.)

Sounds good.

> If anything I'd say the GLOBAL_ROOT_UID check could be taken out since
> otherwise a host-root task effectively cannot drop this capability.

Is this ok to leave for a separate patch?

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

Thanks for the review!

Unless there's other feedback, I'll sit on this until the merge window
is over and then resubmit for consideration for 4.10.

thanks
-john

Re: [RFC][PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[Serge E. Hallyn]

On Tue, Oct 04, 2016 at 08:00:18PM -0700, John Stultz wrote:
> On Tue, Oct 4, 2016 at 5:38 PM, Serge E. Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> wrote:
> > Quoting John Stultz (john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org):
> >> So this patch, as suggested by Tejun,  simply adds a new process
> >> capability flag (CAP_CGROUP_MIGRATE_TASK), and uses it when checking
> >
> > So realistically, what all can this mean?  Freezing tasks, changing
> > cpu/memory limits, changing network and disk throughput, forbid forking,
> > and (most importantly) forbid access to certain devices.
> >
> > I think that's all ok.  (And we still separately check for inode write
> > perms.)
> 
> Sounds good.
> 
> > If anything I'd say the GLOBAL_ROOT_UID check could be taken out since
> > otherwise a host-root task effectively cannot drop this capability.
> 
> Is this ok to leave for a separate patch?

Yeah.  And I'm not sure whether Tejun would object to that idea.

> > Acked-by: Serge Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
> 
> Thanks for the review!
> 
> Unless there's other feedback, I'll sit on this until the merge window
> is over and then resubmit for consideration for 4.10.
> 
> thanks
> -john

Re: [RFC][PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[Serge E. Hallyn]

On Tue, Oct 04, 2016 at 08:00:18PM -0700, John Stultz wrote:
> On Tue, Oct 4, 2016 at 5:38 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> > Quoting John Stultz (john.stultz@linaro.org):
> >> So this patch, as suggested by Tejun,  simply adds a new process
> >> capability flag (CAP_CGROUP_MIGRATE_TASK), and uses it when checking
> >
> > So realistically, what all can this mean?  Freezing tasks, changing
> > cpu/memory limits, changing network and disk throughput, forbid forking,
> > and (most importantly) forbid access to certain devices.
> >
> > I think that's all ok.  (And we still separately check for inode write
> > perms.)
> 
> Sounds good.
> 
> > If anything I'd say the GLOBAL_ROOT_UID check could be taken out since
> > otherwise a host-root task effectively cannot drop this capability.
> 
> Is this ok to leave for a separate patch?

Yeah.  And I'm not sure whether Tejun would object to that idea.

> > Acked-by: Serge Hallyn <serge@hallyn.com>
> 
> Thanks for the review!
> 
> Unless there's other feedback, I'll sit on this until the merge window
> is over and then resubmit for consideration for 4.10.
> 
> thanks
> -john

Re: [RFC][PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[Tejun Heo]

Hello,

On Tue, Oct 04, 2016 at 11:25:29PM -0500, Serge E. Hallyn wrote:
> > > If anything I'd say the GLOBAL_ROOT_UID check could be taken out since
> > > otherwise a host-root task effectively cannot drop this capability.
> > 
> > Is this ok to leave for a separate patch?
> 
> Yeah.  And I'm not sure whether Tejun would object to that idea.

With the capability, I think it'd be better to get rid of the explicit
UID check, but can we please rename the cap to CAP_CGROUP_MIGRATE and
loop in linux-api mailing list and ppl who are more familiar with
CAPs?

Thanks!

-- 
tejun

Re: [RFC][PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[Tejun Heo]

Hello,

On Tue, Oct 04, 2016 at 11:25:29PM -0500, Serge E. Hallyn wrote:
> > > If anything I'd say the GLOBAL_ROOT_UID check could be taken out since
> > > otherwise a host-root task effectively cannot drop this capability.
> > 
> > Is this ok to leave for a separate patch?
> 
> Yeah.  And I'm not sure whether Tejun would object to that idea.

With the capability, I think it'd be better to get rid of the explicit
UID check, but can we please rename the cap to CAP_CGROUP_MIGRATE and
loop in linux-api mailing list and ppl who are more familiar with
CAPs?

Thanks!

-- 
tejun