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@citrix.com>
Signed-off-by: Jason Andryuk <jandryuk@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);
+
if ( ret )
goto done;
--
2.35.1
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@citrix.com> > Signed-off-by: Jason Andryuk <jandryuk@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... Reviewed-by: Roger Pau Monné <roger.pau@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? Thanks, Roger.
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@citrix.com> >> Signed-off-by: Jason Andryuk <jandryuk@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.pau@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
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@citrix.com> > >> Signed-off-by: Jason Andryuk <jandryuk@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.pau@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.
On 07.04.2022 16:51, 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@citrix.com> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
© 2016 - 2024 Red Hat, Inc.