netdev - Re: [PATCH net-next v2 5/7] usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling

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: <20220510081858.GA13058@wunner.de>
Date:   Tue, 10 May 2022 10:18:58 +0200
From:   Lukas Wunner <lukas@...ner.de>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Mark Rutland <mark.rutland@....com>,
        Jakub Kicinski <kuba@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "David S. Miller" <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-usb@...r.kernel.org,
        Steve Glendinning <steve.glendinning@...well.net>,
        UNGLinuxDriver@...rochip.com, Oliver Neukum <oneukum@...e.com>,
        Andre Edich <andre.edich@...rochip.com>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Martyn Welch <martyn.welch@...labora.com>,
        Gabriel Hojda <ghojda@...urs.ro>,
        Christoph Fritz <chf.fritz@...glemail.com>,
        Lino Sanfilippo <LinoSanfilippo@....de>,
        Philipp Rosenberger <p.rosenberger@...bus.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Russell King <linux@...linux.org.uk>,
        Ferry Toth <fntoth@...il.com>
Subject: Re: [PATCH net-next v2 5/7] usbnet: smsc95xx: Forward PHY interrupts
 to PHY driver to avoid polling

On Mon, May 09, 2022 at 09:47:19AM +0100, Marc Zyngier wrote:
> On Fri, 06 May 2022 21:16:47 +0100, Lukas Wunner <lukas@...ner.de> wrote:
> > On Fri, May 06, 2022 at 11:58:43AM +0100, Marc Zyngier wrote:
> > > On Thu, 05 May 2022 19:53:28 +0100, Lukas Wunner <lukas@...ner.de> wrote:
> > > > generic_handle_domain_irq() warns unconditionally on !in_irq(),
> > > > unlike handle_irq_desc(), which constrains the warning to
> > > > handle_enforce_irqctx() (i.e. x86 APIC, arm GIC/GICv3).
> > > > Perhaps that's an oversight in generic_handle_domain_irq(),
> > > > unless __irq_resolve_mapping() becomes unsafe outside in_irq()
> > > > for some reason...
> > > > 
> > > > In any case the unconditional in_irq() necessitates __irq_enter_raw()
> > > > here.
> > > 
> > > Please don't directly use __irq_enter_raw() and similar things
> > > directly in driver code (it doesn't do anything related to RCU, for
> > > example, which could be problematic if used in arbitrary contexts).
> > 
> > As I've pointed out above, it seems like an oversight that Mark
> > didn't make the WARN_ON_ONCE() conditional on handle_enforce_irqctx()
> > (as handle_irq_desc() does).  Sadly you did not respond to that
> > observation.
> 
> When did you make that observation? I can only see an email from you
> being sent *after* the one I am replying to.

I was referring to the above-quoted sentence:

     "generic_handle_domain_irq() warns unconditionally on !in_irq(),
      unlike handle_irq_desc(), which constrains the warning to
      handle_enforce_irqctx() (i.e. x86 APIC, arm GIC/GICv3).
      Perhaps that's an oversight in generic_handle_domain_irq(),
      unless __irq_resolve_mapping() becomes unsafe outside in_irq()
      for some reason..."

Never mind, let's focus on the problem at hand.
It's secondary who said what when.


> > Please clarify whether that is indeed erroneous.
> > Once handle_enforce_irqctx() is added to generic_handle_domain_irq(),
> > there's no need for me to call __irq_enter_raw().  Problem solved.
> 
> I don't see it as an oversight. Drivers shouldn't rely on
> architectural quirks, and it is much clearer to simply forbid
> something that cannot be guaranteed across the board, specially for
> something that is as generic as USB.

Whether a warning is warranted is not dependent on the architecture,
but on the irqchip from which an interrupt normally originates:

* Interrupt normally originates from x86 APIC or arm GIC/GICv3,
  but is synthesized in non-hardirq context:  Warning is warranted.

* Interrupt normally originates from any other top-level irqchip,
  such as irq-bcm2836.c, but is synthesized in non-hardirq context:
  Warning is a false positive!

* Interrupt is always synthesized in non-hardirq context by a
  USB irqchip: Warning is a false positive, regardless whether
  the top-level irqchip is x86 APIC, arm GIC/GICv3 or anything else!


> > Should there be a valid reason for the missing handle_enforce_irqctx(),
> > then I propose adding a generic_handle_domain_irq_safe() function which
> > calls __irq_enter_raw() (or probably __irq_enter() to get accounting),
> > thereby resolving your objection to calling __irq_enter_raw() from a
> > driver.
> 
> Feel free to submit a patch.

Done:

https://lore.kernel.org/lkml/c3caf60bfa78e5fdbdf483096b7174da65d1813a.1652168866.git.lukas@wunner.de/

I'm focussing on eliminating the false-positive warning for now.
Introducing a generic_handle_domain_irq_safe() wrapper which alleviates
drivers from calling local_irq_save() can be done in a separate step.

Thanks,

Lukas