| 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: <20090115133814.a52460fa.nishimura@mxp.nes.nec.co.jp>
Date: Thu, 15 Jan 2009 13:38:14 +0900
From: Daisuke Nishimura <nishimura@....nes.nec.co.jp>
To: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Cc: nishimura@....nes.nec.co.jp, "LKML" <linux-kernel@...r.kernel.org>,
"linux-mm" <linux-mm@...ck.org>,
"Andrew Morton" <akpm@...ux-foundation.org>,
"Balbir Singh" <balbir@...ux.vnet.ibm.com>,
"Pavel Emelyanov" <xemul@...nvz.org>,
"Li Zefan" <lizf@...fujitsu.com>, "Paul Menage" <menage@...gle.com>
Subject: Re: [RFC][PATCH 5/4] memcg: don't call res_counter_uncharge when
obsolete
On Thu, 15 Jan 2009 11:14:20 +0900, Daisuke Nishimura <nishimura@....nes.nec.co.jp> wrote:
> > > > To handle the problem "parent may be obsolete",
> > > >
> > > > call mem_cgroup_get(parent) at create()
> > > > call mem_cgroup_put(parent) at freeing memcg.
> > > > (regardless of use_hierarchy.)
> > > >
> > > > is clearer way to go, I think.
> > > >
> > > > I wonder whether there is mis-accounting problem or not..
> > > >
hmm, after more consideration, although this patch can prevent the BUG,
it can leak memsw accounting of parents because memsw of parents, which
have been incremented by charge, does not decremented.
I'll try pet/put parent approach..
Or any other good ideas ?
Thanks,
Daisuke Nishimura.
> > > > So, adding css_tryget() around problematic code can be a fix.
> > > > --
> > > > mem = swap_cgroup_record();
> > > > if (css_tryget(&mem->css)) {
> > > > res_counter_uncharge(&mem->memsw, PAZE_SIZE);
> > > > css_put(&mem->css)
> > > > }
> > > > --
> > > > I like css_tryget() rather than mem_cgroup_obsolete().
> > > I agree.
> > > The updated version is attached.
> > >
> > >
> > > Thanks,
> > > Daisuke nishimura.
> > >
> > > > To be honest, I'd like to remove memcg special stuff when I can.
> > > >
> > > > Thanks,
> > > > -Kame
> > > >
> > > ===
> > > From: Daisuke Nishimura <nishimura@....nes.nec.co.jp>
> > >
> > > mem_cgroup_get ensures that the memcg that has been got can be accessed
> > > even after the directory has been removed, but it doesn't ensure that parents
> > > of it can be accessed: parents might have been freed already by rmdir.
> > >
> > > This causes a bug in case of use_hierarchy==1, because res_counter_uncharge
> > > climb up the tree.
> > >
> > > Check if the memcg is obsolete by css_tryget, and don't call
> > > res_counter_uncharge when obsole.
> > >
> > > Signed-off-by: Daisuke Nishimura <nishimura@....nes.nec.co.jp>
> > seems nice loock.
> >
> >
> > > ---
> > > mm/memcontrol.c | 15 ++++++++++++---
> > > 1 files changed, 12 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index fb62b43..4e3b100 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -1182,7 +1182,10 @@ int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
> > > /* avoid double counting */
> > > mem = swap_cgroup_record(ent, NULL);
> > > if (mem) {
> > > - res_counter_uncharge(&mem->memsw, PAGE_SIZE);
> > > + if (!css_tryget(&mem->css)) {
> > > + res_counter_uncharge(&mem->memsw, PAGE_SIZE);
> > > + css_put(&mem->css);
> > > + }
> > > mem_cgroup_put(mem);
> > > }
> > > }
> >
> > I think css_tryget() returns "ture" at success....
> >
> > So,
> > ==
> > if (mem && css_tryget(&mem->css))
> > res_counter....
> >
> > is correct.
> >
> > -Kame
> >
> Ooops! you are right.
> Sorry for my silly mistake..
>
> "mem" is checked beforehand, so I think css_tryget would be enough.
> I'm now testing the attached one.
>
>
> Thanks,
> Daisuke Nishimura.
> ===
> From: Daisuke Nishimura <nishimura@....nes.nec.co.jp>
>
> mem_cgroup_get ensures that the memcg that has been got can be accessed
> even after the directory has been removed, but it doesn't ensure that parents
> of it can be accessed: parents might have been freed already by rmdir.
>
> This causes a bug in case of use_hierarchy==1, because res_counter_uncharge
> climb up the tree.
>
> Check if the memcg is obsolete by css_tryget, and don't call
> res_counter_uncharge when obsole.
>
> Signed-off-by: Daisuke Nishimura <nishimura@....nes.nec.co.jp>
> ---
> mm/memcontrol.c | 15 ++++++++++++---
> 1 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index fb62b43..b9d5271 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1182,7 +1182,10 @@ int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
> /* avoid double counting */
> mem = swap_cgroup_record(ent, NULL);
> if (mem) {
> - res_counter_uncharge(&mem->memsw, PAGE_SIZE);
> + if (css_tryget(&mem->css)) {
> + res_counter_uncharge(&mem->memsw, PAGE_SIZE);
> + css_put(&mem->css);
> + }
> mem_cgroup_put(mem);
> }
> }
> @@ -1252,7 +1255,10 @@ void mem_cgroup_commit_charge_swapin(struct page *page, struct mem_cgroup *ptr)
> struct mem_cgroup *memcg;
> memcg = swap_cgroup_record(ent, NULL);
> if (memcg) {
> - res_counter_uncharge(&memcg->memsw, PAGE_SIZE);
> + if (css_tryget(&memcg->css)) {
> + res_counter_uncharge(&memcg->memsw, PAGE_SIZE);
> + css_put(&memcg->css);
> + }
> mem_cgroup_put(memcg);
> }
>
> @@ -1397,7 +1403,10 @@ void mem_cgroup_uncharge_swap(swp_entry_t ent)
>
> memcg = swap_cgroup_record(ent, NULL);
> if (memcg) {
> - res_counter_uncharge(&memcg->memsw, PAGE_SIZE);
> + if (css_tryget(&memcg->css)) {
> + res_counter_uncharge(&memcg->memsw, PAGE_SIZE);
> + css_put(&memcg->css);
> + }
> mem_cgroup_put(memcg);
> }
> }
--
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