| Message ID | 20240416153331.1617772-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | powerpc: drop port I/O helpers for CONFIG_HAS_IOPORT=n | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | fail | sparse (mpc885_ads_defconfig, fedora-38, ppc64) failed at step Build. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | fail | kernel (mpc885_ads_defconfig, korg-5.5.0) failed at step Build. |
| snowpatch_ozlabs/github-powerpc_clang | fail | kernel (mpc885_ads, fedora-38, ppc64) failed at step Build. |
Arnd Bergmann <arnd@kernel.org> writes: > From: Arnd Bergmann <arnd@arndb.de> > > Calling inb()/outb() on powerpc when CONFIG_PCI is disabled causes > a NULL pointer dereference, which is bad for a number of reasons. > > After my patch to turn on -Werror in linux-next, this caused a > compiler-time warning with clang: > > In file included from arch/powerpc/include/asm/io.h:672: > arch/powerpc/include/asm/io-defs.h:43:1: error: performing pointer > arithmetic on a null pointer has undefined behavior > [-Werror,-Wnull-pointer-arithmetic] > 43 | DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 44 | (p, b, c), pio, p) > | ~~~~~~~~~~~~~~~~~~ > > In this configuration, CONFIG_HAS_IOPORT is already disabled, and all > drivers that use inb()/outb() should now depend on that (some patches are > still in the process of getting marged). > > Hide all references to inb()/outb() in the powerpc code and the definitions > when HAS_IOPORT is disabled to remove the possible NULL pointer access. > The same should happin in asm-generic in the near future, but for now > the empty inb() macros are still defined to ensure the generic version > does not get pulled in. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> > -- This needs a small fixup: diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h index 86c212fcbc0c..60c80d0baf40 100644 --- a/arch/powerpc/include/asm/io.h +++ b/arch/powerpc/include/asm/io.h @@ -692,6 +692,7 @@ static inline void name at \ #define writesw writesw #define writesl writesl +#ifdef CONFIG_HAS_IOPORT #define inb inb #define inw inw #define inl inl @@ -704,6 +705,8 @@ static inline void name at \ #define outsb outsb #define outsw outsw #define outsl outsl +#endif // CONFIG_HAS_IOPORT + #ifdef __powerpc64__ #define readq readq #define writeq writeq I'm running it through some randconfig builds now. cheers
Michael Ellerman <mpe@ellerman.id.au> writes: > Arnd Bergmann <arnd@kernel.org> writes: >> From: Arnd Bergmann <arnd@arndb.de> >> >> Calling inb()/outb() on powerpc when CONFIG_PCI is disabled causes >> a NULL pointer dereference, which is bad for a number of reasons. >> >> After my patch to turn on -Werror in linux-next, this caused a >> compiler-time warning with clang: >> >> In file included from arch/powerpc/include/asm/io.h:672: >> arch/powerpc/include/asm/io-defs.h:43:1: error: performing pointer >> arithmetic on a null pointer has undefined behavior >> [-Werror,-Wnull-pointer-arithmetic] >> 43 | DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c), >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> 44 | (p, b, c), pio, p) >> | ~~~~~~~~~~~~~~~~~~ >> >> In this configuration, CONFIG_HAS_IOPORT is already disabled, and all >> drivers that use inb()/outb() should now depend on that (some patches are >> still in the process of getting marged). >> >> Hide all references to inb()/outb() in the powerpc code and the definitions >> when HAS_IOPORT is disabled to remove the possible NULL pointer access. >> The same should happin in asm-generic in the near future, but for now >> the empty inb() macros are still defined to ensure the generic version >> does not get pulled in. >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> >> -- > > This needs a small fixup: Well, only because my tree doesn't have f0a816fb12da ("/dev/port: don't compile file operations without CONFIG_DEVPORT"). cheers
On Thu, Apr 18, 2024, at 08:26, Michael Ellerman wrote: > Arnd Bergmann <arnd@kernel.org> writes: > @@ -692,6 +692,7 @@ static inline void name at > \ > #define writesw writesw > #define writesl writesl > > +#ifdef CONFIG_HAS_IOPORT > #define inb inb > #define inw inw > #define inl inl > @@ -704,6 +705,8 @@ static inline void name at > \ > #define outsb outsb > #define outsw outsw > #define outsl outsl > +#endif // CONFIG_HAS_IOPORT > + > #ifdef __powerpc64__ > #define readq readq > #define writeq writeq I had included this at first, but then I still ran into the same warnings because it ends up pulling in the generic outsb() etc from include/asm-generic/io.h that relies on setting a non-NULL PCI_IOBASE. Arnd
"Arnd Bergmann" <arnd@arndb.de> writes: > On Thu, Apr 18, 2024, at 08:26, Michael Ellerman wrote: >> Arnd Bergmann <arnd@kernel.org> writes: > >> @@ -692,6 +692,7 @@ static inline void name at >> \ >> #define writesw writesw >> #define writesl writesl >> >> +#ifdef CONFIG_HAS_IOPORT >> #define inb inb >> #define inw inw >> #define inl inl >> @@ -704,6 +705,8 @@ static inline void name at >> \ >> #define outsb outsb >> #define outsw outsw >> #define outsl outsl >> +#endif // CONFIG_HAS_IOPORT >> + >> #ifdef __powerpc64__ >> #define readq readq >> #define writeq writeq > > I had included this at first, but then I still ran into > the same warnings because it ends up pulling in the > generic outsb() etc from include/asm-generic/io.h > that relies on setting a non-NULL PCI_IOBASE. Yes you're right. The above fixes the gcc build, but not clang. So I think I'll just cherry pick f0a816fb12da ("/dev/port: don't compile file operations without CONFIG_DEVPORT") into my next and then apply this. But will see if there's any other build failures over night. cheers
Michael Ellerman <mpe@ellerman.id.au> writes: > "Arnd Bergmann" <arnd@arndb.de> writes: >> On Thu, Apr 18, 2024, at 08:26, Michael Ellerman wrote: >>> Arnd Bergmann <arnd@kernel.org> writes: >> >>> @@ -692,6 +692,7 @@ static inline void name at >>> \ >>> #define writesw writesw >>> #define writesl writesl >>> >>> +#ifdef CONFIG_HAS_IOPORT >>> #define inb inb >>> #define inw inw >>> #define inl inl >>> @@ -704,6 +705,8 @@ static inline void name at >>> \ >>> #define outsb outsb >>> #define outsw outsw >>> #define outsl outsl >>> +#endif // CONFIG_HAS_IOPORT >>> + >>> #ifdef __powerpc64__ >>> #define readq readq >>> #define writeq writeq >> >> I had included this at first, but then I still ran into >> the same warnings because it ends up pulling in the >> generic outsb() etc from include/asm-generic/io.h >> that relies on setting a non-NULL PCI_IOBASE. > > Yes you're right. The above fixes the gcc build, but not clang. > > So I think I'll just cherry pick f0a816fb12da ("/dev/port: don't compile > file operations without CONFIG_DEVPORT") into my next and then apply > this. But will see if there's any other build failures over night. That didn't work. Still lots of drivers in my tree (based on rc2) which use inb/outb etc, and barf on the empty #define inb. So I think this patch needs to wait until all the CONFIG_HAS_IOPORT checks have been merged for various drivers. For now the below fixes the clang warning. AFAICS it's safe because any code using inb() etc. with CONFIG_PCI=n is currently just doing a plain load from virtual address ~zero which should fault anyway. cheers diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h index 08c550ed49be..1cd6eb6c8101 100644 --- a/arch/powerpc/include/asm/io.h +++ b/arch/powerpc/include/asm/io.h @@ -37,7 +37,7 @@ extern struct pci_dev *isa_bridge_pcidev; * define properly based on the platform */ #ifndef CONFIG_PCI -#define _IO_BASE 0 +#define _IO_BASE POISON_POINTER_DELTA #define _ISA_MEM_BASE 0 #define PCI_DRAM_OFFSET 0 #elif defined(CONFIG_PPC32)
On Fri, Apr 19, 2024, at 07:12, Michael Ellerman wrote: > Michael Ellerman <mpe@ellerman.id.au> writes: >> "Arnd Bergmann" <arnd@arndb.de> writes: >>> >>> I had included this at first, but then I still ran into >>> the same warnings because it ends up pulling in the >>> generic outsb() etc from include/asm-generic/io.h >>> that relies on setting a non-NULL PCI_IOBASE. >> >> Yes you're right. The above fixes the gcc build, but not clang. >> >> So I think I'll just cherry pick f0a816fb12da ("/dev/port: don't compile >> file operations without CONFIG_DEVPORT") into my next and then apply >> this. But will see if there's any other build failures over night. > > That didn't work. Still lots of drivers in my tree (based on rc2) which > use inb/outb etc, and barf on the empty #define inb. Right, the patches from Niklas only went into linux-next so far, and a few are missing (including the 8250 one I think), so -rc2 at the moment regresses, but that doesn't have the warning either. The idea of my patch was to both fix the current linux-next build regression and have something that works in the long run, I didn't expect it to work by itself. Sorry that wasn't clear from my description. > So I think this patch needs to wait until all the CONFIG_HAS_IOPORT > checks have been merged for various drivers. > > For now the below fixes the clang warning. AFAICS it's safe because any > code using inb() etc. with CONFIG_PCI=n is currently just doing a plain > load from virtual address ~zero which should fault anyway. If the port number is high enough, the current code might end up referencing a user space address, depending on mmap_min_addr, which defaults to 4096. Using POISON_POINTER_DELTA is clearly an improvement over that. > diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h > index 08c550ed49be..1cd6eb6c8101 100644 > --- a/arch/powerpc/include/asm/io.h > +++ b/arch/powerpc/include/asm/io.h > @@ -37,7 +37,7 @@ extern struct pci_dev *isa_bridge_pcidev; > * define properly based on the platform > */ > #ifndef CONFIG_PCI > -#define _IO_BASE 0 > +#define _IO_BASE POISON_POINTER_DELTA > #define _ISA_MEM_BASE 0 > #define PCI_DRAM_OFFSET 0 > #elif defined(CONFIG_PPC32) You may need to double-check, but I think for ppc64 we can just unconditionally set _IO_BASE to ISA_IO_BASE regardless of CONFIG_PCI. 3d5134ee8341 ("[POWERPC] Rewrite IO allocation & mapping on powerpc64") ensured that the I/O space is only ever mapped to this virtual address, and the same method is used with the asm-generic/io.h implementation on arm/arm64/loongarch/ m68k/riscv/xtensa. Using this would be both safer and more efficient than the current version. It should also not cause any regressions ;-) Unfortunately, ppc32 never got that cleanup, so POISON_POINTER_DELTA is probably still best until Niklas's series is merged. You could set _ISA_MEM_BASE to the same here for good measure. [another side note: the non-zero PCI_DRAM_OFFSET looks unnecessary as well now, apparently this was meant for ibm cpc710 and ppc440 platforms that are no longer supported.] Arnd
diff --git a/arch/powerpc/include/asm/dma.h b/arch/powerpc/include/asm/dma.h index d97c66d9ae34..004a868f82c9 100644 --- a/arch/powerpc/include/asm/dma.h +++ b/arch/powerpc/include/asm/dma.h @@ -3,6 +3,12 @@ #define _ASM_POWERPC_DMA_H #ifdef __KERNEL__ +/* The maximum address that we can perform a DMA transfer to on this platform */ +/* Doesn't really apply... */ +#define MAX_DMA_ADDRESS (~0UL) + +#ifdef CONFIG_HAS_IOPORT + /* * Defines for using and allocating dma channels. * Written by Hennus Bergman, 1992. @@ -26,10 +32,6 @@ #define MAX_DMA_CHANNELS 8 #endif -/* The maximum address that we can perform a DMA transfer to on this platform */ -/* Doesn't really apply... */ -#define MAX_DMA_ADDRESS (~0UL) - #ifdef HAVE_REALLY_SLOW_DMA_CONTROLLER #define dma_outb outb_p #else @@ -340,5 +342,7 @@ extern int request_dma(unsigned int dmanr, const char *device_id); /* release it again */ extern void free_dma(unsigned int dmanr); +#endif + #endif /* __KERNEL__ */ #endif /* _ASM_POWERPC_DMA_H */ diff --git a/arch/powerpc/include/asm/io-defs.h b/arch/powerpc/include/asm/io-defs.h index faf8617cc574..8d2209af7759 100644 --- a/arch/powerpc/include/asm/io-defs.h +++ b/arch/powerpc/include/asm/io-defs.h @@ -20,12 +20,14 @@ DEF_PCI_AC_NORET(writeq, (u64 val, PCI_IO_ADDR addr), (val, addr), mem, addr) DEF_PCI_AC_NORET(writeq_be, (u64 val, PCI_IO_ADDR addr), (val, addr), mem, addr) #endif /* __powerpc64__ */ +#ifdef CONFIG_HAS_IOPORT DEF_PCI_AC_RET(inb, u8, (unsigned long port), (port), pio, port) DEF_PCI_AC_RET(inw, u16, (unsigned long port), (port), pio, port) DEF_PCI_AC_RET(inl, u32, (unsigned long port), (port), pio, port) DEF_PCI_AC_NORET(outb, (u8 val, unsigned long port), (val, port), pio, port) DEF_PCI_AC_NORET(outw, (u16 val, unsigned long port), (val, port), pio, port) DEF_PCI_AC_NORET(outl, (u32 val, unsigned long port), (val, port), pio, port) +#endif DEF_PCI_AC_NORET(readsb, (const PCI_IO_ADDR a, void *b, unsigned long c), (a, b, c), mem, a) @@ -40,6 +42,7 @@ DEF_PCI_AC_NORET(writesw, (PCI_IO_ADDR a, const void *b, unsigned long c), DEF_PCI_AC_NORET(writesl, (PCI_IO_ADDR a, const void *b, unsigned long c), (a, b, c), mem, a) +#ifdef CONFIG_HAS_IOPORT DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c), (p, b, c), pio, p) DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c), @@ -52,6 +55,7 @@ DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c), (p, b, c), pio, p) DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c), (p, b, c), pio, p) +#endif DEF_PCI_AC_NORET(memset_io, (PCI_IO_ADDR a, int c, unsigned long n), (a, c, n), mem, a) diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h index 08c550ed49be..86c212fcbc0c 100644 --- a/arch/powerpc/include/asm/io.h +++ b/arch/powerpc/include/asm/io.h @@ -37,7 +37,6 @@ extern struct pci_dev *isa_bridge_pcidev; * define properly based on the platform */ #ifndef CONFIG_PCI -#define _IO_BASE 0 #define _ISA_MEM_BASE 0 #define PCI_DRAM_OFFSET 0 #elif defined(CONFIG_PPC32) @@ -486,8 +485,7 @@ static inline u64 __raw_rm_readq(volatile void __iomem *paddr) * to port it over */ -#ifdef CONFIG_PPC32 - +#if defined(CONFIG_PPC32) && defined(CONFIG_HAS_IOPORT) #define __do_in_asm(name, op) \ static inline unsigned int name(unsigned int port) \ { \ @@ -534,7 +532,7 @@ __do_out_asm(_rec_outb, "stbx") __do_out_asm(_rec_outw, "sthbrx") __do_out_asm(_rec_outl, "stwbrx") -#endif /* CONFIG_PPC32 */ +#endif /* CONFIG_PPC32 && CONFIG_HAS_IOPORT */ /* The "__do_*" operations below provide the actual "base" implementation * for each of the defined accessors. Some of them use the out_* functions @@ -577,6 +575,7 @@ __do_out_asm(_rec_outl, "stwbrx") #define __do_readq_be(addr) in_be64(PCI_FIX_ADDR(addr)) #endif /* !defined(CONFIG_EEH) */ +#ifdef CONFIG_HAS_IOPORT #ifdef CONFIG_PPC32 #define __do_outb(val, port) _rec_outb(val, port) #define __do_outw(val, port) _rec_outw(val, port) @@ -592,6 +591,7 @@ __do_out_asm(_rec_outl, "stwbrx") #define __do_inw(port) readw((PCI_IO_ADDR)_IO_BASE + port); #define __do_inl(port) readl((PCI_IO_ADDR)_IO_BASE + port); #endif /* !CONFIG_PPC32 */ +#endif #ifdef CONFIG_EEH #define __do_readsb(a, b, n) eeh_readsb(PCI_FIX_ADDR(a), (b), (n)) @@ -606,12 +606,14 @@ __do_out_asm(_rec_outl, "stwbrx") #define __do_writesw(a, b, n) _outsw(PCI_FIX_ADDR(a),(b),(n)) #define __do_writesl(a, b, n) _outsl(PCI_FIX_ADDR(a),(b),(n)) +#ifdef CONFIG_HAS_IOPORT #define __do_insb(p, b, n) readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n)) #define __do_insw(p, b, n) readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n)) #define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n)) #define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n)) #define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n)) #define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n)) +#endif #define __do_memset_io(addr, c, n) \ _memset_io(PCI_FIX_ADDR(addr), c, n) @@ -689,6 +691,7 @@ static inline void name at \ #define writesb writesb #define writesw writesw #define writesl writesl + #define inb inb #define inw inw #define inl inl @@ -848,8 +851,16 @@ static inline void iosync(void) #define inl_p(port) inl(port) #define outl_p(val, port) (udelay(1), outl((val), (port))) +#define insb_p insb +#define insw_p insw +#define insl_p insl +#define outsb_p outsb +#define outsw_p outsw +#define outsl_p outsl +#ifdef CONFIG_HAS_IOPORT #define IO_SPACE_LIMIT ~(0UL) +#endif /** * ioremap - map bus memory into CPU space diff --git a/arch/powerpc/kernel/iomap.c b/arch/powerpc/kernel/iomap.c index 72862a4d3a5d..33e36fda1ea8 100644 --- a/arch/powerpc/kernel/iomap.c +++ b/arch/powerpc/kernel/iomap.c @@ -13,7 +13,11 @@ void __iomem *ioport_map(unsigned long port, unsigned int len) { +#ifdef CONFIG_HAS_IOPORT return (void __iomem *) (port + _IO_BASE); +#else + return NULL; +#endif } EXPORT_SYMBOL(ioport_map); diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c index f23430adb68a..b1c34242d394 100644 --- a/arch/powerpc/kernel/traps.c +++ b/arch/powerpc/kernel/traps.c @@ -531,7 +531,7 @@ DEFINE_INTERRUPT_HANDLER_NMI(system_reset_exception) */ static inline int check_io_access(struct pt_regs *regs) { -#ifdef CONFIG_PPC32 +#if defined(CONFIG_PPC32) && defined(CONFIG_HAS_IOPORT) unsigned long msr = regs->msr; const struct exception_table_entry *entry; unsigned int *nip = (unsigned int *)regs->nip;