Re: [PATCH v2] x86/irq: Skip unmap_domain_pirq XSM during destruction

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Apr 07, 2022 at 10:51:50AM -0400, Jason Andryuk wrote:
> xsm_unmap_domain_irq was seen denying unmap_domain_pirq when called from
> complete_domain_destroy as an RCU callback.  The source context was an
> unexpected, random domain.  Since this is a xen-internal operation,
> going through the XSM hook is inapproriate.
> 
> Check d->is_dying and skip the XSM hook when set since this is a cleanup
> operation for a domain being destroyed.
> 
> Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> ---
> v2:
> Style fixes
> Rely on ret=0 initialization
> 
> ---
>  xen/arch/x86/irq.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 285ac399fb..de30ee7779 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2340,8 +2340,14 @@ int unmap_domain_pirq(struct domain *d, int pirq)
>          nr = msi_desc->msi.nvec;
>      }
>  
> -    ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
> -                               msi_desc ? msi_desc->dev : NULL);
> +    /*
> +     * When called by complete_domain_destroy via RCU, current is a random
> +     * domain.  Skip the XSM check since this is a Xen-initiated action.
> +     */
> +    if ( !d->is_dying )
> +        ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
> +                                   msi_desc ? msi_desc->dev : NULL);
> +

Nit: I would remove the extra space here, but that's a question of
taste...

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I wonder if long term we could make this cleaner, maybe by moving the
unbind so it always happen in the context of the caller of the destroy
hypercall instead of in the RCU context?

Thanks, Roger.