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

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

On Tue, 9 Feb 2021 at 20:28, Paul Durrant <xadimgnik@xxxxxxxxx> wrote:
>
> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Sent: 09 February 2021 15:28
> > To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: hongyxia@xxxxxxxxxxxx; iwj@xxxxxxxxxxxxxx; Julien Grall 
> > <jgrall@xxxxxxxxxx>; Jan Beulich
> > <jbeulich@xxxxxxxx>; Paul Durrant <paul@xxxxxxx>
> > Subject: [for-4.15][PATCH v2 3/5] xen/iommu: iommu_map: Don't crash the 
> > domain if it is dying
> >
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > 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
>
> s/held/hold
>
> > relinquish path).
> >
> > 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
>
> s/mappin/mapping
>
> > I didn't try a patch yet because checking d->is_dying can be racy (I
> > can't find a proper lock).
> >
> > Changes in v2:
> >     - Patch added
> > ---
> >  xen/drivers/passthrough/iommu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/xen/drivers/passthrough/iommu.c 
> > b/xen/drivers/passthrough/iommu.c
> > index 879d238bcd31..75419f20f76d 100644
> > --- 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.

Cheers,