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]

Date:   Thu, 12 May 2022 09:10:20 +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

>> 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 :)

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

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.

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

-- 
Thanks,

David / dhildenb