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: <ec8b3c86-d3b2-f898-7297-c20a58ae2ac1@redhat.com>
Date:   Tue, 15 Nov 2022 10:13:24 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
To:     Peter Xu <peterx@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Andrea Arcangeli <aarcange@redhat.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Ives van Hoorne <ives@codesandbox.io>,
        Nadav Amit <nadav.amit@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Mike Rapoport <rppt@linux.vnet.ibm.com>, stable@vger.kernel.org
References: <20221110203132.1498183-1-peterx@redhat.com>
 <20221110203132.1498183-2-peterx@redhat.com>
 <9af36be3-313b-e39c-85bb-bf30011bccb8@redhat.com> <Y3KgYeMTdTM0FN5W@x1n>
Content-Language: en-US
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH v2 1/2] mm/migrate: Fix read-only page got writable when
 recover pte
In-Reply-To: <Y3KgYeMTdTM0FN5W@x1n>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

>>
>> Any particular reason why not to simply glue this to pte_swp_uffd_wp(),
>> because only that needs special care:
>>
>> if (pte_swp_uffd_wp(*pvmw.pte)) {
>> 	pte = pte_wrprotect(pte);
>> 	pte = pte_mkuffd_wp(pte);
>> }
>>
>>
>> And that would match what actually should have been done in commit
>> f45ec5ff16a7 -- only special-case uffd-wp.
>>
>> Note that I think there are cases where we have a PTE that was !writable,
>> but after migration we can map it writable.
> 
> The thing is recovering the pte into its original form is the safest
> approach to me, so I think we need justification on why it's always safe to
> set the write bit.
> 
> Or do you perhaps have solid clue and think it's always safe
The problem I am having with this broader change, is that this changes 
something independent of your original patch/problem.

If we identify this to be an actual problem, it should most probably be 
separate fix + backport.


My understanding is that vma->vm_page_prot always tells you what the 
default PTE protection in a mapping is.

If the mapping is private, it is never writable (due to COW). Similarly, 
if the shared file mapping needs writenotify, it is never writable.


I consider UFFD-wp a special case: while the default VMA protection 
might state that it is writable, you actually want individual PTEs to be 
write-protected and have to manually remove the protection.

softdirty tracking is another special case: however, softdirty tracking 
is enabled for the whole VMA. For remove_migration_pte() that should be 
fine (I guess) because writenotify is active when the VMA needs to track 
softdirty bits, and consequently vma->vm_page_prot has the proper 
default permissions.


I wonder if the following (valid), for example is possible:


1) clear_refs() clears VM_SOFTDIRTY and pte_wrprotect() the pte.
-> writenotify is active and vma->vm_page_prot updated accordingly

VM_SOFTDIRTY is reset due to VMA merging and vma->vm_page_prot is 
updated accordingly. See mmap_region() where we set VM_SOFTDIRTY.

If you now migrate the (still write-protected in the PTE) page, it was 
not writable, but it can be writable on the destination.

> 
>>
>> BTW, does unuse_pte() need similar care?
>>
>> new_pte = pte_mkold(mk_pte(page, vma->vm_page_prot));
>> if (pte_swp_uffd_wp(*pte))
>> 	new_pte = pte_mkuffd_wp(new_pte);
>> set_pte_at(vma->vm_mm, addr, pte, new_pte);
> 
> I think unuse path is fine because unuse only applies to private mappings,
> so we should always have the W bit removed there within mk_pte().

You're right, however, shmem swapping confuses me. Maybe that does not 
apply here.

-- 
Thanks,

David / dhildenb