Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e6b64991-65d7-0d25-3866-6b0b44f171b1@redhat.com>
Date:   Tue, 18 Apr 2023 17:57:47 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH mm-unstable v1] mm: don't check VMA write permissions if
 the PTE/PMD indicates write permissions
Content-Language: en-US
To:     Peter Xu <peterx@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Mel Gorman <mgorman@techsingularity.net>,
        "Kirill A. Shutemov" <kirill@shutemov.name>
References: <20230418142113.439494-1-david@redhat.com> <ZD69l0zD3UL9HD8g@x1n>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
In-Reply-To: <ZD69l0zD3UL9HD8g@x1n>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 18.04.23 17:56, Peter Xu wrote:
> On Tue, Apr 18, 2023 at 04:21:13PM +0200, David Hildenbrand wrote:
>> Staring at the comment "Recheck VMA as permissions can change since
>> migration started" in remove_migration_pte() can result in confusion,
>> because if the source PTE/PMD indicates write permissions, then there
>> should be no need to check VMA write permissions when restoring migration
>> entries or PTE-mapping a PMD.
>>
>> Commit d3cb8bf6081b ("mm: migrate: Close race between migration completion
>> and mprotect") introduced the maybe_mkwrite() handling in
>> remove_migration_pte() in 2014, stating that a race between mprotect() and
>> migration finishing would be possible, and that we could end up with
>> a writable PTE that should be readable.
>>
>> However, mprotect() code first updates vma->vm_flags / vma->vm_page_prot
>> and then walks the page tables to (a) set all present writable PTEs to
>> read-only and (b) convert all writable migration entries to readable
>> migration entries. While walking the page tables and modifying the
>> entries, migration code has to grab the PT locks to synchronize against
>> concurrent page table modifications.
> 
> Makes sense to me.
> 
>>
>> Assuming migration would find a writable migration entry (while holding
>> the PT lock) and replace it with a writable present PTE, surely mprotect()
>> code didn't stumble over the writable migration entry yet (converting it
>> into a readable migration entry) and would instead wait for the PT lock to
>> convert the now present writable PTE into a read-only PTE. As mprotect()
>> didn't finish yet, the behavior is just like migration didn't happen: a
>> writable PTE will be converted to a read-only PTE.
>>
>> So it's fine to rely on the writability information in the source
>> PTE/PMD and not recheck against the VMA as long as we're holding the PT
>> lock to synchronize with anyone who concurrently wants to downgrade write
>> permissions (like mprotect()) by first adjusting vma->vm_flags /
>> vma->vm_page_prot to then walk over the page tables to adjust the page
>> table entries.
>>
>> Running test cases that should reveal such races -- mprotect(PROT_READ)
>> racing with page migration or THP splitting -- for multiple hours did
>> not reveal an issue with this cleanup.
>>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Mel Gorman <mgorman@techsingularity.net>
>> Cc: Peter Xu <peterx@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>
>> This is a follow-up cleanup to [1]:
>> 	[PATCH v1 RESEND 0/6] mm: (pte|pmd)_mkdirty() should not
>> 	unconditionally allow for write access
>>
>> I wanted to be a bit careful and write some test cases to convince myself
>> that I am not missing something important. Of course, there is still the
>> possibility that my test cases are buggy ;)
>>
>> Test cases I'm running:
>> 	https://gitlab.com/davidhildenbrand/scratchspace/-/raw/main/test_mprotect_migration.c
>> 	https://gitlab.com/davidhildenbrand/scratchspace/-/raw/main/test_mprotect_thp_split.c
>>
>>
>> [1] https://lkml.kernel.org/r/20230411142512.438404-1-david@redhat.com
>>
>> ---
>>   mm/huge_memory.c | 4 ++--
>>   mm/migrate.c     | 5 +----
>>   2 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index c23fa39dec92..624671aaa60d 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -2234,7 +2234,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>>   		} else {
>>   			entry = mk_pte(page + i, READ_ONCE(vma->vm_page_prot));
>>   			if (write)
>> -				entry = maybe_mkwrite(entry, vma);
>> +				entry = pte_mkwrite(entry);
> 
> This is another change besides page migration.  I also don't know why it's
> needed, but it's there since day 1 of thp split in eef1b3ba053, so maybe
> worthwhile to copy Kirill too (which I did).

Indeed (I wanted but forgot ...), thanks Peter!

-- 
Thanks,

David / dhildenb