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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary

This patch restricts bonding device control operations to the primary process, installing a reduced `dev_ops` table in secondary processes. The patch adds process-type checks throughout the bonding API and LACP functions.

---

## Errors

### 1. Resource leak on error path in `rte_eth_bond_create()`

**File:** `drivers/net/bonding/rte_eth_bond_api.c`

The new primary-process check at the start of `rte_eth_bond_create()` returns `-1` before any allocations occur, which is safe. However, the function already has several error paths that return without cleanup. While this patch doesn't introduce a new leak, the existing code should be verified for proper cleanup on all error paths after the `rte_malloc()` call (not visible in the patch context).

*(Note: This is not introduced by the patch but should be checked in the full function.)*

---

### 2. Return value inconsistency in LACP query functions

**File:** `drivers/net/bonding/rte_eth_bond_8023ad.c`

Functions `rte_eth_bond_8023ad_ext_distrib_get()` and `rte_eth_bond_8023ad_ext_collect_get()` return positive values on success (enabled state) but now return `-ENOTSUP` on primary-process check failure:

```c
int err;

err = bond_check_primary(__func__, -ENOTSUP);
if (err != 0)
    return err;  /* Returns -ENOTSUP */
```

Later in the same functions:

```c
return port->selected == SELECTED;  /* Returns 0 or 1 */
```

This creates ambiguity: callers cannot distinguish "feature disabled" (0) from "unsupported in secondary" (negative error). The existing return value semantics (boolean for enabled state) are unclear when mixed with error codes. However, since the patch documents that LACP queries are rejected in secondary processes, this is consistent with the design intent.

*(Acceptable per documentation, but note the semantic inconsistency.)*

---

### 3. Documentation claims "Rx and Tx are not supported" but provides no enforcement

**File:** `doc/guides/prog_guide/link_bonding_poll_mode_drv_lib.rst`

The documentation 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 `rx_pkt_burst` or `tx_pkt_burst` handlers. If these are left as `NULL`, the ethdev layer will reject queue setup or invoke undefined behavior. The patch should either:
- Install no-op burst functions that return 0 (Rx) or drop packets (Tx), OR
- Document that queue setup will fail in secondary processes (if that is the intended behavior)

The current state leaves the behavior undefined and inconsistent with the documentation.

---

## Warnings

### 1. Missing const on `bond_check_primary()` string parameter

**File:** `drivers/net/bonding/eth_bond_private.h`

```c
static inline int
bond_check_primary(const char *op, int err)
```

The `op` parameter is used only for logging and should be `const char *` (which it already is). However, the function does not need the operation name for any logic other than the error message, which is fine. No change needed.

*(Actually correct--withdrawing this item.)*

---

### 2. Release notes wording could be more specific

**File:** `doc/guides/rel_notes/release_26_11.rst`

The release note says:

> Supported query and detach paths remain available to secondary processes,
> while bonding device configuration changes are rejected.

This could clarify that **Rx/Tx on bonding devices are also unsupported in secondary processes**, as that is a significant functional limitation. The current wording focuses on control-plane operations but omits the data-plane restriction.

---

### 3. `secondary_dev_ops` omits several query operations present in `default_dev_ops`

**File:** `drivers/net/bonding/rte_eth_bond_pmd.c`

The `secondary_dev_ops` table includes:

```c
.dev_close         = bond_ethdev_close,
.dev_infos_get     = bond_ethdev_info,
.link_update       = bond_ethdev_link_update,
.stats_get         = bond_ethdev_stats_get,
.reta_query        = bond_ethdev_rss_reta_query,
.rss_hash_conf_get = bond_ethdev_rss_hash_conf_get,
```

But omits several other query-like operations present in `default_dev_ops`:

- `.stats_reset`
- `.promiscuous_enable` / `.promiscuous_disable` (query operations for promiscuous state)
- `.allmulticast_enable` / `.allmulticast_disable` (query operations for multicast state)
- `.mac_addr_add` / `.mac_addr_remove` (though these modify state)
- `.mtu_set` (control operation, correctly omitted)
- `.flow_ops_get` (query operation for flow capabilities)

Consider whether `stats_reset`, `promiscuous_enable/disable`, and `allmulticast_enable/disable` should be allowed in secondary processes. If these are purely read-only or local-state queries, they might be safe. If they modify shared state, they should remain restricted.

**Recommendation:** Review whether `stats_reset` and promiscuous/multicast state queries should be added to `secondary_dev_ops`, or document why they are intentionally omitted.

---

### 4. Documentation claims LACP queries are rejected, but some succeed in secondary

**File:** `doc/guides/prog_guide/link_bonding_poll_mode_drv_lib.rst` and `drivers/net/bonding/rte_eth_bond_8023ad.c`

The patch adds primary-process checks to all LACP runtime functions:

- `rte_eth_bond_8023ad_member_info()` -- **rejects secondary**
- `rte_eth_bond_8023ad_ext_collect_get()` -- **rejects secondary**
- `rte_eth_bond_8023ad_ext_distrib_get()` -- **rejects secondary**

However, the documentation does not explicitly list LACP queries as restricted. The release note mentions "supported query and detach paths remain available," but LACP runtime state queries (`member_info`, `collect_get`, `distrib_get`) are now rejected.

**Recommendation:** Clarify in the documentation that LACP runtime state queries are also restricted to the primary process, or explain why these are control operations rather than queries.

---

## Info

### 1. Error code choice for API functions

Most bonding API functions return `-1` on error (legacy style). The patch uses `-1` for `bond_check_primary()` return values in API functions, and `-ENOTSUP` in LACP functions. This is inconsistent but matches the existing pattern in each subsystem.

**Suggestion:** For long-term maintainability, consider standardizing on `-ENOTSUP` or another errno value across all bonding API functions, but this is outside the scope of this patch.

---

### 2. `bond_check_primary()` could use a macro for DRY

The pattern:

```c
int ret;

ret = bond_check_primary(__func__, -ENOTSUP);
if (ret != 0)
    return ret;
```

appears 15 times in the patch. A macro might reduce boilerplate:

```c
#define BOND_CHECK_PRIMARY(err) do { \
    int __ret = bond_check_primary(__func__, (err)); \
    if (__ret != 0) return __ret; \
} while (0)
```

Usage:

```c
BOND_CHECK_PRIMARY(-ENOTSUP);
```

However, the current approach is explicit and easier to grep. This is a style preference, not a requirement.

---

## Conclusion

**Must fix:**
- Error #3: Rx/Tx burst function handling in secondary processes (either install no-op handlers or document queue setup failure)

**Should consider:**
- Warning #3: Review and document why certain query operations are omitted from `secondary_dev_ops`
- Warning #4: Clarify that LACP runtime queries are restricted, or explain why they are control operations

**Minor:**
- Release note could mention Rx/Tx restrictions explicitly

The core design--restricting control operations to the primary process--is sound. The main correctness issue is the undefined behavior for Rx/Tx burst functions in secondary processes.