linux-kernel - Re: [PATCH] genirq/PM: Fix IRQCHIP_ENABLE_WAKEUP_ON

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] [day] [month] [year] [list]

Message-ID: <CAD=FV=X_nkzpWzaPaFb+OC0+r-BYUoXmog=BNV43CR-WHoDTpA@mail.gmail.com>
Date: Tue, 10 Jun 2025 09:43:45 -0700
From: Doug Anderson <dianders@...omium.org>
To: Brian Norris <briannorris@...omium.org>
Cc: Thomas Gleixner <tglx@...utronix.de>, chintanpandya@...gle.com, 
	Marc Zyngier <maz@...nel.org>, Maulik Shah <quic_mkshah@...cinc.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] genirq/PM: Fix IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND if depth
 > 1

Hi,

On Tue, May 13, 2025 at 4:02 PM Brian Norris <briannorris@...omium.org> wrote:
>
> On Mon, May 12, 2025 at 05:32:52PM -0700, Doug Anderson wrote:
> > --- a/kernel/irq/chip.c
> > +++ b/kernel/irq/chip.c
> > @@ -272,7 +272,7 @@ int irq_startup(struct irq_desc *desc, bool resend, bool force)
> >       const struct cpumask *aff = irq_data_get_affinity_mask(d);
> >       int ret = 0;
> >
> > -     desc->depth = 0;
> > +     desc->depth--;
>
> I'm certainly no expert here, but I'm treading on the same code and ran
> into a problem with this line too. It seems wise to make this a
> decrement, and not an unconditional 0. But I'm not sure that we should
> then proceed to unmask an interrupt that might have depth==1 in the
> general case. I think we should defer the unmask until we actually reach
> 0.
>
> In fact, that's one aspect of the very problem I'm dealing with here:
>
> Subject: [PATCH 0/2] genirq: Retain disable-depth across irq_{shutdown,startup}()
> https://lore.kernel.org/all/20250513224402.864767-1-briannorris@chromium.org/
>
> It seems to me (again, not an expert) that maybe you need to solve your
> problems by dodging the disable-depth entirely. But I'm not sure the
> best way to do that.

I can give a shot at spinning the patch, but before doing so I'd love
to get agreement that this problem is worth solving. As I said above,
we're not actually hitting this in any real cases and the issue was
just found during code review. To me it feels like it's a real
(potential) bug and worth solving before it bites someone in the
future, but I won't force the issue and I'll drop the patch if that's
what everyone wants.

If it's agreed that I should move forward, I'd love advice on which
approach I should use. Should I do as Brian says and try to sidestep
disable-depth entirely in this case? I could factor out the "case 1"
case of __enable_irq() and call it directly and then make sure that
all I do is count "depth" while `IRQD_IRQ_ENABLED_ON_SUSPEND` is set.
That doesn't seem like it would be too ugly...

-Doug