Re: [RFC PATCH v2] net/bonding: restrict secondary control operations

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun, 23 Aug 2026 10:16:25 -0500
Weijun Pan <wpan3636@gmail.com> 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 <wpan3636@gmail.com>
> ---

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.