diff mbox series

bus/vmbus: Fix crash when handling packets in secondary process

Message ID 20210720145839.12180-1-jonathan.erb@banduracyber.com (mailing list archive)
State Superseded, archived
Headers
Series bus/vmbus: Fix crash when handling packets in secondary process |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Jonathan Erb July 20, 2021, 2:58 p.m. UTC 
  Have secondary processes construct their own copy of primary channel with
own mappings.

Remove vmbus_channel primary ptr from struct mapped_vmbus_resource as its
not used.

Populate virtual memory address "addr" in struct rte_mem_resource for
secondary processes as netvsc will attempt to reference it thus causing
a crash. It was initialized for primary processes but not for secondary.

Signed-off-by: jerb <jonathan.erb@banduracyber.com>
---
 drivers/bus/vmbus/private.h          |  1 -
 drivers/bus/vmbus/vmbus_channel.c    |  4 +---
 drivers/bus/vmbus/vmbus_common_uio.c | 15 ++++++++++-----
 3 files changed, 11 insertions(+), 9 deletions(-)

Comments

Stephen Hemminger July 20, 2021, 3:28 p.m. UTC | #1 
Looks good, minor comment. You don't have to check for NULL before calling rte_free().
Rte_free(NULL) is a NOP like free(NULL).

Sorry for top posting; but if you send to my Microsoft account you are stuck with what
Outlook can do...

-----Original Message-----
From: jerb <jonathan.erb@banduracyber.com> 
Sent: Tuesday, July 20, 2021 7:59 AM
To: Stephen Hemminger <sthemmin@microsoft.com>; Long Li <longli@microsoft.com>
Cc: dev@dpdk.org; jerb <jonathan.erb@banduracyber.com>
Subject: [PATCH] bus/vmbus: Fix crash when handling packets in secondary process

Have secondary processes construct their own copy of primary channel with
own mappings.

Remove vmbus_channel primary ptr from struct mapped_vmbus_resource as its
not used.

Populate virtual memory address "addr" in struct rte_mem_resource for
secondary processes as netvsc will attempt to reference it thus causing
a crash. It was initialized for primary processes but not for secondary.

Signed-off-by: jerb <jonathan.erb@banduracyber.com>
---
 drivers/bus/vmbus/private.h          |  1 -
 drivers/bus/vmbus/vmbus_channel.c    |  4 +---
 drivers/bus/vmbus/vmbus_common_uio.c | 15 ++++++++++-----
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
index 528d60a42f..746212bd5f 100644
--- a/drivers/bus/vmbus/private.h
+++ b/drivers/bus/vmbus/private.h
@@ -42,7 +42,6 @@ struct mapped_vmbus_resource {
 
 	rte_uuid_t id;
 	int nb_maps;
-	struct vmbus_channel *primary;
 	struct vmbus_map maps[VMBUS_MAX_RESOURCE];
 	char path[PATH_MAX];
 };
diff --git a/drivers/bus/vmbus/vmbus_channel.c b/drivers/bus/vmbus/vmbus_channel.c
index f67f1c438a..119b9b367e 100644
--- a/drivers/bus/vmbus/vmbus_channel.c
+++ b/drivers/bus/vmbus/vmbus_channel.c
@@ -351,10 +351,8 @@ int rte_vmbus_chan_open(struct rte_vmbus_device *device,
 
 	err = vmbus_chan_create(device, device->relid, 0,
 				device->monitor_id, new_chan);
-	if (!err) {
+	if (!err)
 		device->primary = *new_chan;
-		uio_res->primary = *new_chan;
-	}
 
 	return err;
 }
diff --git a/drivers/bus/vmbus/vmbus_common_uio.c b/drivers/bus/vmbus/vmbus_common_uio.c
index 8582e32c1d..7b9a8ef434 100644
--- a/drivers/bus/vmbus/vmbus_common_uio.c
+++ b/drivers/bus/vmbus/vmbus_common_uio.c
@@ -69,8 +69,10 @@ vmbus_uio_map_secondary(struct rte_vmbus_device *dev)
 					     fd, offset,
 					     uio_res->maps[i].size, 0);
 
-		if (mapaddr == uio_res->maps[i].addr)
+		if (mapaddr == uio_res->maps[i].addr) {
+			dev->resource[i].addr = mapaddr;
 			continue;	/* successful map */
+		}
 
 		if (mapaddr == MAP_FAILED)
 			VMBUS_LOG(ERR,
@@ -88,9 +90,9 @@ vmbus_uio_map_secondary(struct rte_vmbus_device *dev)
 	/* fd is not needed in secondary process, close it */
 	close(fd);
 
-	dev->primary = uio_res->primary;
-	if (!dev->primary) {
-		VMBUS_LOG(ERR, "missing primary channel");
+	if (vmbus_chan_create(dev, dev->relid, 0,
+					dev->monitor_id, &dev->primary)) {
+		VMBUS_LOG(ERR, "cannot create primary channel");
 		return -1;
 	}
 
@@ -211,8 +213,11 @@ vmbus_uio_unmap_resource(struct rte_vmbus_device *dev)
 		return;
 
 	/* secondary processes - just free maps */
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+		if (dev->primary != NULL)
+			rte_free(dev->primary);
 		return vmbus_uio_unmap(uio_res);
+	}
 
 	TAILQ_REMOVE(uio_res_list, uio_res, next);
Long Li July 21, 2021, 12:17 a.m. UTC | #2 
> -----Original Message-----
> From: Stephen Hemminger <sthemmin@microsoft.com>
> Sent: Tuesday, July 20, 2021 8:29 AM
> To: jerb <jonathan.erb@banduracyber.com>; Long Li <longli@microsoft.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH] bus/vmbus: Fix crash when handling packets in secondary
> process
> 
> Looks good, minor comment. You don't have to check for NULL before calling
> rte_free().
> Rte_free(NULL) is a NOP like free(NULL).
> 
> Sorry for top posting; but if you send to my Microsoft account you are stuck
> with what Outlook can do...
> 
> -----Original Message-----
> From: jerb <jonathan.erb@banduracyber.com>
> Sent: Tuesday, July 20, 2021 7:59 AM
> To: Stephen Hemminger <sthemmin@microsoft.com>; Long Li
> <longli@microsoft.com>
> Cc: dev@dpdk.org; jerb <jonathan.erb@banduracyber.com>
> Subject: [PATCH] bus/vmbus: Fix crash when handling packets in secondary
> process
> 
> Have secondary processes construct their own copy of primary channel with
> own mappings.
> 
> Remove vmbus_channel primary ptr from struct mapped_vmbus_resource as
> its not used.
> 
> Populate virtual memory address "addr" in struct rte_mem_resource for
> secondary processes as netvsc will attempt to reference it thus causing a crash.
> It was initialized for primary processes but not for secondary.
> 
> Signed-off-by: jerb <jonathan.erb@banduracyber.com>

Looks good.

This should also go to stable.


> ---
>  drivers/bus/vmbus/private.h          |  1 -
>  drivers/bus/vmbus/vmbus_channel.c    |  4 +---
>  drivers/bus/vmbus/vmbus_common_uio.c | 15 ++++++++++-----
>  3 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h index
> 528d60a42f..746212bd5f 100644
> --- a/drivers/bus/vmbus/private.h
> +++ b/drivers/bus/vmbus/private.h
> @@ -42,7 +42,6 @@ struct mapped_vmbus_resource {
> 
>  	rte_uuid_t id;
>  	int nb_maps;
> -	struct vmbus_channel *primary;
>  	struct vmbus_map maps[VMBUS_MAX_RESOURCE];
>  	char path[PATH_MAX];
>  };
> diff --git a/drivers/bus/vmbus/vmbus_channel.c
> b/drivers/bus/vmbus/vmbus_channel.c
> index f67f1c438a..119b9b367e 100644
> --- a/drivers/bus/vmbus/vmbus_channel.c
> +++ b/drivers/bus/vmbus/vmbus_channel.c
> @@ -351,10 +351,8 @@ int rte_vmbus_chan_open(struct rte_vmbus_device
> *device,
> 
>  	err = vmbus_chan_create(device, device->relid, 0,
>  				device->monitor_id, new_chan);
> -	if (!err) {
> +	if (!err)
>  		device->primary = *new_chan;
> -		uio_res->primary = *new_chan;
> -	}
> 
>  	return err;
>  }
> diff --git a/drivers/bus/vmbus/vmbus_common_uio.c
> b/drivers/bus/vmbus/vmbus_common_uio.c
> index 8582e32c1d..7b9a8ef434 100644
> --- a/drivers/bus/vmbus/vmbus_common_uio.c
> +++ b/drivers/bus/vmbus/vmbus_common_uio.c
> @@ -69,8 +69,10 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
> *dev)
>  					     fd, offset,
>  					     uio_res->maps[i].size, 0);
> 
> -		if (mapaddr == uio_res->maps[i].addr)
> +		if (mapaddr == uio_res->maps[i].addr) {
> +			dev->resource[i].addr = mapaddr;
>  			continue;	/* successful map */
> +		}
> 
>  		if (mapaddr == MAP_FAILED)
>  			VMBUS_LOG(ERR,
> @@ -88,9 +90,9 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
> *dev)
>  	/* fd is not needed in secondary process, close it */
>  	close(fd);
> 
> -	dev->primary = uio_res->primary;
> -	if (!dev->primary) {
> -		VMBUS_LOG(ERR, "missing primary channel");
> +	if (vmbus_chan_create(dev, dev->relid, 0,
> +					dev->monitor_id, &dev->primary)) {
> +		VMBUS_LOG(ERR, "cannot create primary channel");
>  		return -1;
>  	}
> 
> @@ -211,8 +213,11 @@ vmbus_uio_unmap_resource(struct
> rte_vmbus_device *dev)
>  		return;
> 
>  	/* secondary processes - just free maps */
> -	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> +		if (dev->primary != NULL)
> +			rte_free(dev->primary);
>  		return vmbus_uio_unmap(uio_res);
> +	}
> 
>  	TAILQ_REMOVE(uio_res_list, uio_res, next);
> 
> --
> 2.17.1
Thomas Monjalon July 22, 2021, 7:15 p.m. UTC | #3 
21/07/2021 02:17, Long Li:
> From: Stephen Hemminger <sthemmin@microsoft.com>
> > 
> > Looks good, minor comment. You don't have to check for NULL before calling
> > rte_free().
> > Rte_free(NULL) is a NOP like free(NULL).
> > 
> > Sorry for top posting; but if you send to my Microsoft account you are stuck
> > with what Outlook can do...
> > 
> From: jerb <jonathan.erb@banduracyber.com>
> > 
> > Have secondary processes construct their own copy of primary channel with
> > own mappings.
> > 
> > Remove vmbus_channel primary ptr from struct mapped_vmbus_resource as
> > its not used.
> > 
> > Populate virtual memory address "addr" in struct rte_mem_resource for
> > secondary processes as netvsc will attempt to reference it thus causing a crash.
> > It was initialized for primary processes but not for secondary.
> > 
> > Signed-off-by: jerb <jonathan.erb@banduracyber.com>

Please give your complete name.

> Looks good.
> 
> This should also go to stable.

Please send a v2 with Fixes and Cc: stable@dpdk.org lines
as documented in the contributor's guide.
If you don't know how to do, the maintainers can help.

Thank you
diff mbox series

Patch

diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
index 528d60a42f..746212bd5f 100644
--- a/drivers/bus/vmbus/private.h
+++ b/drivers/bus/vmbus/private.h
@@ -42,7 +42,6 @@  struct mapped_vmbus_resource {
 
 	rte_uuid_t id;
 	int nb_maps;
-	struct vmbus_channel *primary;
 	struct vmbus_map maps[VMBUS_MAX_RESOURCE];
 	char path[PATH_MAX];
 };
diff --git a/drivers/bus/vmbus/vmbus_channel.c b/drivers/bus/vmbus/vmbus_channel.c
index f67f1c438a..119b9b367e 100644
--- a/drivers/bus/vmbus/vmbus_channel.c
+++ b/drivers/bus/vmbus/vmbus_channel.c
@@ -351,10 +351,8 @@  int rte_vmbus_chan_open(struct rte_vmbus_device *device,
 
 	err = vmbus_chan_create(device, device->relid, 0,
 				device->monitor_id, new_chan);
-	if (!err) {
+	if (!err)
 		device->primary = *new_chan;
-		uio_res->primary = *new_chan;
-	}
 
 	return err;
 }
diff --git a/drivers/bus/vmbus/vmbus_common_uio.c b/drivers/bus/vmbus/vmbus_common_uio.c
index 8582e32c1d..7b9a8ef434 100644
--- a/drivers/bus/vmbus/vmbus_common_uio.c
+++ b/drivers/bus/vmbus/vmbus_common_uio.c
@@ -69,8 +69,10 @@  vmbus_uio_map_secondary(struct rte_vmbus_device *dev)
 					     fd, offset,
 					     uio_res->maps[i].size, 0);
 
-		if (mapaddr == uio_res->maps[i].addr)
+		if (mapaddr == uio_res->maps[i].addr) {
+			dev->resource[i].addr = mapaddr;
 			continue;	/* successful map */
+		}
 
 		if (mapaddr == MAP_FAILED)
 			VMBUS_LOG(ERR,
@@ -88,9 +90,9 @@  vmbus_uio_map_secondary(struct rte_vmbus_device *dev)
 	/* fd is not needed in secondary process, close it */
 	close(fd);
 
-	dev->primary = uio_res->primary;
-	if (!dev->primary) {
-		VMBUS_LOG(ERR, "missing primary channel");
+	if (vmbus_chan_create(dev, dev->relid, 0,
+					dev->monitor_id, &dev->primary)) {
+		VMBUS_LOG(ERR, "cannot create primary channel");
 		return -1;
 	}
 
@@ -211,8 +213,11 @@  vmbus_uio_unmap_resource(struct rte_vmbus_device *dev)
 		return;
 
 	/* secondary processes - just free maps */
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+		if (dev->primary != NULL)
+			rte_free(dev->primary);
 		return vmbus_uio_unmap(uio_res);
+	}
 
 	TAILQ_REMOVE(uio_res_list, uio_res, next);