linux-kernel - Re: kvm-unit-test fail for split irqchip

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

Message-ID: <CANRm+CzN18fE3O+gSHCrtyFBNirBxj0vRBEQ-iREipxDO+8yyQ@mail.gmail.com>
Date:   Thu, 15 Sep 2016 08:58:55 +0800
From:   Wanpeng Li <kernellwp@...il.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Radim Krcmar <rkrcmar@...hat.com>, kvm <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: kvm-unit-test fail for split irqchip

2016-09-14 17:42 GMT+08:00 Paolo Bonzini <pbonzini@...hat.com>:
>
>
> On 14/09/2016 05:57, Wanpeng Li wrote:
>> 2016-09-14 4:43 GMT+08:00 Paolo Bonzini <pbonzini@...hat.com>:
>>>
>>>
>>> On 13/09/2016 21:01, Radim Krcmar wrote:
>>>> kvm_handle_interrupt() does
>>>>
>>>>   interrupt_request |= CPU_INTERRUPT_HARD
>>>>
>>>> which later calls cpu_get_pic_interrupt() in kvm_arch_pre_run(), but
>>>> that function uses stale information from APIC and injects 62 again.
>>>> If we synchronized the APIC, then the test would #GP, because there
>>>> would be no injectable interrupt in LAPIC or PIC, so pic_read_irq()
>>>> would return 15, thinking it was spurious.
>>>>
>>>> I think the bug starts in pic_irq_request(), which should not touch
>>>> LAPIC.  The patch below makes it work (just the second hunk is
>>>> sufficient), but it's still far from sane code ...
>>>
>>> This makes sense.  Most of the functions exported by hw/intc/apic.c
>>> should only be used with a userspace APIC:
>>>
>>> 0000000000000b70 T apic_accept_pic_intr
>>> 00000000000010f0 T apic_deliver_irq
>>> 00000000000011e0 T apic_deliver_pic_intr
>>> 0000000000001310 T apic_get_interrupt
>>> 0000000000001270 T apic_poll_irq
>>> 0000000000000a40 T apic_sipi
>>>
>>> The patch is okay, though for consistency with other code I'd use
>>> !kvm_irqchip_in_kernel() rather than !kvm_irqchip_is_split().
>>>
>>> Wanpeng, can you do that,
>>
>> Yeah, I just sent out a patch to fix the bug. Thanks for the long
>> discussion with me and thanks Radim's proposal. :)
>>
>>> and change hw/intc/apic.c to use a new casting
>>> macro
>>>
>>>     APICCommonState *s = APIC(dev);
>>>
>>> instead of APIC_COMMON?
>>>
>>
>> I'm not familiar with QOM too much, what APIC macro do you like?
>
> The macro would be defined like
>
> #define TYPE_APIC "apic"
> #define APIC(obj) \
>      OBJECT_CHECK(APICCommonState, (obj), TYPE_APIC)
>
> With this change and without your other patch, you should get an
> assertion failure in eventinj.flat (which would have pointed immediately
> at the bug!).

Good point, I just sent out the patch, it catches the bug as you expected. :)

Regards,
Wanpeng Li