Re: [PATCH 01/12] iommu/vt-d: Use iommu_get_domain_for_dev() in debugfs

Robin Murphy
2022-05-31T21:22:45Z

On 2022-05-31 19:51, Jason Gunthorpe wrote:
> On Tue, May 31, 2022 at 07:07:32PM +0100, Robin Murphy wrote:
>
>>> And we expect the iommu driver to be unable to free page table levels
>>> that have IOVA boundaries in them?
>>
>> I'm not entirely sure what you mean there, but in general an unmap request
>> is expected to match some previous map request
>
> atomic cmpxchg is OK for inserting new page table levels but it can't
> protect you against concurrent freeing of page table levels. So
> without locks it means that page tables can't usually be freed. Which
> seems to match what the Intel driver does - at least from a cursory
> look.
>
> This is one of the reasons the mm has the mmap/etc lock and spinlocks
> because we do expect page table levels to get wiped out when VMA's are
> zap'd - all the different locks provide the protection against page
> tables disappearing under from something manipulating them.
>
> Basically every "lockless" walk in (process) MM land is actually
> protected by some kind of lock that blocks zap_page_range() from
> removing the page table levels themselves.

I'm not an expert in the Intel or AMD code, so I can only speak with
confidence about what we do in io-pgtable-arm, but the main reason for
not freeing pagetables is that it's simply not worth the bother of
trying to work out whether a whole sub-tree is empty. Not to mention
whether it's *still* empty by the time that we may have figured out that
it was.

There are only 3 instances where we'll free a table while the domain is
live. The first is the one legitimate race condition, where two map
requests targeting relatively nearby PTEs both go to fill in an
intermediate level of table; whoever loses that race frees the table
they allocated, but it was never visible to anyone else so that's
definitely fine. The second is if we're mapping a block entry, and find
that there's already a table entry there, wherein we assume the table
must be empty, clear the entry, invalidate any walk caches, install the
block entry, then free the orphaned table; since we're mapping the
entire IOVA range covered by that table, there should be no other
operations on that IOVA range attempting to walk the table at the same
time, so it's fine. The third is effectively the inverse, if we get a
block-sized unmap but find a table entry rather than a block at that
point (on the assumption that it's de-facto allowed for a single unmap
to cover multiple adjacent mappings as long as it does so exactly);
similarly we assume that the table must be full, and no other operations
should be racing because we're unmapping its whole IOVA range, so we
remove the table entry, invalidate, and free as before.

Again for efficiency reasons we don't attempt to validate those
assumptions by inspecting the freed tables, so odd behaviour can fall
out if the caller *does* do something bogus. For example if two calls
race to map a block and a page in the same (unmapped) region, the block
mapping will always succeed (and be what ends up in the final pagetable
state), but the page mapping may or may not report failure depending on
the exact timing.

Although we don't have debug dumping for io-pgtable-arm, it's good to be
thinking about this, since it's made me realise that dirty-tracking
sweeps per that proposal might pose a similar kind of concern, so we
might still need to harden these corners for the sake of that. Which
also reminds me that somewhere I have some half-finished patches making
io-pgtable-arm use the iommu_iotlb_gather freelist, so maybe I'll tackle
both concerns at once (perhaps we might even be able to RCU-ify the
freelist generically? I'll see how it goes when I get there).

Cheers,
Robin.