linux-kernel - Re: [PATCH v3 06/13] memcg: kmem controller infrastructure

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAmzW4ONnc7n3kZbYnE6n2Cg0ZyPXW0QU2NMr0uRkyTxnGpNqQ@mail.gmail.com>
Date:	Fri, 21 Sep 2012 01:05:47 +0900
From:	JoonSoo Kim <js1304@...il.com>
To:	Glauber Costa <glommer@...allels.com>
Cc:	linux-kernel@...r.kernel.org, cgroups@...r.kernel.org,
	kamezawa.hiroyu@...fujitsu.com, devel@...nvz.org,
	Tejun Heo <tj@...nel.org>, linux-mm@...ck.org,
	Suleiman Souhlal <suleiman@...gle.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Mel Gorman <mgorman@...e.de>,
	David Rientjes <rientjes@...gle.com>,
	Christoph Lameter <cl@...ux.com>,
	Pekka Enberg <penberg@...helsinki.fi>,
	Michal Hocko <mhocko@...e.cz>,
	Johannes Weiner <hannes@...xchg.org>
Subject: Re: [PATCH v3 06/13] memcg: kmem controller infrastructure

Hi, Glauber.

2012/9/18 Glauber Costa <glommer@...allels.com>:
> +/*
> + * We need to verify if the allocation against current->mm->owner's memcg is
> + * possible for the given order. But the page is not allocated yet, so we'll
> + * need a further commit step to do the final arrangements.
> + *
> + * It is possible for the task to switch cgroups in this mean time, so at
> + * commit time, we can't rely on task conversion any longer.  We'll then use
> + * the handle argument to return to the caller which cgroup we should commit
> + * against. We could also return the memcg directly and avoid the pointer
> + * passing, but a boolean return value gives better semantics considering
> + * the compiled-out case as well.
> + *
> + * Returning true means the allocation is possible.
> + */
> +bool
> +__memcg_kmem_newpage_charge(gfp_t gfp, struct mem_cgroup **_memcg, int order)
> +{
> +       struct mem_cgroup *memcg;
> +       bool ret;
> +       struct task_struct *p;
> +
> +       *_memcg = NULL;
> +       rcu_read_lock();
> +       p = rcu_dereference(current->mm->owner);
> +       memcg = mem_cgroup_from_task(p);
> +       rcu_read_unlock();
> +
> +       if (!memcg_can_account_kmem(memcg))
> +               return true;
> +
> +       mem_cgroup_get(memcg);
> +
> +       ret = memcg_charge_kmem(memcg, gfp, PAGE_SIZE << order) == 0;
> +       if (ret)
> +               *_memcg = memcg;
> +       else
> +               mem_cgroup_put(memcg);
> +
> +       return ret;
> +}

"*_memcg = memcg" should be executed when "memcg_charge_kmem" is success.
"memcg_charge_kmem" return 0 if success in charging.
Therefore, I think this code is wrong.
If I am right, it is a serious bug that affect behavior of all the patchset.

> +void __memcg_kmem_commit_charge(struct page *page, struct mem_cgroup *memcg,
> +                             int order)
> +{
> +       struct page_cgroup *pc;
> +
> +       WARN_ON(mem_cgroup_is_root(memcg));
> +
> +       /* The page allocation failed. Revert */
> +       if (!page) {
> +               memcg_uncharge_kmem(memcg, PAGE_SIZE << order);
> +               return;
> +       }

In case of "!page ", mem_cgroup_put(memcg) is needed,
because we already call "mem_cgroup_get(memcg)" in
__memcg_kmem_newpage_charge().
I know that mem_cgroup_put()/get() will be removed in later patch, but
it is important that every patch works fine.

Thanks.
--
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/