Re: [PATCH v7 3/3] xen/events: rework fifo queue locking

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.11.2020 15:49, Jürgen Groß wrote:
> On 24.11.20 15:02, Jan Beulich wrote:
>> On 24.11.2020 08:01, Juergen Gross wrote:
>>> Two cpus entering evtchn_fifo_set_pending() for the same event channel
>>> can race in case the first one gets interrupted after setting
>>> EVTCHN_FIFO_PENDING and when the other one manages to set
>>> EVTCHN_FIFO_LINKED before the first one is testing that bit. This can
>>> lead to evtchn_check_pollers() being called before the event is put
>>> properly into the queue, resulting eventually in the guest not seeing
>>> the event pending and thus blocking forever afterwards.
>>>
>>> Note that commit 5f2df45ead7c1195 ("xen/evtchn: rework per event channel
>>> lock") made the race just more obvious, while the fifo event channel
>>> implementation had this race from the beginning when an unmask operation
>>> was running in parallel with an event channel send operation.
>>
>> Ah yes, but then also only for inter-domain channels, as it was
>> only in that case that the "wrong" domain's event lock was held.
>> IOW there was a much earlier change already where this issue
>> got widened (when the per-channel locking got introduced). This
>> then got reduced to the original scope by XSA-343's adding of
>> locking to evtchn_unmask(). (Not sure how much of this history
>> wants actually adding here. I'm writing it down not the least to
>> make sure I have a complete enough picture.)
> 
> I think we both agree that this race was possible for quite some time.
> And I even think one customer bug I've been looking into recently
> might be exactly this problem (a dom0 was occasionally hanging in
> cross-cpu function calls, but switching to 2-level events made the
> problem disappear).

IPIs weren't affected earlier on (i.e. in any released version),
if my analysis above is correct.

>>> Additionally when an
>>> event channel needs to change queues both queues need to be locked
>>> initially.
>>
>> Since this was (afaict) intentionally not the case before, I
>> think I would want to see a word spent on the "why", perhaps
>> better in a code comment than here. Even more so that you
>> delete a respective comment justifying the possible race as
>> permissible. And I have to admit right now I'm still uncertain
>> both ways, i.e. I neither have a clear understanding of why it
>> would have been considered fine the other way around before,
>> nor why the double locking is strictly needed.
> 
> I need the double locking to avoid someone entering the locked region
> when dropping the lock for the old queue and taking the one for the
> new queue, as this would open the same race window again.

Well, that's what have already said. Thing is that the code
prior to your change gives the impression as if this race was
benign.

>>> +        lastq.raw = read_atomic(&evtchn->fifo_lastq);
>>> +        old_v = d->vcpu[lastq.last_vcpu_id];
>>> +        if ( q == &v->evtchn_fifo->queue[evtchn->priority] &&
>>> +             old_q == &old_v->evtchn_fifo->queue[lastq.last_priority] )
>>> +            break;
>>> +
>>> +        if ( q != old_q )
>>> +            spin_unlock(&old_q->lock);
>>> +        spin_unlock_irqrestore(&q->lock, flags);
>>> +
>>> +        if ( try == 3 )
>>> +        {
>>> +            gprintk(XENLOG_WARNING,
>>> +                    "dom%d port %d lost event (too many queue changes)\n",
>>> +                    d->domain_id, evtchn->port);
>>> +            return;
>>
>> Originally evtchn_check_pollers() would still have been called
>> in this case. Wouldn't you better retain this, or else justify
>> the possibly observable change in behavior?
> 
> I could retain it, but without having set the event to be pending
> I don't see the value in doing so.

But that's part of my concern - you now don't set PENDING when
bailing here.

Jan