[PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[Lee Jones]

It should be possible for processes with CAP_SYS_NICE capabilities
(privileges) to move lower priority tasks within the same namespace to
different cgroups.

One extremely common example of this is Android's 'system_server',
which moves processes around to different cgroups/cpusets, but should
not require any other root privileges.

Cc: Tejun Heo <tj@kernel.org>
Cc: Zefan Li <lizefan.x@bytedance.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: cgroups@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 kernel/cgroup/cgroup-v1.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index 1f274d7fc934e..56d0d91951f02 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -510,7 +510,8 @@ static ssize_t __cgroup1_procs_write(struct kernfs_open_file *of,
 	tcred = get_task_cred(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_SYS_NICE))
 		ret = -EACCES;
 	put_cred(tcred);
 	if (ret)
-- 
2.32.0

[PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[Lee Jones]

It should be possible for processes with CAP_SYS_NICE capabilities
(privileges) to move lower priority tasks within the same namespace to
different cgroups.

One extremely common example of this is Android's 'system_server',
which moves processes around to different cgroups/cpusets, but should
not require any other root privileges.

Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Zefan Li <lizefan.x-EC8Uxl6Npydl57MIdRCFDg@public.gmane.org>
Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 kernel/cgroup/cgroup-v1.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index 1f274d7fc934e..56d0d91951f02 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -510,7 +510,8 @@ static ssize_t __cgroup1_procs_write(struct kernfs_open_file *of,
 	tcred = get_task_cred(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_SYS_NICE))
 		ret = -EACCES;
 	put_cred(tcred);
 	if (ret)
-- 
2.32.0

Re: [PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[Tejun Heo]

Hello,

On Thu, Jun 17, 2021 at 10:09:41AM +0100, Lee Jones wrote:
> It should be possible for processes with CAP_SYS_NICE capabilities
> (privileges) to move lower priority tasks within the same namespace to
> different cgroups.

I'm not sure that "should" is justified that easily given that cgroup can
affect things like device access permissions and basic system organization.

> One extremely common example of this is Android's 'system_server',
> which moves processes around to different cgroups/cpusets, but should
> not require any other root privileges.

Why is this being brought up now after all the years? Isn't android moving
onto cgroup2 anyway?

Thanks.

-- 
tejun

Re: [PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[Tejun Heo]

Hello,

On Thu, Jun 17, 2021 at 10:09:41AM +0100, Lee Jones wrote:
> It should be possible for processes with CAP_SYS_NICE capabilities
> (privileges) to move lower priority tasks within the same namespace to
> different cgroups.

I'm not sure that "should" is justified that easily given that cgroup can
affect things like device access permissions and basic system organization.

> One extremely common example of this is Android's 'system_server',
> which moves processes around to different cgroups/cpusets, but should
> not require any other root privileges.

Why is this being brought up now after all the years? Isn't android moving
onto cgroup2 anyway?

Thanks.

-- 
tejun

Re: [PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[Lee Jones]

Hi Tejun,

Thanks for your reply.

On Thu, 17 Jun 2021, Tejun Heo wrote:
> On Thu, Jun 17, 2021 at 10:09:41AM +0100, Lee Jones wrote:
> > It should be possible for processes with CAP_SYS_NICE capabilities
> > (privileges) to move lower priority tasks within the same namespace to
> > different cgroups.
> 
> I'm not sure that "should" is justified that easily given that cgroup can
> affect things like device access permissions and basic system organization.

The latter part of that sentence does provide some additional caveats.

> > One extremely common example of this is Android's 'system_server',
> > which moves processes around to different cgroups/cpusets, but should
> > not require any other root privileges.
> 
> Why is this being brought up now after all the years?

This has been discussed before?

I didn't find any evidence of that on the lists.

> Isn't android moving onto cgroup2 anyway?

That I would have to check.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[Lee Jones]

Hi Tejun,

Thanks for your reply.

On Thu, 17 Jun 2021, Tejun Heo wrote:
> On Thu, Jun 17, 2021 at 10:09:41AM +0100, Lee Jones wrote:
> > It should be possible for processes with CAP_SYS_NICE capabilities
> > (privileges) to move lower priority tasks within the same namespace to
> > different cgroups.
> 
> I'm not sure that "should" is justified that easily given that cgroup can
> affect things like device access permissions and basic system organization.

The latter part of that sentence does provide some additional caveats.

> > One extremely common example of this is Android's 'system_server',
> > which moves processes around to different cgroups/cpusets, but should
> > not require any other root privileges.
> 
> Why is this being brought up now after all the years?

This has been discussed before?

I didn't find any evidence of that on the lists.

> Isn't android moving onto cgroup2 anyway?

That I would have to check.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[Suren Baghdasaryan]

On Thu, Jun 17, 2021 at 5:02 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> Hi Tejun,
>
> Thanks for your reply.
>
> On Thu, 17 Jun 2021, Tejun Heo wrote:
> > On Thu, Jun 17, 2021 at 10:09:41AM +0100, Lee Jones wrote:
> > > It should be possible for processes with CAP_SYS_NICE capabilities
> > > (privileges) to move lower priority tasks within the same namespace to
> > > different cgroups.
> >
> > I'm not sure that "should" is justified that easily given that cgroup can
> > affect things like device access permissions and basic system organization.
>
> The latter part of that sentence does provide some additional caveats.
>
> > > One extremely common example of this is Android's 'system_server',
> > > which moves processes around to different cgroups/cpusets, but should
> > > not require any other root privileges.
> >
> > Why is this being brought up now after all the years?

Currently Android uses an out-of-tree patch to work around this issue.

>
> This has been discussed before?
>
> I didn't find any evidence of that on the lists.

IIRC, John Stultz from Linaro tried to upstream a similar patch before.

>
> > Isn't android moving onto cgroup2 anyway?
>
> That I would have to check.

Some of the controllers are moving to cgroup v2 but not all of them
are there yet. For example, there are still some issues with moving
the cpu controller to v2 which I believe were discussed during Android
Microconference at LPC 2021.

>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[Suren Baghdasaryan]

On Thu, Jun 17, 2021 at 5:02 AM Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
> Hi Tejun,
>
> Thanks for your reply.
>
> On Thu, 17 Jun 2021, Tejun Heo wrote:
> > On Thu, Jun 17, 2021 at 10:09:41AM +0100, Lee Jones wrote:
> > > It should be possible for processes with CAP_SYS_NICE capabilities
> > > (privileges) to move lower priority tasks within the same namespace to
> > > different cgroups.
> >
> > I'm not sure that "should" is justified that easily given that cgroup can
> > affect things like device access permissions and basic system organization.
>
> The latter part of that sentence does provide some additional caveats.
>
> > > One extremely common example of this is Android's 'system_server',
> > > which moves processes around to different cgroups/cpusets, but should
> > > not require any other root privileges.
> >
> > Why is this being brought up now after all the years?

Currently Android uses an out-of-tree patch to work around this issue.

>
> This has been discussed before?
>
> I didn't find any evidence of that on the lists.

IIRC, John Stultz from Linaro tried to upstream a similar patch before.

>
> > Isn't android moving onto cgroup2 anyway?
>
> That I would have to check.

Some of the controllers are moving to cgroup v2 but not all of them
are there yet. For example, there are still some issues with moving
the cpu controller to v2 which I believe were discussed during Android
Microconference at LPC 2021.

>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[Tejun Heo]

On Thu, Sep 30, 2021 at 02:20:53PM -0700, Suren Baghdasaryan wrote:
> Some of the controllers are moving to cgroup v2 but not all of them
> are there yet. For example, there are still some issues with moving
> the cpu controller to v2 which I believe were discussed during Android
> Microconference at LPC 2021.

Care to provide a summary?

Thanks.

-- 
tejun

Re: [PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[Tejun Heo]

On Thu, Sep 30, 2021 at 02:20:53PM -0700, Suren Baghdasaryan wrote:
> Some of the controllers are moving to cgroup v2 but not all of them
> are there yet. For example, there are still some issues with moving
> the cpu controller to v2 which I believe were discussed during Android
> Microconference at LPC 2021.

Care to provide a summary?

Thanks.

-- 
tejun

Re: [PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[Suren Baghdasaryan]

On Mon, Oct 4, 2021 at 9:41 AM Tejun Heo <tj@kernel.org> wrote:
>
> On Thu, Sep 30, 2021 at 02:20:53PM -0700, Suren Baghdasaryan wrote:
> > Some of the controllers are moving to cgroup v2 but not all of them
> > are there yet. For example, there are still some issues with moving
> > the cpu controller to v2 which I believe were discussed during Android
> > Microconference at LPC 2021.
>
> Care to provide a summary?

Unfortunately I could not be present at LPC this year but Wei I
believe was the presenter (CC'ing him).
Wei, could you please summarize the issues with moving the cpu
controller to cgroups v2?
Also CC'ing John, who I believe tried to upstream this patch before.
Thanks,
Suren.


>
> Thanks.
>
> --
> tejun

Re: [PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[Suren Baghdasaryan]

On Mon, Oct 4, 2021 at 9:41 AM Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> On Thu, Sep 30, 2021 at 02:20:53PM -0700, Suren Baghdasaryan wrote:
> > Some of the controllers are moving to cgroup v2 but not all of them
> > are there yet. For example, there are still some issues with moving
> > the cpu controller to v2 which I believe were discussed during Android
> > Microconference at LPC 2021.
>
> Care to provide a summary?

Unfortunately I could not be present at LPC this year but Wei I
believe was the presenter (CC'ing him).
Wei, could you please summarize the issues with moving the cpu
controller to cgroups v2?
Also CC'ing John, who I believe tried to upstream this patch before.
Thanks,
Suren.


>
> Thanks.
>
> --
> tejun

Re: [PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[John Stultz]

On Mon, Oct 4, 2021 at 9:57 AM Suren Baghdasaryan <surenb@google.com> wrote:
> On Mon, Oct 4, 2021 at 9:41 AM Tejun Heo <tj@kernel.org> wrote:
> > On Thu, Sep 30, 2021 at 02:20:53PM -0700, Suren Baghdasaryan wrote:
> > > Some of the controllers are moving to cgroup v2 but not all of them
> > > are there yet. For example, there are still some issues with moving
> > > the cpu controller to v2 which I believe were discussed during Android
> > > Microconference at LPC 2021.
> >
> > Care to provide a summary?
>
> Unfortunately I could not be present at LPC this year but Wei I
> believe was the presenter (CC'ing him).
> Wei, could you please summarize the issues with moving the cpu
> controller to cgroups v2?

Tejun: We were sorry you didn't join as we were hoping for your
attendance for the discussion!

For reference, here's the video of the session:
  https://www.youtube.com/watch?v=O_lCFGinFPM&t=2941s

And continued discussion from the BoF:
  https://youtu.be/i5BdYn6SNQc?t=703

But Wei can still chime in with a more focused summary, maybe?


> Also CC'ing John, who I believe tried to upstream this patch before.

We sort of went in a big circle of creating a config time option w/
CAP_SYS_NICE, then a new CAP_CGROUP_MIGRATE then switching to
CAP_SYS_RESOURCE and then back to CAP_CGROUP_MIGRATE, and when that
was panned I gave up and we kept the small patch in the Android tree
that uses CAP_SYS_NICE.

Links to previous attempts & discussion:
v1: https://lore.kernel.org/lkml/1475556090-6278-1-git-send-email-john.stultz@linaro.org/#t
v2: https://lore.kernel.org/lkml/1476743724-9104-1-git-send-email-john.stultz@linaro.org/
v4: https://lore.kernel.org/lkml/1478647728-30357-1-git-send-email-john.stultz@linaro.org/
v5: https://lore.kernel.org/lkml/1481593143-18756-1-git-send-email-john.stultz@linaro.org/

thanks
-john

Re: [PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[John Stultz]

On Mon, Oct 4, 2021 at 9:57 AM Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, Oct 4, 2021 at 9:41 AM Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Thu, Sep 30, 2021 at 02:20:53PM -0700, Suren Baghdasaryan wrote:
> > > Some of the controllers are moving to cgroup v2 but not all of them
> > > are there yet. For example, there are still some issues with moving
> > > the cpu controller to v2 which I believe were discussed during Android
> > > Microconference at LPC 2021.
> >
> > Care to provide a summary?
>
> Unfortunately I could not be present at LPC this year but Wei I
> believe was the presenter (CC'ing him).
> Wei, could you please summarize the issues with moving the cpu
> controller to cgroups v2?

Tejun: We were sorry you didn't join as we were hoping for your
attendance for the discussion!

For reference, here's the video of the session:
  https://www.youtube.com/watch?v=O_lCFGinFPM&t=2941s

And continued discussion from the BoF:
  https://youtu.be/i5BdYn6SNQc?t=703

But Wei can still chime in with a more focused summary, maybe?


> Also CC'ing John, who I believe tried to upstream this patch before.

We sort of went in a big circle of creating a config time option w/
CAP_SYS_NICE, then a new CAP_CGROUP_MIGRATE then switching to
CAP_SYS_RESOURCE and then back to CAP_CGROUP_MIGRATE, and when that
was panned I gave up and we kept the small patch in the Android tree
that uses CAP_SYS_NICE.

Links to previous attempts & discussion:
v1: https://lore.kernel.org/lkml/1475556090-6278-1-git-send-email-john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org/#t
v2: https://lore.kernel.org/lkml/1476743724-9104-1-git-send-email-john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org/
v4: https://lore.kernel.org/lkml/1478647728-30357-1-git-send-email-john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org/
v5: https://lore.kernel.org/lkml/1481593143-18756-1-git-send-email-john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org/

thanks
-john

Re: [PATCH 1/1] cgroup-v1: Grant CAP_SYS_NICE holders permission to move tasks between cgroups

[John Stultz]

On Mon, Oct 4, 2021 at 10:23 AM John Stultz <john.stultz@linaro.org> wrote:
> We sort of went in a big circle of creating a config time option w/
> CAP_SYS_NICE, then a new CAP_CGROUP_MIGRATE then switching to
> CAP_SYS_RESOURCE and then back to CAP_CGROUP_MIGRATE, and when that
> was panned I gave up and we kept the small patch in the Android tree
> that uses CAP_SYS_NICE.
>
> Links to previous attempts & discussion:
> v1: https://lore.kernel.org/lkml/1475556090-6278-1-git-send-email-john.stultz@linaro.org/#t
> v2: https://lore.kernel.org/lkml/1476743724-9104-1-git-send-email-john.stultz@linaro.org/
> v4: https://lore.kernel.org/lkml/1478647728-30357-1-git-send-email-john.stultz@linaro.org/
> v5: https://lore.kernel.org/lkml/1481593143-18756-1-git-send-email-john.stultz@linaro.org/

Whoops I missed one more before I gave up (CAP_CGROUP):
v6: https://lore.kernel.org/lkml/1481949827-23613-1-git-send-email-john.stultz@linaro.org/#t