Re: [PATCH v2 06/19] mm/pagewalk: Check pfnmap for folio_walk_start()

[David Hildenbrand <david@redhat.com>]

On 29.08.24 01:46, Jason Gunthorpe wrote:
> On Wed, Aug 28, 2024 at 03:45:49PM -0400, Peter Xu wrote:
> 
>> Meanwhile I'm actually not 100% sure pte_special is only needed in
>> gup-fast.  See vm_normal_page() and for VM_PFNMAP when pte_special bit is
>> not defined:
>>
>> 		} else {
>> 			unsigned long off;
>> 			off = (addr - vma->vm_start) >> PAGE_SHIFT;
>> 			if (pfn == vma->vm_pgoff + off) <------------------ [1]
>> 				return NULL;
>> 			if (!is_cow_mapping(vma->vm_flags))
>> 				return NULL;
>> 		}
>>
>> I suspect things can go wrong when there's assumption on vm_pgoff [1].  At
>> least vfio-pci isn't storing vm_pgoff for the base PFN, so this check will
>> go wrong when pte_special is not supported on any arch but when vfio-pci is
>> present.  I suspect more drivers can break it.

Fortunately, we did an excellent job at documenting vm_normal_page():

  * There are 2 broad cases. Firstly, an architecture may define a pte_special()
  * pte bit, in which case this function is trivial. Secondly, an architecture
  * may not have a spare pte bit, which requires a more complicated scheme,
  * described below.
  *
  * A raw VM_PFNMAP mapping (ie. one that is not COWed) is always considered a
  * special mapping (even if there are underlying and valid "struct pages").
  * COWed pages of a VM_PFNMAP are always normal.
  *
  * The way we recognize COWed pages within VM_PFNMAP mappings is through the
  * rules set up by "remap_pfn_range()": the vma will have the VM_PFNMAP bit
  * set, and the vm_pgoff will point to the first PFN mapped: thus every special
  * mapping will always honor the rule
  *
  *	pfn_of_page == vma->vm_pgoff + ((addr - vma->vm_start) >> PAGE_SHIFT)
  *
  * And for normal mappings this is false.
  *

remap_pfn_range_notrack() will currently handle that for us:

if (is_cow_mapping(vma->vm_flags)) {
	if (addr != vma->vm_start || end != vma->vm_end)
		return -EINVAL;
}

Even if [1] would succeed, the is_cow_mapping() check will return NULL and it will
all work as expected, even without pte_special().

Because VM_PFNMAP is easy: in a !COW mapping, everything is special.

> 
> I think that is a very important point.
> 
> IIRC this was done magically in one of the ioremap pfns type calls,
> and if VFIO is using fault instead it won't do it.
> 
> This probably needs more hand holding for the driver somehow..

As long as these drivers don't support COW-mappings. It's all good.

And IIUC, we cannot support COW mappings if we don't use remap_pfn_range().

For this reason, remap_pfn_range() also bails out if not the whole VMA is covered
in a COW mapping.

It would be great if we could detect and fail that. Likely when trying to insert
PFNs (*not* using remap_pfn_range) manually we would have to WARN if we stumble over
a COW mapping.

In the meantime, we should really avoid any new VM_PFNMAP COW users ...

> 
>> So I wonder if it's really the case in real life that only gup-fast would
>> need the special bit.  It could be that we thought it like that, but nobody
>> really seriously tried run it without special bit yet to see things broke.
> 
> Indeed.

VM_PFNMAP for sure works.

VM_MIXEDMAP, I am not so sure. The s390x introduction of pte_special() [again,
I posted the commit] raised why they need it: because pfn_valid() could have
returned non-refcounted pages. One would have to dig if that is even still the
case as of today, and if other architectures have similar constraints.


> 
> What arches even use the whole 'special but not special' system?
> 
> Can we start banning some of this stuff on non-special arches?

Again, VM_PFNMAP is not a problem. Only VM_MIXEDMAP, and I would love to
see that go. There are some, but not that many users ... but I'm afraid it's
not that easy :)

-- 
Cheers,

David / dhildenb