cgroups: forbid noprefix if mounting more than just cpuset subsystem
PATCH 1/1 cgroups: forbid noprefix if mounting more than just cpuset subsystem
The 'noprefix' option was introduced for backwards-compatibility of cpuset, but actually it can be used when mounting other subsystems.
This results in possibility of name collision, and now the collision can really happen, because we have 'stat' file in both memory and cpuacct subsystem:
# mount -t cgroup -o noprefix,memory,cpuacct xxx /mnt
Cgroup will happily mount the 2 subsystems, but only 'stat' file of memory subsys can be seen.
We don't want users to use nopreifx, and also want to avoid name collision, so we change to allow noprefix only if mounting just the cpuset subsystem.
--- kernel/cgroup.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
kernel/cgroup.c +10 -0
@@ -842,6 +842,11 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts) {char *token, *o = data ?:"all" ; +unsigned long mask = (unsigned long )-1 ; + +#ifdef CONFIG_CPUSETS + mask = ~(1 << cpuset_subsys_id); +#endif opts->subsys_bits =0 ; opts->flags =0 ;
@@ -886,6 +891,11 @@ static int parse_cgroupfs_options(char *data, } } +/* We allow noprefix only if mounting just the cpuset subsystem */ +if (test_bit (ROOT_NOPREFIX , &opts->flags) && + (opts->subsys_bits & mask)) +return -EINVAL ; +/* We can't have an empty hierarchy */ if (!opts->subsys_bits)return -EINVAL ;
Signature · 1 line
1.5.4.rc3
Quoted 23 lines · On Tue, 02 Jun 2009 10:40:31 +0800 Li Zefan…
The 'noprefix' option was introduced for backwards-compatibility of cpuset, but actually it can be used when mounting other subsystems.
This results in possibility of name collision, and now the collision can really happen, because we have 'stat' file in both memory and cpuacct subsystem:
# mount -t cgroup -o noprefix,memory,cpuacct xxx /mntCgroup will happily mount the 2 subsystems, but only 'stat' file of memory subsys can be seen.
We don't want users to use nopreifx, and also want to avoid name collision, so we change to allow noprefix only if mounting just the cpuset subsystem.
...
kernel/cgroup.c +0
-0
kernel/cgroup.c static int parse_cgroupfs_options(char *data, · line 849
Quoted 8 lines
kernel/cgroup.c +5
-0@@ -842,6 +842,11 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts) {char *token, *o = data ?:"all" ; +unsigned long mask = (unsigned long )-1 ; + +#ifdef CONFIG_CPUSETS + mask = ~(1 << cpuset_subsys_id); +#endif
This will actually produce the wrong result if cpuset_subsys_id >= 32. You want 1UL here.
kernel/cgroup.c static int parse_cgroupfs_options(char *data, · line 898
Quoted 11 lines
kernel/cgroup.c +5
-0opts->subsys_bits =0 ; opts->flags =0 ;@@ -886,6 +891,11 @@ static int parse_cgroupfs_options(char *data, } } +/* We allow noprefix only if mounting just the cpuset subsystem */ +if (test_bit (ROOT_NOPREFIX , &opts->flags) && + (opts->subsys_bits & mask)) +return -EINVAL ; +
uh, OK. I hope that comment is clear enough for anyone who wants to
understand it. It doesn't explain _why_ this is done..
Quoted 9 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ /* We allow noprefix only if mounting just the cpuset subsystem */ + if (test_bit(ROOT_NOPREFIX, &opts->flags) && + (opts->subsys_bits & mask)) + return -EINVAL; +uh, OK. I hope that comment is clear enough for anyone who wants to
understand it. It doesn't explain _why_ this is done..
I agree more explanation is better..
====
From: Li Zefan <[email protected]>
Subject: [PATCH] cgroups: forbid noprefix if mounting more than just cpuset subsystem, fix2
Explain more on the noprefix option.
--- kernel/cgroup.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
kernel/cgroup.c +5 -1
@@ -891,7 +891,11 @@ static int parse_cgroupfs_options(char *data, } } -/* We allow noprefix only if mounting just the cpuset subsystem */ +/* +* Option noprefix was introduced just for backward compatibility +* with the old cpuset, so we allow noprefix only if mounting just +* the cpuset subsystem. +*/ if (test_bit (ROOT_NOPREFIX , &opts->flags) && (opts->subsys_bits & mask))return -EINVAL ;
Signature · 1 line
1.5.4.rc3
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 15 lines · Li Zefan
We don't want users to use nopreifx, and also want to avoid name collision, so we change to allow noprefix only if mounting just the cpuset subsystem.
Signed-off-by Li Zefan <[email protected]>---
kernel/cgroup.c | 10 ++++++++++1 files changed, 10 insertions(+), 0 deletions(-)kernel/cgroup.c +0
-0
kernel/cgroup.c static int parse_cgroupfs_options(char *data, · line 845
Quoted 4 lines
kernel/cgroup.c +1
-0@@ -842,6 +842,11 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts) { char *token, *o = data ?: "all"; + unsigned long mask = (unsigned long)-1;
Patch looks good, but to make it more self-explanatory how about calling this variable noprefix_conflict_mask ?
Paul
Quoted 15 lines · Li Zefan
We don't want users to use nopreifx, and also want to avoid name collision, so we change to allow noprefix only if mounting just the cpuset subsystem.
Signed-off-by Li Zefan <[email protected]>---
kernel/cgroup.c | 10 ++++++++++1 files changed, 10 insertions(+), 0 deletions(-)kernel/cgroup.c +0
-0
kernel/cgroup.c static int parse_cgroupfs_options(char *data, · line 845
Quoted 4 lines
kernel/cgroup.c +1
-0@@ -842,6 +842,11 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts) { char *token, *o = data ?: "all"; + unsigned long mask = (unsigned long)-1;
Patch looks good, but to make it more self-explanatory how about calling this variable noprefix_conflict_mask ?
Paul