Re: [for-4.15][PATCH v2 4/5] xen/iommu: x86: Don't leak the IOMMU page-tables

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 10/02/2021 16:12, Jan Beulich wrote:
> On 10.02.2021 16:04, Julien Grall wrote:
> > On 10/02/2021 14:32, Jan Beulich wrote:
> > > On 09.02.2021 16:28, Julien Grall wrote:
> > > > From: Julien Grall <jgrall@xxxxxxxxxx>
> > > >
> > > > The new IOMMU page-tables allocator will release the pages when
> > > > relinquish the domain resources. However, this is not sufficient when
> > > > the domain is dying because nothing prevents page-table to be allocated.
> > > >
> > > > iommu_alloc_pgtable() is now checking if the domain is dying before
> > > > adding the page in the list. We are relying on &hd->arch.pgtables.lock
> > > > to synchronize d->is_dying.
> > > As said in reply to an earlier patch, I think suppressing
> > > (really: ignoring) new mappings would be better.
> > This is exactly what I suggested in v1 but you wrote:
> >
> > "Ignoring requests there seems fragile to me. Paul - what are your
> > thoughts about bailing early from hvm_add_ioreq_gfn() when the
> > domain is dying?"
> Was this on the thread of this patch? I didn't find such a
> reply of mine. I need more context here because you name
> hvm_add_ioreq_gfn() above, while I refer to iommu_map()
> (and downwards the call stack).
See [1].

> > Are you know saying that the following snipped would be fine:
> >
> > if ( d->is_dying )
> >     return 0;
> In {amd,intel}_iommu_map_page(), after the lock was acquired
> and with it suitably released, yes. And if that's what you
> suggested, then I'm sorry - I don't think I can see anything
> fragile there anymore.
Duplicating the check sounds good to me.

> > > You could
> > > utilize the same lock, but you'd need to duplicate the
> > > checking in {amd,intel}_iommu_map_page().
> > >
> > > I'm not entirely certain in the case about unmap requests:
> > > It may be possible to also suppress/ignore them, but this
> > > may require some further thought.
> > I think the unmap part is quite risky to d->is_dying because the PCI
> > devices may not quiesced and still assigned to the domain.
> Hmm, yes, good point. Of course upon first unmap with is_dying
> observed set we could zap the root page table, but I don't
> suppose that's something we want to do for 4.15.
We would still need to zap the root page table in the relinquish path. 
So I am not sure what benefits it would give us to zap the page tables 
on the first iommu_unmap() afther domain dies.
Cheers,

[1] 
https://lore.kernel.org/xen-devel/f21f1f61-5213-55a8-320c-43e5fe80100f@xxxxxxxx/
--
Julien Grall