linux-kernel - Re: [PATCH] mm: migration :shared anonymous migration test is failing

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: <95b7d92f-a12d-47be-9099-f5efebb56ff2@linux.ibm.com>
Date: Mon, 23 Dec 2024 17:32:37 +0530
From: Donet Tom <donettom@...ux.ibm.com>
To: Dev Jain <dev.jain@....com>, Baolin Wang <baolin.wang@...ux.alibaba.com>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Cc: Ritesh Harjani <ritesh.list@...il.com>,
        "Aneesh Kumar K . V" <aneesh.kumar@...nel.org>,
        Zi Yan <ziy@...dia.com>, David Hildenbrand <david@...hat.com>,
        shuah Khan <shuah@...nel.org>
Subject: Re: [PATCH] mm: migration :shared anonymous migration test is failing


On 12/20/24 10:07, Dev Jain wrote:
>
> On 20/12/24 9:02 am, Baolin Wang wrote:
>>
>>
>> On 2024/12/20 11:12, Donet Tom wrote:
>>>
>>> On 12/20/24 08:01, Baolin Wang wrote:
>>>>
>>>>
>>>> On 2024/12/19 20:47, Donet Tom wrote:
>>>>> The migration selftest is currently failing for shared anonymous
>>>>> mappings due to a race condition.
>>>>>
>>>>> During migration, the source folio's PTE is unmapped by nuking the
>>>>> PTE, flushing the TLB,and then marking the page for migration
>>>>> (by creating the swap entries). The issue arises when, immediately
>>>>> after the PTE is nuked and the TLB is flushed, but before the page
>>>>> is marked for migration, another thread accesses the page. This
>>>>> triggers a page fault, and the page fault handler invokes
>>>>> do_pte_missing() instead of do_swap_page(), as the page is not yet
>>>>> marked for migration.
>>>>>
>>>>> In the fault handling path, do_pte_missing() calls __do_fault()
>>>>> ->shmem_fault() -> shmem_get_folio_gfp() -> filemap_get_entry().
>>>>> This eventually calls folio_try_get(), incrementing the reference
>>>>> count of the folio undergoing migration. The thread then blocks
>>>>> on folio_lock(), as the migration path holds the lock. This
>>>>> results in the migration failing in __migrate_folio(), which expects
>>>>> the folio's reference count to be 2. However, the reference count is
>>>>> incremented by the fault handler, leading to the failure.
>>>>>
>>>>> The issue arises because, after nuking the PTE and before marking the
>>>>> page for migration, the page is accessed. To address this, we have
>>>>> updated the logic to first nuke the PTE, then mark the page for
>>>>> migration, and only then flush the TLB. With this patch, If the 
>>>>> page is
>>>>> accessed immediately after nuking the PTE, the TLB entry is still
>>>>> valid, so no fault occurs. After marking the page for migration,
>>>>
>>>> IMO, I don't think this assumption is correct. At this point, the 
>>>> TLB entry might also be evicted, so a page fault could still occur. 
>>>> It's just a matter of probability.
>>> In this patch, we mark the page for migration before flushing the TLB.
>>> This ensures that if someone accesses the page after the TLB flush,
>>> the page fault will occur and in the page fault handler will wait 
>>> for the
>>> migration to complete. So migration will not fail
>>>
>>> Without this patch, if someone accesses the page after the TLB flush
>>> but before it is marked for migration, the migration will fail.
>>
>> Actually my concern is the same as David's (I did not see David's 
>> reply before sending my comments), which is that your patch does not 
>> "rules out all cases".
>
>
> I like this solution but really the proper solution for this one was 
> to atomically set the migration entry IMHO.
>
Thank you Dev. I will check and come back on this.
>
>>
>>>> Additionally, IIUC, if another thread is accessing the shmem folio 
>>>> causing the migration to fail, I think this is expected, and 
>>>> migration failure is not a vital issue?
>>>>
>>> In my case, the shmem migration test is always failing,
>>> even after retries. Would it be correct to consider this
>>> as expected behavior?
>>
>> IMHO I think your test case is too aggressive and unlikely to occur 
>> in real-world scenarios. Additionally, as I mentioned, migration 
>> failure is not a vital issue in the system, and some temporary refcnt 
>> can also lead to migration failure if you want to create such test 
>> cases. So personally, I don't think it is worthy doing.
>
> Agreed, AFAIR the test case starts faulting exactly on those pages 
> which we want to migrate, making this a very artificial scenario.
>