linux-kernel - Re: [PATCH 4/5] khugepaged: remove unnecessary mem_cgroup_uncharge() in collapse_[file|huge

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] [day] [month] [year] [list]

Date:   Sat, 6 Mar 2021 11:18:38 +0800
From:   Miaohe Lin <linmiaohe@...wei.com>
To:     "Kirill A. Shutemov" <kirill@...temov.name>
CC:     <akpm@...ux-foundation.org>, <riel@...hat.com>,
        <kirill.shutemov@...ux.intel.com>, <ebru.akagunduz@...il.com>,
        <dan.carpenter@...cle.com>, <linux-kernel@...r.kernel.org>,
        <linux-mm@...ck.org>
Subject: Re: [PATCH 4/5] khugepaged: remove unnecessary mem_cgroup_uncharge()
 in collapse_[file|huge_page]

On 2021/3/6 1:38, Kirill A. Shutemov wrote:
> On Thu, Mar 04, 2021 at 07:30:12AM -0500, Miaohe Lin wrote:
>> Since commit 7ae88534cdd9 ("mm: move mem_cgroup_uncharge out of
>> __page_cache_release()"), the mem_cgroup will be uncharged when hpage is
>> freed. Uncharge mem_cgroup here is harmless but it looks confusing and
>> buggy: if mem_cgroup charge failed, we will call mem_cgroup_uncharge()
>> uncorrectly in error path because hpage is not IS_ERR_OR_NULL().
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@...wei.com>
> 
> Hm. I'm not sure about this patch.
> 
> For !NUMA the page will get allocated and freed very early: in
> khugepaged_do_scan() and with the change mem_cgroup_charge() may get
> called twice for two different mm_structs.

Many thanks for point it out.

> > Is it safe?

I'am sorry I missed the !NUMA case! :(

In !NUMA case, hpage may not be freed in the khugepaged_do_scan() while loop. Thus mem_cgroup_charge()
may get called twice for two different mm_structs. In fact, mem_cgroup_uncharge() may also get called
twice __but__ it's safe to do this.

The imbalance of mem_cgroup_charge() and mem_cgroup_uncharge() looks buggy and weird __but__ it's safe
to call mem_cgroup_uncharge() many times with or without a successful mem_cgroup_charge() call.
So I would drop this patch.

> 

Thanks again.