Re: [Xen-devel] [PATCH 2/4] x86/IRQ: bail early from irq_guest_eoi_timer_fn() when nothing is in flight

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/06/2019 12:43, Jan Beulich wrote:
>>>> On 06.06.19 at 13:34, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 06/06/2019 09:17, Jan Beulich wrote:
>>>>>> On 05.06.19 at 19:15, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 08/05/2019 13:46, Jan Beulich wrote:
>>>>> @@ -1130,8 +1130,10 @@ static void irq_guest_eoi_timer_fn(void
>>>>>          }
>>>>>      }
>>>>>  
>>>>> -    if ( action->in_flight != 0 )
>>>>> -        goto out;
>>>>> +    if ( action->in_flight )
>>>>> +        printk(XENLOG_G_WARNING
>>>>> +               "IRQ%d: %d handlers still in flight at forced EOI\n",
>>>>> +               desc->irq, action->in_flight);
>>>> AFACIT, this condition can be triggered by a buggy/malicious guest, by
>>>> it simply ignoring or masking the line interrupt at the vIO-APIC.
>>> I don't think it can, no. Or else the ASSERT_UNREACHABLE() below
>>> here would be invalid to add.
>> Which ASSERT_UNREACHABLE() ?  I know Roger asked for one, but I don't
>> see it anywhere in the code.
> Because so far there was no real reason to re-post. It's right here,
> as Roger did ask for, and as I did (hesitantly) agree:
>
>     if ( action->in_flight )
>     {
>         printk(XENLOG_G_WARNING
>                "IRQ%u: %d/%d handler(s) still in flight at forced EOI\n",
>                irq, action->in_flight, action->nr_guests);
>         ASSERT_UNREACHABLE();
>     }
>
>>>> The message would be far more useful if it identified the domain in
>>>> question, which looks like it can be obtained from the middle of the loop.
>>> That very loop has just taken care of decrementing ->in_flight for
>>> all such guests.
>>>
>>> Also note that there could be more than one offending domain, for
>>> shared IRQs. Plus the loop you're referring to can specifically _not_
>>> be used for identifying the domain(s), because for the ones
>>> processed there we _did_ decrement ->in_flight. If this message
>>> gets logged, we simply have no idea why ->in_flight is _still_ non-
>>> zero. This could be a BUG_ON(), but it seems more in line with our
>>> general idea of how we would like to deal with such cases to try
>>> and keep the system running here in release builds.
>> Ok - lets go with this for now.  It is a net improvement, and we can
>> evaluate the guest-triggerability at a later point.
>>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Thanks much. I'll assume this holds also for the adjustments
> requested by Roger.

Fine.  At least that should make things obvious in a debug build.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel