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

[Stephen Hemminger <stephen@networkplumber.org>]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

On Wed,  4 Feb 2026 13:31:29 +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.
> 
> ---
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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bundle-1748-review.md --]
[-- Type: text/markdown, Size: 10676 bytes --]

# 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