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

[Auger Eric <eric.auger@redhat.com>]

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 <eric.auger@redhat.com>
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> 
> ---
> 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.

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