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

[Juergen Gross <jgross@xxxxxxxx>]

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.

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

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

> > +    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.
Will add that if a respin is needed.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature