Re: [PATCH 4/4] cgroup: move the one-off opts sanity check in cgroup_root_from_opts() to parse_cgroupfs_options()
On Wed, Jan 29, 2014 at 12:02:44PM +0800, Li Zefan wrote: > On 2014/1/28 23:32, Tejun Heo wrote: > > cgroup_root_from_opts() checks whether (!opts->subsys_mask && > > !opts->none) and returns NULL if so. After that, if allocation fails, > > returns ERR_PTR(-ENOMEM). The caller, cgroup_mount(), doesn't treat > > NULL as an error but set opts.new_root to NULL; however, later on, > > cgroup_set_super() fails with -EINVAL if new_root is NULL. > > This patch changes mount semantics. > > If cgroup_root_from_opts() returns NULL, it means we should be looking > for existing superblock only. > > This will fail: > > # mount -t cgroup -o name=abc xxx /mnt > > But this is ok: > > # mount -t cgroup -o none,name=abc xxx /mnt > # mkdir /mnt/sub > # umount /mnt > # mount -t cgroup -o name=abc xxx /mnt <-- this won't work with your patch Ewww urgghhghghghhh. Alright, dropping this patch and will update later patches to maintain the behavior. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] cgroup: move the one-off opts sanity check in cgroup_root_from_opts() to parse_cgroupfs_options()
On Wed, Jan 29, 2014 at 12:02:44PM +0800, Li Zefan wrote: On 2014/1/28 23:32, Tejun Heo wrote: cgroup_root_from_opts() checks whether (!opts-subsys_mask !opts-none) and returns NULL if so. After that, if allocation fails, returns ERR_PTR(-ENOMEM). The caller, cgroup_mount(), doesn't treat NULL as an error but set opts.new_root to NULL; however, later on, cgroup_set_super() fails with -EINVAL if new_root is NULL. This patch changes mount semantics. If cgroup_root_from_opts() returns NULL, it means we should be looking for existing superblock only. This will fail: # mount -t cgroup -o name=abc xxx /mnt But this is ok: # mount -t cgroup -o none,name=abc xxx /mnt # mkdir /mnt/sub # umount /mnt # mount -t cgroup -o name=abc xxx /mnt -- this won't work with your patch Ewww urgghhghghghhh. Alright, dropping this patch and will update later patches to maintain the behavior. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] cgroup: move the one-off opts sanity check in cgroup_root_from_opts() to parse_cgroupfs_options()
On 2014/1/28 23:32, Tejun Heo wrote: > cgroup_root_from_opts() checks whether (!opts->subsys_mask && > !opts->none) and returns NULL if so. After that, if allocation fails, > returns ERR_PTR(-ENOMEM). The caller, cgroup_mount(), doesn't treat > NULL as an error but set opts.new_root to NULL; however, later on, > cgroup_set_super() fails with -EINVAL if new_root is NULL. This patch changes mount semantics. If cgroup_root_from_opts() returns NULL, it means we should be looking for existing superblock only. This will fail: # mount -t cgroup -o name=abc xxx /mnt But this is ok: # mount -t cgroup -o none,name=abc xxx /mnt # mkdir /mnt/sub # umount /mnt # mount -t cgroup -o name=abc xxx /mnt <-- this won't work with your patch > > This is one bizarre error handling sequence especially when all other > opts sanity checks including the very close (!opts->subsys_mask && > !opts->name) check are done in parse_cgroupfs_options(). > > Let's move the one-off check in cgroup_root_from_opts() to > parse_cgroupfs_options() where it can be combined with the > (!opts->subsys_mask && !opts->name) check. cgroup_root_from_opts() is > updated to return NULL on memory allocation failure. > > Signed-off-by: Tejun Heo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/4] cgroup: move the one-off opts sanity check in cgroup_root_from_opts() to parse_cgroupfs_options()
cgroup_root_from_opts() checks whether (!opts->subsys_mask &&
!opts->none) and returns NULL if so. After that, if allocation fails,
returns ERR_PTR(-ENOMEM). The caller, cgroup_mount(), doesn't treat
NULL as an error but set opts.new_root to NULL; however, later on,
cgroup_set_super() fails with -EINVAL if new_root is NULL.
This is one bizarre error handling sequence especially when all other
opts sanity checks including the very close (!opts->subsys_mask &&
!opts->name) check are done in parse_cgroupfs_options().
Let's move the one-off check in cgroup_root_from_opts() to
parse_cgroupfs_options() where it can be combined with the
(!opts->subsys_mask && !opts->name) check. cgroup_root_from_opts() is
updated to return NULL on memory allocation failure.
Signed-off-by: Tejun Heo
---
kernel/cgroup.c | 19 ++-
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index b0e030f..5c8fe40 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1265,10 +1265,10 @@ static int parse_cgroupfs_options(char *data, struct
cgroup_sb_opts *opts)
return -EINVAL;
/*
-* We either have to specify by name or by subsystems. (So all
-* empty hierarchies must have a name).
+* We either have to specify by name or by subsystems. All empty
+* hierarchies must have a name and also "none" option specified.
*/
- if (!opts->subsys_mask && !opts->name)
+ if (!opts->subsys_mask && (!opts->name || !opts->none))
return -EINVAL;
return 0;
@@ -1417,12 +1417,9 @@ static struct cgroupfs_root
*cgroup_root_from_opts(struct cgroup_sb_opts *opts)
{
struct cgroupfs_root *root;
- if (!opts->subsys_mask && !opts->none)
- return NULL;
-
root = kzalloc(sizeof(*root), GFP_KERNEL);
if (!root)
- return ERR_PTR(-ENOMEM);
+ return NULL;
init_cgroup_root(root);
@@ -1461,10 +1458,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_mask && !opts->none);
ret = set_anon_super(sb, NULL);
@@ -1532,8 +1525,8 @@ static struct dentry *cgroup_mount(struct
file_system_type *fs_type,
* reusing an existing hierarchy.
*/
new_root = cgroup_root_from_opts();
- if (IS_ERR(new_root)) {
- ret = PTR_ERR(new_root);
+ if (!new_root) {
+ ret = -ENOMEM;
goto out_err;
}
opts.new_root = new_root;
--
1.8.5.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/4] cgroup: move the one-off opts sanity check in cgroup_root_from_opts() to parse_cgroupfs_options()
cgroup_root_from_opts() checks whether (!opts-subsys_mask
!opts-none) and returns NULL if so. After that, if allocation fails,
returns ERR_PTR(-ENOMEM). The caller, cgroup_mount(), doesn't treat
NULL as an error but set opts.new_root to NULL; however, later on,
cgroup_set_super() fails with -EINVAL if new_root is NULL.
This is one bizarre error handling sequence especially when all other
opts sanity checks including the very close (!opts-subsys_mask
!opts-name) check are done in parse_cgroupfs_options().
Let's move the one-off check in cgroup_root_from_opts() to
parse_cgroupfs_options() where it can be combined with the
(!opts-subsys_mask !opts-name) check. cgroup_root_from_opts() is
updated to return NULL on memory allocation failure.
Signed-off-by: Tejun Heo t...@kernel.org
---
kernel/cgroup.c | 19 ++-
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index b0e030f..5c8fe40 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1265,10 +1265,10 @@ static int parse_cgroupfs_options(char *data, struct
cgroup_sb_opts *opts)
return -EINVAL;
/*
-* We either have to specify by name or by subsystems. (So all
-* empty hierarchies must have a name).
+* We either have to specify by name or by subsystems. All empty
+* hierarchies must have a name and also none option specified.
*/
- if (!opts-subsys_mask !opts-name)
+ if (!opts-subsys_mask (!opts-name || !opts-none))
return -EINVAL;
return 0;
@@ -1417,12 +1417,9 @@ static struct cgroupfs_root
*cgroup_root_from_opts(struct cgroup_sb_opts *opts)
{
struct cgroupfs_root *root;
- if (!opts-subsys_mask !opts-none)
- return NULL;
-
root = kzalloc(sizeof(*root), GFP_KERNEL);
if (!root)
- return ERR_PTR(-ENOMEM);
+ return NULL;
init_cgroup_root(root);
@@ -1461,10 +1458,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_mask !opts-none);
ret = set_anon_super(sb, NULL);
@@ -1532,8 +1525,8 @@ static struct dentry *cgroup_mount(struct
file_system_type *fs_type,
* reusing an existing hierarchy.
*/
new_root = cgroup_root_from_opts(opts);
- if (IS_ERR(new_root)) {
- ret = PTR_ERR(new_root);
+ if (!new_root) {
+ ret = -ENOMEM;
goto out_err;
}
opts.new_root = new_root;
--
1.8.5.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] cgroup: move the one-off opts sanity check in cgroup_root_from_opts() to parse_cgroupfs_options()
On 2014/1/28 23:32, Tejun Heo wrote: cgroup_root_from_opts() checks whether (!opts-subsys_mask !opts-none) and returns NULL if so. After that, if allocation fails, returns ERR_PTR(-ENOMEM). The caller, cgroup_mount(), doesn't treat NULL as an error but set opts.new_root to NULL; however, later on, cgroup_set_super() fails with -EINVAL if new_root is NULL. This patch changes mount semantics. If cgroup_root_from_opts() returns NULL, it means we should be looking for existing superblock only. This will fail: # mount -t cgroup -o name=abc xxx /mnt But this is ok: # mount -t cgroup -o none,name=abc xxx /mnt # mkdir /mnt/sub # umount /mnt # mount -t cgroup -o name=abc xxx /mnt -- this won't work with your patch This is one bizarre error handling sequence especially when all other opts sanity checks including the very close (!opts-subsys_mask !opts-name) check are done in parse_cgroupfs_options(). Let's move the one-off check in cgroup_root_from_opts() to parse_cgroupfs_options() where it can be combined with the (!opts-subsys_mask !opts-name) check. cgroup_root_from_opts() is updated to return NULL on memory allocation failure. Signed-off-by: Tejun Heo t...@kernel.org -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
