[Qemu-devel] [PATCH v5 1/4] vhost: remove assertion to prevent crash

[Qemu-devel] [PATCH v5 1/4] vhost: remove assertion to prevent crash

[Jay Zhou]

Start QEMU with more DIMM devices than limit but without any
vhost backends and then hotplug a vhost user backend, the VM
will be crashed.
Instead of asserting in vhost_user_set_mem_table(), error number
is used to gracefully prevent device to start. This fixes the
crash issue.

Cc: qemu-stable@nongnu.org
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
---
 hw/virtio/vhost-user.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 093675e..8500562 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -317,11 +317,14 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
                                      &offset);
         fd = memory_region_get_fd(mr);
         if (fd > 0) {
+            if (fd_num == VHOST_MEMORY_MAX_NREGIONS) {
+                error_report("Failed preparing vhost-user memory table msg");
+                return -1;
+            }
             msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
             msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
             msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
             msg.payload.memory.regions[fd_num].mmap_offset = offset;
-            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
             fds[fd_num++] = fd;
         }
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v5 1/4] vhost: remove assertion to prevent crash

[Igor Mammedov]

On Wed, 10 Jan 2018 00:39:35 +0800
Jay Zhou <jianjay.zhou@huawei.com> wrote:

> Start QEMU with more DIMM devices than limit but without any
> vhost backends and then hotplug a vhost user backend, the VM
> will be crashed.
> Instead of asserting in vhost_user_set_mem_table(), error number
> is used to gracefully prevent device to start. This fixes the
> crash issue.

I'd rewrite commit message as following:
--
QEMU will assert on vhsot-user backed virtio device hotplug
if QEMU is using more RAM regions than VHOST_MEMORY_MAX_NREGIONS
(for example if it  were started with a lot of DIMM devices).

Fix it by returning error instead of asserting and let
callers of vhost_set_mem_table() handle error condition
gracefully.
--

>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
> ---
>  hw/virtio/vhost-user.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 093675e..8500562 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -317,11 +317,14 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
>                                       &offset);
>          fd = memory_region_get_fd(mr);
>          if (fd > 0) {
> +            if (fd_num == VHOST_MEMORY_MAX_NREGIONS) {
> +                error_report("Failed preparing vhost-user memory table msg");
> +                return -1;
> +            }
>              msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
>              msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
>              msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
>              msg.payload.memory.regions[fd_num].mmap_offset = offset;
> -            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
>              fds[fd_num++] = fd;
>          }
>      }

Re: [Qemu-devel] [PATCH v5 1/4] vhost: remove assertion to prevent crash

[Zhoujian (jay)]

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: Wednesday, January 10, 2018 9:31 PM
> To: Zhoujian (jay) <jianjay.zhou@huawei.com>
> Cc: qemu-devel@nongnu.org; Huangweidong (C) <weidong.huang@huawei.com>;
> mst@redhat.com; wangxin (U) <wangxinxin.wang@huawei.com>; qemu-
> stable@nongnu.org; Gonglei (Arei) <arei.gonglei@huawei.com>; Liuzhe (Ahriy,
> Euler) <liuzhe13@huawei.com>
> Subject: Re: [Qemu-devel] [PATCH v5 1/4] vhost: remove assertion to prevent
> crash
> 
> On Wed, 10 Jan 2018 00:39:35 +0800
> Jay Zhou <jianjay.zhou@huawei.com> wrote:
> 
> > Start QEMU with more DIMM devices than limit but without any vhost
> > backends and then hotplug a vhost user backend, the VM will be
> > crashed.
> > Instead of asserting in vhost_user_set_mem_table(), error number is
> > used to gracefully prevent device to start. This fixes the crash
> > issue.
> 
> I'd rewrite commit message as following:
> --
> QEMU will assert on vhsot-user backed virtio device hotplug if QEMU is using
> more RAM regions than VHOST_MEMORY_MAX_NREGIONS (for example if it  were
> started with a lot of DIMM devices).
> 
> Fix it by returning error instead of asserting and let callers of
> vhost_set_mem_table() handle error condition gracefully.
> --

s/vhsot-user/vhost-user, otherwise much better than mine, will use it
in the next version, thanks!

Jay

> >
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
> > ---
> >  hw/virtio/vhost-user.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index
> > 093675e..8500562 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -317,11 +317,14 @@ static int vhost_user_set_mem_table(struct vhost_dev
> *dev,
> >                                       &offset);
> >          fd = memory_region_get_fd(mr);
> >          if (fd > 0) {
> > +            if (fd_num == VHOST_MEMORY_MAX_NREGIONS) {
> > +                error_report("Failed preparing vhost-user memory table
> msg");
> > +                return -1;
> > +            }
> >              msg.payload.memory.regions[fd_num].userspace_addr = reg-
> >userspace_addr;
> >              msg.payload.memory.regions[fd_num].memory_size  = reg-
> >memory_size;
> >              msg.payload.memory.regions[fd_num].guest_phys_addr = reg-
> >guest_phys_addr;
> >              msg.payload.memory.regions[fd_num].mmap_offset = offset;
> > -            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
> >              fds[fd_num++] = fd;
> >          }
> >      }