bond: static analysis issues fix

PATCH Superseded Declan Doherty 3 participants 10 messages Dec 12, 2014 – Dec 17, 2014 5 messages not in the archive

lore

Share & tools

lore

Declan Doherty

author

2014-12-12 17:39 UTC #0 lore raw

PATCH 1/1 bond: static analysis issues fix

Fixes for link bonding library identified by static analysis tool

- Overflow check for active_slaves array in activate_slave function
- Allocation check of pci_id_table in rte_eth_bond_create
- Use of eth_dev pointer in mac_address_get/set before NULL check

Signed-off-by Declan Doherty <[email protected]>

---
 lib/librte_pmd_bond/rte_eth_bond_api.c | 12 ++++++++----
 lib/librte_pmd_bond/rte_eth_bond_pmd.c |  8 ++++----
 2 files changed, 12 insertions(+), 8 deletions(-)

lib/librte_pmd_bond/rte_eth_bond_api.c +8 -4

index ef5ddf4..9cb1c1f 100644
@@ -115,8 +115,11 @@ activate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id)
 	if (internals->mode == BONDING_MODE_8023AD)
 		bond_mode_8023ad_activate_slave(eth_dev, port_id);
 
-	internals->active_slaves[internals->active_slave_count] = port_id;
-	internals->active_slave_count++;
+	if (internals->active_slave_count <
+			RTE_DIM(internals->active_slaves) - 1) {
+		internals->active_slaves[internals->active_slave_count] = port_id;
+		internals->active_slave_count++;
+	}
 }
 
 void

@@ -144,7 +147,8 @@ deactivate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id)
 					sizeof(internals->active_slaves[0]));
 	}
 
-	internals->active_slave_count = active_count;
+	internals->active_slave_count = active_count < RTE_MAX_ETHPORTS ?
+			active_count : RTE_MAX_ETHPORTS - 1;
 
 	if (eth_dev->data->dev_started && internals->mode == BONDING_MODE_8023AD)
 		bond_mode_8023ad_start(eth_dev);

@@ -210,7 +214,7 @@ rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 		goto err;
 	}
 	pci_id_table = rte_zmalloc_socket(name, sizeof(*pci_id_table), 0, socket_id);
-	if (pci_drv == NULL) {
+	if (pci_id_table == NULL) {
 		RTE_BOND_LOG(ERR, "Unable to malloc pci_id_table on socket");
 		goto err;
 	}

lib/librte_pmd_bond/rte_eth_bond_pmd.c +4 -4

index 3db473b..bb4a537 100644
@@ -764,8 +764,6 @@ mac_address_get(struct rte_eth_dev *eth_dev, struct ether_addr *dst_mac_addr)
 {
 	struct ether_addr *mac_addr;
 
-	mac_addr = eth_dev->data->mac_addrs;
-
 	if (eth_dev == NULL) {
 		RTE_LOG(ERR, PMD, "%s: NULL pointer eth_dev specified\n", __func__);
 		return -1;

@@ -776,6 +774,8 @@ mac_address_get(struct rte_eth_dev *eth_dev, struct ether_addr *dst_mac_addr)
 		return -1;
 	}
 
+	mac_addr = eth_dev->data->mac_addrs;
+
 	ether_addr_copy(mac_addr, dst_mac_addr);
 	return 0;
 }

@@ -785,8 +785,6 @@ mac_address_set(struct rte_eth_dev *eth_dev, struct ether_addr *new_mac_addr)
 {
 	struct ether_addr *mac_addr;
 
-	mac_addr = eth_dev->data->mac_addrs;
-
 	if (eth_dev == NULL) {
 		RTE_BOND_LOG(ERR, "NULL pointer eth_dev specified");
 		return -1;

@@ -797,6 +795,8 @@ mac_address_set(struct rte_eth_dev *eth_dev, struct ether_addr *new_mac_addr)
 		return -1;
 	}
 
+	mac_addr = eth_dev->data->mac_addrs;
+
 	/* If new MAC is different to current MAC then update */
 	if (memcmp(mac_addr, new_mac_addr, sizeof(*mac_addr)) != 0)
 		memcpy(mac_addr, new_mac_addr, sizeof(*mac_addr));

Signature · 1 line

1.7.12.2

A message that is not in the archive (sent off-list or not yet received): 1418405982-21407-1-git-send-email-declan.doherty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

Wodkowski, PawelX

2014-12-12 18:30 UTC #2 lore raw

Quoted 42 lines

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Declan Doherty
Sent: Friday, December 12, 2014 6:40 PM
To: [email protected]
Subject: [dpdk-dev] [PATCH] bond: static analysis issues fix
Fixes for link bonding library identified by static analysis tool
- Overflow check for active_slaves array in activate_slave function
- Allocation check of pci_id_table in rte_eth_bond_create
- Use of eth_dev pointer in mac_address_get/set before NULL check
Signed-off-by Declan Doherty <[email protected]>
---
 lib/librte_pmd_bond/rte_eth_bond_api.c | 12 ++++++++----
 lib/librte_pmd_bond/rte_eth_bond_pmd.c |  8 ++++----
 2 files changed, 12 insertions(+), 8 deletions(-)
lib/librte_pmd_bond/rte_eth_bond_api.c +5 -2
index ef5ddf4..9cb1c1f 100644
@@ -115,8 +115,11 @@ activate_slave(struct rte_eth_dev *eth_dev, uint8_t
 port_id)
 	if (internals->mode == BONDING_MODE_8023AD)
 		bond_mode_8023ad_activate_slave(eth_dev, port_id);
 
-	internals->active_slaves[internals->active_slave_count] = port_id;
-	internals->active_slave_count++;
+	if (internals->active_slave_count <
+			RTE_DIM(internals->active_slaves) - 1) {
+		internals->active_slaves[internals->active_slave_count] =
 port_id;
+		internals->active_slave_count++;
+	}
 }
void

lib/librte_pmd_bond/rte_eth_bond_api.c deactivate_slave(struct rte_eth_dev *eth_dev, uint8_t · line 152

Quoted 7 lines

lib/librte_pmd_bond/rte_eth_bond_api.c +2 -1

@@ -144,7 +147,8 @@ deactivate_slave(struct rte_eth_dev *eth_dev, uint8_t
 port_id)
 					sizeof(internals->active_slaves[0]));
 	}
 
-	internals->active_slave_count = active_count;
+	internals->active_slave_count = active_count < RTE_MAX_ETHPORTS ?
+			active_count : RTE_MAX_ETHPORTS - 1;

Since port might not be added twice and active_slaves array is (should be)
proper size to contain every port you can add to bonding and in fact is
one element bigger and active_slave_count should newer overflow, those changes might only mask real problems in user application and/or library itself. I think if you want to make this static analysis tool happy it should be changed to RTE_VERIFY(), assert(), rte_panic() or something like that to indicate undefined state.

Pawel

Declan Doherty

author

2014-12-15 17:13 UTC #3 lore raw

PATCH v2 1/1 bond: static analysis issues fix

In reply to a message not in the archive

-v2:
Incorporates Pawel's comments regarding assertion's check on activate_slave array indexing

Fixes for link bonding library identified by static analysis tool

- Overflow assert for active_slaves array in activate_slave function
- Allocation check of pci_id_table in rte_eth_bond_create
- Use of eth_dev pointer in mac_address_get/set before NULL check

Signed-off-by Declan Doherty <[email protected]>

---
 lib/librte_pmd_bond/rte_eth_bond_api.c | 7 ++++++-
 lib/librte_pmd_bond/rte_eth_bond_pmd.c | 8 ++++----
 2 files changed, 10 insertions(+), 5 deletions(-)

lib/librte_pmd_bond/rte_eth_bond_api.c +6 -1

index ef5ddf4..87a6a23 100644
@@ -115,8 +115,12 @@ activate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id)
 	if (internals->mode == BONDING_MODE_8023AD)
 		bond_mode_8023ad_activate_slave(eth_dev, port_id);
 
+	RTE_VERIFY(internals->active_slave_count <
+			(RTE_DIM(internals->active_slaves) - 1));
+
 	internals->active_slaves[internals->active_slave_count] = port_id;
 	internals->active_slave_count++;
+
 }
 
 void

@@ -144,6 +148,7 @@ deactivate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id)
 					sizeof(internals->active_slaves[0]));
 	}
 
+	RTE_VERIFY(active_count < RTE_DIM(internals->active_slaves));
 	internals->active_slave_count = active_count;
 
 	if (eth_dev->data->dev_started && internals->mode == BONDING_MODE_8023AD)

@@ -210,7 +215,7 @@ rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 		goto err;
 	}
 	pci_id_table = rte_zmalloc_socket(name, sizeof(*pci_id_table), 0, socket_id);
-	if (pci_drv == NULL) {
+	if (pci_id_table == NULL) {
 		RTE_BOND_LOG(ERR, "Unable to malloc pci_id_table on socket");
 		goto err;
 	}

lib/librte_pmd_bond/rte_eth_bond_pmd.c +4 -4

index 3db473b..bb4a537 100644
@@ -764,8 +764,6 @@ mac_address_get(struct rte_eth_dev *eth_dev, struct ether_addr *dst_mac_addr)
 {
 	struct ether_addr *mac_addr;
 
-	mac_addr = eth_dev->data->mac_addrs;
-
 	if (eth_dev == NULL) {
 		RTE_LOG(ERR, PMD, "%s: NULL pointer eth_dev specified\n", __func__);
 		return -1;

@@ -776,6 +774,8 @@ mac_address_get(struct rte_eth_dev *eth_dev, struct ether_addr *dst_mac_addr)
 		return -1;
 	}
 
+	mac_addr = eth_dev->data->mac_addrs;
+
 	ether_addr_copy(mac_addr, dst_mac_addr);
 	return 0;
 }

@@ -785,8 +785,6 @@ mac_address_set(struct rte_eth_dev *eth_dev, struct ether_addr *new_mac_addr)
 {
 	struct ether_addr *mac_addr;
 
-	mac_addr = eth_dev->data->mac_addrs;
-
 	if (eth_dev == NULL) {
 		RTE_BOND_LOG(ERR, "NULL pointer eth_dev specified");
 		return -1;

@@ -797,6 +795,8 @@ mac_address_set(struct rte_eth_dev *eth_dev, struct ether_addr *new_mac_addr)
 		return -1;
 	}
 
+	mac_addr = eth_dev->data->mac_addrs;
+
 	/* If new MAC is different to current MAC then update */
 	if (memcmp(mac_addr, new_mac_addr, sizeof(*mac_addr)) != 0)
 		memcpy(mac_addr, new_mac_addr, sizeof(*mac_addr));

Signature · 1 line

1.7.12.2

A message that is not in the archive (sent off-list or not yet received): 1418663630-27409-1-git-send-email-declan.doherty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

Wodkowski, PawelX

Acked-by

2014-12-16 08:50 UTC #5 lore raw

Quoted 11 lines

-----Original Message-----
From: Doherty, Declan
Sent: Monday, December 15, 2014 6:14 PM
To: [email protected]
Cc: Wodkowski, PawelX; Doherty, Declan
Subject: [PATCH v2] bond: static analysis issues fix
-v2:
Incorporates Pawel's comments regarding assertion's check on activate_slave array indexing

Quoted 9 lines

Fixes for link bonding library identified by static analysis tool
- Overflow assert for active_slaves array in activate_slave function
- Allocation check of pci_id_table in rte_eth_bond_create
- Use of eth_dev pointer in mac_address_get/set before NULL check
Signed-off-by Declan Doherty <[email protected]>
---

Acked-by Wodkowski, Pawel <[email protected]>

Thomas Monjalon

2014-12-16 17:31 UTC #6 lore raw

In reply to a message not in the archive

2014-12-15 17:13, Declan Doherty:

Quoted 2 lines

-v2:
Incorporates Pawel's comments regarding assertion's check on activate_slave array indexing

Changelog should be below three dashes to be excluded from git history.

Quoted 5 lines

Fixes for link bonding library identified by static analysis tool
- Overflow assert for active_slaves array in activate_slave function
- Allocation check of pci_id_table in rte_eth_bond_create
- Use of eth_dev pointer in mac_address_get/set before NULL check

Please send 3 patches. 1 bug = 1 fix with its explanation. The main advantage is to help referencing regressions.

Quoted 1 line

Signed-off-by Declan Doherty <[email protected]>

[...]

lib/librte_pmd_bond/rte_eth_bond_api.c activate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id) · line 124

Quoted 13 lines

lib/librte_pmd_bond/rte_eth_bond_api.c +4 -0

@@ -115,8 +115,12 @@ activate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id)
 	if (internals->mode == BONDING_MODE_8023AD)
 		bond_mode_8023ad_activate_slave(eth_dev, port_id);
 
+	RTE_VERIFY(internals->active_slave_count <
+			(RTE_DIM(internals->active_slaves) - 1));
+
 	internals->active_slaves[internals->active_slave_count] = port_id;
 	internals->active_slave_count++;
+
 }

Why a blank line here?

Signature · 1 line

Thomas

Declan Doherty

author

2014-12-17 11:46 UTC #7 lore raw

v3 cover bond: static analysis issues fix

In reply to a message not in the archive

-v3:
Split patches

-v2:
Incorporates Pawel's comments regarding assertion's check on activate_slave array indexing

Fixes for link bonding library identified by static analysis tool

Declan Doherty (3):
bond: add bounds check before assigning active slave count value
bond: fix pci_id_table allocation check in rte_eth_bond_create
bond: eth_dev parameter used before NULL check mac_address_get/set

 lib/librte_pmd_bond/rte_eth_bond_api.c | 6 +++++-
 lib/librte_pmd_bond/rte_eth_bond_pmd.c | 8 ++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

Signature · 1 line

1.7.12.2

A message that is not in the archive (sent off-list or not yet received): 1418816819-13517-1-git-send-email-declan.doherty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

Declan Doherty

author

2014-12-17 11:46 UTC #9 lore raw

PATCH v3 1/3 bond: add bounds check before assigning active slave count value

Signed-off-by Declan Doherty <[email protected]>

---
 lib/librte_pmd_bond/rte_eth_bond_api.c | 4 ++++
 1 file changed, 4 insertions(+)

lib/librte_pmd_bond/rte_eth_bond_api.c +4 -0

index ef5ddf4..b124784 100644
@@ -115,6 +115,9 @@ activate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id)
 	if (internals->mode == BONDING_MODE_8023AD)
 		bond_mode_8023ad_activate_slave(eth_dev, port_id);
 
+	RTE_VERIFY(internals->active_slave_count <
+			(RTE_DIM(internals->active_slaves) - 1));
+
 	internals->active_slaves[internals->active_slave_count] = port_id;
 	internals->active_slave_count++;
 }

@@ -144,6 +147,7 @@ deactivate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id)
 					sizeof(internals->active_slaves[0]));
 	}
 
+	RTE_VERIFY(active_count < RTE_DIM(internals->active_slaves));
 	internals->active_slave_count = active_count;
 
 	if (eth_dev->data->dev_started && internals->mode == BONDING_MODE_8023AD)

Signature · 1 line

1.7.12.2

A message that is not in the archive (sent off-list or not yet received): 1418816819-13517-2-git-send-email-declan.doherty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

Declan Doherty

author

2014-12-17 11:46 UTC #11 lore raw

PATCH v3 2/3 bond: fix pci_id_table allocation check in rte_eth_bond_create

Signed-off-by Declan Doherty <[email protected]>

---
 lib/librte_pmd_bond/rte_eth_bond_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

lib/librte_pmd_bond/rte_eth_bond_api.c +1 -1

index b124784..c2a99a3 100644
@@ -214,7 +214,7 @@ rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 		goto err;
 	}
 	pci_id_table = rte_zmalloc_socket(name, sizeof(*pci_id_table), 0, socket_id);
-	if (pci_drv == NULL) {
+	if (pci_id_table == NULL) {
 		RTE_BOND_LOG(ERR, "Unable to malloc pci_id_table on socket");
 		goto err;
 	}

Signature · 1 line

1.7.12.2

A message that is not in the archive (sent off-list or not yet received): 1418816819-13517-3-git-send-email-declan.doherty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

Declan Doherty

author

2014-12-17 11:46 UTC #13 lore raw

PATCH v3 3/3 bond: eth_dev parameter used before NULL check in mac_address_get/set

Signed-off-by Declan Doherty <[email protected]>

---
 lib/librte_pmd_bond/rte_eth_bond_pmd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

lib/librte_pmd_bond/rte_eth_bond_pmd.c +4 -4

index 3db473b..bb4a537 100644
@@ -764,8 +764,6 @@ mac_address_get(struct rte_eth_dev *eth_dev, struct ether_addr *dst_mac_addr)
 {
 	struct ether_addr *mac_addr;
 
-	mac_addr = eth_dev->data->mac_addrs;
-
 	if (eth_dev == NULL) {
 		RTE_LOG(ERR, PMD, "%s: NULL pointer eth_dev specified\n", __func__);
 		return -1;

@@ -776,6 +774,8 @@ mac_address_get(struct rte_eth_dev *eth_dev, struct ether_addr *dst_mac_addr)
 		return -1;
 	}
 
+	mac_addr = eth_dev->data->mac_addrs;
+
 	ether_addr_copy(mac_addr, dst_mac_addr);
 	return 0;
 }

@@ -785,8 +785,6 @@ mac_address_set(struct rte_eth_dev *eth_dev, struct ether_addr *new_mac_addr)
 {
 	struct ether_addr *mac_addr;
 
-	mac_addr = eth_dev->data->mac_addrs;
-
 	if (eth_dev == NULL) {
 		RTE_BOND_LOG(ERR, "NULL pointer eth_dev specified");
 		return -1;

@@ -797,6 +795,8 @@ mac_address_set(struct rte_eth_dev *eth_dev, struct ether_addr *new_mac_addr)
 		return -1;
 	}
 
+	mac_addr = eth_dev->data->mac_addrs;
+
 	/* If new MAC is different to current MAC then update */
 	if (memcmp(mac_addr, new_mac_addr, sizeof(*mac_addr)) != 0)
 		memcpy(mac_addr, new_mac_addr, sizeof(*mac_addr));

Signature · 1 line

1.7.12.2

Thomas Monjalon

Applied

2014-12-17 23:43 UTC #14 lore raw

In reply to a message not in the archive

Quoted 13 lines

-v3:
Split patches
-v2:
Incorporates Pawel's comments regarding assertion's check on activate_slave array indexing
Fixes for link bonding library identified by static analysis tool
- Overflow assert for active_slaves array in activate_slave function
- Allocation check of pci_id_table in rte_eth_bond_create
- Use of eth_dev pointer in mac_address_get/set before NULL check

Quoted 4 lines

Declan Doherty (3):
bond: add bounds check before assigning active slave count value
bond: fix pci_id_table allocation check in rte_eth_bond_create
bond: eth_dev parameter used before NULL check mac_address_get/set

Applied

Thanks

Signature · 1 line

Thomas