Re: [PATCH net-next v2 3/3] net: bridge: cfm: notify userspace on CFM config changes

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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] Config-change notifications are only sent for changes made
  through br_cfm_parse().

--

net: bridge: cfm: notify userspace on CFM config changes

This patch makes br_cfm_parse() send an RTM_NEWLINK with
RTEXT_FILTER_CFM_CONFIG once any CFM config group in the request has
been applied. The notification is sent even if a later group in the
same request fails. It uses br_info_notify() the same way
br_cfm_notify() already does for status.

> diff --git a/net/bridge/br_cfm_netlink.c b/net/bridge/br_cfm_netlink.c
> index 91b9922dc3f25..56309e6f4b1be 100644
> --- a/net/bridge/br_cfm_netlink.c
> +++ b/net/bridge/br_cfm_netlink.c

[ ... ]

> @@ -399,59 +400,72 @@ int br_cfm_parse(struct net_bridge *br, struct net_bridge_port *p,

[ ... ]

>  	if (tb[IFLA_BRIDGE_CFM_CC_CCM_TX]) {
>  		err = br_cc_ccm_tx_parse(br, tb[IFLA_BRIDGE_CFM_CC_CCM_TX],
>  					 extack);
>  		if (err)
> -			return err;
> +			goto out;
> +		changed = true;
>  	}
>  
> -	return 0;
> +out:
> +	/* Earlier groups may have been applied even if a later one failed */
> +	if (changed)
> +		br_info_notify(RTM_NEWLINK, br, NULL, RTEXT_FILTER_CFM_CONFIG);
> +
> +	return err;
>  }

[Severity: High]

This sends a CFM config notification for changes that come in through
br_cfm_parse(). The kernel also changes the reported CFM config in a
couple of other places. Do those still go unreported?

The commit message says:

  Today the only way to see these changes is
  to poll RTM_GETLINK with RTEXT_FILTER_CFM_CONFIG.

One such place is ccm_tx_work_expired() in net/bridge/br_cfm.c, when the
transmission period ends:

	if (time_before_eq(mep->ccm_tx_end, jiffies)) {
		/* Transmission period has ended */
		mep->cc_ccm_tx_info.period = 0;
		return;
	}

br_cfm_config_fill_info() reports this field as
IFLA_BRIDGE_CFM_CC_CCM_TX_PERIOD. With this patch a listener sees
period=N when CCM transmission starts. Does it ever see the period drop
to 0 when transmission stops on its own?

The other place is port removal. del_nbp()->br_cfm_port_del() deletes
every MEP whose ifindex matches the port being removed:

	hlist_for_each_entry_safe(mep, n_store, &br->mep_list, head)
		if (mep->create.ifindex == port->dev->ifindex)
			mep_delete_implementation(br, mep);

After that, the only notification in del_nbp() is:

	br_ifinfo_notify(RTM_DELLINK, NULL, p);

That is a port message using RTEXT_FILTER_BRVLAN_COMPRESSED. Also,
br_fill_ifinfo() never puts IFLA_BRIDGE_CFM in port messages.

The previous patch in this series ("net: bridge: cfm: send an empty CFM
nest when a bridge has no MEPs") adds the empty nest so a listener can
tell that the last MEP is gone. If that last MEP is removed because its
port leaves the bridge (ip link del, or ip link set ... nomaster), no
bridge CFM config message is sent at all.

In these two cases, would a listener that relies only on the new
notifications keep deleted MEPs and non-zero CCM_TX periods in its view,
so it still has to poll?

Should ccm_tx_work_expired() and br_cfm_port_del() also send an
RTEXT_FILTER_CFM_CONFIG notification?

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