On Fri, 13 Feb 2026 19:53:09 +0100 [email protected] wrote: > From: Martin Spinler <[email protected]> > > This series implements real multiport for better user experience. > > The existing driver creates one ethdev/port for one PCI device. > As the CESNET-NDK based cards aren't capable to represent each > Ethernet port by own PCI device, new driver implementation > processes real port configuration from firmware/card and switches > from rte_eth_dev_pci_generic_probe to multiple rte_eth_dev_create calls. > > ---
There is one copy-paste bug (found by AI review). I can fix that during merge. Summary of Findings Patch Severity Issue 2/8 Error TX queue map bounds check uses max_rx_queues instead of max_tx_queues — copy-paste bug 2/8 Warning __rte_internal / RTE_EXPORT_INTERNAL_SYMBOL misuse for driver-internal functions 5/8 Warning err_map_inconsistent error path leaks already-opened MAC handles Full report # Deep Dive Review: net/nfb multiport patch series (v8, 8 patches) **Author:** Martin Spinler <[email protected]> **Series:** [PATCH v8 1/8] through [PATCH v8 8/8] **Reviewer:** AI review per AGENTS.md guidelines --- ## Changes from v7 Several issues from the v7 review have been addressed: - **Fixed:** The double `TAILQ_REMOVE` bug — `nfb_eth_common_remove()` no longer removes from the list; only `nfb_eth_dev_uninit()` does. - **Fixed:** Buffer overflow in queue map filling — bounds checks added with `cnt >= max_rx_queues` guard and `goto err_queue_map`. - **Fixed:** `strtoul` base changed from 0 to 10, preventing octal/hex port numbers. - **Fixed:** Bounds check added in `nfb_nc_eth_init()` for MAC mapping consistency. - **New:** Added `priv->ready` flag for secondary process safety in patch 1. - **New:** Added bounds checks in `nfb_eth_rx_queue_setup` and `nfb_eth_tx_queue_setup` (`rx_queue_id >= priv->max_rx_queues`). - **New:** Added NULL check on `rte_eth_dev_get_by_name()` return in `nfb_eth_dev_create_for_ifc`. - **New:** Added `ifc_params.ret` field to pass back error codes from `rte_kvargs_process` callback. - **New:** Added `errno = 0` before `strtoul` and `errno` check after. --- ## Patch 2/8: net/nfb: create one ethdev per ethernet port ### Error: TX queue map bounds check uses `max_rx_queues` instead of `max_tx_queues` In the TX queue mapping loop: ```c cnt = 0; for (i = 0; i < mi->txq_cnt; i++) { if (mi->txq[i].ifc == ifc->id) { if (cnt >= max_rx_queues) { /* <--- BUG: should be max_tx_queues */ NFB_LOG(ERR, "TX queue mapping inconsistent"); ret = -EINVAL; goto err_queue_map; } priv->queue_map_tx[cnt++] = mi->txq[i].id; } } ``` The bounds check compares `cnt` against `max_rx_queues` but this is filling `queue_map_tx` which was allocated with `max_tx_queues` slots. If `max_tx_queues < max_rx_queues`, this allows writing past the end of the TX portion of the buffer. If `max_tx_queues > max_rx_queues`, it rejects valid mappings prematurely. **Suggested fix:** Change `max_rx_queues` to `max_tx_queues`: ```c if (cnt >= max_tx_queues) { ``` ### Warning: `__rte_internal` + `RTE_EXPORT_INTERNAL_SYMBOL` for driver-internal functions `nfb.h` declares `nfb_eth_common_probe` and `nfb_eth_common_remove` with `__rte_internal`, and the `.c` file uses `RTE_EXPORT_INTERNAL_SYMBOL`. These functions are shared between `nfb_ethdev.c` and `nfb_vdev.c` within the same driver — they are not cross-library internal symbols. `__rte_internal` is meant for functions exported between DPDK libraries/drivers via the linker map, not for functions shared within a single PMD. Plain `extern` declarations would be more appropriate. --- ## Patch 5/8: net/nfb: init only MACs associated with device ### Warning: `err_map_inconsistent` path doesn't close already-opened MACs In `nfb_nc_eth_init()`, if the bounds check triggers: ```c if (rxm >= ifc->eth_cnt || txm >= ifc->eth_cnt) { NFB_LOG(ERR, "MAC mapping inconsistent"); ret = -EINVAL; goto err_map_inconsistent; } ``` The error path frees `intl->txmac` and `intl->rxmac` arrays but does not close any MAC handles already opened via `nc_rxmac_open` / `nc_txmac_open` in previous loop iterations. These handles would be leaked. **Suggested fix:** Close all opened MACs before freeing the arrays: ```c err_map_inconsistent: for (int j = 0; j < txm; j++) nc_txmac_close(intl->txmac[j]); for (int j = 0; j < rxm; j++) nc_rxmac_close(intl->rxmac[j]); free(intl->txmac); err_alloc_txmac: free(intl->rxmac); err_alloc_rxmac: return ret; ``` Alternatively, set `intl->max_rxmac = rxm; intl->max_txmac = txm;` before the goto and call `nfb_nc_eth_deinit(intl)` instead. --- ## Summary of Findings | Patch | Severity | Issue | |-------|----------|-------| | 2/8 | **Error** | TX queue map bounds check uses `max_rx_queues` instead of `max_tx_queues` — copy-paste bug | | 2/8 | Warning | `__rte_internal` / `RTE_EXPORT_INTERNAL_SYMBOL` misuse for driver-internal functions | | 5/8 | Warning | `err_map_inconsistent` error path leaks already-opened MAC handles |
