On Mon, Sep 28, 2020 at 11:05:01AM +0200, Eugenio Perez Martin wrote:
On Fri, Sep 4, 2020 at 6:34 AM Jason Wang <jasowang@redhat.com> wrote:
On 2020/9/4 上午12:14, Eugenio Pérez wrote:
Device IOTLB invalidations can unmap arbitrary ranges, eiter outside of
the memory region or even [0, ~0ULL] for all the space. The assertion
could be hit by a guest, and rhel7 guest effectively hit it.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
softmmu/memory.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 8694fc7cf7..e723fcbaa1 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1895,6 +1895,7 @@ void memory_region_notify_iommu_one(IOMMUNotifier
*notifier,
{
IOMMUTLBEntry *entry = &event->entry;
hwaddr entry_end = entry->iova + entry->addr_mask;
+ IOMMUTLBEntry tmp = *entry;
if (event->type == IOMMU_NOTIFIER_UNMAP) {
assert(entry->perm == IOMMU_NONE);
@@ -1908,10 +1909,18 @@ void memory_region_notify_iommu_one(IOMMUNotifier
*notifier,
return;
}
- assert(entry->iova >= notifier->start && entry_end <= notifier->end);
+ if (notifier->notifier_flags & IOMMU_NOTIFIER_DEVIOTLB_UNMAP) {
+ /* Crop (iova, addr_mask) to range */
+ tmp.iova = MAX(tmp.iova, notifier->start);
+ tmp.addr_mask = MIN(entry_end, notifier->end) - tmp.iova;
+ /* Confirm no underflow */
+ assert(MIN(entry_end, notifier->end) >= tmp.iova);
It's still not clear to me why we need such assert. Consider
notifier->end is the possible IOVA range but not possible device IOTLB
invalidation range (e.g it allows [0, ULLONG_MAX]).
Thanks
As far as I understood the device should admit that out of bounds
notifications in that case,
and the assert just makes sure that there was no underflow in
tmp.addr_mask, i.e., that something
very wrong that should never happen in production happened.
Peter, would you mind to confirm/correct it?
I think Jason is right - since we have checked at the entry that the two
regions cross over each other:
/*
* Skip the notification if the notification does not overlap
* with registered range.
*/
if (notifier->start > entry_end || notifier->end < entry->iova) {
return;
}
Then I don't see how this assertion can fail any more.
But imho not a big problem either, and it shouldn't hurt to even keep the
assertion of above isn't that straightforward.
Is there anything else needed to pull this patch?
I didn't post a pull for this only because I shouldn't :) - the plan was that
all vt-d patches will still go via Michael's tree, iiuc. Though at least to me
I think this series is acceptable for merging.