linux-kernel - Re: [PATCH RFC 1/2] x86/apic: Do not make an exception for PIC_CASCADE_IR when marking legacy irqs in irq

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: <878s6k7ss1.fsf@vitty.brq.redhat.com>
Date:   Thu, 18 Mar 2021 09:47:10 +0100
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Thomas Gleixner <tglx@...utronix.de>, x86@...nel.org
Cc:     Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 1/2] x86/apic: Do not make an exception for
 PIC_CASCADE_IR when marking legacy irqs in irq_matrix

Thomas Gleixner <tglx@...utronix.de> writes:

> On Wed, Mar 17 2021 at 22:40, Thomas Gleixner wrote:
>> af174783b925 ("x86: I/O APIC: Never configure IRQ2")
>>
>> has a very nice explanation why.
>>
>> Back then the logic was quite different. All legacy PIC interrupts
>> (0-15) were bound to the legacy vectors at boot and never moved away.
>>
>> There was a check in the back then setup routing which prevented the
>> IOAPIC routing of IRQ2 which got lost at some point. Haven't figured out
>> yet where this might be. Still digging in those ancient horrors.
>
> So the commit in question is:
>
>   d32932d02e18 ("x86/irq: Convert IOAPIC to use hierarchical irqdomain interfaces")
>
> For almost 6 years nobody complained about this wreckage, which might
> indicate that we could lift this requirement, but OTOH for any system
> which actually has a PIC IRQ2 is unusable by design.
>
> And my history based paranoia vs. these kind of changes just makes me
> tend to the safe side, so restoring the original behaviour makes more
> sense than just papering over it.
>
> Just to be clear: The reservation of PIC pin2 (aka. IRQ2) as a permanent
> system vector cannot go away for any system which has a PIC. If that
> ever fires then the PIC is hosed and you really want the warning about
> the unhandled vector which is easy to identify because it is fixed.
>
> You might think that's not relevant for modern CPUs where the PIC is
> emulated, but I've dealt with reports where that emulation goes south so
> I rather keep those sanity checks around as long as that legacy mess is
> relevant. No idea why it still exists, but that's a different story.
>
> Thanks,
>
>         tglx
> ---
> Subject: x86/ioapic: Ignore IRQ2 again
> From: Thomas Gleixner <tglx@...utronix.de>
> Date: Wed, 17 Mar 2021 23:10:02 +0100
>
> Why did I have to do the archaeology?
>
> < INSERT PROPER CHANGELOG >
>
> Fixes: d32932d02e18 ("x86/irq: Convert IOAPIC to use hierarchical irqdomain interfaces")
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
>  arch/x86/kernel/apic/io_apic.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -1032,6 +1032,16 @@ static int mp_map_pin_to_irq(u32 gsi, in
>  	if (idx >= 0 && test_bit(mp_irqs[idx].srcbus, mp_bus_not_pci)) {
>  		irq = mp_irqs[idx].srcbusirq;
>  		legacy = mp_is_legacy_irq(irq);
> +		/*
> +		 * IRQ2 is unusable for historical reasons on systems which
> +		 * have a legacy PIC. See the comment vs. IRQ2 further down.
> +		 *
> +		 * If this gets removed at some point then the related code
> +		 * in lapic_assign_system_vectors() needs to be adjusted as
> +		 * well.
> +		 */
> +		if (legacy && irq == PIC_CASCADE_IR)
> +			return -EINVAL;
>  	}
>  
>  	mutex_lock(&ioapic_mutex);
>

This seems to work well for me, thanks! Please feel free to add

Tested-by: Vitaly Kuznetsov <vkuznets@...hat.com>

Alternatively, just let me know and I'll pick this up, write a changelog and
send out with your Suggested-by:.

-- 
Vitaly