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....@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...
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....@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. Right, I don't doubt it. Thanks, Roger.
