On 01.12.22 23:30, Andrew Morton wrote:
On Thu, 1 Dec 2022 16:42:52 +0100 David Hildenbrand <david@xxxxxxxxxx> wrote:
On 01.12.22 16:28, Peter Xu wrote:
I didn't reply here because I have already replied with the question in
previous version with a few attempts. Quotting myself:
https://lore.kernel.org/all/Y3KgYeMTdTM0FN5W@x1n/
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.
I've also got another longer email trying to explain why I think it's the
other way round to be justfied, rather than justifying removal of the write
bit for a read migration entry, here:
And I disagree for this patch that is supposed to fix this hunk:
@@ -243,11 +243,15 @@ static bool remove_migration_pte(struct page *page, struct vm_area_struct *vma,
entry = pte_to_swp_entry(*pvmw.pte);
if (is_write_migration_entry(entry))
pte = maybe_mkwrite(pte, vma);
+ else if (pte_swp_uffd_wp(*pvmw.pte))
+ pte = pte_mkuffd_wp(pte);
if (unlikely(is_zone_device_page(new))) {
if (is_device_private_page(new)) {
entry = make_device_private_entry(new, pte_write(pte));
pte = swp_entry_to_pte(entry);
+ if (pte_swp_uffd_wp(*pvmw.pte))
+ pte = pte_mkuffd_wp(pte);
}
}
David, I'm unclear on what you mean by the above. Can you please
expand?
There is really nothing to justify the other way around here.
If it's broken fix it independently and properly backport it independenty.
But we don't know about any such broken case.
I have no energy to spare to argue further ;)
This is a silent data loss bug, which is about as bad as it gets.
Under obscure conditions, fortunately. But please let's keep working
it. Let's aim for something minimal for backporting purposes. We can
revisit any cleanliness issues later.
David, do you feel that the proposed fix will at least address the bug
without adverse side-effects?
Just to answer that question clearly: it will fix this bug, but it's
likely that other similar bugs remain (suspecting NUMA hinting).
Adverse side effect will be that some PTEs that could we writable won't
be writable. I assume it's not too bad in practice for this particular case.
I proposed an alternative fix and identified other possible broken
cases. Again, I don't NAK this patch as is, it just logically doesn't
make sense to me to handle this case differently to the other
vma->vm_page_prot users. (more details in the other thread)
--
Thanks,
David / dhildenb
