* [PATCH 1/2] cgroup: use cgroup_addrm_files in cgroup_clear_directory @ 2012-11-30 16:21 Gao feng 2012-11-30 16:21 ` [PATCH 2/2] cgroup: remove subsystem files when remounting cgroup Gao feng 2012-11-30 19:47 ` [PATCH] cgroup: use cgroup_addrm_files() in cgroup_clear_directory() Tejun Heo 0 siblings, 2 replies; 7+ messages in thread From: Gao feng @ 2012-11-30 16:21 UTC (permalink / raw) To: cgroups; +Cc: tj, lizefan, linux-kernel, containers, Gao feng cgroup_rm_file only remove the first file. after remount, the cgroup subsystem will be unloaded with it's files left. use cgroup_addrm_files to remove all files of subsystem. Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> --- kernel/cgroup.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/kernel/cgroup.c b/kernel/cgroup.c index f24f724..e5233b7 100644 --- a/kernel/cgroup.c +++ b/kernel/cgroup.c @@ -970,6 +970,8 @@ static int cgroup_rm_file(struct cgroup *cgrp, const struct cftype *cft) return -ENOENT; } +static int cgroup_addrm_files(struct cgroup *cgrp, struct cgroup_subsys *subsys, + struct cftype cfts[], bool is_add); /** * cgroup_clear_directory - selective removal of base and subsystem files * @dir: directory containing the files @@ -987,7 +989,7 @@ static void cgroup_clear_directory(struct dentry *dir, bool base_files, if (!test_bit(ss->subsys_id, &subsys_mask)) continue; list_for_each_entry(set, &ss->cftsets, node) - cgroup_rm_file(cgrp, set->cfts); + cgroup_addrm_files(cgrp, NULL, set->cfts, false); } if (base_files) { while (!list_empty(&cgrp->files)) -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] cgroup: remove subsystem files when remounting cgroup 2012-11-30 16:21 [PATCH 1/2] cgroup: use cgroup_addrm_files in cgroup_clear_directory Gao feng @ 2012-11-30 16:21 ` Gao feng 2012-11-30 19:27 ` Tejun Heo 2012-12-03 1:28 ` [PATCH RESEND] " Gao feng 2012-11-30 19:47 ` [PATCH] cgroup: use cgroup_addrm_files() in cgroup_clear_directory() Tejun Heo 1 sibling, 2 replies; 7+ messages in thread From: Gao feng @ 2012-11-30 16:21 UTC (permalink / raw) To: cgroups; +Cc: tj, lizefan, linux-kernel, containers, Gao feng cgroup_clear_directroy is called by cgroup_d_remove_dir and cgroup_remount. when we call cgroup_remount to remount the cgroup,the subsystem may be unlinked from cgroupfs_root->subsys_list in rebind_subsystem,this subsystem's files will not be removed in cgroup_clear_directroy. And the system will panic when we try to access these files. this patch fix it. Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> --- kernel/cgroup.c | 86 ++++++++++++++++++++++++++++++++++-------------------- 1 files changed, 54 insertions(+), 32 deletions(-) diff --git a/kernel/cgroup.c b/kernel/cgroup.c index e5233b7..90429d5 100644 --- a/kernel/cgroup.c +++ b/kernel/cgroup.c @@ -982,10 +982,14 @@ static void cgroup_clear_directory(struct dentry *dir, bool base_files, unsigned long subsys_mask) { struct cgroup *cgrp = __d_cgrp(dir); - struct cgroup_subsys *ss; + int i; - for_each_subsys(cgrp->root, ss) { + for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) { struct cftype_set *set; + struct cgroup_subsys *ss = subsys[i]; + + if (ss == NULL) + continue; if (!test_bit(ss->subsys_id, &subsys_mask)) continue; list_for_each_entry(set, &ss->cftsets, node) @@ -1160,6 +1164,40 @@ static int cgroup_show_options(struct seq_file *seq, struct dentry *dentry) return 0; } +static int cgroup_get_module_refcounts(unsigned long subsys_mask) +{ + int i; + bool module_pin_failed = false; + + for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) { + unsigned long bit = 1UL << i; + + if (!(bit & subsys_mask)) + continue; + if (!try_module_get(subsys[i]->module)) { + module_pin_failed = true; + break; + } + } + if (module_pin_failed) { + /* + * oops, one of the modules was going away. this means that we + * raced with a module_delete call, and to the user this is + * essentially a "subsystem doesn't exist" case. + */ + for (i--; i >= 0; i--) { + /* drop refcounts only on the ones we took */ + unsigned long bit = 1UL << i; + + if (!(bit & subsys_mask)) + continue; + module_put(subsys[i]->module); + } + return -ENOENT; + } + return 0; +} + struct cgroup_sb_opts { unsigned long subsys_mask; unsigned long flags; @@ -1185,7 +1223,6 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts) bool all_ss = false, one_ss = false; unsigned long mask = (unsigned long)-1; int i; - bool module_pin_failed = false; BUG_ON(!mutex_is_locked(&cgroup_mutex)); @@ -1324,34 +1361,8 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts) * take duplicate reference counts on a subsystem that's already used, * but rebind_subsystems handles this case. */ - for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) { - unsigned long bit = 1UL << i; - if (!(bit & opts->subsys_mask)) - continue; - if (!try_module_get(subsys[i]->module)) { - module_pin_failed = true; - break; - } - } - if (module_pin_failed) { - /* - * oops, one of the modules was going away. this means that we - * raced with a module_delete call, and to the user this is - * essentially a "subsystem doesn't exist" case. - */ - for (i--; i >= 0; i--) { - /* drop refcounts only on the ones we took */ - unsigned long bit = 1UL << i; - - if (!(bit & opts->subsys_mask)) - continue; - module_put(subsys[i]->module); - } - return -ENOENT; - } - - return 0; + return cgroup_get_module_refcounts(opts->subsys_mask); } static void drop_parsed_module_refcounts(unsigned long subsys_mask) @@ -1392,23 +1403,34 @@ static int cgroup_remount(struct super_block *sb, int *flags, char *data) removed_mask = root->subsys_mask & ~opts.subsys_mask; /* Don't allow flags or name to change at remount */ + ret = -EINVAL; if (opts.flags != root->flags || (opts.name && strcmp(opts.name, root->name))) { - ret = -EINVAL; drop_parsed_module_refcounts(opts.subsys_mask); goto out_unlock; } - ret = rebind_subsystems(root, opts.subsys_mask); + /* + * Add the reference of these removed subsystem,we + * need use subsystems to remove their cgroup files. + */ + ret = cgroup_get_module_refcounts(removed_mask); if (ret) { drop_parsed_module_refcounts(opts.subsys_mask); goto out_unlock; } + ret = rebind_subsystems(root, opts.subsys_mask); + if (ret) { + drop_parsed_module_refcounts(opts.subsys_mask | removed_mask); + goto out_unlock; + } + /* clear out any existing files and repopulate subsystem files */ cgroup_clear_directory(cgrp->dentry, false, removed_mask); /* re-populate subsystem files */ cgroup_populate_dir(cgrp, false, added_mask); + drop_parsed_module_refcounts(removed_mask); if (opts.release_agent) strcpy(root->release_agent_path, opts.release_agent); -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] cgroup: remove subsystem files when remounting cgroup 2012-11-30 16:21 ` [PATCH 2/2] cgroup: remove subsystem files when remounting cgroup Gao feng @ 2012-11-30 19:27 ` Tejun Heo 2012-12-03 0:45 ` Gao feng 2012-12-03 1:28 ` [PATCH RESEND] " Gao feng 1 sibling, 1 reply; 7+ messages in thread From: Tejun Heo @ 2012-11-30 19:27 UTC (permalink / raw) To: Gao feng; +Cc: cgroups, lizefan, linux-kernel, containers Hello, Gao. On Sat, Dec 01, 2012 at 12:21:29AM +0800, Gao feng wrote: > cgroup_clear_directroy is called by cgroup_d_remove_dir > and cgroup_remount. > > when we call cgroup_remount to remount the cgroup,the subsystem > may be unlinked from cgroupfs_root->subsys_list in rebind_subsystem,this > subsystem's files will not be removed in cgroup_clear_directroy. > And the system will panic when we try to access these files. > > this patch fix it. I'd really appreciated if you explain how it's fixed. > Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> > --- > kernel/cgroup.c | 86 ++++++++++++++++++++++++++++++++++-------------------- > 1 files changed, 54 insertions(+), 32 deletions(-) > > diff --git a/kernel/cgroup.c b/kernel/cgroup.c > index e5233b7..90429d5 100644 > --- a/kernel/cgroup.c > +++ b/kernel/cgroup.c > @@ -982,10 +982,14 @@ static void cgroup_clear_directory(struct dentry *dir, bool base_files, > unsigned long subsys_mask) > { > struct cgroup *cgrp = __d_cgrp(dir); > - struct cgroup_subsys *ss; > + int i; > > - for_each_subsys(cgrp->root, ss) { > + for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) { > struct cftype_set *set; > + struct cgroup_subsys *ss = subsys[i]; > + > + if (ss == NULL) > + continue; Ummm... I can't say I like it. It tries to work around the problem that the backing subsys may go away earlier by changing how the subsystems are iterated so that the "gone" subsystems can be iterated, which really doesn't seem like a good diea. I mean, what if somebody tries to access the files inbetween? root_cgrp->subsys[] would be %NULL and segfault all the same. > static void drop_parsed_module_refcounts(unsigned long subsys_mask) > @@ -1392,23 +1403,34 @@ static int cgroup_remount(struct super_block *sb, int *flags, char *data) > removed_mask = root->subsys_mask & ~opts.subsys_mask; > > /* Don't allow flags or name to change at remount */ > + ret = -EINVAL; > if (opts.flags != root->flags || > (opts.name && strcmp(opts.name, root->name))) { > - ret = -EINVAL; > drop_parsed_module_refcounts(opts.subsys_mask); > goto out_unlock; > } > > - ret = rebind_subsystems(root, opts.subsys_mask); > + /* > + * Add the reference of these removed subsystem,we > + * need use subsystems to remove their cgroup files. > + */ > + ret = cgroup_get_module_refcounts(removed_mask); > if (ret) { > drop_parsed_module_refcounts(opts.subsys_mask); > goto out_unlock; > } > > + ret = rebind_subsystems(root, opts.subsys_mask); > + if (ret) { > + drop_parsed_module_refcounts(opts.subsys_mask | removed_mask); > + goto out_unlock; > + } > + > /* clear out any existing files and repopulate subsystem files */ > cgroup_clear_directory(cgrp->dentry, false, removed_mask); > /* re-populate subsystem files */ > cgroup_populate_dir(cgrp, false, added_mask); > + drop_parsed_module_refcounts(removed_mask); So, how about performing cgroup_clear_directory() *before* rebind_subsystems() and then restore it afterwards? Thanks. -- tejun ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] cgroup: remove subsystem files when remounting cgroup 2012-11-30 19:27 ` Tejun Heo @ 2012-12-03 0:45 ` Gao feng 0 siblings, 0 replies; 7+ messages in thread From: Gao feng @ 2012-12-03 0:45 UTC (permalink / raw) To: Tejun Heo; +Cc: cgroups, lizefan, linux-kernel, containers Hello Tejun on 2012/12/01 03:27, Tejun Heo wrote: > Hello, Gao. > > On Sat, Dec 01, 2012 at 12:21:29AM +0800, Gao feng wrote: >> cgroup_clear_directroy is called by cgroup_d_remove_dir >> and cgroup_remount. >> >> when we call cgroup_remount to remount the cgroup,the subsystem >> may be unlinked from cgroupfs_root->subsys_list in rebind_subsystem,this >> subsystem's files will not be removed in cgroup_clear_directroy. >> And the system will panic when we try to access these files. >> >> this patch fix it. > > I'd really appreciated if you explain how it's fixed. > >> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> >> --- >> kernel/cgroup.c | 86 ++++++++++++++++++++++++++++++++++-------------------- >> 1 files changed, 54 insertions(+), 32 deletions(-) >> >> diff --git a/kernel/cgroup.c b/kernel/cgroup.c >> index e5233b7..90429d5 100644 >> --- a/kernel/cgroup.c >> +++ b/kernel/cgroup.c >> @@ -982,10 +982,14 @@ static void cgroup_clear_directory(struct dentry *dir, bool base_files, >> unsigned long subsys_mask) >> { >> struct cgroup *cgrp = __d_cgrp(dir); >> - struct cgroup_subsys *ss; >> + int i; >> >> - for_each_subsys(cgrp->root, ss) { >> + for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) { >> struct cftype_set *set; >> + struct cgroup_subsys *ss = subsys[i]; >> + >> + if (ss == NULL) >> + continue; > > Ummm... I can't say I like it. It tries to work around the problem > that the backing subsys may go away earlier by changing how the > subsystems are iterated so that the "gone" subsystems can be iterated, > which really doesn't seem like a good diea. I mean, what if somebody > tries to access the files inbetween? root_cgrp->subsys[] would be > %NULL and segfault all the same. > Yes,You are right.I didn't consider this problem. >> static void drop_parsed_module_refcounts(unsigned long subsys_mask) >> @@ -1392,23 +1403,34 @@ static int cgroup_remount(struct super_block *sb, int *flags, char *data) >> removed_mask = root->subsys_mask & ~opts.subsys_mask; >> >> /* Don't allow flags or name to change at remount */ >> + ret = -EINVAL; >> if (opts.flags != root->flags || >> (opts.name && strcmp(opts.name, root->name))) { >> - ret = -EINVAL; >> drop_parsed_module_refcounts(opts.subsys_mask); >> goto out_unlock; >> } >> >> - ret = rebind_subsystems(root, opts.subsys_mask); >> + /* >> + * Add the reference of these removed subsystem,we >> + * need use subsystems to remove their cgroup files. >> + */ >> + ret = cgroup_get_module_refcounts(removed_mask); >> if (ret) { >> drop_parsed_module_refcounts(opts.subsys_mask); >> goto out_unlock; >> } >> >> + ret = rebind_subsystems(root, opts.subsys_mask); >> + if (ret) { >> + drop_parsed_module_refcounts(opts.subsys_mask | removed_mask); >> + goto out_unlock; >> + } >> + >> /* clear out any existing files and repopulate subsystem files */ >> cgroup_clear_directory(cgrp->dentry, false, removed_mask); >> /* re-populate subsystem files */ >> cgroup_populate_dir(cgrp, false, added_mask); >> + drop_parsed_module_refcounts(removed_mask); > > So, how about performing cgroup_clear_directory() *before* > rebind_subsystems() and then restore it afterwards? > Get it,will fix this problem as you said. Thanks! ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH RESEND] cgroup: remove subsystem files when remounting cgroup 2012-11-30 16:21 ` [PATCH 2/2] cgroup: remove subsystem files when remounting cgroup Gao feng 2012-11-30 19:27 ` Tejun Heo @ 2012-12-03 1:28 ` Gao feng 2012-12-03 16:34 ` Tejun Heo 1 sibling, 1 reply; 7+ messages in thread From: Gao feng @ 2012-12-03 1:28 UTC (permalink / raw) To: tj; +Cc: cgroups, linux-kernel, containers, lizefan, Gao feng cgroup_clear_directroy is called by cgroup_d_remove_dir and cgroup_remount. when we call cgroup_remount to remount the cgroup,the subsystem may be unlinked from cgroupfs_root->subsys_list in rebind_subsystem,this subsystem's files will not be removed in cgroup_clear_directroy. And the system will panic when we try to access these files. this patch removes subsystems's files before rebind_subsystems, if rebind_subsystems failed, repopulate these removed files. With help from Tejun. Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> --- kernel/cgroup.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-) diff --git a/kernel/cgroup.c b/kernel/cgroup.c index e1293a9..afabd2a 100644 --- a/kernel/cgroup.c +++ b/kernel/cgroup.c @@ -1349,14 +1349,21 @@ static int cgroup_remount(struct super_block *sb, int *flags, char *data) goto out_unlock; } + /* + * Clear out the files of subsystems that should be removed,do + * this job before rebind_subsystems,since rebind_subsystems may + * change this hierarchy's subsys_list. + */ + cgroup_clear_directory(cgrp->dentry, false, removed_mask); + ret = rebind_subsystems(root, opts.subsys_mask); if (ret) { + /* rebind_subsystems failed,re-populate the removed files */ + cgroup_populate_dir(cgrp, false, removed_mask); drop_parsed_module_refcounts(opts.subsys_mask); goto out_unlock; } - /* clear out any existing files and repopulate subsystem files */ - cgroup_clear_directory(cgrp->dentry, false, removed_mask); /* re-populate subsystem files */ cgroup_populate_dir(cgrp, false, added_mask); -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] cgroup: remove subsystem files when remounting cgroup 2012-12-03 1:28 ` [PATCH RESEND] " Gao feng @ 2012-12-03 16:34 ` Tejun Heo 0 siblings, 0 replies; 7+ messages in thread From: Tejun Heo @ 2012-12-03 16:34 UTC (permalink / raw) To: Gao feng; +Cc: cgroups, linux-kernel, containers, lizefan On Mon, Dec 03, 2012 at 09:28:18AM +0800, Gao feng wrote: > cgroup_clear_directroy is called by cgroup_d_remove_dir > and cgroup_remount. > > when we call cgroup_remount to remount the cgroup,the subsystem > may be unlinked from cgroupfs_root->subsys_list in rebind_subsystem,this > subsystem's files will not be removed in cgroup_clear_directroy. > And the system will panic when we try to access these files. > > this patch removes subsystems's files before rebind_subsystems, > if rebind_subsystems failed, repopulate these removed files. > > With help from Tejun. > > Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> Applied to cgroup/for-3.8 with spaces added after commas. Thanks! -- tejun ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] cgroup: use cgroup_addrm_files() in cgroup_clear_directory() 2012-11-30 16:21 [PATCH 1/2] cgroup: use cgroup_addrm_files in cgroup_clear_directory Gao feng 2012-11-30 16:21 ` [PATCH 2/2] cgroup: remove subsystem files when remounting cgroup Gao feng @ 2012-11-30 19:47 ` Tejun Heo 1 sibling, 0 replies; 7+ messages in thread From: Tejun Heo @ 2012-11-30 19:47 UTC (permalink / raw) To: Gao feng; +Cc: cgroups, lizefan, linux-kernel, containers >From 879a3d9dbbde823ac77d39131e7a287f31b8296f Mon Sep 17 00:00:00 2001 From: Gao feng <gaofeng@cn.fujitsu.com> Date: Sat, 1 Dec 2012 00:21:28 +0800 cgroup_clear_directory() incorrectly invokes cgroup_rm_file() on each cftset of the target subsystems, which only removes the first file of each set. This leaves dangling files after subsystems are removed from a cgroup root via remount. Use cgroup_addrm_files() to remove all files of target subsystems. tj: Move cgroup_addrm_files() prototype decl upwards next to other global declarations. Commit message updated. Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> Signed-off-by: Tejun Heo <tj@kernel.org> --- Applied to cgroup/for-3.8 with minor updates. Thanks! kernel/cgroup.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/kernel/cgroup.c b/kernel/cgroup.c index b186a7e..e1293a9 100644 --- a/kernel/cgroup.c +++ b/kernel/cgroup.c @@ -246,6 +246,8 @@ static DEFINE_SPINLOCK(hierarchy_id_lock); static int need_forkexit_callback __read_mostly; static int cgroup_destroy_locked(struct cgroup *cgrp); +static int cgroup_addrm_files(struct cgroup *cgrp, struct cgroup_subsys *subsys, + struct cftype cfts[], bool is_add); #ifdef CONFIG_PROVE_LOCKING int cgroup_lock_is_held(void) @@ -964,7 +966,7 @@ static void cgroup_clear_directory(struct dentry *dir, bool base_files, if (!test_bit(ss->subsys_id, &subsys_mask)) continue; list_for_each_entry(set, &ss->cftsets, node) - cgroup_rm_file(cgrp, set->cfts); + cgroup_addrm_files(cgrp, NULL, set->cfts, false); } if (base_files) { while (!list_empty(&cgrp->files)) -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-12-03 16:34 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-11-30 16:21 [PATCH 1/2] cgroup: use cgroup_addrm_files in cgroup_clear_directory Gao feng 2012-11-30 16:21 ` [PATCH 2/2] cgroup: remove subsystem files when remounting cgroup Gao feng 2012-11-30 19:27 ` Tejun Heo 2012-12-03 0:45 ` Gao feng 2012-12-03 1:28 ` [PATCH RESEND] " Gao feng 2012-12-03 16:34 ` Tejun Heo 2012-11-30 19:47 ` [PATCH] cgroup: use cgroup_addrm_files() in cgroup_clear_directory() Tejun Heo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).