From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tejun Heo Subject: Re: [PATCH 2/2] cgroup: remove subsystem files when remounting cgroup Date: Fri, 30 Nov 2012 11:27:35 -0800 Message-ID: <20121130192735.GF3873@htj.dyndns.org> References: <1354292489-18434-1-git-send-email-gaofeng@cn.fujitsu.com> <1354292489-18434-2-git-send-email-gaofeng@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=86/NNo7b5z4/XzGMvd0qSZIVjXsDRhRcA82LWGRDq94=; b=gUOCPhMDq+gNAwq+T9PSKX1y4gONBT1B7TdgNw+w/ZQ39fJTMVh7nJ9aXVxCyiFniU nKuEasu1ZeQfuJo7I0S3LrOehbI+1m2iT8ttQfnwfu4cn5kQYVAJyd0Fed9DBAbsignB 12ynmoh48QxnhjlxXpso4oZkd5qPX6XFuyPlueqdewfqNTR6cuwabtH7Sy1/A9E3bGdF RrKbyab/sIWjgWqqGlDsETseH8LALlpf36yNfxg7rRa8QOTmpQvDKUajmwFfJ/CsoT4R iLT+kil2607XyUAh9X/jVErTCK5xVWHIPnnYaH1nd2T5SZQYNdIf5Re0HDIf2dNU64m+ VkPw== Content-Disposition: inline In-Reply-To: <1354292489-18434-2-git-send-email-gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Gao feng Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 > --- > 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