Re: [PATCH net v2] net: bridge: disable MRP ports before the FDB flush in del_nbp()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · 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).

- [High] The fix needs an RCU grace period between br_stp_disable_port()
  writing p->state = BR_STATE_DISABLED (br_if.c:348) and the final…
- [High] The reorder assumes br_mrp_port_del() always clears
  BR_MRP_AWARE_BIT on the port being deleted, so that…
- [Medium] The commit message's safety argument describes code that is
  not in the target tree.

--

net: bridge: disable MRP ports before the FDB flush in del_nbp()

This patch moves br_mrp_port_del() and br_cfm_port_del() ahead of
br_stp_disable_port() in del_nbp(). An MRP ring port then has
BR_MRP_AWARE_BIT cleared before STP disables it. The goal is for the port
to really reach BR_STATE_DISABLED before the FDB flush, so that no frame
is learned with dst == p and later forwarded to the freed port.

Link: https://lore.kernel.org/netdev/20261012123051.GA714215@shredder/

> Remove the MRP and CFM instances before disabling the port, so that
> br_stp_disable_port() actually transitions the port to BR_STATE_DISABLED.
> The synchronize_net() in nbp_vlan_flush() then drains the receive paths
> that could still learn with the old state, and the FDB flush that
> follows removes their entries.  No frame can be learned after that
> flush and before netdev_rx_handler_unregister().

[Severity: Medium]
Does this description match the tree the patch applies to?

Earlier in the message the flush is called br_fdb_delete_by_port(), but
that function no longer exists under net/bridge/. The current del_nbp()
does this:

	vg = nbp_vlan_group(p);
	nbp_vlan_group_unpublish(p);
	br_fdb_cleanup_by_dst(br, br_port_to_dst(p), 0, 1);
	nbp_vlan_flush(p, vg);

nbp_vlan_flush() contains no synchronize_net(), and it runs after the FDB
flush:

net/bridge/br_vlan.c:nbp_vlan_flush() {
	ASSERT_RTNL();

	__vlan_flush(port->br, port, vg);
	__vlan_group_free(vg);
}

The only grace period before the flush is in nbp_vlan_group_unpublish():

net/bridge/br_vlan.c:nbp_vlan_group_unpublish() {
	ASSERT_RTNL();

	RCU_INIT_POINTER(port->vlgrp, NULL);
	synchronize_net();
}

This layout came from commit 83edc87a9172 ("net: bridge: vlan: quiesce
readers before freeing port VLANs"). The testing in the changelog was
done on v7.3, which still has the older ordering.

Could the message name nbp_vlan_group_unpublish() and
br_fdb_cleanup_by_dst() instead, or describe both layouts if a stable
backport is intended? Could the patch also be tested on the target tree?

> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 2c05ebc1299d1..fb25ce3e5e646 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -341,13 +341,13 @@ static void del_nbp(struct net_bridge_port *p)
>  
>  	nbp_delete_promisc(p);
>  
> +	br_mrp_port_del(br, p);
> +	br_cfm_port_del(br, p);
> +

[Severity: High]
Does br_mrp_port_del() always clear BR_MRP_AWARE_BIT on the port being
deleted?

br_mrp_set_port_role() can leave a port MRP-aware with no instance
pointing at it. Take a ring with primary A and secondary B. Setting
IFLA_BRIDGE_MRP_PORT_ROLE=PRIMARY on B is accepted, because B is
MRP-aware and br_mrp_find_port() finds the instance:

net/bridge/br_mrp.c:br_mrp_set_port_role() {
	...
	case BR_MRP_PORT_ROLE_PRIMARY:
		rcu_assign_pointer(mrp->p_port, p);
		break;
	...
}

Now p_port == s_port == B. A keeps BR_MRP_AWARE_BIT and stays in the
BR_STATE_FORWARDING state that br_mrp_add() set.

Later, when A is removed from the bridge, br_mrp_find_port(br, A) checks
only p_port, s_port and i_port. So br_mrp_port_del() returns early:

	if (!mrp)
		return;

br_stp_disable_port()->br_set_state() also returns early:

	if (test_bit(BR_MRP_AWARE_BIT, &p->flags))
		return;

As a result, A stays FORWARDING.

If VLAN filtering is disabled at runtime, br_allowed_ingress() returns
true without looking at the unpublished VLAN group. A frame that arrives
between br_fdb_cleanup_by_dst() and netdev_rx_handler_unregister() is then
learned with dst == A. Nothing flushes that entry before
call_rcu(&p->rcu, destroy_nbp_rcu). A later frame to that MAC would reach
br_forward() with the freed port.

This looks like the same use-after-free as the one in the changelog. It
needs no reader preemption, only traffic during the teardown window.

Should teardown force the port out of MRP-aware mode, or disable it
directly, even when no instance references it? Alternatively, should
br_mrp_set_port_role() avoid orphaning the port it displaces?

>  	spin_lock_bh(&br->lock);
>  	br_stp_disable_port(p);
>  	spin_unlock_bh(&br->lock);

[Severity: High]
Is there still a window here with CONFIG_BRIDGE_VLAN_FILTERING=n?

The fix needs a grace period between br_stp_disable_port() writing
BR_STATE_DISABLED and the final br_fdb_cleanup_by_dst(br,
br_port_to_dst(p), 0, 1) in del_nbp(). The only synchronize_net() in that
window is in nbp_vlan_group_unpublish(). Without VLAN filtering, that
function is an empty stub:

net/bridge/br_private.h:
static inline void nbp_vlan_group_unpublish(struct net_bridge_port *port)
{
}

Nothing else in the window synchronizes. That includes br_ifinfo_notify(),
list_del_rcu(), update_headroom() and netdev_reset_rx_headroom().

One possible interleaving:

CPU1 (RX softirq, br_handle_frame_finish())
    reads p->state and sees FORWARDING
    passes the BR_STATE_DISABLED check

CPU0 (del_nbp())
    br_stp_disable_port()
        br_set_state(p, BR_STATE_DISABLED)
    br_fdb_cleanup_by_dst(br, br_port_to_dst(p), 0, 1)

CPU1
    br_fdb_update(br, p, vlan, eth_hdr(skb)->h_source, 0)
        fdb_create(br, dst, addr, vid, flags)   /* dst == p */

br_fdb_update() does not check the port state again. No FDB flush runs
after this point: netdev_rx_handler_unregister() only drains, and
destroy_nbp() does no FDB cleanup. After call_rcu(destroy_nbp_rcu), a
frame to the learned MAC would reach br_forward() with the freed port.

BRIDGE_MRP and BRIDGE_CFM depend only on BRIDGE. BRIDGE_VLAN_FILTERING is
default n and depends on VLAN_8021Q. So MRP=y with VLAN_FILTERING=n is a
valid configuration. The window is short on !PREEMPT_RT, but on PREEMPT_RT
the softirq reader can be preempted inside it.

This also seems to contradict this line in the changelog: "No frame can
be learned after that flush and before netdev_rx_handler_unregister()".

Would an unconditional synchronize_net() after br_stp_disable_port() and
before the final FDB flush close this window?

>  
> -	br_mrp_port_del(br, p);
> -	br_cfm_port_del(br, p);
> -
>  	br_ifinfo_notify(RTM_DELLINK, NULL, p);
>  
>  	list_del_rcu(&p->list);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179333317814.2102764.8780283783548366593%40gmail.com