| 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
| ||
|
Message-ID: <e86786e5-a514-54ed-dfeb-ed1f57b173d4@huawei.com>
Date: Wed, 8 Jun 2022 21:31:58 +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/6/8 18:05, David Hildenbrand wrote:
> On 07.06.22 04:20, Miaohe Lin wrote:
>> On 2022/6/2 16:47, David Hildenbrand wrote:
>>> On 02.06.22 09:40, Miaohe Lin wrote:
>>>> On 2022/6/1 18:31, David Hildenbrand wrote:
>>>>> On 31.05.22 14:37, Miaohe Lin wrote:
>>>>>> On 2022/5/31 19:59, David Hildenbrand wrote:
>>>>>>> Sorry for the late reply, was on vacation.
>>>>>>
>>>>>> That's all right. Hope you have a great time. ;)
>>>>>>
>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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 :)
>>>>>>
>>>>>> I think I found the things we missed in another email [1].
>>>>>> [1]: https://lore.kernel.org/all/948ea45e-3b2b-e16c-5b8c-4c34de0ea593@huawei.com/
>>>>>>
>>>>>> Paste the main content of [1] here:
>>>>>>
>>>>>> "
>>>>>> There are 3 cases in unmap_and_move:
>>>>>>
>>>>>> 1.page is freed through "if (page_count(page) == 1)" code block. This works
>>>>>> as PG_active and PG_unevictable are cleared here.
>>>>>>
>>>>>> 2. Failed to migrate the page. The page won't be release so we don't care about it.
>>>>>
>>>>> Right, page is un-isolated.
>>>>>
>>>>>>
>>>>>> 3. The page is migrated successfully. The PG_active and PG_unevictable are cleared
>>>>>> via folio_migrate_flags():
>>>>>>
>>>>>> if (folio_test_clear_active(folio)) {
>>>>>> VM_BUG_ON_FOLIO(folio_test_unevictable(folio), folio);
>>>>>> folio_set_active(newfolio);
>>>>>> } else if (folio_test_clear_unevictable(folio))
>>>>>> folio_set_unevictable(newfolio);
>>>>>
>>>>> Right.
>>>>>
>>>>>>
>>>>>> For the above race case, the page won't be freed through "if (page_count(page) == 1)" code block.
>>>>>> It will just be migrated and freed via put_page() after folio_migrate_flags() having cleared PG_active
>>>>>> and PG_unevictable.
>>>>>> "
>>>>>> Or Am I miss something again? :)
>>>>>
>>>>> For #1, I'm still not sure what would happen on a speculative reference.
>>>>>
>>>>> It's worth summarizing that
>>>>>
>>>>> a) free_pages_prepare() will clear both flags via page->flags &=
>>>>> ~PAGE_FLAGS_CHECK_AT_PREP;
>>>>>
>>>>> b) free_pages_prepare() will bail out if any flag is set in
>>>>> check_free_page().
>>>>>
>>>>> As we've never seen b) in the wild, this certainly has low priority, and
>>>>> maybe it really cannot happen right now.
>>>>>
>>>>> However, maybe really allowing these flags to be set when freeing the
>>>>> page and removing the "page_count(page) == 1" case from migration code
>>>>> would be the clean thing to do.
>>>>
>>>> IMHO, check_free_page is used to catch possible problem. There's the comment of PAGE_FLAGS_CHECK_AT_FREE:
>>>>
>>>> /*
>>>> * Flags checked when a page is freed. Pages being freed should not have
>>>> * these flags set. If they are, there is a problem.
>>>> */
>>>> #define PAGE_FLAGS_CHECK_AT_FREE
>>>>
>>>> There might be an assumption: when page is freed, it shouldn't be an active or unevictable page. It should be
>>>> inactive and evictable. So allowing these flags to be set when freeing the page might not be a good idea?
>>>
>>> Yeah, and we'd be lifting that restriction because there is good reason
>>> to do so.
>>>
>>> Maybe we *could* special case for isolated pages; however, that adds
>>> runtime overhead. Of course, we could perform different checks for e.g.,
>>> DEBUG_VM vs !DEBUG_VM.
>>
>> I found there is one assumption about PG_active and PG_unevictable, i.e. in __folio_clear_lru_flags:
>>
>> /* this shouldn't happen, so leave the flags to bad_page() */
>> if (folio_test_active(folio) && folio_test_unevictable(folio))
>> return;
>>
>> If PG_active and PG_unevictable are both set, this case will be caught in the bad_page() via check_free_page().
>> There might be some other assumptions about PG_active and PG_unevictable. So I think it's not safe to lift that
>> restriction.
>>
>> But maybe we could limit this check within DEBUG_VM as you suggested. Am I supposed to do it?
>
> Well, if you want, you can look into ways of cleaning that up and
> removing the "if there is more than one reference, the owner hasn't
> freed the page" condition, because there are corner cases where the
> owner might have freed the page but speculative references keep the
> refcount temporarily incremented.>
Let me queue it to my TODO list. :)
Thanks for your valuable suggestion!
Powered by blists - more mailing lists