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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Tue, Apr 5, 2022 at 4:18 AM Jan Beulich <jbeulich@xxxxxxxx> 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