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

[Stephen Hemminger]

On Wed, 26 Aug 2026 11:10:09 -0500
Weijun Pan <wpan3636 at gmail.com> wrote:

> 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
> 
> Signed-off-by: Weijun Pan <wpan3636 at gmail.com>
> ---

I would like to see link update use rte_eth_linkstatus_set() helper.
And do another check that documentation matches reality after this patch.


Overly verbose (sorry) AI review was:

Applies cleanly to main (d55ccd4), diffstat matches, and builds with
-Dwerror=true for net/bonding.  checkpatches.sh is clean.

Fixed since v2
--------------

- rte_eth_bond_8023ad_member_info(), _ext_collect_get() and
  _ext_distrib_get() are now rejected in a secondary, so they no
  longer return all-zero LACP state read out of the process-local
  bond_mode_8023ad_ports[].

- eth_dev_priv_dump is no longer in the secondary ops table, so
  dump_lacp() cannot walk that array from a secondary either.

- The two near-identical primary-process helpers are collapsed into
  bond_check_primary().

- The stray double blank line in rte_eth_bond_member_add() is gone.

- The programmer's guide now states the secondary datapath is a
  stub, which matches bond_ethdev_rx_secondary() and
  bond_ethdev_tx_secondary().

The set of API gated in this version looks right to me.  Every
mutator in rte_eth_bond_api.c and rte_eth_bond_8023ad.c is covered,
rte_eth_bond_free() is deliberately left open for detach, and the
getters that are left ungated (mode_get, primary_get, members_get,
xmit_policy_get, 8023ad_conf_get, 8023ad_agg_selection_get, ...) all
read bond_dev_private, which is in shared memory.  Omitting dev_stop
from secondary_dev_ops is also correct: rte_eth_dev_stop() writes
dev->data->dev_started, and rte_eth_dev_close() has its own
secondary path so detach still works.

Warnings
--------

1. link_update is retained in secondary_dev_ops, so dev_link is now
   written from two processes without using the ethdev accessors.

   bond_ethdev_link_update() assigns ethdev->data->dev_link.* field by
   field, and dev_link lives in the shared rte_eth_dev_data.  With
   .link_update present, any rte_eth_link_get() in a secondary
   republishes the primary's link record:

	if (dev->data->dev_conf.intr_conf.lsc && dev->data->dev_started)
		rte_eth_linkstatus_get(dev, eth_link);
	else {
		if (dev->dev_ops->link_update == NULL)
			return -ENOTSUP;
		dev->dev_ops->link_update(dev, 1);
		*eth_link = dev->data->dev_link;
	}

   Readers on the LSC path use rte_eth_linkstatus_get(), a 64-bit
   atomic load, so a reader can observe a torn combination of
   link_status, link_speed and link_duplex.

   The driver should use rte_eth_linkstatus_set() rather than touching
   dev->data->dev_link directly.  That is one atomic exchange of the
   whole word, which closes the window on the primary path as well and
   makes the secondary's recomputation harmless -- it derives the same
   value from the same shared member state.  Build the result locally
   and publish once:

	struct rte_eth_link link;

	rte_eth_linkstatus_get(ethdev, &link);
	link.link_speed = RTE_ETH_SPEED_NUM_NONE;

	if (ethdev->data->dev_started == 0 ||
			bond_ctx->active_member_count == 0) {
		link.link_status = RTE_ETH_LINK_DOWN;
		goto out;
	}

	link.link_status = RTE_ETH_LINK_UP;
	...
   out:
	rte_eth_linkstatus_set(ethdev, &link);
	return 0;

   Seeding from rte_eth_linkstatus_get() keeps link_duplex and
   link_autoneg for the modes that never set them; the interior
   "return 0" paths become "goto out".

   The rest of the driver has the same problem and is worth a
   preparatory patch: bond_ethdev_start() and bond_ethdev_stop() set
   link_status directly (rte_eth_bond_pmd.c:2068, 2220), as does
   bond_ethdev_lsc_event_callback() at 3101, and
   bond_ethdev_member_link_status_change() writes link_autoneg and
   link_duplex at 1439.  Nothing in bonding currently uses the
   accessors.

2. The documentation does not match what the code now rejects.

   The new prog_guide paragraph lists configure, start/stop, queue
   setup, member changes, mode, RSS, MAC, MTU and rte_flow.  It omits
   everything else the patch turns into an error in a secondary:

	- all of rte_eth_bond_8023ad_* except conf_get and
	  agg_selection_get, including member_info(),
	  ext_collect_get() and ext_distrib_get(), which are queries
	- dedicated queue enable/disable
	- xmit policy, link monitoring interval, link up/down
	  propagation delays, primary member selection
	- stats reset, promiscuous and allmulticast, VLAN filter,
	  private dump

   The commit message and the release note both say "query ... remain
   available", and the guide says "supported query and detach
   operations", so a reader would reasonably conclude
   rte_eth_bond_8023ad_member_info() still works in a secondary.  It
   no longer does.  Please say explicitly which queries survive
   (device info, stats, link, RETA and RSS hash config, bonding mode,
   members, and the LACP configuration) and that LACP runtime state
   queries do not.

   Worth stating the supported teardown too: rte_eth_dev_stop()
   returns -ENOTSUP in a secondary and rte_eth_dev_close() is the
   detach call.  A secondary that does the usual stop-then-close will
   now see an error from the stop.

3. Release note is in the wrong section.

   This changes the behaviour of already-exported functions -- calls
   that previously returned 0 now return -ENOTSUP or -1 -- so it
   belongs under "API Changes", not "New Features".  Same comment as
   on v2.

4. bond_ethdev_stats_get() ignores the rte_eth_stats_get() return.

   Pre-existing, but the patch puts .stats_get in secondary_dev_ops,
   which makes the failure case realistic:

	struct rte_eth_stats member_stats;
	...
	for (i = 0; i < internals->member_count; i++) {
		rte_eth_stats_get(internals->members[i].port_id,
				  &member_stats);
		stats->ipackets += member_stats.ipackets;

   eth_stats_qstats_get() runs RTE_ETH_VALID_PORTID_OR_ERR_RET before
   the memset, so on -ENODEV member_stats is left untouched.  On the
   first iteration that is uninitialized stack; on later iterations it
   is the previous member's counters, double-counted.  A secondary
   that attached to the bonding device but did not probe the member
   ports (blocklist, or members added by the primary after the
   secondary started) hits exactly this.

	ret = rte_eth_stats_get(internals->members[i].port_id,
				&member_stats);
	if (ret != 0)
		continue;

   Better as a separate fix ahead of this patch, since it is not
   secondary-specific.

Info
----

- Three of the 8023ad functions declare a second int purely for the
  new check while an existing one is right there:

	struct port *port;
	int res;
	int ret;

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

  ext_distrib_get() and ext_collect_get() reuse err instead, which
  reads better.  Reusing res in ext_collect(), ext_distrib() and
  ext_slowtx() would drop three declarations and make the series
  consistent.

- rte_eth_bond_api.c passes -1 and rte_eth_bond_8023ad.c passes
  -ENOTSUP.  Each matches its own file's existing convention, so this
  is defensible, but a caller cannot distinguish "wrong process type"
  from "bad port id" in the api.c cases.  -ENOTSUP everywhere would
  be clearer if you are willing to change those return values.

- The <stdbool.h> addition to eth_bond_private.h is unrelated to this
  change; bool is already used at line 184 and was working by
  transitive include.  It is a real fix, just not this patch's.

- Dropping eth_dev_priv_dump entirely costs some debuggability.  Only
  dump_lacp() touches the process-local array; dump_basic() reads
  bond_dev_private, which is shared.  Keeping .eth_dev_priv_dump in
  secondary_dev_ops and skipping the LACP section in a secondary
  would let a secondary still dump mode, members and offloads.