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

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

On Fri, Apr 08, 2022 at 02:04:56PM +0200, Jan Beulich wrote:
> On 08.04.2022 13:10, Roger Pau Monné wrote:
> > 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...

Er, sorry, s/space/newline/.

> Which extra space are you referring to? The only candidate I can spot
> are the two adjacent spaces in the comment, between the two sentences.
> But that's several lines up. And I think we have examples of both
> single and double spaces in the code base for such cases. I know I'm
> not even consistent myself in this regard - the longer a comment gets,
> the more likely I am to use two spaces between sentences.
> 
> > 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?
> 
> This would be nice, but when I looked at this long ago it didn't seem
> straightforward to achieve.

Right, I don't doubt it.

Thanks, Roger.