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

[Dan Williams]

Dan Williams wrote:
> 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.

Not U64_MAX, but it starts searching for free physical address space
starting at MAX_PHYSMEM_BITS, see gfr_start(). That aspect of private
memory has always bothered me, but I am not sure that is the historical
assumption violation you are referring to above.