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....@citrix.com> >> Signed-off-by: Jason Andryuk <jandr...@gmail.com> >> --- >> 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...
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....@citrix.com> > > 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. Jan
