[dpdk-dev,1/1] bonding: fix error handling in rte_eth_bond_create()
diff mbox

Message ID 1438701978-29248-1-git-send-email-bernard.iremonger@intel.com
State Superseded, archived
Headers show

Commit Message

Bernard Iremonger Aug. 4, 2015, 3:26 p.m. UTC
if the name parameter to rte_eth_bond_create() was NULL,
there was a segmentation fault.

Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 drivers/net/bonding/rte_eth_bond_api.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Comments

Thomas Monjalon Aug. 4, 2015, 3:52 p.m. UTC | #1
2015-08-04 16:26, Bernard Iremonger:
> if the name parameter to rte_eth_bond_create() was NULL,
> there was a segmentation fault.

What is the root cause of the crash?
I guess it is rte_free(eth_dev->data->mac_addrs) because eth_dev is NULL.

[...]
> -err:
> -	rte_free(pci_dev);
> -	rte_free(internals);
> +err4:
>  	rte_free(eth_dev->data->mac_addrs);
> -
> +err3:
> +	rte_free(internals);
> +err2:
> +	rte_free(pci_dev);
> +err1:
>  	return -1;
>  }

rte_free should be harmless with NULL parameter.
It is easier to add "if (eth_dev != NULL)" than maintaining many error labels.

By the way, why rte_eth_dev_release_port() is not called?
Bernard Iremonger Aug. 5, 2015, 12:28 p.m. UTC | #2
Hi Thomas,


<snip>
 
> What is the root cause of the crash?
> I guess it is rte_free(eth_dev->data->mac_addrs) because eth_dev is NULL.

Yes, that is the roor cause.

> 
> [...]
> > -err:
> > -	rte_free(pci_dev);
> > -	rte_free(internals);
> > +err4:
> >  	rte_free(eth_dev->data->mac_addrs);
> > -
> > +err3:
> > +	rte_free(internals);
> > +err2:
> > +	rte_free(pci_dev);
> > +err1:
> >  	return -1;
> >  }
> 
> rte_free should be harmless with NULL parameter.
> It is easier to add "if (eth_dev != NULL)" than maintaining many error labels.

I will revise patch to remove multible error labels.

> 
> By the way, why rte_eth_dev_release_port() is not called?
I will add a call to rte_eth_dev_release_port().

Regards,

Bernard.

Patch
diff mbox

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 4ca26dd..ffac3c9 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -194,19 +194,19 @@  rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 
 	if (name == NULL) {
 		RTE_BOND_LOG(ERR, "Invalid name specified");
-		goto err;
+		goto err1;
 	}
 
 	if (socket_id >= number_of_sockets()) {
 		RTE_BOND_LOG(ERR,
 				"Invalid socket id specified to create bonded device on.");
-		goto err;
+		goto err1;
 	}
 
 	pci_dev = rte_zmalloc_socket(name, sizeof(*pci_dev), 0, socket_id);
 	if (pci_dev == NULL) {
 		RTE_BOND_LOG(ERR, "Unable to malloc pci dev on socket");
-		goto err;
+		goto err1;
 	}
 
 	pci_drv = &rte_bond_pmd.pci_drv;
@@ -214,14 +214,14 @@  rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 	internals = rte_zmalloc_socket(name, sizeof(*internals), 0, socket_id);
 	if (internals == NULL) {
 		RTE_BOND_LOG(ERR, "Unable to malloc internals on socket");
-		goto err;
+		goto err2;
 	}
 
 	/* reserve an ethdev entry */
 	eth_dev = rte_eth_dev_allocate(name, RTE_ETH_DEV_VIRTUAL);
 	if (eth_dev == NULL) {
 		RTE_BOND_LOG(ERR, "Unable to allocate rte_eth_dev");
-		goto err;
+		goto err3;
 	}
 
 	pci_dev->numa_node = socket_id;
@@ -239,6 +239,10 @@  rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 
 	eth_dev->data->mac_addrs = rte_zmalloc_socket(name, ETHER_ADDR_LEN, 0,
 			socket_id);
+	if (eth_dev->data->mac_addrs == NULL) {
+		RTE_BOND_LOG(ERR, "Unable to malloc mac_addrs");
+		goto err3;
+	}
 
 	eth_dev->data->dev_started = 0;
 	eth_dev->data->promiscuous = 0;
@@ -277,16 +281,18 @@  rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 	if (bond_ethdev_mode_set(eth_dev, mode)) {
 		RTE_BOND_LOG(ERR, "Failed to set bonded device %d mode too %d",
 				 eth_dev->data->port_id, mode);
-		goto err;
+		goto err4;
 	}
 
 	return eth_dev->data->port_id;
 
-err:
-	rte_free(pci_dev);
-	rte_free(internals);
+err4:
 	rte_free(eth_dev->data->mac_addrs);
-
+err3:
+	rte_free(internals);
+err2:
+	rte_free(pci_dev);
+err1:
 	return -1;
 }