linux-kernel - Re: [PATCH] genirq/manage: Reduce priority of forced secondary IRQ handler

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: <aP8T7rahGYzJqnP5@wunner.de>
Date: Mon, 27 Oct 2025 07:40:46 +0100
From: Lukas Wunner <lukas@...ner.de>
To: Crystal Wood <crwood@...hat.com>
Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Juri Lelli <juri.lelli@...hat.com>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Clark Williams <clrkwllms@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
	Valentin Schneider <vschneid@...hat.com>,
	linux-kernel@...r.kernel.org, Attila Fazekas <afazekas@...hat.com>,
	linux-pci@...r.kernel.org, linux-rt-devel@...ts.linux.dev,
	Bjorn Helgaas <helgaas@...nel.org>,
	Mahesh J Salgaonkar <mahesh@...ux.ibm.com>,
	Oliver OHalloran <oohall@...il.com>
Subject: Re: [PATCH] genirq/manage: Reduce priority of forced secondary IRQ
 handler

On Fri, Oct 24, 2025 at 04:00:02PM -0500, Crystal Wood wrote:
> On Fri, 2025-10-24 at 15:33 +0200, Sebastian Andrzej Siewior wrote:
> > On 2025-10-03 13:25:53 [-0500], Crystal Wood wrote:
> > > On Sun, 2025-09-21 at 15:12 +0200, Lukas Wunner wrote:
> > > > On Sat, Sep 20, 2025 at 11:20:26PM +0200, Thomas Gleixner wrote:
> > > > > I obviously understand that the proposed change squashs the whole
> > > > > class of similar (not yet detected) issues, but that made me look at
> > > > > that particular instance nevertheless.
> > > > > 
> > > > > All aer_irq() does is reading two PCI config words, writing one and
> > > > > then sticking 64bytes into a KFIFO. All of that is hard interrupt
> > > > > safe. So arguably this AER problem can be nicely solved by the below
> > > > > one-liner, no?
> > > > 
> > > > The one-liner (which sets IRQF_NO_THREAD) was what Crystal originally
> > > > proposed:
> > > > 
> > > > https://lore.kernel.org/r/20250902224441.368483-1-crwood@redhat.com/
> > > 
> > > So, is the plan to apply the original patch then?
> > 
> > Did we settle on something?
> > I wasn't sure if you can mix IRQF_NO_THREAD with IRQF_ONESHOT for shared
> > handlers. If that is a thing, we Crystal's original would do it.
> 
> Do you mean mixing IRQF_NO_THREAD on this irq (which should eliminate
> the forced IRQF_ONESHOT) with another shared irq that still has
> IRQF_ONESHOT?
> 
> I suspect it was a non-issue because of IRQCHIP_ONESHOT_SAFE disabling
> the forced oneshot (the other irq was pciehp).  Given that these are
> pcie-specific, do they ever get used without MSI (which sets
> IRQCHIP_ONESHOT_SAFE)[1]?

It seems fragile to depend on IRQCHIP_ONESHOT_SAFE.  What about irqchips
which don't set that?  What about PCIe ports which use legacy INTx
instead of MSI?

As Sebastian pointed out, setting IRQF_NO_THREAD for the AER driver but
not for any of the other PCIe port services risks starving the AER
primary handler if any of the other PCIe port services' (threaded)
primary handlers take a long time:

https://lore.kernel.org/r/20250904073024.YsLeZqK_@linutronix.de/

I went through all PCIe port services to see if IRQF_NO_THREAD could be
set on all of them.  Alas that's not possible:

pciehp's primary handler acquires a spinlock because of runtime PM API
calls (device->power.lock).  It's not a raw_spin_lock_t, so it becomes
a sleeping lock on RT which cannot be acquired in hardirq context and
converting to a raw_spin_lock_t would be non-trivial and probably
undesirable.

pme's primary handler also acquires a spinlock, this one could be
converted to a raw_spin_lock_t.

FWIW, PCIe port services requesting a (potentially shared) interrupt are:
drivers/pci/hotplug/pciehp_hpc.c
drivers/pci/pcie/aer.c
drivers/pci/pcie/dpc.c
drivers/pci/pcie/pme.c
drivers/pci/pcie/bwctrl.c

Long story short, I'll respin the patch to reduce the forced secondary
thread's priority, taking into account Thomas' feedback.
(Apologies for not having done this earlier.)

Thanks,

Lukas