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
