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

[Thomas Gleixner <tglx@linutronix.de>]

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() ...

Thanks,

        tglx