Re: [PATCH net-next 2/2] net: dsa: user: document the write memory barrier on LAG conduit migration

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The comment added in dsa_conduit_lag_leave()
  (net/dsa/user.c:3438-3441) and the commit message both say the wmb()…

--

net: dsa: user: document the write memory barrier on LAG conduit migration

This patch adds a comment above the wmb() in dsa_conduit_lag_leave(). The
comment says the barrier makes the new dp->cpu_dp values visible to RX
path readers of lag_dev->dsa_ptr before the new pointer itself is visible.

> diff --git a/net/dsa/user.c b/net/dsa/user.c
> index cfad269be75c8..96393d4b5da1b 100644
> --- a/net/dsa/user.c
> +++ b/net/dsa/user.c
> @@ -3435,6 +3435,10 @@ static void dsa_conduit_lag_leave(struct net_device *conduit,
>  		 * physical CPU port
>  		 */
>  		lag_dev->dsa_ptr = new_cpu_dp;
> +		/* Ensure the new dp->cpu_dp values are visible to concurrent
> +		 * RX path readers of lag_dev->dsa_ptr before the pointer
> +		 * itself becomes visible
> +		 */
>  		wmb();

[Severity: Medium]
Can the wmb() in this position give the ordering the new comment
describes?

In dsa_conduit_lag_leave(), the dp->cpu_dp stores and the store to
lag_dev->dsa_ptr all happen before the barrier:

net/dsa/user.c:dsa_conduit_lag_leave() {
    ...
		dsa_tree_for_each_user_port(dp, dst)
			if (dsa_port_to_conduit(dp) == lag_dev)
				dp->cpu_dp = new_cpu_dp;
    ...
		lag_dev->dsa_ptr = new_cpu_dp;
		wmb();
    ...
}

A write barrier orders the stores before it against the stores after it.
Here all of these stores come before it, so nothing orders the dp->cpu_dp
stores against the dsa_ptr store. On a weakly ordered CPU, the new
dsa_ptr can become visible before some of the dp->cpu_dp updates.

The publish pattern in dsa_conduit_setup() puts the barrier before the
pointer store:

net/dsa/conduit.c:dsa_conduit_setup() {
    ...
	wmb();

	dev->dsa_ptr = cpu_dp;
    ...
}

The placement in dsa_conduit_lag_leave() instead matches
dsa_conduit_teardown(). That function stores dev->dsa_ptr = NULL, then
calls wmb(), and its comment says only that packets "from this point"
take the new path.

The readers also use plain loads, with no matching read barrier or
acquire. For example, dsa_find_designated_bridge_port_by_vid() in
net/dsa/tag.h and dsa_tag_8021q_find_port_by_vbid() in
net/dsa/tag_8021q.c both do this:

	struct dsa_port *cpu_dp = conduit->dsa_ptr;
	...
		if (dp->cpu_dp != cpu_dp)
			continue;

Both run on the RX path, dsa_switch_rcv() -> tagger rcv with
dev == lag_dev. That path runs in softirq without rtnl, while
dsa_conduit_lag_leave() runs from the netdev notifier under rtnl.

Suppose a reader sees the new dsa_ptr but a stale dp->cpu_dp. Would it
skip the designated bridge port, return NULL, and drop the packet?

The opposite case, an old dsa_ptr with a new dp->cpu_dp, also produces a
mismatch. So no single barrier on the writer side can fully give what the
comment promises.

The barrier placement and the reader-side race date back to commit
acc43b7bf52a ("net: dsa: allow masters to join a LAG"). What this patch
adds is the comment, plus this statement in the commit message, which
claim an ordering the code does not give:

  "The wmb() orders the dp->cpu_dp stores before the new lag_dev->dsa_ptr
   is published to the concurrent RX path readers."

Should the barrier move between the dp->cpu_dp loop and the dsa_ptr
store, using smp_wmb() or smp_store_release() paired with READ_ONCE() or
an acquire on the readers? Or should the comment describe what the
barrier does order, which is all the earlier stores against the later
ones in dsa_conduit_lag_teardown()?

Also, this is ordering between CPUs, so smp_wmb() would be enough here
rather than the mandatory wmb().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109120607.344544-2-kuangkai%40kylinos.cn