Re: [PATCH net-next v2 2/3] net: bridge: cfm: send an empty CFM nest when a bridge has no MEPs

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] At this commit alone, the new `!IS_ENABLED(CONFIG_BRIDGE_CFM)`
  branch in br_fill_ifinfo() (net/bridge/br_netlink.c:609) cannot send…
- [Low] The patch also changes GETLINK dump replies, and the commit
  message does not say so plainly.
- [Low] This patch removed the only caller of br_cfm_created() (in
  br_fill_ifinfo()), and nothing calls it any more.

Pre-existing issues:
- [Low] In br_fill_ifinfo(), two earlier sections jump to `done` and skip
  the CFM block: - the VLAN section: `if (!vg ||…

--

net: bridge: cfm: send an empty CFM nest when a bridge has no MEPs

This patch changes br_fill_ifinfo() so that a bridge master with no MEPs
gets an empty IFLA_BRIDGE_CFM nest when CFM config or status is requested.
Before, it got no nest at all. The goal is to let notification listeners
see that the last MEP has been deleted. Ports still get no CFM nest.

> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 5160e801d62db..68ec4529f0d2b 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -603,7 +603,10 @@ static int br_fill_ifinfo(struct sk_buff *skb,
>  		struct nlattr *cfm_nest = NULL;
>  		int err;
>  
> -		if (!br_cfm_created(br) || port)
> +		/* A bridge with no MEPs gets an empty IFLA_BRIDGE_CFM, so a
> +		 * listener can tell that the last MEP is gone.
> +		 */
> +		if (!IS_ENABLED(CONFIG_BRIDGE_CFM) || port)
>  			goto done;

[Severity: Low]
With only this patch applied, can any notification carry the new empty
nest?

This block only runs when filter_mask contains RTEXT_FILTER_CFM_CONFIG or
RTEXT_FILTER_CFM_STATUS. The generic bridge notification path asks for
neither:

net/bridge/br_netlink.c:br_ifinfo_notify() {
    ...
	u32 filter = RTEXT_FILTER_BRVLAN_COMPRESSED;
    ...
}

The IFLA_BRIDGE_CFM case in br_afspec() does not set *changed. Also,
br_cfm_notify() only fires from the peer MEP CCM paths, which need a live
MEP.

So at this point in the series, deleting the last MEP sends no
CFM-filtered RTM_NEWLINK. The only visible change is in RTM_GETLINK
replies.

The next patch in the series, "net: bridge: cfm: notify userspace on CFM
config changes", seems to fix this. It sets changed after a successful
br_mep_delete_parse() and adds this to br_cfm_parse():

	if (changed)
		br_info_notify(RTM_NEWLINK, br, NULL, RTEXT_FILTER_CFM_CONFIG);

Could the commit message mention that it depends on that follow-up patch?

Even at the end of the series, one case still seems to send no
CFM_CONFIG notification. When a port is deleted, its MEPs are removed
implicitly:

del_nbp()
  br_cfm_port_del()
    mep_delete_implementation()

In that path only br_ifinfo_notify(RTM_DELLINK, NULL, p) is sent for the
port. How is a listener expected to notice that the MEPs are gone?

[Severity: Low]
This also changes RTM_GETLINK and dump replies, not just notifications.

Before, a bridge with no MEPs took goto done with an empty IFLA_AF_SPEC,
and the epilogue cancelled that nest:

done:
	if (af) {
		if (nlmsg_get_pos(skb) - (void *)af > nla_attr_size(0))
			nla_nest_end(skb, af);
		else
			nla_nest_cancel(skb, af);
	}

Now, with CONFIG_BRIDGE_CFM enabled, every bridge master dumped with
RTEXT_FILTER_CFM_CONFIG or RTEXT_FILTER_CFM_STATUS gets an IFLA_AF_SPEC
that holds an empty IFLA_BRIDGE_CFM. Before, the reply for such a bridge
had no IFLA_AF_SPEC at all.

The commit message says:

    For a dump that is fine, but a notification sent after the last
    MEP is deleted then looks like any other RTM_NEWLINK for the bridge,

Could the commit message also say that dump output changes for bridges
without MEPs? Userspace can no longer treat the presence of
IFLA_BRIDGE_CFM as meaning "MEPs are configured".

The buffer sizing in br_get_link_af_size_filtered() already counts the
empty nest, so that part looks fine.

[Severity: Low]
This isn't a bug, but this removes the last caller of br_cfm_created().
Three pieces are left behind:

- the definition in net/bridge/br_cfm.c
- the prototype in net/bridge/br_private.h
- the !CONFIG_BRIDGE_CFM static inline stub in net/bridge/br_private.h

The later patches in the series add no new user. Should these be removed
here too?

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. Two
earlier sections of br_fill_ifinfo() can jump to done before the CFM
block runs:

	if (!vg || !READ_ONCE(vg->num_vlans)) {
		rcu_read_unlock();
		goto done;
	}
    ...
		if (!br_mrp_enabled(br) || port)
			goto done;

Suppose an RTM_GETLINK request combines RTEXT_FILTER_CFM_* with either
RTEXT_FILTER_BRVLAN* on a bridge with no VLANs, or RTEXT_FILTER_MRP on a
bridge without MRP. The reply then has no IFLA_BRIDGE_CFM at all: neither
the new empty nest nor real MEP data when MEPs exist.

Is that intended for combined filter masks? The notification added later
in the series uses RTEXT_FILTER_CFM_CONFIG alone, so it does not hit
these early exits.

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