| Message ID | 20200905024811.74701-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | arm64: PCI: fix memleak when calling pci_iomap/unmap() | expand |
On Sat, Sep 05, 2020 at 10:48:11AM +0800, Yang Yingliang wrote: > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > index 1006ed2d7c604..ddfa1c53def48 100644 > --- a/arch/arm64/kernel/pci.c > +++ b/arch/arm64/kernel/pci.c > @@ -217,4 +217,9 @@ void pcibios_remove_bus(struct pci_bus *bus) > acpi_pci_remove_bus(bus); > } > > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr) > +{ > + iounmap(addr); > +} > +EXPORT_SYMBOL(pci_iounmap); So, what's wrong with the generic pci_iounmap() implementation? Shouldn't it call iounmap() already?
+ Lorenzo On Mon, Sep 07, 2020 at 10:51:21AM +0000, George Cherian wrote: > Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Sat, Sep 05, 2020 at 10:48:11AM +0800, Yang Yingliang wrote: > > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index > > > 1006ed2d7c604..ddfa1c53def48 100644 > > > --- a/arch/arm64/kernel/pci.c > > > +++ b/arch/arm64/kernel/pci.c > > > @@ -217,4 +217,9 @@ void pcibios_remove_bus(struct pci_bus *bus) > > > acpi_pci_remove_bus(bus); > > > } > > > > > > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr) { > > > + iounmap(addr); > > > +} > > > +EXPORT_SYMBOL(pci_iounmap); > > > > So, what's wrong with the generic pci_iounmap() implementation? > > Shouldn't it call iounmap() already? > > Since ARM64 selects CONFIG_GENERIC_PCI_IOMAP and not > CONFIG_GENERIC_IOMAP, the pci_iounmap function is reduced to a NULL > function. Due to this, even the managed release variants or even the explicit > pci_iounmap calls doesn't really remove the mappings leading to leak. Ah, I missed the fact that pci_iounmap() depends on a different config option. > https://lkml.org/lkml/2020/8/20/28 So is this going to be fixed in the generic code? That would be my preference. A problem with the iounmap() in the proposed patch is that the region may have been an I/O port, so we could end up unmapping the I/O space.
On Mon, Sep 07, 2020 at 12:21:19PM +0100, Catalin Marinas wrote: > + Lorenzo > > On Mon, Sep 07, 2020 at 10:51:21AM +0000, George Cherian wrote: > > Catalin Marinas <catalin.marinas@arm.com> wrote: > > > On Sat, Sep 05, 2020 at 10:48:11AM +0800, Yang Yingliang wrote: > > > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index > > > > 1006ed2d7c604..ddfa1c53def48 100644 > > > > --- a/arch/arm64/kernel/pci.c > > > > +++ b/arch/arm64/kernel/pci.c > > > > @@ -217,4 +217,9 @@ void pcibios_remove_bus(struct pci_bus *bus) > > > > acpi_pci_remove_bus(bus); > > > > } > > > > > > > > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr) { > > > > + iounmap(addr); > > > > +} > > > > +EXPORT_SYMBOL(pci_iounmap); > > > > > > So, what's wrong with the generic pci_iounmap() implementation? > > > Shouldn't it call iounmap() already? > > > > Since ARM64 selects CONFIG_GENERIC_PCI_IOMAP and not > > CONFIG_GENERIC_IOMAP, the pci_iounmap function is reduced to a NULL > > function. Due to this, even the managed release variants or even the explicit > > pci_iounmap calls doesn't really remove the mappings leading to leak. > > Ah, I missed the fact that pci_iounmap() depends on a different > config option. > > > https://lkml.org/lkml/2020/8/20/28 > > So is this going to be fixed in the generic code? That would be my > preference. > > A problem with the iounmap() in the proposed patch is that the region > may have been an I/O port, so we could end up unmapping the I/O space. It boils down to finding a way to match a VA to a BAR resource so that we can mirror on pci_iounmap() what's done in pci_iomap_range() (ie check BAR resource flags to define how/if to unmap them), that would do as a generic pci_iounmap() implementation. In the pcim_* interface that looks easy to do, in the non-managed case ideas welcome - at the end of the day the deal is having a way to detect in a generic way what's behind a void __iomem *. Lorenzo
On Wed, Sep 09, 2020 at 12:36:13PM +0100, Lorenzo Pieralisi wrote: > On Mon, Sep 07, 2020 at 12:21:19PM +0100, Catalin Marinas wrote: > > On Mon, Sep 07, 2020 at 10:51:21AM +0000, George Cherian wrote: > > > Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > On Sat, Sep 05, 2020 at 10:48:11AM +0800, Yang Yingliang wrote: > > > > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index > > > > > 1006ed2d7c604..ddfa1c53def48 100644 > > > > > --- a/arch/arm64/kernel/pci.c > > > > > +++ b/arch/arm64/kernel/pci.c > > > > > @@ -217,4 +217,9 @@ void pcibios_remove_bus(struct pci_bus *bus) > > > > > acpi_pci_remove_bus(bus); > > > > > } > > > > > > > > > > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr) { > > > > > + iounmap(addr); > > > > > +} > > > > > +EXPORT_SYMBOL(pci_iounmap); > > > > > > > > So, what's wrong with the generic pci_iounmap() implementation? > > > > Shouldn't it call iounmap() already? > > > > > > Since ARM64 selects CONFIG_GENERIC_PCI_IOMAP and not > > > CONFIG_GENERIC_IOMAP, the pci_iounmap function is reduced to a NULL > > > function. Due to this, even the managed release variants or even the explicit > > > pci_iounmap calls doesn't really remove the mappings leading to leak. > > > > Ah, I missed the fact that pci_iounmap() depends on a different > > config option. > > > > > https://lkml.org/lkml/2020/8/20/28 > > > > So is this going to be fixed in the generic code? That would be my > > preference. > > > > A problem with the iounmap() in the proposed patch is that the region > > may have been an I/O port, so we could end up unmapping the I/O space. > > It boils down to finding a way to match a VA to a BAR resource so that > we can mirror on pci_iounmap() what's done in pci_iomap_range() (ie > check BAR resource flags to define how/if to unmap them), that would do > as a generic pci_iounmap() implementation. In the !CONFIG_GENERIC_IOMAP case (arm64), for IORESOURCE_IO, pci_iomap_range() calls __pci_ioport_map() which, with the default ioport_map(), it ends up with a simple PCI_IOBASE + (port & IO_SPACE_LIMIT). pci_iounmap() could check whether the pointer is in the PCI_IOBASE - PCI_IOBASE+IO_SPACE_LIMIT range before calling ioremap(), unless the arch code re-defined ioport_map. Something like below (not even compiled): diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index dabf8cb7203b..fada420c9cd6 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -919,6 +919,11 @@ extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max); #define pci_iounmap pci_iounmap static inline void pci_iounmap(struct pci_dev *dev, void __iomem *p) { +#ifndef ARCH_HAS_IOPORT_MAP + if (p >= PCI_IOBASE && p < PCI_IOBASE + IO_SPACE_LIMIT) + return; + iounmap(p); +#endif } #endif #endif /* CONFIG_GENERIC_IOMAP */ @@ -1009,7 +1014,9 @@ static inline void __iomem *ioremap_uc(phys_addr_t offset, size_t size) #ifdef CONFIG_HAS_IOPORT_MAP #ifndef CONFIG_GENERIC_IOMAP -#ifndef ioport_map +#ifdef ioport_map +#define ARCH_HAS_IOPORT_MAP +#else #define ioport_map ioport_map static inline void __iomem *ioport_map(unsigned long port, unsigned int nr) {
On Wed, Sep 09, 2020 at 02:54:01PM +0100, Catalin Marinas wrote: > On Wed, Sep 09, 2020 at 12:36:13PM +0100, Lorenzo Pieralisi wrote: > > On Mon, Sep 07, 2020 at 12:21:19PM +0100, Catalin Marinas wrote: > > > On Mon, Sep 07, 2020 at 10:51:21AM +0000, George Cherian wrote: > > > > Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > > On Sat, Sep 05, 2020 at 10:48:11AM +0800, Yang Yingliang wrote: > > > > > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index > > > > > > 1006ed2d7c604..ddfa1c53def48 100644 > > > > > > --- a/arch/arm64/kernel/pci.c > > > > > > +++ b/arch/arm64/kernel/pci.c > > > > > > @@ -217,4 +217,9 @@ void pcibios_remove_bus(struct pci_bus *bus) > > > > > > acpi_pci_remove_bus(bus); > > > > > > } > > > > > > > > > > > > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr) { > > > > > > + iounmap(addr); > > > > > > +} > > > > > > +EXPORT_SYMBOL(pci_iounmap); > > > > > > > > > > So, what's wrong with the generic pci_iounmap() implementation? > > > > > Shouldn't it call iounmap() already? > > > > > > > > Since ARM64 selects CONFIG_GENERIC_PCI_IOMAP and not > > > > CONFIG_GENERIC_IOMAP, the pci_iounmap function is reduced to a NULL > > > > function. Due to this, even the managed release variants or even the explicit > > > > pci_iounmap calls doesn't really remove the mappings leading to leak. > > > > > > Ah, I missed the fact that pci_iounmap() depends on a different > > > config option. > > > > > > > https://lkml.org/lkml/2020/8/20/28 > > > > > > So is this going to be fixed in the generic code? That would be my > > > preference. > > > > > > A problem with the iounmap() in the proposed patch is that the region > > > may have been an I/O port, so we could end up unmapping the I/O space. > > > > It boils down to finding a way to match a VA to a BAR resource so that > > we can mirror on pci_iounmap() what's done in pci_iomap_range() (ie > > check BAR resource flags to define how/if to unmap them), that would do > > as a generic pci_iounmap() implementation. > > In the !CONFIG_GENERIC_IOMAP case (arm64), for IORESOURCE_IO, > pci_iomap_range() calls __pci_ioport_map() which, with the default > ioport_map(), it ends up with a simple PCI_IOBASE + (port & > IO_SPACE_LIMIT). > > pci_iounmap() could check whether the pointer is in the PCI_IOBASE - > PCI_IOBASE+IO_SPACE_LIMIT range before calling ioremap(), unless the > arch code re-defined ioport_map. Something like below (not even > compiled): I gave it some thought - with the current state of affairs (which is not ideal - this *_IOMAP stuff is ways too complex) it is likely to be the safest/only way we can have this in generic code, short of implementing what I mentioned (but that implies keeping track of BAR VA mappings) or cleaning up this nest of defines. Lorenzo > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > index dabf8cb7203b..fada420c9cd6 100644 > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -919,6 +919,11 @@ extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max); > #define pci_iounmap pci_iounmap > static inline void pci_iounmap(struct pci_dev *dev, void __iomem *p) > { > +#ifndef ARCH_HAS_IOPORT_MAP > + if (p >= PCI_IOBASE && p < PCI_IOBASE + IO_SPACE_LIMIT) > + return; > + iounmap(p); > +#endif > } > #endif > #endif /* CONFIG_GENERIC_IOMAP */ > @@ -1009,7 +1014,9 @@ static inline void __iomem *ioremap_uc(phys_addr_t offset, size_t size) > > #ifdef CONFIG_HAS_IOPORT_MAP > #ifndef CONFIG_GENERIC_IOMAP > -#ifndef ioport_map > +#ifdef ioport_map > +#define ARCH_HAS_IOPORT_MAP > +#else > #define ioport_map ioport_map > static inline void __iomem *ioport_map(unsigned long port, unsigned int nr) > { > > -- > Catalin
On Wed, Sep 09, 2020 at 02:54:01PM +0100, Catalin Marinas wrote: > On Wed, Sep 09, 2020 at 12:36:13PM +0100, Lorenzo Pieralisi wrote: > > On Mon, Sep 07, 2020 at 12:21:19PM +0100, Catalin Marinas wrote: > > > On Mon, Sep 07, 2020 at 10:51:21AM +0000, George Cherian wrote: > > > > Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > > On Sat, Sep 05, 2020 at 10:48:11AM +0800, Yang Yingliang wrote: > > > > > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index > > > > > > 1006ed2d7c604..ddfa1c53def48 100644 > > > > > > --- a/arch/arm64/kernel/pci.c > > > > > > +++ b/arch/arm64/kernel/pci.c > > > > > > @@ -217,4 +217,9 @@ void pcibios_remove_bus(struct pci_bus *bus) > > > > > > acpi_pci_remove_bus(bus); > > > > > > } > > > > > > > > > > > > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr) { > > > > > > + iounmap(addr); > > > > > > +} > > > > > > +EXPORT_SYMBOL(pci_iounmap); > > > > > > > > > > So, what's wrong with the generic pci_iounmap() implementation? > > > > > Shouldn't it call iounmap() already? > > > > > > > > Since ARM64 selects CONFIG_GENERIC_PCI_IOMAP and not > > > > CONFIG_GENERIC_IOMAP, the pci_iounmap function is reduced to a NULL > > > > function. Due to this, even the managed release variants or even the explicit > > > > pci_iounmap calls doesn't really remove the mappings leading to leak. > > > > > > Ah, I missed the fact that pci_iounmap() depends on a different > > > config option. > > > > > > > https://lkml.org/lkml/2020/8/20/28 > > > > > > So is this going to be fixed in the generic code? That would be my > > > preference. > > > > > > A problem with the iounmap() in the proposed patch is that the region > > > may have been an I/O port, so we could end up unmapping the I/O space. > > > > It boils down to finding a way to match a VA to a BAR resource so that > > we can mirror on pci_iounmap() what's done in pci_iomap_range() (ie > > check BAR resource flags to define how/if to unmap them), that would do > > as a generic pci_iounmap() implementation. > > In the !CONFIG_GENERIC_IOMAP case (arm64), for IORESOURCE_IO, > pci_iomap_range() calls __pci_ioport_map() which, with the default > ioport_map(), it ends up with a simple PCI_IOBASE + (port & > IO_SPACE_LIMIT). > > pci_iounmap() could check whether the pointer is in the PCI_IOBASE - > PCI_IOBASE+IO_SPACE_LIMIT range before calling ioremap(), unless the > arch code re-defined ioport_map. Something like below (not even > compiled): For everyone's information, I shall post a fix when I manage to make the kbuild bot happy on all arches. Lorenzo > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > index dabf8cb7203b..fada420c9cd6 100644 > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -919,6 +919,11 @@ extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max); > #define pci_iounmap pci_iounmap > static inline void pci_iounmap(struct pci_dev *dev, void __iomem *p) > { > +#ifndef ARCH_HAS_IOPORT_MAP > + if (p >= PCI_IOBASE && p < PCI_IOBASE + IO_SPACE_LIMIT) > + return; > + iounmap(p); > +#endif > } > #endif > #endif /* CONFIG_GENERIC_IOMAP */ > @@ -1009,7 +1014,9 @@ static inline void __iomem *ioremap_uc(phys_addr_t offset, size_t size) > > #ifdef CONFIG_HAS_IOPORT_MAP > #ifndef CONFIG_GENERIC_IOMAP > -#ifndef ioport_map > +#ifdef ioport_map > +#define ARCH_HAS_IOPORT_MAP > +#else > #define ioport_map ioport_map > static inline void __iomem *ioport_map(unsigned long port, unsigned int nr) > { > > -- > Catalin
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h index ff50dd731852d..4d8da06ac295f 100644 --- a/arch/arm64/include/asm/io.h +++ b/arch/arm64/include/asm/io.h @@ -18,6 +18,11 @@ #include <asm/alternative.h> #include <asm/cpufeature.h> +#ifdef CONFIG_PCI +struct pci_dev; +#define pci_iounmap pci_iounmap +extern void pci_iounmap(struct pci_dev *dev, void __iomem *addr); +#endif /* * Generic IO read/write. These perform native-endian accesses. */ diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 1006ed2d7c604..ddfa1c53def48 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -217,4 +217,9 @@ void pcibios_remove_bus(struct pci_bus *bus) acpi_pci_remove_bus(bus); } +void pci_iounmap(struct pci_dev *dev, void __iomem *addr) +{ + iounmap(addr); +} +EXPORT_SYMBOL(pci_iounmap); #endif
config GENERIC_IOMAP is disabled on arm64, so pci_iounmap() does nothing, when we using pci_iomap/pci_iounmap(), it will lead to memory leak. Implements pci_iounmap() for arm64 to fix this leak. Fixes: 09a5723983e3 ("arm64: Use include/asm-generic/io.h") Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- arch/arm64/include/asm/io.h | 5 +++++ arch/arm64/kernel/pci.c | 5 +++++ 2 files changed, 10 insertions(+)