linux-kernel - RE: [PATCH v4 1/2] kexec: Consolidate machine_kexec_mask

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 for Android: free password hash cracker in your pocket

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

Message-ID: <cd8809e379dc4dd089350ec2d1f8c210@amazon.com>
Date: Sat, 30 Nov 2024 20:08:44 +0000
From: "Farber, Eliav" <farbere@...zon.com>
To: Thomas Gleixner <tglx@...utronix.de>, "linux@...linux.org.uk"
	<linux@...linux.org.uk>, "catalin.marinas@....com" <catalin.marinas@....com>,
	"will@...nel.org" <will@...nel.org>, "mpe@...erman.id.au"
	<mpe@...erman.id.au>, "npiggin@...il.com" <npiggin@...il.com>,
	"christophe.leroy@...roup.eu" <christophe.leroy@...roup.eu>,
	"naveen@...nel.org" <naveen@...nel.org>, "maddy@...ux.ibm.com"
	<maddy@...ux.ibm.com>, "paul.walmsley@...ive.com" <paul.walmsley@...ive.com>,
	"palmer@...belt.com" <palmer@...belt.com>, "aou@...s.berkeley.edu"
	<aou@...s.berkeley.edu>, "ebiederm@...ssion.com" <ebiederm@...ssion.com>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>, "bhe@...hat.com"
	<bhe@...hat.com>, "hbathini@...ux.ibm.com" <hbathini@...ux.ibm.com>,
	"sourabhjain@...ux.ibm.com" <sourabhjain@...ux.ibm.com>,
	"adityag@...ux.ibm.com" <adityag@...ux.ibm.com>, "songshuaishuai@...ylab.org"
	<songshuaishuai@...ylab.org>, "takakura@...inux.co.jp"
	<takakura@...inux.co.jp>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linuxppc-dev@...ts.ozlabs.org"
	<linuxppc-dev@...ts.ozlabs.org>, "linux-riscv@...ts.infradead.org"
	<linux-riscv@...ts.infradead.org>, "kexec@...ts.infradead.org"
	<kexec@...ts.infradead.org>, Marc Zyngier <maz@...nel.org>
CC: "Chocron, Jonathan" <jonnyc@...zon.com>
Subject: RE: [PATCH v4 1/2] kexec: Consolidate machine_kexec_mask_interrupts()
 implementation

On 11/29/2024 3:30 PM, Thomas Gleixner wrote:
>> Move the machine_kexec_mask_interrupts function to a common location in
>> kernel/kexec_core.c, removing duplicate implementations from architecture
>> specific files (arch/arm, arch/arm64, arch/powerpc, and arch/riscv).
>
> Can you please move this into kernel/irq/kexec.c?
>
> It's pure interrupt core internal code and there is no point to make
> core internal functions visible to random other code just because.
Done (in v5 series)

>> +void machine_kexec_mask_interrupts(void)
>> +{
>> +     unsigned int i;
>> +     struct irq_desc *desc;
>
>         struct irq_desc *desc;
>         unsigned int i;
>
> please
Done (in v5 series)

>> +     for_each_irq_desc(i, desc) {
>> +             struct irq_chip *chip;
>> +             int check_eoi = 1;
>> +
>> +             chip = irq_desc_get_chip(desc);
>> +             if (!chip)
>> +                     continue;
>> +
>> +             if (IS_ENABLED(CONFIG_ARM64)) {
>
> This should not be CONFIG_ARM64. Add something like:
>
> config GENERIC_IRQ_KEXEC_CLEAR_VM_FORWARD
>         bool
>
> and select this from ARM64?
Done (in v5 series)

>> +                     /*
>> +                      * First try to remove the active state. If this fails, try to EOI the
>> +                      * interrupt.
>
> This comment does not really explain what this is about. I know you
> copied it from the ARM64 implementation, but it should explain what this
> actually means. Something like:
>
>          First try to remove the active state from an interrupt which is
>          forwarded to a VM. If the interrupt is not forwarded, try to
>          EOI the interrupt.
>
> or something like that.
Done (in v5 series)

>> +                      */
>> +                     check_eoi = irq_set_irqchip_state(i, IRQCHIP_STATE_ACTIVE, false);
>
> Looking deeper. This function actually cannot be called from this
> context. It does:
>
>           irq_get_desc_buslock(irq, &flags, 0);
>
> which means for any interrupt which has an actual buslock implementation
> it will end up in a sleepable function and deadlock in the worst case.
>
> Marc?
I will wait for Marc's response regarding this issue.
Regardless, if any changes are required, I believe it would be better
to address them in a separate patch, as this behavior existed before my
modification.

Thanks, Eliav