Re: [PATCH v8 0/8] net/nfb: rework to real multiport

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 13 Feb 2026 19:53:09 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> 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 <spinler@cesnet.cz>
**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 |