memory: remove assert to avoid unnecessary coredump

[PATCH v1] memory: remove assert to avoid unnecessary coredump

Posted by Yi Sun 5 years, 8 months ago

It is too strict to use assert to make qemu coredump if
the notification does not overlap with registered range.
Skip it is fine enough.

During test, we found such a case for vhost net device:
    memory_region_notify_one: entry->iova=0xfee00000, entry_end=0xfeffffff, notifier->start=0xfef00000, notifier->end=0xffffffffffffffff

Skip this notification but not coredump makes everything
work well.

Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
---
 memory.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/memory.c b/memory.c
index 06484c2bff..62ad0f3377 100644
--- a/memory.c
+++ b/memory.c
@@ -1921,12 +1921,11 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
      * Skip the notification if the notification does not overlap
      * with registered range.
      */
-    if (notifier->start > entry_end || notifier->end < entry->iova) {
+    if (notifier->start > entry_end || notifier->end < entry->iova ||
+        entry->iova < notifier->start || entry_end > notifier->end) {
         return;
     }
 
-    assert(entry->iova >= notifier->start && entry_end <= notifier->end);
-
     if (entry->perm & IOMMU_RW) {
         request_flags = IOMMU_NOTIFIER_MAP;
     } else {
-- 
2.15.1

Re: [PATCH v1] memory: remove assert to avoid unnecessary coredump

Posted by Yan Zhao 5 years, 8 months ago

On Tue, Mar 03, 2020 at 11:11:14AM +0800, Yi Sun wrote:
> It is too strict to use assert to make qemu coredump if
> the notification does not overlap with registered range.
> Skip it is fine enough.
> 
> During test, we found such a case for vhost net device:
>     memory_region_notify_one: entry->iova=0xfee00000, entry_end=0xfeffffff, notifier->start=0xfef00000, notifier->end=0xffffffffffffffff
>
so for range from 0xfef00000 to 0xfefffff,  would notification for this
range get lost?

Thanks
Yan

> Skip this notification but not coredump makes everything
> work well.
> 
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> ---
>  memory.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 06484c2bff..62ad0f3377 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1921,12 +1921,11 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
>       * Skip the notification if the notification does not overlap
>       * with registered range.
>       */
> -    if (notifier->start > entry_end || notifier->end < entry->iova) {
> +    if (notifier->start > entry_end || notifier->end < entry->iova ||
> +        entry->iova < notifier->start || entry_end > notifier->end) {
>          return;
>      }
>  
> -    assert(entry->iova >= notifier->start && entry_end <= notifier->end);
> -
>      if (entry->perm & IOMMU_RW) {
>          request_flags = IOMMU_NOTIFIER_MAP;
>      } else {
> -- 
> 2.15.1
>

Re: [PATCH v1] memory: remove assert to avoid unnecessary coredump

Posted by Yi Sun 5 years, 8 months ago

On 20-03-02 22:36:39, Yan Zhao wrote:
> On Tue, Mar 03, 2020 at 11:11:14AM +0800, Yi Sun wrote:
> > It is too strict to use assert to make qemu coredump if
> > the notification does not overlap with registered range.
> > Skip it is fine enough.
> > 
> > During test, we found such a case for vhost net device:
> >     memory_region_notify_one: entry->iova=0xfee00000, entry_end=0xfeffffff, notifier->start=0xfef00000, notifier->end=0xffffffffffffffff
> >
> so for range from 0xfef00000 to 0xfefffff,  would notification for this
> range get lost?
> 
Yes, that is an issue although there is no any problem found during test
with this fix.

I think we should notify the intersection between entry and notifier. How
do you think?

> Thanks
> Yan
> 
> > Skip this notification but not coredump makes everything
> > work well.
> > 
> > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> > ---
> >  memory.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/memory.c b/memory.c
> > index 06484c2bff..62ad0f3377 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -1921,12 +1921,11 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
> >       * Skip the notification if the notification does not overlap
> >       * with registered range.
> >       */
> > -    if (notifier->start > entry_end || notifier->end < entry->iova) {
> > +    if (notifier->start > entry_end || notifier->end < entry->iova ||
> > +        entry->iova < notifier->start || entry_end > notifier->end) {
> >          return;
> >      }
> >  
> > -    assert(entry->iova >= notifier->start && entry_end <= notifier->end);
> > -
> >      if (entry->perm & IOMMU_RW) {
> >          request_flags = IOMMU_NOTIFIER_MAP;
> >      } else {
> > -- 
> > 2.15.1
> >

Re: [PATCH v1] memory: remove assert to avoid unnecessary coredump

Posted by Yan Zhao 5 years, 8 months ago

On Tue, Mar 03, 2020 at 01:22:26PM +0800, Yi Sun wrote:
> On 20-03-02 22:36:39, Yan Zhao wrote:
> > On Tue, Mar 03, 2020 at 11:11:14AM +0800, Yi Sun wrote:
> > > It is too strict to use assert to make qemu coredump if
> > > the notification does not overlap with registered range.
> > > Skip it is fine enough.
> > > 
> > > During test, we found such a case for vhost net device:
> > >     memory_region_notify_one: entry->iova=0xfee00000, entry_end=0xfeffffff, notifier->start=0xfef00000, notifier->end=0xffffffffffffffff
> > >
> > so for range from 0xfef00000 to 0xfefffff,  would notification for this
> > range get lost?
> > 
> Yes, that is an issue although there is no any problem found during test
> with this fix.
> 
> I think we should notify the intersection between entry and notifier. How
> do you think?
> 
no. please refer to the link below.
https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04218.html

Thanks
Yan

> > Thanks
> > Yan
> > 
> > > Skip this notification but not coredump makes everything
> > > work well.
> > > 
> > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> > > ---
> > >  memory.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/memory.c b/memory.c
> > > index 06484c2bff..62ad0f3377 100644
> > > --- a/memory.c
> > > +++ b/memory.c
> > > @@ -1921,12 +1921,11 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
> > >       * Skip the notification if the notification does not overlap
> > >       * with registered range.
> > >       */
> > > -    if (notifier->start > entry_end || notifier->end < entry->iova) {
> > > +    if (notifier->start > entry_end || notifier->end < entry->iova ||
> > > +        entry->iova < notifier->start || entry_end > notifier->end) {
> > >          return;
> > >      }
> > >  
> > > -    assert(entry->iova >= notifier->start && entry_end <= notifier->end);
> > > -
> > >      if (entry->perm & IOMMU_RW) {
> > >          request_flags = IOMMU_NOTIFIER_MAP;
> > >      } else {
> > > -- 
> > > 2.15.1
> > >