Re: [for-4.15][PATCH v2 3/5] xen/iommu: iommu_map: Don't crash the domain if it is dying

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 10/02/2021 14:14, Jan Beulich wrote:
> On 09.02.2021 22:14, Julien Grall wrote:
> > On Tue, 9 Feb 2021 at 20:28, Paul Durrant <xadimgnik@xxxxxxxxx> wrote:
> > > > From: Julien Grall <julien@xxxxxxx>
> > > > Sent: 09 February 2021 15:28
> > > >
> > > > It is a bit pointless to crash a domain that is already dying. This will
> > > > become more an annoyance with a follow-up change where page-table
> > > > allocation will be forbidden when the domain is dying.
> > > >
> > > > Security wise, there is no change as the devices would still have access
> > > > to the IOMMU page-tables even if the domain has crashed until Xen
> > > > start to relinquish the resources.
> > > >
> > > > For x86, we rely on dom_iommu(d)->arch.mapping.lock to ensure
> > > > d->is_dying is correctly observed (a follow-up patch will held it in the
> > > > relinquish path).
> Am I to understand this to mean that at this point of the series
> things aren't really correct yet in this regard? If so, wouldn't
> it be better to re-order?
You asked this specific order... So are you saying you want me to use 
the original ordering?
> > > > For Arm, there is still a small race possible. But there is so far no
> > > > failure specific to a domain dying.
> > > >
> > > > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> > > >
> > > > ---
> > > >
> > > > This was spotted when trying to destroy IOREQ servers while the domain
> > > > is dying. The code will try to add the entry back in the P2M and
> > > > therefore update the P2M (see arch_ioreq_server_disable() ->
> > > > hvm_add_ioreq_gfn()).
> > > >
> > > > It should be possible to skip the mappin in hvm_add_ioreq_gfn(), however
> > > > I didn't try a patch yet because checking d->is_dying can be racy (I
> > > > can't find a proper lock).
> I understand the concern. I find it odd though that we permit
> iommu_map() to do anything at all when the domain is already
> dying. So irrespective of the remark below, how about bailing
> from iommu_map() earlier when the domain is dying?
I felt this was potentially too racy to use it. But it should be fine if 
keep the !d->is_dying below.
> > > > --- a/xen/drivers/passthrough/iommu.c
> > > > +++ b/xen/drivers/passthrough/iommu.c
> > > > @@ -272,7 +272,7 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
> > > >                               flush_flags) )
> > > >                   continue;
> > > >
> > > > -        if ( !is_hardware_domain(d) )
> > > > +        if ( !is_hardware_domain(d) && !d->is_dying )
> > > >               domain_crash(d);
> > > Would it make more sense to check is_dying inside domain_crash() (and turn it 
> > > into a no-op in that case)?
> > Jan also suggested moving the check in domain_crash(). However, I felt
> > it is potentially a too risky change for 4.15 as there are quite a few
> > callers.
> This is a fair point. However, in such a case I'd prefer symmetry
> at least throughout this one source file (there are three more
> places), unless there are strong reasons against doing so.
I can have a look and see if the decision is easy to make.

Cheers,

--
Julien Grall