| Message ID | 20230629091146.28801-2-mkoutny@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cpuset: Allow setscheduler regardless of manipulated task | expand |
On 6/29/23 05:11, Michal Koutný wrote: > When we migrate a task between two cgroups, one of the checks is a > verification whether we can modify task's scheduler settings > (cap_task_setscheduler()). > > An implicit migration occurs also when enabling a controller on the > unified hierarchy (think of parent to child migration). The > aforementioned check may be problematic if the caller of the migration > (enabling a controller) has no permissions over migrated tasks. > For instance, a user's cgroup that ends up running a process of a > different user. Although cgroup permissions are configured favorably, > the enablement fails due to the foreign process [1]. > > Change the behavior by relaxing the permissions check on the unified > hierarchy (or in v2 mode). This is in accordance with unified hierarchy > attachment behavior when permissions of the source to target cgroups are > decisive whereas the migrated task is opaque (as opposed to more > restrictive check in __cgroup1_procs_write()). The is_in_v2_mode() check is for supporting the v2 mode in cgroup v1. However, there is no controller enabling in v1. So I think you should just use cgroup_subsys_on_dfl(cpuset_cgrp_subsys) as the v2 check if your focus is just to prevent problem when enabling cpuset controller. > > [1] https://github.com/systemd/systemd/issues/18293#issuecomment-831205649 > > Signed-off-by: Michal Koutný <mkoutny@suse.com> > --- > kernel/cgroup/cpuset.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c > index e4ca2dd2b764..3b5f87a9a150 100644 > --- a/kernel/cgroup/cpuset.c > +++ b/kernel/cgroup/cpuset.c > @@ -2495,6 +2495,13 @@ static int cpuset_can_attach(struct cgroup_taskset *tset) > ret = task_can_attach(task, cs->effective_cpus); > if (ret) > goto out_unlock; > + > + /* > + * Skip rights over task check in v2, migration permission derives > + * from hierarchy ownership in cgroup_procs_write_permission()). > + */ > + if (is_in_v2_mode()) > + continue; > ret = security_task_setscheduler(task); > if (ret) > goto out_unlock; This change will likely conflict with the latest cpuset change on tracking # of dl tasks in a cpuset. You will have to, at least, move the dl task check before the security_task_setscheduler() check. Another fact about cpuset controller enabling is that both cpus_allowed and mems_allowed are empty at that point. You may also add these checks as a preconditions for disabling the security_task_setscheduler check. Cheers, Longman
On Thu, Jun 29, 2023 at 08:11:33AM -0400, Waiman Long <longman@redhat.com> wrote: > So I think you should just use > cgroup_subsys_on_dfl(cpuset_cgrp_subsys) as the v2 check if your focus > is just to prevent problem when enabling cpuset controller. I thought the bare cgroup_subsys_on_dfl(cpuset_cgrp_subsys) is not used in cpuset.c but I was wrong -- yes, I'll change this. > This change will likely conflict with the latest cpuset change on tracking # > of dl tasks in a cpuset. You will have to, at least, move the dl task check > before the security_task_setscheduler() check. > > Another fact about cpuset controller enabling is that both cpus_allowed and > mems_allowed are empty at that point. You may also add these checks as a > preconditions for disabling the security_task_setscheduler check. Ah, I will rebase on fresh mainline (or do you mean another reference?). Thanks for the hints, Michal
On 6/29/23 08:26, Michal Koutný wrote: > On Thu, Jun 29, 2023 at 08:11:33AM -0400, Waiman Long <longman@redhat.com> wrote: >> So I think you should just use >> cgroup_subsys_on_dfl(cpuset_cgrp_subsys) as the v2 check if your focus >> is just to prevent problem when enabling cpuset controller. > I thought the bare cgroup_subsys_on_dfl(cpuset_cgrp_subsys) is not used > in cpuset.c but I was wrong -- yes, I'll change this. > >> This change will likely conflict with the latest cpuset change on tracking # >> of dl tasks in a cpuset. You will have to, at least, move the dl task check >> before the security_task_setscheduler() check. >> >> Another fact about cpuset controller enabling is that both cpus_allowed and >> mems_allowed are empty at that point. You may also add these checks as a >> preconditions for disabling the security_task_setscheduler check. > Ah, I will rebase on fresh mainline (or do you mean another reference?). Yes, those changes have just been merged into the mainline. Cheers, Longman
On Thu, Jun 29, 2023 at 08:11:33AM -0400, Waiman Long <longman@redhat.com> wrote: > Another fact about cpuset controller enabling is that both cpus_allowed and > mems_allowed are empty at that point. You may also add these checks as a > preconditions for disabling the security_task_setscheduler check. I considered relying on that, however, there is more generic case when migrating between two sibling that should be allowed in v2 too. See the added test_cpuset_perms_object(). (Admittedly, it doesn't stress the case when the two siblings had different CPUs but it could.) Anyway, let's move on to v2 (where I addressed remaining comments). Thanks, Michal
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c index e4ca2dd2b764..3b5f87a9a150 100644 --- a/kernel/cgroup/cpuset.c +++ b/kernel/cgroup/cpuset.c @@ -2495,6 +2495,13 @@ static int cpuset_can_attach(struct cgroup_taskset *tset) ret = task_can_attach(task, cs->effective_cpus); if (ret) goto out_unlock; + + /* + * Skip rights over task check in v2, migration permission derives + * from hierarchy ownership in cgroup_procs_write_permission()). + */ + if (is_in_v2_mode()) + continue; ret = security_task_setscheduler(task); if (ret) goto out_unlock;
When we migrate a task between two cgroups, one of the checks is a verification whether we can modify task's scheduler settings (cap_task_setscheduler()). An implicit migration occurs also when enabling a controller on the unified hierarchy (think of parent to child migration). The aforementioned check may be problematic if the caller of the migration (enabling a controller) has no permissions over migrated tasks. For instance, a user's cgroup that ends up running a process of a different user. Although cgroup permissions are configured favorably, the enablement fails due to the foreign process [1]. Change the behavior by relaxing the permissions check on the unified hierarchy (or in v2 mode). This is in accordance with unified hierarchy attachment behavior when permissions of the source to target cgroups are decisive whereas the migrated task is opaque (as opposed to more restrictive check in __cgroup1_procs_write()). [1] https://github.com/systemd/systemd/issues/18293#issuecomment-831205649 Signed-off-by: Michal Koutný <mkoutny@suse.com> --- kernel/cgroup/cpuset.c | 7 +++++++ 1 file changed, 7 insertions(+)