Re: [PATCH] x86/io-apic: prevent early exit from i8259 loop detection

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.12.2024 10:00, Roger Pau Monne wrote:
> Avoid exiting early from the loop when a pin that could be connected to the
> i8259 is found, as such early exit would leave the EOI handler translation
> array only partially allocated and/or initialized.
> 
> Otherwise on systems with multiple IO-APICs and an unmasked ExtINT pin on
> any IO-APIC that's no the last one the following NULL pointer dereference
> triggers:
> 
> (XEN) Enabling APIC mode.  Using 2 I/O APICs
> (XEN) ----[ Xen-4.20-unstable  x86_64  debug=y  Not tainted ]----
> (XEN) CPU:    0
> (XEN) RIP:    e008:[<ffff82d040328046>] __ioapic_write_entry+0x83/0x95
> [...]
> (XEN) Xen call trace:
> (XEN)    [<ffff82d040328046>] R __ioapic_write_entry+0x83/0x95
> (XEN)    [<ffff82d04027464b>] F amd_iommu_ioapic_update_ire+0x1ea/0x273
> (XEN)    [<ffff82d0402755a1>] F iommu_update_ire_from_apic+0xa/0xc
> (XEN)    [<ffff82d040328056>] F __ioapic_write_entry+0x93/0x95
> (XEN)    [<ffff82d0403283c1>] F 
> arch/x86/io_apic.c#clear_IO_APIC_pin+0x7c/0x10e
> (XEN)    [<ffff82d040328480>] F arch/x86/io_apic.c#clear_IO_APIC+0x2d/0x61
> (XEN)    [<ffff82d0404448b7>] F enable_IO_APIC+0x2e3/0x34f
> (XEN)    [<ffff82d04044c9b0>] F smp_prepare_cpus+0x254/0x27a
> (XEN)    [<ffff82d04044bec2>] F __start_xen+0x1ce1/0x23ae
> (XEN)    [<ffff82d0402033ae>] F __high_start+0x8e/0x90
> (XEN)
> (XEN) Pagetable walk from 0000000000000000:
> (XEN)  L4[0x000] = 000000007dbfd063 ffffffffffffffff
> (XEN)  L3[0x000] = 000000007dbfa063 ffffffffffffffff
> (XEN)  L2[0x000] = 000000007dbcc063 ffffffffffffffff
> (XEN)  L1[0x000] = 0000000000000000 ffffffffffffffff
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) FATAL PAGE FAULT
> (XEN) [error_code=0002]
> (XEN) Faulting linear address: 0000000000000000
> (XEN) ****************************************
> (XEN)
> (XEN) Reboot in five seconds...
> 
> Reported-by: Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>
> Fixes: 86001b3970fe ('x86/io-apic: fix directed EOI when using AMD-Vi 
> interrupt remapping')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Hmm, considering the earlier change was backported, I'm even inclined to
delay 4.18.4 a little, for taking this one there as well.

> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1389,14 +1389,15 @@ void __init enable_IO_APIC(void)
>              /* If the interrupt line is enabled and in ExtInt mode
>               * I have found the pin where the i8259 is connected.
>               */
> -            if ((entry.mask == 0) && (entry.delivery_mode == dest_ExtINT)) {
> +            if ( ioapic_i8259.apic == -1 && entry.mask == 0 &&
> +                 entry.delivery_mode == dest_ExtINT )
> +            {
> +                ASSERT(ioapic_i8259.pin == -1);

I'm not sure of the value of this assertion. It is provable that ...

>                  ioapic_i8259.apic = apic;
>                  ioapic_i8259.pin  = pin;

... both fields are updated together (and not earlier on), and hence once
we've been here neither field will still be -1.

Looking around I further notice that it's generally ioapic_i8259.pin that
we check against -1, so I wonder whether - just for consistency - the if()
condition wouldn't better too use that one.

Preferably with these adjustments:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan