linux-kernel - Re: [PATCH v2] arm64: kexec: Check if IRQ is already masked before masking

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: <87o71zy75c.ffs@tglx>
Date: Thu, 28 Nov 2024 11:39:11 +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, akpm@...ux-foundation.org, bhe@...hat.com,
 farbere@...zon.com, hbathini@...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
Cc: jonnyc@...zon.com
Subject: Re: [PATCH v2] arm64: kexec: Check if IRQ is already masked before
 masking

On Wed, Nov 27 2024 at 15:22, Eliav Farber wrote:
> diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
> index 80ceb5bd2680..54d0bd1bd449 100644
> --- a/arch/arm/kernel/machine_kexec.c
> +++ b/arch/arm/kernel/machine_kexec.c
> @@ -142,11 +142,8 @@ static void machine_kexec_mask_interrupts(void)
>  		if (chip->irq_eoi && irqd_irq_inprogress(&desc->irq_data))
>  			chip->irq_eoi(&desc->irq_data);
>  
> -		if (chip->irq_mask)
> -			chip->irq_mask(&desc->irq_data);
> -
> -		if (chip->irq_disable && !irqd_irq_disabled(&desc->irq_data))
> -			chip->irq_disable(&desc->irq_data);
> +		irq_set_status_flags(i, IRQ_DISABLE_UNLAZY);
> +		irq_disable(desc);

This is just wrong. If the interrupt was torn down, then its state is
deactivated and it was masked already. So the EOI handling and the
mask/disable dance are neither required nor make sense.

So this whole thing should be:

		chip = irq_desc_get_chip(desc);
-		if (!chip)
+		if (!chip || !irqd_is_started(&desc->irq_data))
                	continue;

But what's worse is that we have 4 almost identical variants of the same
code.

So instead of exposing core functionality and "fixing" up four variants,
can we please have a consolidated version of this function in the core
code:
                struct irq_chip *chip;
                int check_eoi = 1;

		chip = irq_desc_get_chip(desc);
		if (!chip || !irqd_is_started(&desc->irq_data))
                	continue;

                if (IS_ENABLED(CONFIG_.....)) {
                        /*
                         * Add a sensible comment which explains this.
                         */
                	check_eoi = irq_set_irqchip_state(....);
                }

		if (check_eoi && ....)
                	chip->irq_eoi(&desc->irq_data);

		irq_shutdown(desc);

No?

Thanks,

        tglx