[PATCH v2] x86/ioremap: Use is_ioremap_addr() in iounmap()

[PATCH v2] x86/ioremap: Use is_ioremap_addr() in iounmap()

[Max Ramanouski]

On systems that use HMM (most notably amdgpu driver) high_memory
can jump over VMALLOC_START due to pages at the end of physical
space being added with add_pages(), while gap for new pages left
by KASLR is as small as 10TB. This results in early exit from
iounmap() leading to leaking, and additional problems with rebinding
devices to vfio_pci from other drivers with error of conflicting
memtypes, as memtypes aren't freed in iounmap().

Replace comparison against high_memory with is_ioremap_addr() to
fix the issue and make x86 iounmap() implementation more similar
to generic one, it also uses is_ioremap_addr() to validate pointer.

Fixes: 41e94a851304 ("add devm_memremap_pages")
Signed-off-by: Max Ramanouski <max8rr8@gmail.com>
---
 arch/x86/mm/ioremap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index aa7d27932..464344da4 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -11,6 +11,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
+#include <linux/ioremap.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/mmiotrace.h>
@@ -457,7 +458,7 @@ void iounmap(volatile void __iomem *addr)
 {
 	struct vm_struct *p, *o;
 
-	if ((void __force *)addr <= high_memory)
+	if (!is_ioremap_addr(addr))
 		return;
 
 	/*
-- 
2.45.2

Re: [PATCH v2] x86/ioremap: Use is_ioremap_addr() in iounmap()

[Thomas Gleixner]

On Mon, Aug 12 2024 at 23:35, Max Ramanouski wrote:
> On systems that use HMM (most notably amdgpu driver) high_memory
> can jump over VMALLOC_START due to pages at the end of physical
> space being added with add_pages(), while gap for new pages left
> by KASLR is as small as 10TB. This results in early exit from
> iounmap() leading to leaking, and additional problems with rebinding
> devices to vfio_pci from other drivers with error of conflicting
> memtypes, as memtypes aren't freed in iounmap().
>
> Replace comparison against high_memory with is_ioremap_addr() to
> fix the issue and make x86 iounmap() implementation more similar
> to generic one, it also uses is_ioremap_addr() to validate pointer.
>
> Fixes: 41e94a851304 ("add devm_memremap_pages")

This fixes absolutely nothing as we discussed already. The underlying
problem is that high_memory can spill over into the VMALLOC area.

Seriously?

Thanks,

        tglx

Re: [PATCH v2] x86/ioremap: Use is_ioremap_addr() in iounmap()

[Christoph Hellwig]

Modulo the fixes discussion (and any commit log adjustments related to
that), is_ioremap_addr is the right interface to check for an
ioremap address.  So for the actual code change:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2] x86/ioremap: Use is_ioremap_addr() in iounmap()

[Thomas Gleixner]

On Tue, Aug 13 2024 at 21:28, Christoph Hellwig wrote:
> Modulo the fixes discussion (and any commit log adjustments related to
> that), is_ioremap_addr is the right interface to check for an
> ioremap address.  So for the actual code change:

I'm not opposed to use is_ioremap_addr() as it restricts the check to
the actual ioremp region.

That said, I'm wondering why iounmap() silently bails out when invoked
with an address which is outside of the ioremap region. I'd say, any
invocation with an address outside of it, is broken, but I might be
missing something as always.

Thanks,

        tglx

Re: [PATCH v2] x86/ioremap: Use is_ioremap_addr() in iounmap()

[Alistair Popple]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Tue, Aug 13 2024 at 21:28, Christoph Hellwig wrote:
>> Modulo the fixes discussion (and any commit log adjustments related to
>> that), is_ioremap_addr is the right interface to check for an
>> ioremap address.  So for the actual code change:
>
> I'm not opposed to use is_ioremap_addr() as it restricts the check to
> the actual ioremp region.
>
> That said, I'm wondering why iounmap() silently bails out when invoked
> with an address which is outside of the ioremap region. I'd say, any
> invocation with an address outside of it, is broken, but I might be
> missing something as always.

I would tend to agree and had the same thought when we found this. At
least some kind of message (WARN_ON, WARN_ON_ONCE, printk, etc) would
have made the issue we were debugging much more obvious. FWIW I have
tested running with a WARN_ON() there and it never fired except in the
bug scenario.

 - Alistair

> Thanks,
>
>         tglx

Re: [PATCH v2] x86/ioremap: Use is_ioremap_addr() in iounmap()

[Christoph Hellwig]

On Wed, Aug 14, 2024 at 10:08:23PM +1000, Alistair Popple wrote:
> I would tend to agree and had the same thought when we found this. At
> least some kind of message (WARN_ON, WARN_ON_ONCE, printk, etc) would
> have made the issue we were debugging much more obvious. FWIW I have
> tested running with a WARN_ON() there and it never fired except in the
> bug scenario.

Various architectures had either an early ioremap variant that got
silently ignored here, or magic carveout that don't get remapped at all.
None of this should currently apply to x86, though.

Re: [PATCH v2] x86/ioremap: Use is_ioremap_addr() in iounmap()

[Thomas Gleixner]

On Wed, Aug 14 2024 at 05:16, Christoph Hellwig wrote:
> On Wed, Aug 14, 2024 at 10:08:23PM +1000, Alistair Popple wrote:
>> I would tend to agree and had the same thought when we found this. At
>> least some kind of message (WARN_ON, WARN_ON_ONCE, printk, etc) would
>> have made the issue we were debugging much more obvious. FWIW I have
>> tested running with a WARN_ON() there and it never fired except in the
>> bug scenario.
>
> Various architectures had either an early ioremap variant that got
> silently ignored here, or magic carveout that don't get remapped at all.
> None of this should currently apply to x86, though.

So I'm inclined to have:

       if (WARN_ON_ONCE(is_ioremap_addr(addr)))
       		return;

in the x86 variant then.

Thanks,

        tglx

Re: [PATCH v2] x86/ioremap: Use is_ioremap_addr() in iounmap()

[Christoph Hellwig]

On Wed, Aug 14, 2024 at 04:11:31PM +0200, Thomas Gleixner wrote:
> > Various architectures had either an early ioremap variant that got
> > silently ignored here, or magic carveout that don't get remapped at all.
> > None of this should currently apply to x86, though.
> 
> So I'm inclined to have:
> 
>        if (WARN_ON_ONCE(is_ioremap_addr(addr)))
>        		return;
> 
> in the x86 variant then.

And we should do the same for the generic code eventually after
accounting for all exceptions.  Those should these days mostly be
handled before hand and have explicit address ranges as well, but it'll
need some time to figure all that out.

Re: [PATCH v2] x86/ioremap: Use is_ioremap_addr() in iounmap()

[Thomas Gleixner]

On Wed, Aug 14 2024 at 16:11, Thomas Gleixner wrote:

> On Wed, Aug 14 2024 at 05:16, Christoph Hellwig wrote:
>> On Wed, Aug 14, 2024 at 10:08:23PM +1000, Alistair Popple wrote:
>>> I would tend to agree and had the same thought when we found this. At
>>> least some kind of message (WARN_ON, WARN_ON_ONCE, printk, etc) would
>>> have made the issue we were debugging much more obvious. FWIW I have
>>> tested running with a WARN_ON() there and it never fired except in the
>>> bug scenario.
>>
>> Various architectures had either an early ioremap variant that got
>> silently ignored here, or magic carveout that don't get remapped at all.
>> None of this should currently apply to x86, though.
>
> So I'm inclined to have:
>
>        if (WARN_ON_ONCE(is_ioremap_addr(addr)))
>        		return;
>
> in the x86 variant then.

Max, care to provide that with a reasonable change log?

Thanks,

        tglx