| 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: Thu, 6 Jan 2011 11:12:00 +0530
From: Balbir Singh <balbir@...ux.vnet.ibm.com>
To: Daisuke Nishimura <nishimura@....nes.nec.co.jp>
Cc: Minchan Kim <minchan.kim@...il.com>,
Johannes Weiner <hannes@...xchg.org>,
Andrew Morton <akpm@...ux-foundation.org>,
KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
LKML <linux-kernel@...r.kernel.org>,
linux-mm <linux-mm@...ck.org>
Subject: Re: [BUGFIX][PATCH v3] memcg: fix memory migration of shmem
swapcache
* nishimura@....nes.nec.co.jp <nishimura@....nes.nec.co.jp> [2011-01-06 12:34:15]:
> > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > > index 159a076..cc5a8fd 100644
> > > --- a/include/linux/memcontrol.h
> > > +++ b/include/linux/memcontrol.h
> > > @@ -93,7 +93,7 @@ extern int
> > > mem_cgroup_prepare_migration(struct page *page,
> > > struct page *newpage, struct mem_cgroup **ptr);
> > > extern void mem_cgroup_end_migration(struct mem_cgroup *mem,
> > > - struct page *oldpage, struct page *newpage);
> > > + struct page *oldpage, struct page *newpage, bool success);
> >
> > The term "success" implies present or future tense.
> > The event this variable cares about in the past so "succeed" might be
> > a more appropriate term.
> > Sorry to be picky about the English but there is an important
> > distinction here since we don't have any comment about the variable.
> >
> > Am I being too fussy?
> Not at all. Your comments are very helpful to make the code more readable :)
>
> ===
> From: Daisuke Nishimura <nishimura@....nes.nec.co.jp>
>
> In current implimentation, mem_cgroup_end_migration() decides whether the page
> migration has succeeded or not by checking "oldpage->mapping".
>
> But if we are tring to migrate a shmem swapcache, the page->mapping of it is
> NULL from the begining, so the check would be invalid.
> As a result, mem_cgroup_end_migration() assumes the migration has succeeded
> even if it's not, so "newpage" would be freed while it's not uncharged.
>
> This patch fixes it by passing mem_cgroup_end_migration() the result of the
> page migration.
>
> Signed-off-by: Daisuke Nishimura <nishimura@....nes.nec.co.jp>
> Reviewed-by: Minchan Kim <minchan.kim@...il.com>
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
> ---
> v2->v3
> - s/success/succeed
>
> v1->v2
> - pass mem_cgroup_end_migration() "bool" instead of "int".
>
> include/linux/memcontrol.h | 5 ++---
> mm/memcontrol.c | 5 ++---
> mm/migrate.c | 2 +-
> 3 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 159a076..9f52b57 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -93,7 +93,7 @@ extern int
> mem_cgroup_prepare_migration(struct page *page,
> struct page *newpage, struct mem_cgroup **ptr);
> extern void mem_cgroup_end_migration(struct mem_cgroup *mem,
> - struct page *oldpage, struct page *newpage);
> + struct page *oldpage, struct page *newpage, bool succeed);
Sorry for nit-picking but succeed is not as good as succeeded,
successful, successful_migration or migration_ok
>
> /*
> * For memory reclaim.
> @@ -231,8 +231,7 @@ mem_cgroup_prepare_migration(struct page *page, struct page *newpage,
> }
>
> static inline void mem_cgroup_end_migration(struct mem_cgroup *mem,
> - struct page *oldpage,
> - struct page *newpage)
> + struct page *oldpage, struct page *newpage, bool succeed)
> {
> }
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 61678be..71a39bc 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2856,7 +2856,7 @@ int mem_cgroup_prepare_migration(struct page *page,
>
> /* remove redundant charge if migration failed*/
> void mem_cgroup_end_migration(struct mem_cgroup *mem,
> - struct page *oldpage, struct page *newpage)
> + struct page *oldpage, struct page *newpage, bool succeed)
> {
> struct page *used, *unused;
> struct page_cgroup *pc;
> @@ -2865,8 +2865,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *mem,
> return;
> /* blocks rmdir() */
> cgroup_exclude_rmdir(&mem->css);
> - /* at migration success, oldpage->mapping is NULL. */
> - if (oldpage->mapping) {
> + if (!succeed) {
> used = oldpage;
> unused = newpage;
> } else {
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 6ae8a66..be66b23 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -756,7 +756,7 @@ rcu_unlock:
> rcu_read_unlock();
> uncharge:
> if (!charge)
> - mem_cgroup_end_migration(mem, page, newpage);
> + mem_cgroup_end_migration(mem, page, newpage, rc == 0);
> unlock:
> unlock_page(page);
>
> --
> 1.7.1
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@...ck.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
> Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>
--
Three Cheers,
Balbir
--
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