On Sun, 23 Aug 2026 10:16:25 -0500 Weijun Pan <[email protected]> wrote:
> The bonding PMD currently supports secondary attach with safe fallback > burst functions, but bonding configuration and LACP state are owned by > the primary process. > > Use a secondary-specific dev_ops table so unsupported operations are > rejected by ethdev before PMD callbacks can mutate shared device state. > Also reject bonding-specific control APIs from non-primary processes, > while keeping secondary detach and query paths available. > > This keeps secondary process behavior safe while leaving room for future > limited datapath support. > > Bugzilla ID: 1900 > > Signed-off-by: Weijun Pan <[email protected]> > --- More indepth AI review with Fable saw some possible issues. Review: [RFC PATCH v2] net/bonding: restrict secondary control operations Applies cleanly to main, builds with -Dwerror=true. The secondary dev_ops table is a good approach; everything left in it is read-only against shared memory, and bond_ethdev_close() already returns early for non-primary so dev_close stays safe. Warning: prog guide describes a secondary datapath that does not exist "Secondary process datapath support is limited and bonding mode specific ... unless support for the selected mode is explicitly documented." bond_probe() installs bond_ethdev_rx_secondary() (returns 0) and bond_ethdev_tx_secondary() (frees and returns nb_pkts) for every mode. There is no mode with datapath support. Say that plainly: Rx and Tx are not supported on a bonding device in a secondary process; receive returns no packets and transmit drops them. Warning: LACP query paths return zeroed state in a secondary process bond_mode_8023ad_ports[] is a plain global array, populated only in the primary. rte_eth_bond_8023ad_member_info(), _ext_collect_get() and _ext_distrib_get() validate against shared internals (which pass), then read actor/partner state from the secondary's untouched copy and return all zeros with rc 0. bond_ethdev_priv_dump(), kept in secondary_dev_ops, prints the same zeros through dump_lacp(). Since the patch's premise is that query paths are safe in a secondary, either give these three the same bond_8023ad_check_primary() guard (and drop eth_dev_priv_dump from secondary_dev_ops or make dump_lacp() skip in secondary), or document that LACP per-member state is only visible to the primary. Warning: rte_eth_bond_api.c: two blank lines after bond_api_check_primary(). checkpatch will flag it. Info: three spellings of the same test rte_eth_bond_pmd.c already open-codes rte_eal_process_type() in bond_ethdev_mode_set(), bond_ethdev_close(), bond_probe() and bond_remove(). This patch adds bond_process_is_primary() plus two near-identical logging wrappers with different return values (-1 and -ENOTSUP). The return values match each file's conventions, so not wrong, but one helper in eth_bond_private.h taking the error code would remove the duplication. Info: link_update writes shared state from the secondary bond_ethdev_link_update() assigns ethdev->data->dev_link fields directly. Keeping it in secondary_dev_ops means a secondary calling rte_eth_link_get() races the primary on that shared struct. Pre-existing behaviour, but worth a thought given the patch's "must not change device state" rule. ---------------------------------------------------------------------- Suggested commit message (the Bugzilla entry has the background; no need to restate it): net/bonding: restrict secondary control operations Bonding configuration and LACP state are owned by the primary process. Install a reduced dev_ops table in secondary processes so ethdev rejects control operations, and reject the bonding control API when called from a non-primary process. Query and detach remain available. Bugzilla ID: 1900 ---------------------------------------------------------------------- Suggested release note: * **Restricted bonding device control to the primary process.** Secondary processes can query and detach a bonding device but can no longer change its configuration.
