dpaa2_dev_mac_setup_stats() allocates and frees DMA buffers on every
xstats_get() call. This is wasteful and not thread-safe: concurrent
callers can overwrite priv pointers, leading to use-after-free. It
also does not check for allocation failure before passing IOVAs to
the MC firmware.
Move the DMA buffer allocation to dpaa2_dev_init() (only when MC
version supports MAC stats) and free them in dpaa2_dev_close(). In
xstats_get(), just check if the buffers were allocated.
Not tested, found by code review.
Fixes: d1cdef2ab5 ("net/dpaa2: add dpmac counters in xstats")
Cc: [email protected]
Reported-by: Stephen Hemminger <[email protected]>
Signed-off-by: Maxime Leroy <[email protected]>
---
drivers/net/dpaa2/dpaa2_ethdev.c | 65 +++++++++++++++-----------------
1 file changed, 30 insertions(+), 35 deletions(-)
diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 3875164794..d5e30ce5fb 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -1550,6 +1550,11 @@ dpaa2_dev_close(struct rte_eth_dev *dev)
rte_free(priv->extract.qos_extract_param);
+ rte_free(priv->cnt_idx_dma_mem);
+ rte_free(priv->cnt_values_dma_mem);
+ priv->cnt_idx_dma_mem = NULL;
+ priv->cnt_values_dma_mem = NULL;
+
DPAA2_PMD_INFO("%s: netdev deleted", dev->data->name);
return 0;
}
@@ -1905,7 +1910,6 @@ dpaa2_dev_xstats_get(struct rte_eth_dev *dev,
unsigned int i = 0, j = 0, num = RTE_DIM(dpaa2_xstats_strings);
struct dpaa2_dev_priv *priv = dev->data->dev_private;
union dpni_statistics value[13] = {};
- struct mc_version mc_ver_info = {0};
struct dpni_rx_tc_policing_cfg cfg;
uint8_t page_id, stats_id;
uint64_t *cnt_values;
@@ -1976,44 +1980,24 @@ dpaa2_dev_xstats_get(struct rte_eth_dev *dev,
i++;
}
- if (mc_get_version(dpni, CMD_PRI_LOW, &mc_ver_info))
- DPAA2_PMD_WARN("Unable to obtain MC version");
-
- /* mac_statistics supported on MC version > 10.39.0 */
- if (mc_ver_info.major >= MC_VER_MAJOR &&
- mc_ver_info.minor >= MC_VER_MINOR &&
- mc_ver_info.revision > 0) {
- dpaa2_dev_mac_setup_stats(dev);
- retcode = dpni_get_mac_statistics(dpni, CMD_PRI_LOW,
priv->token,
- priv->cnt_idx_iova,
- priv->cnt_values_iova,
- DPAA2_MAC_NUM_STATS);
- if (retcode) {
- while (i >= (num - DPAA2_MAC_NUM_STATS) && i < num) {
- xstats[i].id = i;
- xstats[i].value = 0;
- i++;
- }
- }
- if (!retcode) {
- cnt_values = priv->cnt_values_dma_mem;
- while (i >= (num - DPAA2_MAC_NUM_STATS) && i < num) {
- /* mac counters value */
- xstats[i].id = i;
- xstats[i].value =
rte_le_to_cpu_64(*cnt_values++);
- i++;
- }
- }
- rte_free(priv->cnt_values_dma_mem);
- rte_free(priv->cnt_idx_dma_mem);
- priv->cnt_idx_dma_mem = NULL;
- priv->cnt_values_dma_mem = NULL;
- } else {
+ if (priv->cnt_idx_dma_mem &&
+ !dpni_get_mac_statistics(dpni, CMD_PRI_LOW, priv->token,
+ priv->cnt_idx_iova,
+ priv->cnt_values_iova,
+ DPAA2_MAC_NUM_STATS)) {
+ cnt_values = priv->cnt_values_dma_mem;
while (i >= (num - DPAA2_MAC_NUM_STATS) && i < num) {
xstats[i].id = i;
- xstats[i].value = 0;
+ xstats[i].value = rte_le_to_cpu_64(*cnt_values++);
i++;
}
+ return i;
+ }
+
+ while (i >= (num - DPAA2_MAC_NUM_STATS) && i < num) {
+ xstats[i].id = i;
+ xstats[i].value = 0;
+ i++;
}
return i;
@@ -3155,6 +3139,17 @@ dpaa2_dev_init(struct rte_eth_dev *eth_dev)
priv->speed_capa = dpaa2_dev_get_speed_capability(eth_dev);
+ /* mac_statistics supported on MC version > 10.39.0 */
+ {
+ struct mc_version mc_ver_info = {0};
+
+ if (!mc_get_version(dpni_dev, CMD_PRI_LOW, &mc_ver_info) &&
+ mc_ver_info.major >= MC_VER_MAJOR &&
+ mc_ver_info.minor >= MC_VER_MINOR &&
+ mc_ver_info.revision > 0)
+ dpaa2_dev_mac_setup_stats(eth_dev);
+ }
+
return 0;
init_err:
dpaa2_dev_close(eth_dev);
--
2.43.0
