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

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

[Jason Andryuk]

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,
we don't want the XSM hook denying the operation.

Check d->is_dying and skip the check when the domain is dead.  The RCU
callback runs when a domain is in that state.

Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
Dan wants to change current to point at DOMID_IDLE when the RCU callback
runs.  I think Juergen's commit 53594c7bd197 "rcu: don't use
stop_machine_run() for rcu_barrier()" may have changed this since it
mentions stop_machine_run scheduled the idle vcpus to run the callbacks
for the old code.

Would that be as easy as changing rcu_do_batch() to do:

+        /* Run as "Xen" not a random domain's vcpu. */
+        vcpu = get_current();
+        set_current(idle_vcpu[smp_processor_id()]);
         list->func(list);
+        set_current(vcpu);

or is using set_current() only acceptable as part of context_switch?

 xen/arch/x86/irq.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 285ac399fb..16488d287c 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2340,10 +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);
-    if ( ret )
-        goto done;
+    /* 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 != DOMDYING_dead ) {
+        ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
+                                   msi_desc ? msi_desc->dev : NULL);
+        if ( ret )
+            goto done;
+    }
 
     forced_unbind = pirq_guest_force_unbind(d, info);
     if ( forced_unbind )
-- 
2.35.1

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

[Daniel P. Smith]

On 3/30/22 14:17, 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,
> we don't want the XSM hook denying the operation.
> 
> Check d->is_dying and skip the check when the domain is dead.  The RCU
> callback runs when a domain is in that state.
> 
> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
> Dan wants to change current to point at DOMID_IDLE when the RCU callback
> runs.  I think Juergen's commit 53594c7bd197 "rcu: don't use
> stop_machine_run() for rcu_barrier()" may have changed this since it
> mentions stop_machine_run scheduled the idle vcpus to run the callbacks
> for the old code.
> 
> Would that be as easy as changing rcu_do_batch() to do:
> 
> +        /* Run as "Xen" not a random domain's vcpu. */
> +        vcpu = get_current();
> +        set_current(idle_vcpu[smp_processor_id()]);
>          list->func(list);
> +        set_current(vcpu);
> 
> or is using set_current() only acceptable as part of context_switch?
> 
>  xen/arch/x86/irq.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 285ac399fb..16488d287c 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2340,10 +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);
> -    if ( ret )
> -        goto done;
> +    /* 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 != DOMDYING_dead ) {
> +        ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
> +                                   msi_desc ? msi_desc->dev : NULL);
> +        if ( ret )
> +            goto done;
> +    }
>  
>      forced_unbind = pirq_guest_force_unbind(d, info);
>      if ( forced_unbind )

I think this is sufficient though IMHO it would make it stronger if
current accurately reflected the idle domain and the condition was added
to the if statement check this fact.

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

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

[Jan Beulich]

On 30.03.2022 20:17, 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,
> we don't want the XSM hook denying the operation.
> 
> Check d->is_dying and skip the check when the domain is dead.  The RCU
> callback runs when a domain is in that state.

One question which has always been puzzling me (perhaps to Daniel): While
I can see why mapping of an IRQ needs to be subject to an XSM check, it's
not really clear to me why unmapping would need to be, at least as long
as it's the domain itself which requests the unmap (and which I would
view to extend to the domain being cleaned up). But maybe that's why it's
XSM_HOOK ...

> ---
> Dan wants to change current to point at DOMID_IDLE when the RCU callback
> runs.  I think Juergen's commit 53594c7bd197 "rcu: don't use
> stop_machine_run() for rcu_barrier()" may have changed this since it
> mentions stop_machine_run scheduled the idle vcpus to run the callbacks
> for the old code.
> 
> Would that be as easy as changing rcu_do_batch() to do:
> 
> +        /* Run as "Xen" not a random domain's vcpu. */
> +        vcpu = get_current();
> +        set_current(idle_vcpu[smp_processor_id()]);
>          list->func(list);
> +        set_current(vcpu);
> 
> or is using set_current() only acceptable as part of context_switch?

Indeed I would question any uses outside of context_switch() (and
system bringup).

> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2340,10 +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);
> -    if ( ret )
> -        goto done;
> +    /* When called by complete_domain_destroy via RCU, current is a random
> +     * domain.  Skip the XSM check since this is a Xen-initiated action. */

Comment style.

> +    if ( d->is_dying != DOMDYING_dead ) {

Please use !d->is_dying. Also please correct the placement of the brace.
Or you could avoid the need for a brace by leveraging that ret is zero
ahead of this if(), i.e. ...

> +        ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
> +                                   msi_desc ? msi_desc->dev : NULL);
> +        if ( ret )
> +            goto done;
> +    }


    if ( !d->is_dying )
        ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
                                   msi_desc ? msi_desc->dev : NULL);
    if ( ret )
        goto done;

Jan

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

[Daniel P. Smith]

On 4/5/22 04:18, Jan Beulich wrote:
> On 30.03.2022 20:17, 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,
>> we don't want the XSM hook denying the operation.
>>
>> Check d->is_dying and skip the check when the domain is dead.  The RCU
>> callback runs when a domain is in that state.
> 
> One question which has always been puzzling me (perhaps to Daniel): While
> I can see why mapping of an IRQ needs to be subject to an XSM check, it's
> not really clear to me why unmapping would need to be, at least as long
> as it's the domain itself which requests the unmap (and which I would
> view to extend to the domain being cleaned up). But maybe that's why it's
> XSM_HOOK ...

There are situations for instance where there is a flask-based system
with one or more domains (v-platform-mgr) that are each responsible for
the management of a subset of domains and are responsible for
hotplugging in and out a device, i.e. granting the privilege to a
v-platform-mgr to call PHYSDEVOP_map_pirq/PHYSDEVOP_unmap_pirq, for the
domains each one is managing.

>> ---
>> Dan wants to change current to point at DOMID_IDLE when the RCU callback
>> runs.  I think Juergen's commit 53594c7bd197 "rcu: don't use
>> stop_machine_run() for rcu_barrier()" may have changed this since it
>> mentions stop_machine_run scheduled the idle vcpus to run the callbacks
>> for the old code.
>>
>> Would that be as easy as changing rcu_do_batch() to do:
>>
>> +        /* Run as "Xen" not a random domain's vcpu. */
>> +        vcpu = get_current();
>> +        set_current(idle_vcpu[smp_processor_id()]);
>>          list->func(list);
>> +        set_current(vcpu);
>>
>> or is using set_current() only acceptable as part of context_switch?
> 
> Indeed I would question any uses outside of context_switch() (and
> system bringup).

I am not familiar with the details of the scheduler, but from a higher
level, conceptual perspective, I do not understand why an idle domain
task is being executed without an explicit context switch to the idle
domain to ensure the current world view is consistent with the task
execution scope. Just seems to me like this is creating a situation
where things have the potential to go sideways/wrong.

v/r,
dps

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

[Jason Andryuk]

On Tue, Apr 5, 2022 at 4:18 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 30.03.2022 20:17, 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,
> > we don't want the XSM hook denying the operation.
> >
> > Check d->is_dying and skip the check when the domain is dead.  The RCU
> > callback runs when a domain is in that state.
>
> One question which has always been puzzling me (perhaps to Daniel): While
> I can see why mapping of an IRQ needs to be subject to an XSM check, it's
> not really clear to me why unmapping would need to be, at least as long
> as it's the domain itself which requests the unmap (and which I would
> view to extend to the domain being cleaned up). But maybe that's why it's
> XSM_HOOK ...
>
> > ---
> > Dan wants to change current to point at DOMID_IDLE when the RCU callback
> > runs.  I think Juergen's commit 53594c7bd197 "rcu: don't use
> > stop_machine_run() for rcu_barrier()" may have changed this since it
> > mentions stop_machine_run scheduled the idle vcpus to run the callbacks
> > for the old code.
> >
> > Would that be as easy as changing rcu_do_batch() to do:
> >
> > +        /* Run as "Xen" not a random domain's vcpu. */
> > +        vcpu = get_current();
> > +        set_current(idle_vcpu[smp_processor_id()]);
> >          list->func(list);
> > +        set_current(vcpu);
> >
> > or is using set_current() only acceptable as part of context_switch?
>
> Indeed I would question any uses outside of context_switch() (and
> system bringup).
>
> > --- a/xen/arch/x86/irq.c
> > +++ b/xen/arch/x86/irq.c
> > @@ -2340,10 +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);
> > -    if ( ret )
> > -        goto done;
> > +    /* When called by complete_domain_destroy via RCU, current is a random
> > +     * domain.  Skip the XSM check since this is a Xen-initiated action. */
>
> Comment style.

Yes.  Sorry about that.

> > +    if ( d->is_dying != DOMDYING_dead ) {
>
> Please use !d->is_dying. Also please correct the placement of the brace.
> Or you could avoid the need for a brace by leveraging that ret is zero
> ahead of this if(), i.e. ...

Here I was patting myself on the back for remembering the spaces
inside the parens, and I screwed up the brace...  Sorry.

I intentionally chose DOMDYING_dead because, from my reading of the
code, complete_domain_destroy should only reach here when dead (and
not dying).  If this function is reached when DOMDYING_dying, then
that is unexpected.  That would be a guest-initiated action and
therefore the XSM check should apply.

Just checking is_dying is fine, but I want to explain and highlight this aspect.

> > +        ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
> > +                                   msi_desc ? msi_desc->dev : NULL);
> > +        if ( ret )
> > +            goto done;
> > +    }
>
>
>     if ( !d->is_dying )
>         ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
>                                    msi_desc ? msi_desc->dev : NULL);
>     if ( ret )
>         goto done;

I'm planning to just do it this way.

Thank you for reviewing.

-Jason