cgroup: move a check to parse_cgroupfs_options()

PATCH In review Li Zefan 2 participants 5 messages Dec 22, 2011 – Dec 27, 2011 cgroup

lore

Share & tools

lore

Li Zefan

author

2011-12-22 08:41 UTC #0 lore raw

PATCH 1/1 cgroup: move a check to parse_cgroupfs_options()

Always check the validity of mount options in parse_cgroupfs_options().

No functional change.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

kernel/cgroup.c +7 -8

index 4936d88..bdb7994 100644
@@ -1218,7 +1218,6 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 	    (opts->subsys_bits & mask))
 		return -EINVAL;
 
-
 	/* Can't specify "none" and some subsystems */
 	if (opts->subsys_bits && opts->none)
 		return -EINVAL;

@@ -1231,6 +1230,13 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 		return -EINVAL;
 
 	/*
+	 * This can happen only when none of the subsystems is enabled
+	 * in the system.
+	 */
+	if (!opts->subsys_bits && !opts->none)
+		return -EINVAL;
+
+	/*
 	 * Grab references on all the modules we'll need, so the subsystems
 	 * don't dance around before rebind_subsystems attaches them. This may
 	 * take duplicate reference counts on a subsystem that's already used,

@@ -1401,9 +1407,6 @@ static struct cgroupfs_root *cgroup_root_from_opts(struct cgroup_sb_opts *opts)
 {
 	struct cgroupfs_root *root;
 
-	if (!opts->subsys_bits && !opts->none)
-		return NULL;
-
 	root = kzalloc(sizeof(*root), GFP_KERNEL);
 	if (!root)
 		return ERR_PTR(-ENOMEM);

@@ -1442,10 +1445,6 @@ static int cgroup_set_super(struct super_block *sb, void *data)
 	int ret;
 	struct cgroup_sb_opts *opts = data;
 
-	/* If we don't have a new root, we can't set up a new sb */
-	if (!opts->new_root)
-		return -EINVAL;
-
 	BUG_ON(!opts->subsys_bits && !opts->none);
 
 	ret = set_anon_super(sb, NULL);

Signature · 1 line

1.7.3.1

Tejun Heo

maintainer

2011-12-22 15:20 UTC #1 lore raw

Hello,

Quoted 22 lines · Li Zefan

Always check the validity of mount options in parse_cgroupfs_options().

No functional change.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

kernel/cgroup.c +0 -1

index 4936d88..bdb7994 100644
@@ -1218,7 +1218,6 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 	    (opts->subsys_bits & mask))
 		return -EINVAL;
 
-
 	/* Can't specify "none" and some subsystems */
 	if (opts->subsys_bits && opts->none)
 		return -EINVAL;

kernel/cgroup.c static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts) · line 1237

Quoted 8 lines

kernel/cgroup.c +5 -0

@@ -1231,6 +1230,13 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 		return -EINVAL;
 
 	/*
+	 * This can happen only when none of the subsystems is enabled
+	 * in the system.
+	 */
+	if (!opts->subsys_bits && !opts->none)
+		return -EINVAL;

The three ifs here share a lot and I think sharing the common parts would make the logic clearer. e.g. The first and this can be easily combined to (bool)opts->subsys_bits != (bool)opts->none which would explain the logic better too.

kernel/cgroup.c static int cgroup_set_super(struct super_block *sb, void *data) · line 1448

Quoted 8 lines

kernel/cgroup.c +0 -4

@@ -1442,10 +1445,6 @@ static int cgroup_set_super(struct super_block *sb, void *data)
 	int ret;
 	struct cgroup_sb_opts *opts = data;
 
-	/* If we don't have a new root, we can't set up a new sb */
-	if (!opts->new_root)
-		return -EINVAL;
-

And where did this one go?

Thanks.

Signature · 1 line

tejun

Li Zefan

author

2011-12-23 03:38 UTC #2 lore raw

Quoted 27 lines · Tejun Heo

Hello,

Quoted 24 lines · Li Zefan

Always check the validity of mount options in parse_cgroupfs_options().

No functional change.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

kernel/cgroup.c +0 -1

index 4936d88..bdb7994 100644
@@ -1218,7 +1218,6 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 	    (opts->subsys_bits & mask))
 		return -EINVAL;
 
-
 	/* Can't specify "none" and some subsystems */
 	if (opts->subsys_bits && opts->none)
 		return -EINVAL;

@@ -1231,6 +1230,13 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 		return -EINVAL;

Quoted 12 lines

kernel/cgroup.c static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts) · line 1237
Quoted 6 lines
kernel/cgroup.c +5 -0
 	/*
+	 * This can happen only when none of the subsystems is enabled
+	 * in the system.
+	 */
+	if (!opts->subsys_bits && !opts->none)
+		return -EINVAL;
The three ifs here share a lot and I think sharing the common parts would make the logic clearer. e.g. The first and this can be easily combined to (bool)opts->subsys_bits != (bool)opts->none which would explain the logic better too.

ok.

Quoted 11 lines

kernel/cgroup.c static int cgroup_set_super(struct super_block *sb, void *data) · line 1448
Quoted 8 lines
kernel/cgroup.c +0 -4
@@ -1442,10 +1445,6 @@ static int cgroup_set_super(struct super_block *sb, void *data)
 	int ret;
 	struct cgroup_sb_opts *opts = data;
 
-	/* If we don't have a new root, we can't set up a new sb */
-	if (!opts->new_root)
-		return -EINVAL;
-
And where did this one go?

The other one I removed sets opts->new_root to NULL, and this one detects the NULL ptr:

if (!opts->subsys_bits && !opts->none)

	return NULL;

...
opts->new_root = NULL;
...
if (!opts->new_root)

	return -EINVAL;

shortcut to:

if (!opts->subsys_bits && !opts->none)

	return -EINVAL;

Tejun Heo

maintainer

2011-12-23 22:52 UTC #3 lore raw

Hello,

Quoted 13 lines · Li Zefan

Quoted 1 line
And where did this one go?
The other one I removed sets opts->new_root to NULL, and this one detects the NULL ptr:
if (!opts->subsys_bits && !opts->none)
return NULL;
...
opts->new_root = NULL;
...
if (!opts->new_root)
return -EINVAL;

Quoted 4 lines

shortcut to:
if (!opts->subsys_bits && !opts->none)
return -EINVAL;

Thanks for the explanation. Maybe explaining it briefly in the commit message would be nice?

Signature · 1 line

tejun

Li Zefan

author

2011-12-27 01:46 UTC #4 lore raw

Quoted 16 lines · Tejun Heo

Hello,
Quoted 13 lines · Li Zefan
Quoted 1 line
And where did this one go?
The other one I removed sets opts->new_root to NULL, and this one detects the NULL ptr:
if (!opts->subsys_bits && !opts->none)
       return NULL;
...
opts->new_root = NULL;
...
if (!opts->new_root)
       return -EINVAL;

Quoted 8 lines

Quoted 4 lines
shortcut to:
if (!opts->subsys_bits && !opts->none)
       return -EINVAL;
Thanks for the explanation. Maybe explaining it briefly in the commit message would be nice?

I recalled why we have this check in this place, that is to allow mounting by hierarchy name, but it has been broken for a long time. I'll send a fix with detailed changelog.