On Fri, Sep 4, 2020 at 6:34 AM Jason Wang <jasow...@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 <epere...@redhat.com>
> > Reviewed-by: Peter Xu <pet...@redhat.com>
> > Reviewed-by: Juan Quintela <quint...@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?
Is there anything else needed to pull this patch?
Thanks!
>
> > + } else {
> > + assert(entry->iova >= notifier->start && entry_end <=
> > notifier->end);
> > + }
> >
> > if (event->type & notifier->notifier_flags) {
> > - notifier->notify(notifier, entry);
> > + notifier->notify(notifier, &tmp);
> > }
> > }
> >
>
