Re: [Qemu-devel] [PATCH v4] memory: assert on out of scope notification

[Yan Zhao]

On Tue, Jun 25, 2019 at 03:36:57PM +0800, Auger Eric wrote:
> Hi Yan,
> 
> [ + Peter]
> 
> 
> On 6/25/19 5:21 AM, Yan Zhao wrote:
> > It is wrong for an entry to have parts out of scope of notifier's range.
> > assert this condition.
> > 
> > Out of scope mapping/unmapping would cause problem, as in below case:
> > 
> > 1. initially there are two notifiers with ranges
> > 0-0xfedfffff, 0xfef00000-0xffffffffffffffff,
> > IOVAs from 0x3c000000 - 0x3c1fffff is in shadow page table.
> > 
> > 2. in vfio, memory_region_register_iommu_notifier() is followed by
> > memory_region_iommu_replay(), which will first call address space
> > unmap,
> > and walk and add back all entries in vtd shadow page table. e.g.
> > (1) for notifier 0-0xfedfffff,
> >     IOVAs from 0 - 0xffffffff get unmapped,
> >     and IOVAs from 0x3c000000 - 0x3c1fffff get mapped
> > (2) for notifier 0xfef00000-0xffffffffffffffff
> >     IOVAs from 0 - 0x7fffffffff get unmapped,
> >     but IOVAs from 0x3c000000 - 0x3c1fffff cannot get mapped back.
> > 
> > Cc: Eric Auger <address@hidden>
> > Signed-off-by: Yan Zhao <address@hidden>
> > 
> > ---
> > v4:
> > 1. modified commit title
> > 2. using "assert" instead of printing warning message> (Eric Auger)
> Sorry my last reply mentioning the relevance of an assert was rather a
> question (for you and other reviewers) actually. Your bug report
> emphasized the fact that having an entry which has non inclusive overlap
> with the notifier can be dangerous.
> 
> Now we also have memory_region_notify_iommu() that loops over all
> notifiers of the iommu mr and notifies each of them with the entry. Some
> other callers than vtd (spapr_iommu.c, tz-mpc.c, s390-pci-inst.c,
> smmuv3) may hit this assert and this may cause a regression. I checked
> with smmuv3 and it looks OK.
> 
> Also if we enforce the rule that the entry span shall be within the
> notifier one, this should be documented in memory.h.
>
Yes, actually I was also this concern that if we only print warning and
do not assert, then if there's bugs in viommu (or others), only viommu (or
others) does not function well, but qemu can still boot up. if we assert
here, qemu will fail to boot up too.

so, it depends on the strict level you want. :)

> Thanks
> 
> Eric
> > 
> > v3:
> > refined code style and message format
> > 
> > v2:
> > 1. added a local variable entry_end (Eric Auger)
> > 2. using PRIx64 as format for address range in warning message
> > (Eric Auger)
> > ---
> >  memory.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/memory.c b/memory.c
> > index 0a089a7..35e8e77 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -1937,16 +1937,18 @@ void memory_region_notify_one(IOMMUNotifier 
> > *notifier,
> >                                IOMMUTLBEntry *entry)
> >  {
> >      IOMMUNotifierFlag request_flags;
> > +    hwaddr entry_end = entry->iova + entry->addr_mask;
> >  
> >      /*
> >       * Skip the notification if the notification does not overlap
> >       * with registered range.
> >       */
> > -    if (notifier->start > entry->iova + entry->addr_mask ||
> > -        notifier->end < entry->iova) {
> > +    if (notifier->start > entry_end || notifier->end < entry->iova) {
> >          return;
> >      }
> >  
> > +    assert(entry->iova >= notifier->start && entry_end <= notifier->end);
> > +
> >      if (entry->perm & IOMMU_RW) {
> >          request_flags = IOMMU_NOTIFIER_MAP;
> >      } else {
> >