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

[David Hildenbrand <david@redhat.com>]

On 28.05.25 19:47, Jason Gunthorpe wrote:
> On Wed, May 28, 2025 at 07:32:59PM +0200, David Hildenbrand wrote:
>> On 28.05.25 18:29, Jason Gunthorpe wrote:
>>> On Wed, May 28, 2025 at 12:06:07PM -0400, Peter Xu wrote:
>>>> #define VM_PFNMAP	0x00000400	/* Page-ranges managed without "struct page", just pure PFN */
>>>>
>>>> I'm not confident to blame any driver yet to have those special cases for
>>>> VM_PFNMAP, because it only says "managed without struct page", it didn't
>>>> say "it must not contain struct page"..  Hence it hints the core mm "please
>>>> do not manage these mappings with struct page at all".  Still sounds fair
>>>> contract, even if not ideal.
>>>
>>> I think it is pretty clear, if a VMA has VM_PFNMAP then nothing must
>>> ever try to obtain a struct page from any PTEs in it, for any reason,
>>> even if things in it might have a struct page. In practice it means
>>> nothing can call vm_normal_page() on a VM_PFNMAP.
>>
>> No, not until we remove any COW mappings of VM_PFNMAP.
> 
> I stand by the statement, the COW mapping thing is broken. Just
> because it is wrong doesn't mean it is allowed to call
> vm_normal_page() on VM_PFNMAP.
> 
> It makes no sense at all to have VM_PFNMAP where someone has COW'd the
> pages and effectively turned it into a MIXEDMAP.
> 

Well, not quite.

MIXEDMAP is clearly documented that whatever has a refcount (struct 
page) is refcounted. That's not the case for PFNMAP, obviously, where 
nothing is refcounted, except ... anon pages in COW mappings.

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.

> It should just be a MIXEDMAP at that point!!
 >
 > IMHO we should have the core code complain with a dmesg for 
VM_PFNMAP> without VM_SHARED and get driver authors to fix it. Either 
add the
> missed VM_SHARED or changed to MIXEDMAP. Make it WARN_ON in a few
> years.
> 
> If they can't follow the rules required to use MIXEDMAP then they
> certainly do not have working COW!!
> 
 >>> In either case GUP doesn't really work on the VMA. PFNMAP is 
totally>>> blocked, and for MIXEDMAP userspace has no way to discover which
>>> subset of the VMA is GUPable. I think that GUP is supported on
>>> MIXEDMAP at all is a bit of a weirdo thing.
>>
>> IIRC (after recent discussions with Lorenzo) there are use cases for that.
> 
> Really? Somehow userspace can know which sub slice of the VMA is
> GUP'able? Gross :P

Well, the primary use case is mapping kernel allocations into user space 
and GUP'ing them. So the pages are refcounted ...

MIXEDMAP is likely just abused for that ... so I take back that that's a 
requirement, rather that MIXEDMAP is automatically used when using the 
interfaces -- vm_insert_pages -- to insert refcounted pages into page 
tables :)

... and that should definitely be changed, such that MIXEDMAP is really 
only used when actually having non-refcounted things in there.

-- 
Cheers,

David / dhildenb