virtio/vhost-user: Fix wrong vhost notifier GPtrArray size

[PATCH] virtio/vhost-user: Fix wrong vhost notifier GPtrArray size

Posted by Yajun Wu 1 year, 11 months ago

In fetch_or_create_notifier, idx begins with 0. So the GPtrArray size
should be idx + 1 and g_ptr_array_set_size should be called with idx + 1.

This wrong GPtrArray size causes fetch_or_create_notifier return an invalid
address. Passing this invalid pointer to vhost_user_host_notifier_remove
causes assert fail:

    qemu/include/qemu/int128.h:27: int128_get64: Assertion `r == a' failed.
	shutting down, reason=crashed

Backends like dpdk-vdpa which sends out vhost notifier requests almost always
hit qemu crash.

Fixes: 503e355465 ("virtio/vhost-user: dynamically assign VhostUserHostNotifiers")
Signed-off-by: Yajun Wu <yajunw@nvidia.com>
Acked-by: Parav Pandit <parav@nvidia.com>
Change-Id: I87e0f7591ca9a59d210879b260704a2d9e9d6bcd
---
 hw/virtio/vhost-user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index b040c1ad2b..dbc690d16c 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1525,7 +1525,7 @@ static VhostUserHostNotifier *fetch_or_create_notifier(VhostUserState *u,
 {
     VhostUserHostNotifier *n = NULL;
     if (idx >= u->notifiers->len) {
-        g_ptr_array_set_size(u->notifiers, idx);
+        g_ptr_array_set_size(u->notifiers, idx + 1);
     }
 
     n = g_ptr_array_index(u->notifiers, idx);
-- 
2.36.0

RE: [PATCH] virtio/vhost-user: Fix wrong vhost notifier GPtrArray size

Posted by Dong, Eddie 1 year, 10 months ago

Reviewed-by: Eddie Dong <eddie.dong@intel.com>

> -----Original Message-----
> From: Qemu-devel <qemu-devel-
> bounces+eddie.dong=intel.com@nongnu.org> On Behalf Of Yajun Wu
> Sent: Wednesday, May 25, 2022 8:49 PM
> To: qemu-devel@nongnu.org; mst@redhat.com; alex.bennee@linaro.org;
> yajunw@nvidia.com
> Cc: Parav Pandit <parav@nvidia.com>
> Subject: [PATCH] virtio/vhost-user: Fix wrong vhost notifier GPtrArray size
> 
> In fetch_or_create_notifier, idx begins with 0. So the GPtrArray size should
> be idx + 1 and g_ptr_array_set_size should be called with idx + 1.
> 
> This wrong GPtrArray size causes fetch_or_create_notifier return an invalid
> address. Passing this invalid pointer to vhost_user_host_notifier_remove
> causes assert fail:
> 
>     qemu/include/qemu/int128.h:27: int128_get64: Assertion `r == a' failed.
> 	shutting down, reason=crashed
> 
> Backends like dpdk-vdpa which sends out vhost notifier requests almost
> always hit qemu crash.
> 
> Fixes: 503e355465 ("virtio/vhost-user: dynamically assign
> VhostUserHostNotifiers")
> Signed-off-by: Yajun Wu <yajunw@nvidia.com>
> Acked-by: Parav Pandit <parav@nvidia.com>
> Change-Id: I87e0f7591ca9a59d210879b260704a2d9e9d6bcd
> ---
>  hw/virtio/vhost-user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index
> b040c1ad2b..dbc690d16c 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1525,7 +1525,7 @@ static VhostUserHostNotifier
> *fetch_or_create_notifier(VhostUserState *u,  {
>      VhostUserHostNotifier *n = NULL;
>      if (idx >= u->notifiers->len) {
> -        g_ptr_array_set_size(u->notifiers, idx);
> +        g_ptr_array_set_size(u->notifiers, idx + 1);
>      }
> 
>      n = g_ptr_array_index(u->notifiers, idx);
> --
> 2.36.0
>

Re: [PATCH] virtio/vhost-user: Fix wrong vhost notifier GPtrArray size

Posted by Alex Bennée 1 year, 11 months ago

Yajun Wu <yajunw@nvidia.com> writes:

> In fetch_or_create_notifier, idx begins with 0. So the GPtrArray size
> should be idx + 1 and g_ptr_array_set_size should be called with idx + 1.
>
> This wrong GPtrArray size causes fetch_or_create_notifier return an invalid
> address. Passing this invalid pointer to vhost_user_host_notifier_remove
> causes assert fail:
>
>     qemu/include/qemu/int128.h:27: int128_get64: Assertion `r == a' failed.
> 	shutting down, reason=crashed
>
> Backends like dpdk-vdpa which sends out vhost notifier requests almost always
> hit qemu crash.

My bad. I was looking for ways to exercise this code but the internal
tests didn't do it. I guess I should look at getting a test setup for
dpdk. Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


>
> Fixes: 503e355465 ("virtio/vhost-user: dynamically assign VhostUserHostNotifiers")
> Signed-off-by: Yajun Wu <yajunw@nvidia.com>
> Acked-by: Parav Pandit <parav@nvidia.com>
> Change-Id: I87e0f7591ca9a59d210879b260704a2d9e9d6bcd
> ---
>  hw/virtio/vhost-user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index b040c1ad2b..dbc690d16c 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1525,7 +1525,7 @@ static VhostUserHostNotifier *fetch_or_create_notifier(VhostUserState *u,
>  {
>      VhostUserHostNotifier *n = NULL;
>      if (idx >= u->notifiers->len) {
> -        g_ptr_array_set_size(u->notifiers, idx);
> +        g_ptr_array_set_size(u->notifiers, idx + 1);
>      }
>  
>      n = g_ptr_array_index(u->notifiers, idx);


-- 
Alex Bennée

RE: [PATCH] virtio/vhost-user: Fix wrong vhost notifier GPtrArray size

Posted by Yajun Wu 1 year, 10 months ago

Hi Michael,

User space vhost clients are broken for few weeks now without this fix.
With Alex's review, can you please merge it if there are no further comments?

Thanks.

-----Original Message-----
From: Alex Bennée <alex.bennee@linaro.org> 
Sent: Thursday, May 26, 2022 3:09 PM
To: Yajun Wu <yajunw@nvidia.com>
Cc: qemu-devel@nongnu.org; mst@redhat.com; Parav Pandit <parav@nvidia.com>
Subject: Re: [PATCH] virtio/vhost-user: Fix wrong vhost notifier GPtrArray size

External email: Use caution opening links or attachments


Yajun Wu <yajunw@nvidia.com> writes:

> In fetch_or_create_notifier, idx begins with 0. So the GPtrArray size 
> should be idx + 1 and g_ptr_array_set_size should be called with idx + 1.
>
> This wrong GPtrArray size causes fetch_or_create_notifier return an 
> invalid address. Passing this invalid pointer to 
> vhost_user_host_notifier_remove causes assert fail:
>
>     qemu/include/qemu/int128.h:27: int128_get64: Assertion `r == a' failed.
>       shutting down, reason=crashed
>
> Backends like dpdk-vdpa which sends out vhost notifier requests almost 
> always hit qemu crash.

My bad. I was looking for ways to exercise this code but the internal tests didn't do it. I guess I should look at getting a test setup for dpdk. Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


>
> Fixes: 503e355465 ("virtio/vhost-user: dynamically assign 
> VhostUserHostNotifiers")
> Signed-off-by: Yajun Wu <yajunw@nvidia.com>
> Acked-by: Parav Pandit <parav@nvidia.com>
> Change-Id: I87e0f7591ca9a59d210879b260704a2d9e9d6bcd
> ---
>  hw/virtio/vhost-user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 
> b040c1ad2b..dbc690d16c 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1525,7 +1525,7 @@ static VhostUserHostNotifier 
> *fetch_or_create_notifier(VhostUserState *u,  {
>      VhostUserHostNotifier *n = NULL;
>      if (idx >= u->notifiers->len) {
> -        g_ptr_array_set_size(u->notifiers, idx);
> +        g_ptr_array_set_size(u->notifiers, idx + 1);
>      }
>
>      n = g_ptr_array_index(u->notifiers, idx);


--
Alex Bennée