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

[<vrd@xxxxxxxxxx>]

Hey Julien,

On 12/18/19 2:57 PM, Julien Grall wrote:
> Hi Varad,
>
> Please send new version of a patch in a new thread rather than 
> in-reply to the first version.
> On 18/12/2019 10: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.
> >
> > Make __pirq_guest_unbind safe to be called multiple times by letting xen
> > continue if a shared pirq has already been unbound from this guest. The
> > PIRQ will be cleaned up from the domain's pirq_tree during the 
> > destruction
> > in complete_domain_destroy anyways.
> >
> > Signed-off-by: Varad Gautam <vrd@xxxxxxxxx>
> > CC: Jan Beulich <jbeulich@xxxxxxxx>
> > CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >
> > v2: Split the check on action->nr_guests > 0 and make it an ASSERT, 
> > reword.
> > ---
> >   xen/arch/x86/irq.c | 11 ++++++++++-
> >   1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> > index 5d0d94c..3eb7b22 100644
> > --- 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 ) {
> The { should be a new line.
>
> > +        ASSERT(action->nr_guests > 0) ;
> The space before ; is not necessary.
>
> > +        /* 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. */
> The coding style for comment is:
>
> /*
>  * Foo
>  * Bar
>  */
>
> > +        if ( action->shareable )
> > +            return NULL;
> > +        BUG();
> Given that the previous BUG_ON() was hit, would it make sense to try 
> to avoid a new BUG().
> So why not just returning NULL as you do for action->shareable?
Thanks, I've done the style fixups in v3.

I'd argue that is indeed a BUG, if the pirq was _not_ shareable and the 
loop above couldn't find a matching domain for it - that implies the 
pirq shouldn't have existed in the first place.

> > +    }
> > +
> >       memmove(&action->guest[i], &action->guest[i+1],
> >               (action->nr_guests-i-1) * sizeof(action->guest[0]));
> >       action->nr_guests--;
> Cheers,



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