Re: [PATCH] x86/physdev: Call xsm_unmap_domain_irq earlier

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Mon, Mar 28, 2022 at 8:44 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 28.03.2022 13:40, Roger Pau Monné wrote:
> > On Fri, Mar 25, 2022 at 10:18:26AM -0400, Jason Andryuk wrote:
> >> Pull the XSM check up out of unmap_domain_pirq into physdev_map_pirq.
> >>
> >> 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.
> >>
> >> Move the XSM hook up into physdev_unmap_pirq, which is the
> >> guest-accessible path.  This requires moving some of the sanity check
> >> upwards as well since the hook needs the additional data to make its
> >> decision.  Since complete_domain_destroy still calls unmap_domain_pirq,
> >> replace the moved runtime checking with assert.  Only valid pirqs should
> >> make their way into unmap_domain_pirq from complete_domain_destroy.
> >>
> >> This is mostly code movement, but one style change is to pull `irq =
> >> info->arch.irq` out of the if condition.
>
> And why is this better? You now have an extra conditional expression
> there.

It's better because it is clearer.  The location is more readily
visible when scanning the code.  It fits the pattern of `variable =
something`.  Assigning inside the if condition makes it harder to see
where a variable is assigned, which is why I made the change.

This is the non-movement diff:

     info = pirq_info(d, pirq);
-    if ( !info || (irq = info->arch.irq) <= 0 )
+    irq = info ? info->arch.irq : 0;
+    if ( !info || irq <= 0 )
     {

But given comments below, it seems this movement is not going to
happen, so this change will be dropped.

> >> Label done is now unused and removed.
> >>
> >> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> >> ---
> >> unmap_domain_pirq is also called in vioapic_hwdom_map_gsi and
> >> vpci_msi_disable.  vioapic_hwdom_map_gsi is a cleanup path after going
> >> through map_domain_pirq, and I don't think the vpci code is directly
> >> guest-accessible.  So I think those are okay, but I not familiar with
> >> that code.  Hence, I am highlighting it.
> >
> > Hm, for vpci_msi_disable it's not technically correct to drop the XSM
> > check. This is a guests accessible path, however the value of PIRQ is
> > not settable by the guest, so it's kind of OK just for that reason.

Right, that's why I was figuring it was okay.  If Xen is handling it
internally, it doesn't have to worry about untrusted data.

> Kind of - perhaps. But better to avoid if possible.

But I can also see how it is safer to leave the check in place.  It's
better to go through an unnecessary XSM check than to not have a check
at all.

> Hence I would prefer
> the ->is_dying alternative you suggest at the end.

I had not considered the ->is_dying option.  At first glance, it seems
like it would work.

I was hoping that we could determine that only sanitized data would
make it into unmap_domain_pirq.  Then we can restructure the code
instead of adding a condition to skip the XSM hook.  But if this
function is guest accessible via vpci, then the XSM check should
remain.

Regards,
Jason