[PATCH] cgroups: forbid noprefix if mounting more than just cpuset subsystem

[PATCH] cgroups: forbid noprefix if mounting more than just cpuset subsystem

[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 /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.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/cgroup.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index a7267bf..ad17f9d 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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;
-- 
1.5.4.rc3

Re: [PATCH] cgroups: forbid noprefix if mounting more than just cpuset subsystem

[Andrew Morton]

On Tue, 02 Jun 2009 10:40:31 +0800 Li Zefan <lizf@cn.fujitsu.com> wrote:

> 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.
> 
> ...
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index a7267bf..ad17f9d 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -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.


>  	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;
> +

uh, OK.  I hope that comment is clear enough for anyone who wants to
understand it.   It doesn't explain _why_ this is done..

Re: [PATCH] cgroups: forbid noprefix if mounting more than just cpuset subsystem

[Li Zefan]

>> +	/* 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 <lizf@cn.fujitsu.com>
Subject: [PATCH] cgroups: forbid noprefix if mounting more than just cpuset subsystem, fix2

Explain more on the noprefix option.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/cgroup.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index ad17f9d..d15432c 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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;
-- 
1.5.4.rc3

Re: [PATCH] cgroups: forbid noprefix if mounting more than just cpuset subsystem

[Paul Menage]

On Mon, Jun 1, 2009 at 7:40 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>
> 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 <lizf@cn.fujitsu.com>
> ---
>  kernel/cgroup.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index a7267bf..ad17f9d 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -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