|WARNING| [RFC, v5, 2/2] net/bonding: restrict secondary control operations

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/168632

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-30

# DPDK Patch Review

## Patch 1/2: net/bonding: skip unavailable member stats

### Errors

None.

### Warnings

None.

### Info

None.

---

## Patch 2/2: net/bonding: restrict secondary control operations

### Errors

**1. Resource leak on error path in `bond_ethdev_link_update()`**

The new `bond_link` variable is a stack-allocated structure.
After the loop that updates it, the function has three early-exit paths via `goto out`
before the final `rte_eth_linkstatus_set()` call.
If any of these error paths are taken, the caller receives stale link information
because `ethdev->data->dev_link` was not updated.

While this is not a traditional resource leak (no memory/fd/lock),
it is a correctness bug: the caller expects `rte_eth_link_get[_nowait]()` to fail
or return current link state, but instead gets whatever was in `ethdev->data->dev_link` before.

**Suggested fix**: On each `goto out` error path, call `rte_eth_linkstatus_set(ethdev, &bond_link)`
*before* the goto, or restructure to ensure the caller can distinguish error from success.
The current code unconditionally calls `rte_eth_linkstatus_set()` at the `out:` label,
but that means errors return 0 with potentially stale link state visible to the caller.

Example:

```c
if (ret < 0) {
	bond_link.link_speed = RTE_ETH_SPEED_NUM_NONE;
	bond_link.link_status = RTE_ETH_LINK_DOWN;  /* mark link down on error */
	RTE_BOND_LOG(ERR, "Member (port %u) link get failed: %s",
		bond_ctx->active_members[idx], rte_strerror(-ret));
	rte_eth_linkstatus_set(ethdev, &bond_link);
	return ret;  /* propagate error to caller */
}
```

Or set `bond_link.link_status = RTE_ETH_LINK_DOWN` before each `goto out`
so the caller at least sees link-down state instead of stale data.

---

**2. Statistics accumulation on uninitialized `member_stats` when `rte_eth_stats_get()` fails (from Patch 1)**

Patch 1 adds a `continue` when `rte_eth_stats_get()` returns non-zero,
skipping the accumulation loop body.
However, `struct rte_eth_stats member_stats;` is declared at the top of `bond_ethdev_stats_get()`
and is **not initialized**.
If `rte_eth_stats_get()` fails, `member_stats` contains stack garbage.

The `continue` prevents accumulation of that garbage, so Patch 1 is correct.
However, if `rte_eth_stats_get()` succeeds *partially* (fills some fields but not others)
or if future code accidentally removes the `continue`, uninitialized fields will be accumulated.

**Suggested fix**: Initialize `member_stats` to zero at declaration:

```c
struct rte_eth_stats member_stats = {0};
```

or use `memset(&member_stats, 0, sizeof(member_stats));` before the loop.

This is a **minor correctness issue** (Patch 1 already skips the bad case)
but prevents future bugs if the code is modified.

---

### Warnings

**1. Release notes claim "Added support for querying ... from secondary processes" but documentation says "Rx and Tx are not supported"**

The release note says:

> Added support for querying and detaching bonding devices from secondary processes.

The prog_guide addition says:

> Rx and Tx are not supported on a bonding device in a secondary process;
> receive returns no packets and transmit drops packets.

This implies that the Rx/Tx data-plane burst functions silently fail in secondary processes,
but the patch does not add any code to enforce this or document where this behavior is implemented.

**Suggested fix**: Either:
- Explicitly set `eth_dev->rx_pkt_burst` and `eth_dev->tx_pkt_burst` to stub functions
  that return 0 / drop packets in `bond_probe()` for secondary processes, or
- Document that the existing code already does this (if true), or
- Clarify in the release notes that only control-plane queries are added,
  and data-plane was already unavailable.

---

**2. `bond_ethdev_stop()` not in `secondary_dev_ops` but documentation says "stop ... restricted to primary"**

The `secondary_dev_ops` table includes only:
- `dev_close`
- `dev_infos_get`
- `link_update`
- `stats_get`
- `reta_query`
- `rss_hash_conf_get`
- `eth_dev_priv_dump`

The documentation (prog_guide) says:

> Control operations are restricted to the primary process. This includes ... starting or stopping the device ...

and:

> In a secondary process, `rte_eth_dev_stop()` returns `-ENOTSUP` ...

However, `dev_stop` is **not** in `secondary_dev_ops`, which means `rte_eth_dev_stop()` will return `-ENOTSUP`
because the ethdev layer sees a NULL `dev_ops->dev_stop`.
This is correct behavior (matches the documentation), but it is implicit rather than explicit.

**Suggested clarification**: The code is correct (ethdev returns `-ENOTSUP` when the op is missing),
but it might be clearer to add an explicit stub:

```c
static int
bond_ethdev_stop_secondary(__rte_unused struct rte_eth_dev *dev)
{
	return -ENOTSUP;
}
```

and include `.dev_stop = bond_ethdev_stop_secondary` in `secondary_dev_ops`.
This makes the restriction self-documenting in the code rather than relying on ethdev's implicit NULL check.

Not required, but improves readability.

---

### Info

**1. New `bond_check_primary()` helper is clear and concise**

The `bond_check_primary()` inline helper consistently enforces primary-only restrictions
across all control APIs. This is good defensive programming.

---

**2. Private dump restriction is well-documented**

The code restricts LACP dump to primary processes:

```c
if (internals->mode == BONDING_MODE_8023AD &&
		rte_eal_process_type() == RTE_PROC_PRIMARY)
	dump_lacp(dev->data->port_id, f);
```

and the prog_guide documents:

> Private dump is limited to shared bonding information and skips LACP runtime state in a secondary process.

This is correct and matches LACP state ownership restrictions.

---

## Summary

**Correctness issues requiring fixes:**
1. `bond_ethdev_link_update()` error paths do not properly signal failure to caller
   (early `goto out` paths return 0 but leave stale link state)
2. `member_stats` in `bond_ethdev_stats_get()` should be zero-initialized
   to prevent future bugs (current patch 1 is safe, but fragile)

**Documentation/clarity improvements:**
1. Clarify Rx/Tx behavior in secondary processes (where is it enforced?)
2. Consider explicit `-ENOTSUP` stub for `dev_stop` in `secondary_dev_ops`
   (current implicit NULL handling is correct but less obvious)