Re: [PATCH] mm: fix COW mapping handing in generic_access_phys

[David Hildenbrand <david@redhat.com>]

On 28.05.25 20:29, Jason Gunthorpe wrote:
> On Wed, May 28, 2025 at 08:22:08PM +0200, David Hildenbrand wrote:
>> On 28.05.25 20:15, Jason Gunthorpe wrote:
>>> On Wed, May 28, 2025 at 08:00:34PM +0200, David Hildenbrand wrote:
>>>
>>>> Having refcounted anon page in a VM_PFNMAP doesn't suddenly turn the whole
>>>> thing into a MIXEDMAP where other things with a "struct page" are suddenly
>>>> refcounted as well.
>>>
>>> The special COW rules for PFNMAP require a single linear mapping so
>>> that vma->vm_pgoff can encode the "special" range.
>>
>> Yes, I have a semi-ugly patch to adjust remove that special casing.
>>
>> (less ugly than current handling: *still* lookup the struct page, and if it
>> references an anonymous folio, we ... have an anonymous folio. Because
>> nobody should ever,ever,ever PFNMAP an anonymous folio. Ever.)
> 
> Hmm, maybe so.

I'll try to send out a prototype of that patch once I managed to ... finish
never ending stream of review :)

> 
>> In remap_pfn_range_internal() we sanity-check that right now. And fail if it
>> is not in place.
>>
>> Any COW VM_PFNMAP that doesn't go through remap_pfn_range() would be ...
>> broken. I don't think we have them.
> 
> It is a really easy driver mistake to make though, you have to omit
> the rejection of !VM_SHARED, then go on and do something else.

As I just reviewed related code, I think when not using remap_pfn_range(), a
driver -- like vfio -- will be using vmf_insert_pfn().

There, we sanity check it as well:

vmf_insert_pfn()->vmf_insert_pfn_prot()

	BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));

> 
> How many drivers use VM_PFNMAP without remap_pfn_range() and don't
> check for VM_SHARED?

Not sure, only a handful even use vmf_insert_pfn(), and the BUG_ON would at
least catch it.

-- 
Cheers,

David / dhildenb