| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20081210115830.dd13b90b.kamezawa.hiroyu@jp.fujitsu.com>
Date: Wed, 10 Dec 2008 11:58:30 +0900
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
To: Daisuke Nishimura <nishimura@....nes.nec.co.jp>
Cc: "linux-mm@...ck.org" <linux-mm@...ck.org>,
"balbir@...ux.vnet.ibm.com" <balbir@...ux.vnet.ibm.com>,
"lizf@...fujitsu.com" <lizf@...fujitsu.com>,
"menage@...gle.com" <menage@...gle.com>,
"kosaki.motohiro@...fujitsu.com" <kosaki.motohiro@...fujitsu.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH 1/6] memcg: fix pre_destory handler
On Wed, 10 Dec 2008 11:28:15 +0900
Daisuke Nishimura <nishimura@....nes.nec.co.jp> wrote:
> On Tue, 9 Dec 2008 20:06:47 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com> wrote:
> > better name for new flag is welcome.
> >
> > ==
> > Because pre_destroy() handler is moved out to cgroup_lock() for
> > avoiding dead-lock, now, cgroup's rmdir() does following sequence.
> >
> > cgroup_lock()
> > check children and tasks.
> > (A)
> > cgroup_unlock()
> > (B)
> > pre_destroy() for subsys;-----(1)
> > (C)
> > cgroup_lock();
> > (D)
> > Second check:check for -EBUSY again because we released the lock.
> > (E)
> > mark cgroup as removed.
> > (F)
> > unlink from lists.
> > cgroup_unlock();
> > dput()
> > => when dentry's refcnt goes down to 0
> > destroy() handers for subsys
> >
> > memcg marks itself as "obsolete" when pre_destroy() is called at (1)
> > But rmdir() can fail after pre_destroy(). So marking "obsolete" is bug.
> > I'd like to fix sanity of pre_destroy() in cgroup layer.
> >
> > Considering above sequence, new tasks can be added while
> > (B) and (C)
> > swap-in recored can be charged back to a cgroup after pre_destroy()
> > at (C) and (D), (E)
> > (means cgrp's refcnt not comes from task but from other persistent objects.)
> >
> > This patch adds "cgroup_is_being_removed()" check. (better name is welcome)
> > After this,
> >
> > - cgroup is marked as CGRP_PRE_REMOVAL at (A)
> > - If Second check fails, CGRP_PRE_REMOVAL flag is removed.
> > - memcg's its own obsolete flag is removed.
> > - While CGROUP_PRE_REMOVAL, task attach will fail by -EBUSY.
> > (task attach via clone() will not hit the case.)
> >
> > By this, we can trust pre_restroy()'s result.
> >
> >
> I agrree to the direction of this patch, but I think it would be better
> to split this into cgroup and memcg part.
>
Hmm, but "showing usage" part is necessary for this kind of patches.
> > Note: if CGRP_REMOVED can be set and cleared, it should be used instead of
> > CGRP_PRE_REMOVAL.
> >
> >
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
> >
> > ---
> > include/linux/cgroup.h | 5 +++++
> > kernel/cgroup.c | 18 +++++++++++++++++-
> > mm/memcontrol.c | 36 ++++++++++++++++++++++++++----------
> > 3 files changed, 48 insertions(+), 11 deletions(-)
> >
> > Index: mmotm-2.6.28-Dec08/mm/memcontrol.c
> > ===================================================================
> > --- mmotm-2.6.28-Dec08.orig/mm/memcontrol.c
> > +++ mmotm-2.6.28-Dec08/mm/memcontrol.c
> > @@ -166,7 +166,6 @@ struct mem_cgroup {
> > */
> > bool use_hierarchy;
> > unsigned long last_oom_jiffies;
> > - int obsolete;
> > atomic_t refcnt;
> >
> > unsigned int swappiness;
> > @@ -211,6 +210,24 @@ pcg_default_flags[NR_CHARGE_TYPE] = {
> > static void mem_cgroup_get(struct mem_cgroup *mem);
> > static void mem_cgroup_put(struct mem_cgroup *mem);
> >
> > +static bool memcg_is_obsolete(struct mem_cgroup *mem)
> > +{
> > + struct cgroup *cg = mem->css.cgroup;
> > + /*
> > + * "Being Removed" means pre_destroy() handler is called.
> > + * After "pre_destroy" handler is called, memcg should not
> > + * have any additional charges.
> > + * This means there are small races for mis-accounting. But this
> > + * mis-accounting should happen only under swap-in opration.
> > + * (Attachin new task will fail if cgroup is under rmdir()).
> > + */
> > +
> > + if (!cg || cgroup_is_removed(cg) || cgroup_is_being_removed(cg))
> > + return true;
> > + return false;
> > +}
> > +
> > +
> > static void mem_cgroup_charge_statistics(struct mem_cgroup *mem,
> > struct page_cgroup *pc,
> > bool charge)
> > @@ -597,7 +614,7 @@ mem_cgroup_get_first_node(struct mem_cgr
> > struct cgroup *cgroup;
> > struct mem_cgroup *ret;
> > bool obsolete = (root_mem->last_scanned_child &&
> > - root_mem->last_scanned_child->obsolete);
> > + memcg_is_obsolete(root_mem->last_scanned_child);
> >
> > /*
> > * Scan all children under the mem_cgroup mem
> > @@ -1070,7 +1087,7 @@ int mem_cgroup_try_charge_swapin(struct
> > ent.val = page_private(page);
> >
> > mem = lookup_swap_cgroup(ent);
> > - if (!mem || mem->obsolete)
> > + if (!mem || memcg_is_obsolete(mem))
> > goto charge_cur_mm;
> > *ptr = mem;
> > return __mem_cgroup_try_charge(NULL, mask, ptr, true);
> > @@ -1104,7 +1121,7 @@ int mem_cgroup_cache_charge_swapin(struc
> > ent.val = page_private(page);
> > if (do_swap_account) {
> > mem = lookup_swap_cgroup(ent);
> > - if (mem && mem->obsolete)
> > + if (mem && memcg_is_obsolete(mem))
> > mem = NULL;
> > if (mem)
> > mm = NULL;
> > @@ -2050,9 +2067,6 @@ static struct mem_cgroup *mem_cgroup_all
> > * the number of reference from swap_cgroup and free mem_cgroup when
> > * it goes down to 0.
> > *
> > - * When mem_cgroup is destroyed, mem->obsolete will be set to 0 and
> > - * entry which points to this memcg will be ignore at swapin.
> > - *
> > * Removal of cgroup itself succeeds regardless of refs from swap.
> > */
> >
> > @@ -2081,7 +2095,7 @@ static void mem_cgroup_get(struct mem_cg
> > static void mem_cgroup_put(struct mem_cgroup *mem)
> > {
> > if (atomic_dec_and_test(&mem->refcnt)) {
> > - if (!mem->obsolete)
> > + if (!memcg_is_obsolete(mem))
> > return;
> > mem_cgroup_free(mem);
> > }
> > @@ -2148,14 +2162,16 @@ static void mem_cgroup_pre_destroy(struc
> > struct cgroup *cont)
> > {
> > struct mem_cgroup *mem = mem_cgroup_from_cont(cont);
> > - mem->obsolete = 1;
> > mem_cgroup_force_empty(mem, false);
> > }
> >
> > static void mem_cgroup_destroy(struct cgroup_subsys *ss,
> > struct cgroup *cont)
> > {
> > - mem_cgroup_free(mem_cgroup_from_cont(cont));
> > + struct mem_cgroup *mem = mem_cgroup_from_cont(cont):
> > + mem_cgroup_free(mem);
> > + /* forget */
> > + mem->css.cgroup = NULL;
> > }
> >
> Is it OK to access "mem" while it may have been freed ?
>
no, will fix.
> > static int mem_cgroup_populate(struct cgroup_subsys *ss,
> > Index: mmotm-2.6.28-Dec08/include/linux/cgroup.h
> > ===================================================================
> > --- mmotm-2.6.28-Dec08.orig/include/linux/cgroup.h
> > +++ mmotm-2.6.28-Dec08/include/linux/cgroup.h
> > @@ -98,6 +98,8 @@ enum {
> > CGRP_RELEASABLE,
> > /* Control Group requires release notifications to userspace */
> > CGRP_NOTIFY_ON_RELEASE,
> > + /* Control Group is preparing for death */
> > + CGRP_PRE_REMOVAL,
> > };
> >
> > struct cgroup {
> > @@ -303,8 +305,11 @@ int cgroup_add_files(struct cgroup *cgrp
> > const struct cftype cft[],
> > int count);
> >
> > +
> > int cgroup_is_removed(const struct cgroup *cgrp);
> >
> > +int cgroup_is_being_removed(const struct cgroup *cgrp);
> > +
> > int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen);
> >
> > int cgroup_task_count(const struct cgroup *cgrp);
> > Index: mmotm-2.6.28-Dec08/kernel/cgroup.c
> > ===================================================================
> > --- mmotm-2.6.28-Dec08.orig/kernel/cgroup.c
> > +++ mmotm-2.6.28-Dec08/kernel/cgroup.c
> > @@ -123,6 +123,11 @@ inline int cgroup_is_removed(const struc
> > return test_bit(CGRP_REMOVED, &cgrp->flags);
> > }
> >
> > +inline int cgroup_is_being_removed(const struct cgroup *cgrp)
> > +{
> > + return test_bit(CGRP_PRE_REMOVAL, &cgrp->flags);
> > +}
> > +
> > /* bits in struct cgroupfs_root flags field */
> > enum {
> > ROOT_NOPREFIX, /* mounted subsystems have no named prefix */
> > @@ -1217,6 +1222,13 @@ int cgroup_attach_task(struct cgroup *cg
> > if (cgrp == oldcgrp)
> > return 0;
> >
> > + /*
> > + * This cgroup is under rmdir() operation. Never fails here when this
> > + * is called from clone().
> > + */
> I don't think clone() calls cgroup_attach_task().
> Do you mean cgroup_clone() ?
> (I'm sorry that I'm not good at ns_cgroup.)
>
yes, cgroup_clone(). I'll update.
> > + if (cgroup_is_being_removed(cgrp))
> > + return -EBUSY;
> > +
> > for_each_subsys(root, ss) {
> > if (ss->can_attach) {
> > retval = ss->can_attach(ss, cgrp, tsk);
> > @@ -2469,12 +2481,14 @@ static int cgroup_rmdir(struct inode *un
> > mutex_unlock(&cgroup_mutex);
> > return -EBUSY;
> > }
> > - mutex_unlock(&cgroup_mutex);
> >
> > /*
> > * Call pre_destroy handlers of subsys. Notify subsystems
> > * that rmdir() request comes.
> > */
> > + set_bit(CGRP_PRE_REMOVAL, &cgrp->flags);
> > + mutex_unlock(&cgroup_mutex);
> > +
> > cgroup_call_pre_destroy(cgrp);
> >
> Is there any case where pre_destory is called simultaneusly ?
>
I can't catch what is your concern.
AFAIK, vfs_rmdir() is done under mutex to dentry
==
mutex_lock_nested(&nd.path.dentry->d_inode->i_mutex, I_MUTEX_PARENT);
...
vfs_rmdir()
==
And calls to cgroup_rmdir() will be serialized.
Thanks,
-Kame
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists