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: <87sek2no5s.ffs@tglx>
Date: Sat, 14 Jun 2025 21:01:03 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Doug Anderson <dianders@...omium.org>, Brian Norris
 <briannorris@...omium.org>
Cc: 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

On Tue, Jun 10 2025 at 09:43, Doug Anderson wrote:
> On Tue, May 13, 2025 at 4:02 PM Brian Norris <briannorris@...omium.org> wrote:
>> 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.

I don't have a strong opinion either way.

> 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...

No. That's creating inconstent state. It's already ugly enough. So if we
go there then we make it explicit like we did for the managed case,
i.e. something like this

void enable_wakeup_irq(struct irq_desc *desc)
{
	irqd_set(irqd, IRQD_IRQ_ENABLED_ON_SUSPEND);
	desc->saved_depth = desc->depth;
	desc->depth = 1;
	__enable_irq(desc);
}

and then have a counterpart which disables and restores the state.

Thanks,

        tglx