| 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
| ||
|
Date: Wed, 10 Dec 2008 11:28:15 +0900
From: Daisuke Nishimura <nishimura@....nes.nec.co.jp>
To: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
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>,
nishimura@....nes.nec.co.jp
Subject: Re: [RFC][PATCH 1/6] memcg: fix pre_destory handler
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.
> 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 ?
> 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.)
> + 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 ?
> mutex_lock(&cgroup_mutex);
> @@ -2483,12 +2497,14 @@ static int cgroup_rmdir(struct inode *un
> if (atomic_read(&cgrp->count)
> || !list_empty(&cgrp->children)
> || cgroup_has_css_refs(cgrp)) {
> + clear_bit(CGRP_PRE_REMOVAL, &cgrp->flags);
> mutex_unlock(&cgroup_mutex);
> return -EBUSY;
> }
>
> spin_lock(&release_list_lock);
> set_bit(CGRP_REMOVED, &cgrp->flags);
> + clear_bit(CGRP_PRE_REMOVAL, &cgrp->flags);
> if (!list_empty(&cgrp->release_list))
> list_del(&cgrp->release_list);
> spin_unlock(&release_list_lock);
>
Thanks,
Daisuke Nishimura.
--
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