On Mon, Mar 28, 2022 at 8:44 AM Jan Beulich <jbeul...@suse.com> 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 <jandr...@gmail.com>
> >> ---
> >> 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
