linux-kernel - Re: [PATCH v2 2/4] mm/migration: remove unneeded lock page and PageMovable check

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: <0724b4c4-15f6-e429-f945-f57c619c7270@redhat.com>
Date:   Tue, 31 May 2022 13:59:54 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Miaohe Lin <linmiaohe@...wei.com>
Cc:     ying.huang@...el.com, hch@....de, dhowells@...hat.com,
        cl@...ux.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        akpm@...ux-foundation.org, mike.kravetz@...cle.com,
        naoya.horiguchi@....com, Minchan Kim <minchan@...nel.org>
Subject: Re: [PATCH v2 2/4] mm/migration: remove unneeded lock page and
 PageMovable check

Sorry for the late reply, was on vacation.

>>>
>>> But for isolated page, PageLRU is cleared. So when the isolated page is released, __clear_page_lru_flags
>>> won't be called. So we have to clear the PG_active and PG_unevictable here manully. So I think
>>> this code block works. Or am I miss something again?
>>
>> Let's assume the following: page as freed by the owner and we enter
>> unmap_and_move().
>>
>>
>> #1: enter unmap_and_move() // page_count is 1
>> #2: enter isolate_movable_page() // page_count is 1
>> #2: get_page_unless_zero() // page_count is now 2
>> #1: if (page_count(page) == 1) { // does not trigger
>> #2: put_page(page); // page_count is now 1
>> #1: put_page(page); // page_count is now 0 -> freed
>>
>>
>> #1 will trigger __put_page() -> __put_single_page() ->
>> __page_cache_release() will not clear the flags because it's not an LRU
>> page at that point in time, right (-> isolated)?
> 
> Sorry, you're right. I thought the old page will be freed via putback_lru_page which will
> set PageLRU back instead of put_page directly. So if the above race occurs, PG_active and
> PG_unevictable will remain set while page goes to the buddy and check_free_page will complain
> about it. But it seems this is never witnessed?

Maybe

a) we were lucky so far and didn't trigger it
b) the whole code block is dead code because we are missing something
c) we are missing something else :)

> 
>>
>> We did not run that code block that would clear PG_active and
>> PG_unevictable.
>>
>> Which still leaves the questions:
>>
>> a) If PG_active and PG_unevictable was cleared, where?
> 
> For LRU pages, PG_active and PG_unevictable are cleared via __page_cache_release. And for isolated
> (LRU) pages, PG_active and PG_unevictable should be cleared ourselves?
> 
>> b) Why is that code block that conditionally clears the flags of any
>> value and why can't we simply drop it?
>>
> 
> To fix the issue, should we clear PG_active and PG_unevictable unconditionally here?

I wonder if we should simply teach actual freeing code to simply clear
both flags when freeing an isolated page? IOW, to detect "isolated LRU"
is getting freed and fixup?

-- 
Thanks,

David / dhildenb