[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 5/5] memory: Skip bad range assertion if notifier is DEVIOTLB

From: Jason Wang
Subject: Re: [PATCH 5/5] memory: Skip bad range assertion if notifier is DEVIOTLB_UNMAP type
Date: Thu, 15 Oct 2020 15:50:44 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0

On 2020/10/4 上午1:38, Michael S. Tsirkin wrote:
On Mon, Sep 28, 2020 at 01:48:57PM -0400, Peter Xu wrote:
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 
       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 

-    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]).


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) {

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.
Sure, that's ok.

Eugenio, you sent patch 0 as a response to another series, which
made me miss the series. Pls don't do that in the future.

Looks like Jason reviewed the series - Jason, is that right? -
so I'd like his ack if possible.


Euenio. If it's possible I would prefer to remove the assertion but it's ok it you leave it.

And please repost the series.


Though it would always be good too if Jason would still like to review it.

Jason, what's your opinion?


Peter Xu

reply via email to

[Prev in Thread] Current Thread [Next in Thread]