[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: |
Peter Xu |
Subject: |
Re: [PATCH 5/5] memory: Skip bad range assertion if notifier is DEVIOTLB_UNMAP type |
Date: |
Mon, 28 Sep 2020 13:48:57 -0400 |
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.
Though it would always be good too if Jason would still like to review it.
Jason, what's your opinion?
Thanks,
--
Peter Xu
- [PATCH 0/5] memory: Skip assertion in memory_region_unregister_iommu_notifier, Eugenio Pérez, 2020/09/03
- [PATCH 1/5] memory: Rename memory_region_notify_one to memory_region_notify_iommu_one, Eugenio Pérez, 2020/09/03
- [PATCH 2/5] memory: Add IOMMUTLBEvent, Eugenio Pérez, 2020/09/03
- [PATCH 3/5] memory: Add IOMMU_NOTIFIER_DEVIOTLB_UNMAP IOMMUTLBNotificationType, Eugenio Pérez, 2020/09/03
- [PATCH 4/5] intel_iommu: Skip page walking on device iotlb invalidations, Eugenio Pérez, 2020/09/03
- [PATCH 5/5] memory: Skip bad range assertion if notifier is DEVIOTLB_UNMAP type, Eugenio Pérez, 2020/09/03