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

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

On Fri, Nov 29 2024 at 11:31, Eliav Farber 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.

> +void machine_kexec_mask_interrupts(void)
> +{
> +	unsigned int i;
> +	struct irq_desc *desc;

	struct irq_desc *desc;
        unsigned int i;

please

> +	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?

> +			/*
> +			 * 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.

> +			 */
> +			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?

> +		}
> +
> +		if (check_eoi && chip->irq_eoi && irqd_irq_inprogress(&desc->irq_data))
> +			chip->irq_eoi(&desc->irq_data);

Thanks,

        tglx