Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Dan Williams]

Thomas Gleixner wrote:
> On Thu, Aug 08 2024 at 08:58, Dan Williams wrote:
> > Thomas Gleixner wrote:
> >> On Thu, Aug 10 2023 at 13:00, Max Ramanouski wrote:
> >> 
> >> > On systems that use HMM (most notably amdgpu driver)
> >> > high_memory can jump over VMALLOC_START. That causes
> >> > some iounmap to exit early. This in addition to leaking,
> >> > causes problems with rebinding devices to vfio_pci from
> >> > other drivers with error of conflicting memtypes,
> >> > as they aren't freed in iounmap.
> >> >
> >> > Replace comparison against high_memory with is_vmalloc_addr to
> >> > fix the issue and make x86 iounmap implementation more similar
> >> > to generic one, it also uses is_vmalloc_addr to validate pointer.
> >> 
> >> So this lacks a Fixes tag and some deep analysis of similar potential
> >> problems. While at it please use func() notation for functions. In the
> >> middle of a sentence iounmap does not immediately stand out, but
> >> iounmap() does. It's documented ...
> >> 
> >> This add_pages() hackery in pagemap_range() is really nasty as it ends
> >> up violating historical assumptions about max_pfn and high_memory.
> >> 
> >> Dan, who did the analysis of this when the device private memory muck
> >> was added?
> >
> > So that plain add_pages() usage originated here:
> >
> >     4ef589dc9b10 mm/hmm/devmem: device memory hotplug using ZONE_DEVICE
> >
> > The original memremap_pages() only ever used arch_add_memory()
> >
> >     41e94a851304 add devm_memremap_pages
> >
> > When HMM merged into memremap_pages() I indeed did not pick up on the
> > nuance that HMM was wrong to use add_pages() instead of
> > arch_add_memory() which updates the high_memory variable:
> >
> >     69324b8f4833 mm, devm_memremap_pages: add MEMORY_DEVICE_PRIVATE support
> 
> arch_add_memory() calls add_pages() ...

Apologies was trying to quickly reverse engineer how private memory
might be different than typical memremap_pages(), but it is indeed the
same in this aspect.

So the real difference is that the private memory case tries to
allocate physical memory by searching for holes in the iomem_resource
starting from U64_MAX. That might explain why only the private memory
case is violating assumptions with respect to high_memory spilling into
vmalloc space.