Re: [PATCH] mm: Fix __wp_page_copy_user fallback path for remote mm

[Asahi Lina <lina@asahilina.net>]

On 11/12/24 7:00 PM, David Hildenbrand wrote:
> On 12.11.24 10:48, Asahi Lina wrote:
>>
>>
>> On 11/11/24 8:24 AM, Alistair Popple wrote:
>>>
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> On 07.11.24 18:32, Asahi Lina wrote:
>>>>> On 11/8/24 2:14 AM, David Hildenbrand wrote:
>>>>>> I recall that there is still a problem with false-positives on
>>>>>> folio_test_anon() with ZONE_DEVICE pages, so it's maybe not that
>>>>>> easy ... and the whole get_dev_pagemap() stuff is nasty.
>>>
>>> Specifically FS DAX reuses PAGE_MAPPING_ANON in
>>> include/linux/page-flags.h
>>>
>>>      /*
>>>       * Different with flags above, this flag is used only for fsdax
>>> mode.  It
>>>       * indicates that this page->mapping is now under reflink case.
>>>       */
>>>      #define PAGE_MAPPING_DAX_SHARED    ((void *)0x1)
>>>
>>> FS DAX pages are never anon though, so you could probably test for
>>> !vma_is_dax() and/or add an implementation of is_fsdax_page().
>>>
>>>>>> Likely we would have to do what GUP does, and temporarily grab a
>>>>>> pgmap
>>>>>> reference. Gah.
>>>>>>
>>>>>>
>>>>>> So if we sort out the pagemap stuff and the possibly wrong
>>>>>> folio_test_anon() on some ZONE_DEVICE pages (but not all, because
>>>>>> IIRC
>>>>>> DEVICE_PRIVATE can be anon ...), it might be doable.
>>>
>>> Correct, DEVICE_PRIVATE and DEVICE_COHERENT pages are always anon (at
>>> least for now).
>>>
>>>>>> But it sounds ugly, especially because that code might change soon
>>>>>> and
>>>>>> not require messing with ZONE_DEVICE pages on that level.
>>>
>>> Yes, I'm hopoing to get the next version of that series posted this
>>> week. I found a couple of other FS DAX bugs that slowed me down.
>>>
>>>   - Alistair
>>>
>>>>>> And then, we'd not be able to handle VM_PFNMAP cleanly ...
>>>>>>
>>
>> If this is all going to be fixed another way soon then I think there's
>> no rush to get a workaround in earlier than that, I just don't want it
>> to fall by the wayside.
>>
>> We have my original patch downstream in libkrunfw (which despite the
>> lockdep complaints does work in practice)
> 
> I assume it's sufficient to deadlock when a writer pops up after you
> succeeded with the first read-locking, and before you start the second
> read-locking. IIRC, rwsem is a fair lock, so read-locking when already-
> read-locked is not guaranteed to work.
> 
> That's why lockdep complains.
> 

That's fair, I just mean that "works most of the time" is probably good
enough for the time being considering that this codepath is only invoked
by debuggers in practice anyway.

~~ Lina