[dpdk-dev] [PATCH 0/3] vhost: Fix virtio-net on VHOST_USER_RESET_OWNER

[dpdk-dev] [PATCH 0/3] vhost: Fix virtio-net on VHOST_USER_RESET_OWNER

[Jerome Jutteau]

Hi,

I have a bug when Qemu with two vhost interfaces gently stops (SIGINT).
When stopping, it sends two RESET_OWNER for each interface:
- Before stopping, we have two interfaces identifers: 0 and 1.
- The first reset_owner call resets device 1 (and this id device_fh) to zero,
  the device list now contains two devices with id 0.
- The second call don't find device 1 and segfault as reset_owner don't
  check if the device has been found or not.
- Later, user_get_vring_base can also segfault for the same reason.

This series of patches propose to fix the way reset_owner alter a device and
add more checks when searching for a device.

Jerome Jutteau (3):
  vhost: avoid device identifier to be reset to 0 in reset_owner
  vhost: check that a device exists during reset_owner
  vhost: protect user_get_vring_base from unknown devices

 lib/librte_vhost/vhost_user/virtio-net-user.c | 2 ++
 lib/librte_vhost/virtio-net.c                 | 5 +++++
 2 files changed, 7 insertions(+)

-- 
jerome

[dpdk-dev] [PATCH 1/3] vhost: avoid device identifier to be reset to 0 in reset_owner

[Jerome Jutteau]

virtio-net clean and init device after a VHOST_USER_RESET_OWNER.
This reset device identifier to 0 and break ll_root listing logic.
This patch keep the old device identifier and re-write it on the cleaned
device.

Signed-off-by: Jerome Jutteau <jerome.jutteau@outscale.com>
---
 lib/librte_vhost/virtio-net.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index d0f1764..955a29d 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -399,11 +399,14 @@ static int
 reset_owner(struct vhost_device_ctx ctx)
 {
 	struct virtio_net_config_ll *ll_dev;
+	uint64_t device_fh;
 
 	ll_dev = get_config_ll_entry(ctx);
+	device_fh = ll_dev->dev.device_fh;
 
 	cleanup_device(&ll_dev->dev);
 	init_device(&ll_dev->dev);
+	ll_dev->dev.device_fh = device_fh;
 
 	return 0;
 }
-- 
jerome

[dpdk-dev] [PATCH 2/3] vhost: check that a device exists during reset_owner

[Jerome Jutteau]

virtio-net search for it's device in reset_owner.
The function don't check the return result of get_config_ll_entry
which can be NULL.

Signed-off-by: Jerome Jutteau <jerome.jutteau@outscale.com>
---
 lib/librte_vhost/virtio-net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index 955a29d..ec6a575 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -402,6 +402,8 @@ reset_owner(struct vhost_device_ctx ctx)
 	uint64_t device_fh;
 
 	ll_dev = get_config_ll_entry(ctx);
+	if (ll_dev == NULL)
+		return -1;
 	device_fh = ll_dev->dev.device_fh;
 
 	cleanup_device(&ll_dev->dev);
-- 
jerome

Re: [dpdk-dev] [PATCH 2/3] vhost: check that a device exists during reset_owner

[Yuanhan Liu]

On Fri, Oct 16, 2015 at 11:08:49AM +0200, Jerome Jutteau wrote:
> virtio-net search for it's device in reset_owner.
> The function don't check the return result of get_config_ll_entry
> which can be NULL.
> 
> Signed-off-by: Jerome Jutteau <jerome.jutteau@outscale.com>
> ---
>  lib/librte_vhost/virtio-net.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index 955a29d..ec6a575 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -402,6 +402,8 @@ reset_owner(struct vhost_device_ctx ctx)
>  	uint64_t device_fh;
>  
>  	ll_dev = get_config_ll_entry(ctx);
> +	if (ll_dev == NULL)
> +		return -1;

I would call get_device() here, which dumps an error message when
it's NULL so that user will get noticed.

Besides that, the 3 patches look good to me, and,

Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

	--yliu

>  	device_fh = ll_dev->dev.device_fh;
>  
>  	cleanup_device(&ll_dev->dev);
> -- 
> jerome

Re: [dpdk-dev] [PATCH 2/3] vhost: check that a device exists during reset_owner

[Jérôme Jutteau]

Hi Yuanhan,

2015-10-19 6:35 GMT+02:00 Yuanhan Liu <yuanhan.liu@linux.intel.com>:
> I would call get_device() here, which dumps an error message when
> it's NULL so that user will get noticed.
>
> Besides that, the 3 patches look good to me, and,
>
> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Thanks for your review, a v2 is coming soon with get_device().

-- 
Jérôme

[dpdk-dev] [PATCH 3/3] vhost: protect user_get_vring_base from unknown devices

[Jerome Jutteau]

get_device return is not checked and may cause segfault when device is
not found. This patch fix this.

Signed-off-by: Jerome Jutteau <jerome.jutteau@outscale.com>
---
 lib/librte_vhost/vhost_user/virtio-net-user.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.c b/lib/librte_vhost/vhost_user/virtio-net-user.c
index 4689927..e0bc2a4 100644
--- a/lib/librte_vhost/vhost_user/virtio-net-user.c
+++ b/lib/librte_vhost/vhost_user/virtio-net-user.c
@@ -276,6 +276,8 @@ user_get_vring_base(struct vhost_device_ctx ctx,
 {
 	struct virtio_net *dev = get_device(ctx);
 
+	if (dev == NULL)
+		return -1;
 	/* We have to stop the queue (virtio) if it is running. */
 	if (dev->flags & VIRTIO_DEV_RUNNING)
 		notify_ops->destroy_device(dev);
-- 
jerome