| Message ID | 20240812203538.82548-1-max8rr8@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] x86/ioremap: Use is_ioremap_addr() in iounmap() | expand |
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
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>
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
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
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.
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
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.
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
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; /*
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(-)