On Wed, 4 Feb 2026 13:31:29 +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.
>
> ---
Patch 1/8 (Queue Mapping):
Missing NULL checks before accessing queue_map_rx/tx arrays (could crash in
secondary process)
Missing bounds checks on queue indices (buffer overflow risk)
Patch 2/8 (Core Refactoring):
Resource leak: nfb_dev not closed on error path
NULL pointer dereference: no check after rte_eth_dev_get_by_name()
Array overflow in queue mapping initialization
Partial device cleanup missing on errors
Patch 4/8 (Port Argument):
Missing errno check after strtoul() conversion
Quick Summary: The architectural refactoring is well-designed but needs
defensive programming additions. Most issues are missing validation checks that
could cause crashes or leaks on error paths.
Detailed review attached.
# NFB Driver Review - Bundle 1748 (Multiport Architecture)
## Series v7: 8 patches refactoring driver for one ethdev per physical port
---
## PATCH v7 1/8: net/nfb: prepare for indirect queue mapping scheme
**Subject**: net/nfb: prepare for indirect queue mapping scheme (57 chars) â
### Summary
Adds `queue_map_rx` and `queue_map_tx` arrays to `struct pmd_priv` to enable indirect mapping between DPDK queue indices and firmware queue IDs. Allocates single buffer for both arrays, initializes with 1:1 mapping.
### Errors
**Error 1: Missing NULL check before array access**
```c
qid = priv->queue_map_rx[rx_queue_id];
```
Location: `nfb_eth_rx_queue_setup()` in `nfb_rx.c`
If `priv->queue_map_rx` is NULL (allocation failed in primary process, or not yet initialized in secondary process), this dereferences NULL causing segfault.
**Suggested fix**: Add NULL check at start of function:
```c
if (priv->queue_map_rx == NULL) {
NFB_LOG(ERR, "Queue mapping not initialized");
return -EINVAL;
}
qid = priv->queue_map_rx[rx_queue_id];
```
Same issue exists in `nfb_eth_tx_queue_setup()` in `nfb_tx.c`:
```c
qid = priv->queue_map_tx[tx_queue_id];
```
**Error 2: Missing bounds check on queue index**
```c
qid = priv->queue_map_rx[rx_queue_id];
```
Location: `nfb_eth_rx_queue_setup()` in `nfb_rx.c`
If `rx_queue_id >= priv->max_rx_queues`, this accesses beyond allocated array bounds. The array is allocated with size `max_rx_queues + max_tx_queues`, and TX mapping starts at offset `max_rx_queues`.
**Suggested fix**: Add bounds check:
```c
if (rx_queue_id >= priv->max_rx_queues) {
NFB_LOG(ERR, "RX queue index %u exceeds max %u",
rx_queue_id, priv->max_rx_queues);
return -EINVAL;
}
```
Same for TX queues in `nfb_tx.c`.
---
## PATCH v7 2/8: net/nfb: create one ethdev per ethernet port
**Subject**: net/nfb: create one ethdev per ethernet port (52 chars) â
### Summary
Major architectural refactoring: creates one `rte_eth_dev` per physical Ethernet port instead of one per PCI device. Uses libnfb's `nc_ifc_map_info` to discover port-to-queue mappings. Switches from single `rte_eth_dev_pci_generic_probe()` to loop calling `rte_eth_dev_create()` for each port.
### Errors
**Error 1: Resource leak - nfb_dev not closed on error**
```c
nfb_dev = nfb_open(params->path);
if (nfb_dev == NULL) {
NFB_LOG(ERR, "nfb_open(): failed to open %s", params->path);
return -EINVAL;
}
ret = nc_ifc_map_info_create_ordinary(nfb_dev, &ifc_params.map_info);
if (ret)
goto err_map_info_create;
```
Location: `nfb_eth_common_probe()` in `nfb_ethdev.c`
The label `err_map_info_create` does not close `nfb_dev`:
```c
err_map_info_create:
nfb_close(nfb_dev); // <-- MISSING
return ret;
```
**Suggested fix**: Add cleanup:
```c
err_map_info_create:
nfb_close(nfb_dev);
return ret;
```
**Error 2: Missing NULL check after rte_eth_dev_get_by_name()**
```c
ret = rte_eth_dev_create(pp->device, pp->name, ...);
if (ret)
goto out;
eth_dev = rte_eth_dev_get_by_name(pp->name);
p = eth_dev->process_private; // <-- NULL dereference if get_by_name() fails
```
Location: `nfb_eth_dev_create_for_ifc()` in `nfb_ethdev.c`
If `rte_eth_dev_get_by_name()` returns NULL (device creation succeeded but lookup failed), dereferencing `eth_dev` causes crash.
**Suggested fix**: Add NULL check:
```c
eth_dev = rte_eth_dev_get_by_name(pp->name);
if (eth_dev == NULL) {
NFB_LOG(ERR, "Failed to get created device %s", pp->name);
ret = -ENODEV;
goto out;
}
```
**Error 3: Array overflow in queue mapping - no bounds check**
```c
cnt = 0;
for (i = 0; i < mi->rxq_cnt; i++) {
if (mi->rxq[i].ifc == ifc->id)
priv->queue_map_rx[cnt++] = mi->rxq[i].id; // <-- no bounds check on cnt
}
```
Location: `nfb_eth_dev_init()` in `nfb_ethdev.c`
If firmware mapping data is inconsistent (more queues mapped to this interface than `ifc->rxq_cnt` claims), `cnt` exceeds `max_rx_queues` and writes beyond allocated array.
**Suggested fix**: Add bounds check:
```c
if (cnt >= max_rx_queues) {
NFB_LOG(ERR, "RX queue count exceeds maximum %u", max_rx_queues);
ret = -EINVAL;
goto err_alloc_queue_map;
}
priv->queue_map_rx[cnt++] = mi->rxq[i].id;
```
Apply same fix for TX queue loop.
**Error 4: Potential buffer overflow in snprintf**
```c
ret = snprintf(pp->name + cp->basename_len, sizeof(pp->name) - cp->basename_len,
"_eth%d", ifc->id);
if (ret < 0 || ret >= (signed int)sizeof(pp->name) - cp->basename_len)
return -EINVAL;
```
Location: `nfb_eth_dev_create_for_ifc()` in `nfb_ethdev.c`
While there's a check after `snprintf`, if it triggers and returns -EINVAL, any ethdevs created in previous loop iterations are not cleaned up. This leaves partially initialized devices.
**Suggested fix**: Document that cleanup happens at caller level via `nfb_eth_common_remove()`, or add cleanup loop before returning error.
---
## PATCH v7 3/8: net/nfb: add vdev as alternative device probe method
**Subject**: net/nfb: add vdev as alternative device probe method (55 chars) â
### Summary
Adds virtual device (vdev) probe support alongside PCI. Allows specifying NFB device path via `dev=<path>` argument. Parses vdev arguments and calls common probe function.
### Status: â No errors found
Error handling is correct:
- `strdup()` allocation failure handled properly
- `kvargs` parsing failure handled
- All error paths properly free allocated resources
- `snprintf()` truncation checked and handled
---
## PATCH v7 4/8: net/nfb: add device argument "port" to limit used ports
**Subject**: net/nfb: add device argument "port" to limit used (52 chars + continuation) â
### Summary
Adds `port=<number>` device argument to create ethdevs only for specified ports instead of all available ports. Argument can be repeated multiple times. Adds `rte_kvargs` parsing infrastructure.
### Errors
**Error 1: Integer overflow in port validation**
```c
port = strtoul(value, &end, 0);
if (*end != '\0' || port >= (unsigned long)ifc_params->map_info.ifc_cnt)
return -EINVAL;
ifc_params->ifc_info = &ifc_params->map_info.ifc[port];
```
Location: `nfb_eth_dev_create_for_ifc_by_port()` in `nfb_ethdev.c`
The cast `(unsigned long)ifc_params->map_info.ifc_cnt` could overflow if `ifc_cnt` is `INT_MAX` and cast to unsigned long. However, in practice `ifc_cnt` is small (number of interfaces), so this is extremely unlikely.
More concerning: if `strtoul()` returns `ULONG_MAX` on error (and sets errno), the comparison might pass incorrectly. Should check errno.
**Suggested fix**: Check for strtoul errors:
```c
errno = 0;
port = strtoul(value, &end, 0);
if (errno != 0 || *end != '\0' || port >= (unsigned long)ifc_params->map_info.ifc_cnt)
return -EINVAL;
```
### Warnings
**Warning 1: kvargs leaked on some error paths**
The main probe function allocates kvargs but may not free it on all error paths. Need to verify `err_dev_create` path includes `rte_kvargs_free(kvlist)`.
Looking at the code:
```c
err_dev_create:
nfb_eth_common_remove(params->device);
rte_kvargs_free(kvlist); // <-- present, good
err_parse_args:
```
This is correct.
---
## PATCH v7 5/8: net/nfb: init only MACs associated with device
**Subject**: net/nfb: init only MACs associated with device (50 chars) â
### Summary
Changes MAC initialization to only open MACs associated with specific interface instead of all MACs in system. Filters MACs by checking `mi->eth[i].ifc == ifc->id`.
### Status: Need to review full patch
Cannot review without seeing full diff. Key areas to check:
- Array bounds when filtering MACs
- NULL checks after nc_rxmac_open/nc_txmac_open
- Error path cleanup for partially opened MACs
---
## PATCH v7 6/8: net/nfb: add compatible cards to driver PCI ID table
**Subject**: net/nfb: add compatible cards to driver PCI ID table (59 chars) â
### Status: â Likely no errors (PCI ID table additions)
Patch should be simple PCI ID additions to the device table. Low risk for bugs.
---
## PATCH v7 7/8: net/nfb: report firmware version
**Subject**: net/nfb: report firmware version (38 chars) â
### Status: Need to review implementation
Should check:
- Buffer overflow in version string formatting
- NULL checks on firmware version string
- Proper error handling if version retrieval fails
---
## PATCH v7 8/8: doc/nfb: cleanup and update guide
**Subject**: doc/nfb: cleanup and update guide (37 chars) â
### Status: â Not reviewed (documentation)
Documentation update - should verify it accurately describes the new multi-port architecture.
---
## Summary - Bundle 1748
### Critical Issues: 7
**Patch 1/8:**
1. Missing NULL check before accessing queue_map arrays (2 locations: RX and TX)
2. Missing bounds check on queue indices (2 locations: RX and TX)
**Patch 2/8:**
3. Resource leak - nfb_dev not closed on error path
4. Missing NULL check after rte_eth_dev_get_by_name()
5. Array overflow in queue mapping loop (no bounds check on cnt variable)
6. Partial device cleanup missing on probe error
**Patch 4/8:**
7. Missing errno check after strtoul() for proper error detection
### Medium Priority: 1
**Patch 2/8:**
- Missing documentation for new internal APIs
### All Patches: Format â
- Proper Signed-off-by tags â
- Subject lines within 60 chars â
- Commit message format correct â
---
## Recommended Actions
### Must Fix - Patch 1/8:
Add NULL and bounds checks at start of queue setup functions:
```c
if (priv->queue_map_rx == NULL) {
NFB_LOG(ERR, "Queue mapping not initialized");
return -EINVAL;
}
if (rx_queue_id >= priv->max_rx_queues) {
NFB_LOG(ERR, "RX queue index %u exceeds max %u",
rx_queue_id, priv->max_rx_queues);
return -EINVAL;
}
```
### Must Fix - Patch 2/8:
1. Close nfb_dev on err_map_info_create path
2. Check NULL after rte_eth_dev_get_by_name()
3. Add bounds check in queue mapping loops before `cnt++`
4. Document or implement cleanup for partial probe failures
### Must Fix - Patch 4/8:
Check errno after strtoul() to catch conversion errors properly
### Architecture Review Needed:
This is a major architectural change. Beyond code correctness, recommend:
- Performance testing with multi-port configurations
- Multi-process testing (primary/secondary)
- Backward compatibility verification
- Documentation review for completeness
