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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

Plus the way you do it the early success path remains; ideally that case
would also fail for an already dying domain.

> +    else
> +    {
> +        old = global_virq_handlers[virq];
> +        global_virq_handlers[virq] = d;
> +    }
>      spin_unlock(&global_virq_handlers_lock);

Nit: Much like you do at the top of your addition, a new blank line at the
bottom would be nice.

Jan