[dpdk-dev,v2] bond: static analysis issues fix
Commit Message
-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 <declan.doherty@intel.com>
---
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(-)
Comments
> -----Original Message-----
> From: Doherty, Declan
> Sent: Monday, December 15, 2014 6:14 PM
> To: dev@dpdk.org
> 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
>
> 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 <declan.doherty@intel.com>
> ---
>
Acked-by: Wodkowski, Pawel <pawelx.wodkowski@intel.com>
2014-12-15 17:13, Declan Doherty:
> -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.
> 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.
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
[...]
> --- a/lib/librte_pmd_bond/rte_eth_bond_api.c
> +++ b/lib/librte_pmd_bond/rte_eth_bond_api.c
> @@ -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?
-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
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(-)
> -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
>
>
> 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
@@ -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;
}
@@ -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));