Re: [Xen-devel] [PATCH v2] x86: irq: Do not BUG_ON multiple unbind calls for shared pirqs

[<vrd@xxxxxxxxxx>]

Hey Jan,

On 12/18/19 2:42 PM, Jan Beulich wrote:
> On 18.12.2019 11:53, Varad Gautam wrote:
> > XEN_DOMCTL_destroydomain creates a continuation if domain_kill -ERESTARTS.
> > In that scenario, it is possible to receive multiple _pirq_guest_unbind
> > calls for the same pirq from domain_kill, if the pirq has not yet been
> > removed from the domain's pirq_tree, as:
> >    domain_kill()
> >      -> domain_relinquish_resources()
> >        -> pci_release_devices()
> >          -> pci_clean_dpci_irq()
> >            -> pirq_guest_unbind()
> >              -> __pirq_guest_unbind()
> >
> > For a shared pirq (nr_guests > 1), the first call would zap the current
> > domain from the pirq's guests[] list, but the action handler is never freed
> > as there are other guests using this pirq. As a result, on the second call,
> > __pirq_guest_unbind searches for the current domain which has been removed
> > from the guests[] list, and hits a BUG_ON.
> There must be more to this, seeing the cleanup_domain_irq_pirq()
> invocation at the end of pirq_guest_unbind(), which ought to be
> reached in the case you describe.

The calls to __pirq_guest_unbind and cleanup_domain_irq_pirq from 
pirq_guest_unbind are going to be mutually exclusive, since irq defaults 
to 0.

> > --- a/xen/arch/x86/irq.c
> > +++ b/xen/arch/x86/irq.c
> > @@ -1863,7 +1863,16 @@ static irq_guest_action_t *__pirq_guest_unbind(
> >   
> >       for ( i = 0; (i < action->nr_guests) && (action->guest[i] != d); i++ )
> >           continue;
> > -    BUG_ON(i == action->nr_guests);
> > +    if ( i == action->nr_guests ) {
> Brace on its own line please.
>
> > +        ASSERT(action->nr_guests > 0) ;
> Stray blank.
>
> > +        /* In case the pirq was shared, unbound for this domain in an earlier 
> > call, but still
> > +         * existed on the domain's pirq_tree, we still reach here if there are 
> > any later
> > +         * unbind calls on the same pirq. Return if such an unbind happens. */
> > +        if ( action->shareable )
> Long lines and malformed comment.

Handled all style fixups in v3.


> Do you perhaps also want to check that you take this path only
> for dying guests?

The patch is about making pirq_guest_unbind() resilient/safe to multiple 
calls. At present, this happens when domain_kill -ERESTARTs for dying 
guests. It might also happen in other cases - and shouldn't cause xen to 
crash.

> Jan
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel