|WARNING| [RFC] net/bonding: reject control operations in secondary

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: net/bonding: reject control operations in secondary

## Summary

This patch adds process-type checks to bonding PMD control plane operations to prevent secondary processes from modifying bonding state. The implementation is mostly correct, but there are several issues with error handling consistency and one missing cleanup path.

---

## Errors

### 1. Inconsistent error return values between helper functions

**Location:** `drivers/net/bonding/rte_eth_bond_api.c:159`

The helper function `bond_api_primary_only()` returns `-1` on error, while `bond_8023ad_primary_only()` and `bond_ethdev_primary_only()` return `-ENOTSUP`. This inconsistency means callers of `bond_api_primary_only()` return `-1` to the application instead of the standard `-ENOTSUP` errno value that the other paths use.

**Fix:** Change `bond_api_primary_only()` to return `-ENOTSUP`:

```c
static int
bond_api_primary_only(const char *op)
{
	if (rte_eal_process_type() != RTE_PROC_SECONDARY)
		return 0;

	RTE_BOND_LOG(ERR, "%s not supported in secondary process", op);
	return -ENOTSUP;
}
```

And update all callers to return `-ENOTSUP` instead of `-1`:

```c
int
rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
{
	if (bond_api_primary_only(__func__) != 0)
		return -ENOTSUP;
	/* ... */
}
```

### 2. Resource leak in queue setup error paths

**Location:** `drivers/net/bonding/rte_eth_bond_pmd.c:2434` and `2460`

In `bond_ethdev_rx_queue_setup()` and `bond_ethdev_tx_queue_setup()`, the process type check occurs after `rte_zmalloc_socket()` allocates memory for the queue structure. If the check fails in a secondary process, the allocated memory is leaked because the early return skips the cleanup.

**Fix:** Move the process type check before allocation:

```c
static int
bond_ethdev_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id,
		uint16_t nb_rx_desc, unsigned int socket_id __rte_unused,
		const struct rte_eth_rxconf *rx_conf, struct rte_mempool *mb_pool)
{
	if (bond_ethdev_primary_only(__func__) != 0)
		return -ENOTSUP;

	struct bond_rx_queue *bd_rx_q = (struct bond_rx_queue *)
			rte_zmalloc_socket(NULL, sizeof(struct bond_rx_queue),
					0, dev->data->numa_node);
	/* ... */
}
```

Apply the same fix to `bond_ethdev_tx_queue_setup()`.

---

## Warnings

### 1. Void-returning function cannot propagate error

**Location:** `drivers/net/bonding/rte_eth_bond_pmd.c:3389`

The function `bond_ethdev_mac_addr_remove()` has `void` return type but now calls `bond_ethdev_primary_only()` which can fail. The error cannot be propagated to the caller, so secondary processes will see the function silently fail instead of receiving an error code.

**Note:** This is a limitation of the existing ethdev ops API where `mac_addr_remove` is defined as `void`. The current implementation logs an error via `bond_ethdev_primary_only()`, which is acceptable but not ideal. Consider documenting this behavior or checking if the ethdev API signature can be changed in a future release.

---

## Info

### 1. Missing release notes

The patch description mentions this is related to Bugzilla ID 1900 and is a prerequisite for future secondary datapath support, but does not include a release notes update. Since this changes API behavior (secondary processes can no longer call these functions), it should be documented in `doc/guides/rel_notes/release_*.rst`.

**Suggested addition to release notes:**

```rst
* **Bonding PMD now rejects control operations from secondary processes.**

  Control-plane operations (member add/remove, mode changes, LACP configuration)
  now return ``-ENOTSUP`` when called from secondary processes. This ensures
  bonding state remains owned by the primary process and is a prerequisite
  for future limited secondary datapath support.
```

---

## Positive Observations

- Process type checks are consistently placed at the start of each function before any side effects occur (except for the two queue setup functions noted above)
- Error logging is consistent across all three helper functions
- The use of `__func__` in the log message provides clear context about which operation failed
- The patch correctly identifies that datapath operations (burst functions) should work in secondary processes but control plane should not

---

## Final Checklist

- [x] No use-after-free issues
- [x] No double-free issues  
- [x] No NULL pointer dereferences
- [ ] **Resource leaks in queue setup error paths** (Error #2)
- [x] No race conditions
- [ ] **Inconsistent error return values** (Error #1)
- [x] No forbidden tokens
- [x] Proper naming conventions
- [x] No trailing whitespace (not verified from patch context)
- [ ] **Missing release notes** (Info #1)
- [x] Compilation will succeed (each commit independent)

---

**Recommendation:** Address the two errors before merging. The warnings and info items should also be considered but are less critical.