Re: [PATCH v6 1/7] xen/events: fix race with set_global_virq_handler()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2025 16:37, Juergen Gross wrote:
> On 07.01.25 16:23, Jan Beulich wrote:
>> On 07.01.2025 11:17, Juergen Gross wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -979,6 +979,7 @@ void send_global_virq(uint32_t virq)
>>>   int set_global_virq_handler(struct domain *d, uint32_t virq)
>>>   {
>>>       struct domain *old;
>>> +    int rc = 0;
>>>   
>>>       if (virq >= NR_VIRQS)
>>>           return -EINVAL;
>>> @@ -992,14 +993,23 @@ int set_global_virq_handler(struct domain *d, 
>>> uint32_t virq)
>>>           return -EINVAL;
>>>   
>>>       spin_lock(&global_virq_handlers_lock);
>>> -    old = global_virq_handlers[virq];
>>> -    global_virq_handlers[virq] = d;
>>> +
>>> +    if ( d->is_dying != DOMDYING_alive )
>>> +    {
>>> +        old = d;
>>> +        rc = -EINVAL;
>>> +    }
>>
>> While I can see how this eliminates the zombie domain aspect, this doesn't
>> fully eliminate the race. Doing so would require (also) using the domain's
>> event lock. Assuming we're okay with the remaining race, imo a code comment
>> would be needed to state this (including the fact that it's then
>> unpredictable whether this operation might still succeed for a domain
>> already having d->is_dying != DOMDYING_alive).
> 
> AFAIU you mean that it is still possible to set a domain to handle a virq
> when it is in the process of going down, especially if is_dying is set just
> after it has been tested to be DOMDYING_alive?
> 
> I don't see this being a problem, as the same would happen if the domain
> would go down just a millisecond later. This is something we will never be
> able to handle.

Right, but the sequence of events in the case you mention is different: The
insertion into the array would still happen while the domain isn't marked
dying.

> And after all the call of clear_global_virq_handlers() will now reset the
> handling domain to the hardware domain in all cases.

Of course, but in the meantime an event may be sent to such a domain already
marked dying. That likely isn't going to cause problems, but is unexpected
with what description here says is being addressed.

>> Plus the way you do it the early success path remains; ideally that case
>> would also fail for an already dying domain.
> 
> Same again: clear_global_virq_handlers() will reset the handling domain.

Right.

In summary: As indicated, we may be okay with the remaining race, but then
we also should be making clear that we've decided to leave it at that.
Hence my earlier request: If we accept this, say (and briefly justify) this
in a code comment.

Jan