linux-kernel - Re: [PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from

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 for Android: free password hash cracker in your pocket

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

Message-ID: <YB0DnAlCaQza4Uf9@dhcp22.suse.cz>
Date:   Fri, 5 Feb 2021 09:36:44 +0100
From:   Michal Hocko <mhocko@...e.com>
To:     Muchun Song <songmuchun@...edance.com>
Cc:     hannes@...xchg.org, vdavydov.dev@...il.com,
        akpm@...ux-foundation.org, cgroups@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: memcontrol: remove rcu_read_lock from
 get_mem_cgroup_from_page

On Fri 05-02-21 14:27:19, Muchun Song wrote:
> The get_mem_cgroup_from_page() is called under page lock, so the page
> memcg cannot be changed under us.

Where is the page lock enforced?

> Also, css_get is enough because page
> has a reference to the memcg.

tryget used to be there to guard against offlined memcg but we have
concluded this is impossible in this path. tryget stayed there to catch
some unexpected cases IIRC.

> If we really want to make the get_mem_cgroup_from_page() suitable for
> arbitrary page, we should use page_memcg_rcu() instead of page_memcg()
> and call it after rcu_read_lock().

What is the primary motivation to change this code? is the overhead of
tryget/RCU something that needs optimizing?
 
> Signed-off-by: Muchun Song <songmuchun@...edance.com>
> ---
>  mm/memcontrol.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 87f01bc05d1f..6c7f1ea3955e 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1063,16 +1063,15 @@ EXPORT_SYMBOL(get_mem_cgroup_from_mm);
>   */
>  struct mem_cgroup *get_mem_cgroup_from_page(struct page *page)
>  {
> -	struct mem_cgroup *memcg = page_memcg(page);
> +	struct mem_cgroup *memcg;
>  
>  	if (mem_cgroup_disabled())
>  		return NULL;
>  
> -	rcu_read_lock();
>  	/* Page should not get uncharged and freed memcg under us. */
> -	if (!memcg || WARN_ON_ONCE(!css_tryget(&memcg->css)))
> -		memcg = root_mem_cgroup;
> -	rcu_read_unlock();
> +	memcg = page_memcg(page) ? : root_mem_cgroup;
> +	css_get(&memcg->css);
> +
>  	return memcg;
>  }
>  EXPORT_SYMBOL(get_mem_cgroup_from_page);
> -- 
> 2.11.0

-- 
Michal Hocko
SUSE Labs