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

[Julien Grall <julien@xxxxxxx>]

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?

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

--
Julien Grall

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