|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. Use-after-free risk in `bond_ethdev_link_update`**

The patch introduces `bond_link` as a local variable and copies `ethdev->data->dev_link` into it at function entry. However, the existing code reads from `bond_ctx->mode4.member_link` fields in the 8023AD case:

```c
bond_link.link_autoneg = bond_ctx->mode4.member_link.link_autoneg;
bond_link.link_duplex = bond_ctx->mode4.member_link.link_duplex;
```

If this function is called from a secondary process where `bond_ctx->mode4` may not be fully initialized or synchronized with the primary, accessing these fields could read stale or uninitialized data. The original code wrote directly to `ethdev->data->dev_link`, which in a secondary process shares memory with the primary. The new pattern copies into a local variable then writes back with `rte_eth_linkstatus_set()`, but it still reads from `bond_ctx->mode4.member_link` which may not be valid in the secondary.

**Suggested fix:** Verify that `bond_ctx->mode4.member_link` is valid before reading it in a secondary process context, or document that link update in secondary processes should not access mode-specific state. Alternatively, if link update is not supported in secondary processes, add a primary-process check at function entry and return `-ENOTSUP`.

**2. Missing release notes for internal API restriction**

The patch documents that "LACP runtime state queries are restricted to the primary process" and adds primary-process checks to several `rte_eth_bond_8023ad_*` functions. However, the release notes only mention "support for querying and detaching bonding devices from secondary processes" -- they do not mention that some query APIs are now restricted to the primary. Applications that previously called `rte_eth_bond_8023ad_member_info()` or `rte_eth_bond_8023ad_ext_collect_get()` from a secondary process will now get `-ENOTSUP`. This is a behavior change that should be noted.

**Suggested fix:** Update `doc/guides/rel_notes/release_26_11.rst` to clarify that LACP runtime state query APIs are restricted to the primary process.

---

### Warnings

**1. `bond_ethdev_link_update` error path inconsistency**

The function now uses a `goto out` pattern to centralize the `rte_eth_linkstatus_set()` call. However, on the error paths (e.g., when `link_update()` fails for a member), the function sets `bond_link.link_speed = RTE_ETH_SPEED_NUM_NONE` and then does `goto out`, which writes this partially-updated link state back to the ethdev. The original code returned 0 immediately on these errors without updating the link at all. This changes the observable behavior: after an error, the device link will now reflect the partial update (DOWN status with NONE speed) rather than retaining its previous state.

**Suggested approach:** If the intent is to mark the link as DOWN on error, document this behavior change in the commit message. If the intent is to preserve the original behavior (no update on error), return 0 directly from the error paths without going through `out`.

**2. Documentation claims secondary processes "do not support Rx and Tx" but does not disable them**

The guide states: "Rx and Tx are not supported on a bonding device in a secondary process; receive returns no packets and transmit drops packets." However, the `secondary_dev_ops` table does not install stub Rx/Tx burst functions to enforce this. The device will inherit whatever Rx/Tx function pointers were set by the primary process when the device was created. If an application in a secondary process calls `rte_eth_rx_burst()` or `rte_eth_tx_burst()`, those calls will execute the primary's burst functions, which may access shared queue state in undefined ways.

**Suggested fix:** Either install stub burst functions in `secondary_dev_ops` that return 0 (no packets) and document this clearly, or clarify in the documentation that the Rx/Tx function pointers remain active but behavior is undefined in a secondary process.

**3. `bond_ethdev_stop` returns `-ENOTSUP` in secondary but is listed in `secondary_dev_ops`**

The documentation states: "In a secondary process, `rte_eth_dev_stop()` returns `-ENOTSUP`". However, the `secondary_dev_ops` table does not include a `.dev_stop` entry, meaning it will be NULL and ethdev will return `-ENOTSUP` by default. This is correct behavior, but the statement in the docs could be clearer: rather than saying the function "returns" `-ENOTSUP`, say that stopping is not supported (ethdev rejects it).

**Suggested wording:** "In a secondary process, `rte_eth_dev_stop()` is not supported and returns `-ENOTSUP`."

---

### Info

**1. `bond_check_primary` helper implementation**

The helper logs an error and returns `false` when called from a secondary process. Consider whether a DEBUG-level log would be more appropriate, since attempting a control operation from a secondary process is a programming error in the calling application, not a runtime operational error. However, this is a minor style preference.

**2. Release notes wording**

The release notes say "Added support for querying and detaching bonding devices from secondary processes." The emphasis is on what is now supported. Consider also mentioning what is restricted (control operations, LACP runtime queries) to give a complete picture of the feature. This overlaps with Warning #2 above but is worth noting as a clarity improvement.

---

## Summary

- **Patch 1/2**: Clean, no issues.
- **Patch 2/2**: Two correctness concerns (link update accessing mode-specific state in secondary process, missing release notes for API restriction) and three documentation/behavior warnings (error path inconsistency, Rx/Tx not disabled, stop operation wording).