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

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

Message-ID: <025d0dc8-a446-b720-14a8-97c041055f48@huawei.com>
Date:   Thu, 12 May 2022 21:26:44 +0800
From:   Miaohe Lin <linmiaohe@...wei.com>
To:     David Hildenbrand <david@...hat.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

On 2022/5/12 15:10, David Hildenbrand wrote:
>>> If PG_isolated is still set, it will get cleared in the buddy when
>>> freeing the page via
>>>
>>> 	page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>
>> Yes, check_free_page only complains about flags belonging to PAGE_FLAGS_CHECK_AT_FREE and PG_isolated
>> will be cleared in the buddy when freeing the page. But it might not be a good idea to reply on this ?
>> IMHO, it should be better to clear the PG_isolated explicitly ourselves.
> 
> I think we can pretty much rely on this handling in the buddy :)

So is the below code change what you're suggesting?

	if (page_count(page) == 1) {
		/* page was freed from under us. So we are done. */
		ClearPageActive(page);
		ClearPageUnevictable(page);
-		if (unlikely(__PageMovable(page)))
-			ClearPageIsolated(page);
		goto out;
	}
> 
>>
>>>
>>>>
>>>>>
>>>>>
>>>>> Also, I am not sure how reliable that page count check is here: if we'd
>>>>> have another speculative reference to the page, we might see
>>>>> "page_count(page) > 1" and not take that path, although the previous
>>>>> owner released the last reference.
>>>>
>>>> IIUC, there should not be such speculative reference. The driver should have taken care
>>>> of it.
>>>
>>> How can you prevent any kind of speculative references?
>>>
>>> See isolate_movable_page() as an example, which grabs a speculative
>>> reference to then find out that the page is already isolated by someone
>>> else, to then back off.
>>
>> You're right. isolate_movable_page will be an speculative references case. But the page count check here
>> is just an optimization. If we encounter speculative references, it still works with useless effort of
>> migrating to be released page.
> 
> 
> Not really. The issue is that PAGE_FLAGS_CHECK_AT_FREE contains
> PG_active and PG_unevictable.
> 
> We only clear those 2 flags if "page_count(page) == 1". Consequently,
> with a speculative reference, we'll run into the check_free_page_bad()
> when dropping the last reference.

It seems if a speculative reference happens after the "page_count(page) == 1" check,
it's ok because we cleared the PG_active and PG_unevictable. And if it happens before
the check, this code block is skipped and the page will be freed after migration. The
PG_active and PG_unevictable will be correctly cleared when page is actually freed via
__folio_clear_active. (Please see below comment)

> 
> This is just shaky. Special casing on "page_count(page) == 1" for
> detecting "was this freed by the owner" is not 100% water proof.
> 
> In an ideal world, we'd just get rid of that whole block of code and let
> the actual freeing code clear PG_active and PG_unevictable. But that
> would require changes to free_pages_prepare().
> 
> 
> Now I do wonder, if we ever even have PG_active or PG_unevictable still
> set when the page was freed by the owner in this code. IOW, maybe that
> is dead code as well and we can just remove the whole shaky
> "page_count(page) == 1" code block.

Think about below common scene: Anonymous page is actively used by the sole owner process, so it
will have PG_active set. Then process exited while vm tries to migrate that page. So the page
should have refcnt == 1 while PG_active is set? Note normally PG_active should be cleared when
the page is released:

__put_single_page
  PageLRU
    __clear_page_lru_flags
      __folio_clear_active
      __folio_clear_unevictable

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?

Thanks!

> 
> Ccing Minchan, who added clearing of the pageflags at that point.
>