linux-kernel - Re: [RFC 31/32] usb: handle HAS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YdMhFKOdBsDvFStt@rowland.harvard.edu>
Date:   Mon, 3 Jan 2022 11:15:16 -0500
From:   Alan Stern <stern@...land.harvard.edu>
To:     Niklas Schnelle <schnelle@...ux.ibm.com>
Cc:     Arnd Bergmann <arnd@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        John Garry <john.garry@...wei.com>,
        Nick Hu <nickhu@...estech.com>,
        Greentime Hu <green.hu@...il.com>,
        Vincent Chen <deanbo422@...il.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>, Guo Ren <guoren@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        linux-pci@...r.kernel.org, linux-riscv@...ts.infradead.org,
        linux-csky@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [RFC 31/32] usb: handle HAS_IOPORT dependencies

On Mon, Jan 03, 2022 at 12:35:45PM +0100, Niklas Schnelle wrote:
> On Fri, 2021-12-31 at 12:15 -0500, Alan Stern wrote:
> > On Fri, Dec 31, 2021 at 12:06:24PM +0100, Niklas Schnelle wrote:
> > > On Mon, 2021-12-27 at 15:36 -0500, Alan Stern wrote:
> > > > On Mon, Dec 27, 2021 at 05:43:16PM +0100, Niklas Schnelle wrote:

> > > > > diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
> > > > > index 8ae5ccd26753..8e30116b6fd2 100644
> > > > > --- a/drivers/usb/host/uhci-hcd.h
> > > > > +++ b/drivers/usb/host/uhci-hcd.h
> > > > > @@ -586,12 +586,14 @@ static inline int uhci_aspeed_reg(unsigned int reg)
> > > > >  
> > > > >  static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
> > > > >  {
> > > > > +#ifdef CONFIG_HAS_IOPORT
> > > > >  	if (uhci_has_pci_registers(uhci))
> > > > >  		return inl(uhci->io_addr + reg);
> > > > > -	else if (uhci_is_aspeed(uhci))
> > > > > +#endif
> > > > 
> > > > Instead of making all these changes (here and in the hunks below), you
> > > > can simply modify the definition of uhci_has_pci_registers() so that it
> > > > always gives 0 when CONFIG_HAS_IOPORT is N.
> > > > 
> > > > Alan Stern
> > > 
> > > I don't think that works, for example in the hunk you quoted returning
> > > 0 from uhci_has_pci_registers() only skips over the inl() at run-time.
> > > We're aiming to have inl() undeclared if HAS_IOPORT is unset though.
> > 
> > I see.  Do you think the following would be acceptable?  Add:
> > 
> > #ifdef CONFIG_HAS_IOPORT
> > #define UHCI_IN(x)	x
> > #define UHCI_OUT(x)	x
> > #else
> > #define UHCI_IN(x)	0
> > #define UHCI_OUT(x)
> > #endif
> > 
> > and then replace for example inl(uhci->io_addr + reg) with 
> > UHCI_IN(inl(uhci->io_addr + reg)).
> 
> In principle that looks like a valid approach. Not sure this is better
> than explicit ifdefs though.

The general preference in the kernel is to avoid sprinkling #ifdef's 
throughout function definitions, and instead encapsulate their effects 
with macros or inline functions -- like this.

>  With this approach one could add
> UHCI_IN()/UHCI_OUT() calls which end up as nops without realizing it as
> it would disable any compile time warning for using them without
> guarding against CONFIG_HAS_IOPORT being undefined.

To help prevent that, we can add

#undef UHCI_IN
#undef UHCI_OUT

at the end of this section.

> > The definition of uhci_has_pci_registers() should be updated in any 
> > case; there's no reason for it to do a runtime check of uhci->io_addr 
> > when HAS_IOPORT is disabled.
> 
> Agree. Interestingly same as with the "if
> (IS_ENABLED(CONFIG_HAS_IOPORT))" it seems having
> uhci_has_pci_registers() compile-time defined to 0 (I added a
> defined(CONFIG_HAS_IOPORT) to it) makes the compiler ignore the missing
> inl() decleration already. But I'm not sure if we should rely on that.

I definitely would not rely on it.

Alan Stern