Re: [PATCH net v2 1/2] bridge: cfm: Store the new CC configuration before enabling peers

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (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] At this commit, cc_peer_disable()
  (net/bridge/br_cfm.c:156-159) only calls cancel_delayed_work_sync().

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the code
  still mishandles it.

--

bridge: cfm: Store the new CC configuration before enabling peers

This patch reorders br_cfm_cc_config_set() so that mep->cc_config holds
the new CC configuration before cc_peer_enable() is called on the
disabled-to-enabled transition. The goal is for the first RX timer to use
the new exp_interval instead of the previous one.

> diff --git a/net/bridge/br_cfm.c b/net/bridge/br_cfm.c
> index 9dcc97d63a6fc..28b40dbf40734 100644
> --- a/net/bridge/br_cfm.c
> +++ b/net/bridge/br_cfm.c

[ ... ]

> @@ -648,17 +649,20 @@ int br_cfm_cc_config_set(struct net_bridge *br,
>  	if (memcmp(config, &mep->cc_config, sizeof(*config)) == 0)
>  		return 0;
>  
> -	if (config->enable && !mep->cc_config.enable)
> -		/* CC is enabled */
> -		hlist_for_each_entry(peer_mep, &mep->peer_mep_list, head)
> -			cc_peer_enable(peer_mep);
> -
>  	if (!config->enable && mep->cc_config.enable)
>  		/* CC is disabled */
>  		hlist_for_each_entry(peer_mep, &mep->peer_mep_list, head)
>  			cc_peer_disable(peer_mep);
>  
> +	enable = config->enable && !mep->cc_config.enable;
> +
>  	mep->cc_config = *config;

[Severity: Medium]
This isn't a bug introduced by this patch, but what happens when
exp_interval changes while CC stays enabled?

In that case the disable branch and the new enable branch are both false,
so only the config store runs. No peer is re-armed, and ccm_rx_count_miss
is not reset.

ccm_rx_timer_start() reads exp_interval only at the moment it queues the
work:

ccm_rx_timer_start() {
	interval_us = interval_to_us(peer_mep->mep->cc_config.exp_interval);
	...
	queue_delayed_work(system_percpu_wq, &peer_mep->ccm_rx_dwork,
			   usecs_to_jiffies(interval_us / 4));
}

So a ccm_rx_dwork that is already pending still fires after the delay
computed from the old interval. For example, changing from 10 min to
3.3 ms without disabling CC could leave the next RX check up to 150 s
away instead of about 825 us.

The 13-tick threshold in ccm_rx_work_expired() would then be counted with
a mix of old and new intervals.

Should the peer RX timers also be restarted when exp_interval changes on
an enabled MEP? The follow-up commit "bridge: cfm: ignore CCM while CC is
disabled" doesn't seem to touch this path either.

> +
> +	if (enable)
> +		/* CC is enabled */
> +		hlist_for_each_entry(peer_mep, &mep->peer_mep_list, head)
> +			cc_peer_enable(peer_mep);

[Severity: Medium]
Can ccm_rx_dwork already be pending when this loop runs?

At this commit cc_peer_disable() only calls cancel_delayed_work_sync().
br_cfm_frame_rx() runs in softirq and does not check
mep->cc_config.enable. If ccm_defect was still set when CC was disabled,
a valid CCM that arrives later queues the work again, using the disabled
config's interval:

br_cfm_frame_rx() {
	...
		if (peer_mep->cc_status.ccm_defect) {
			peer_mep->cc_status.ccm_defect = false;
			...
			ccm_rx_timer_start(peer_mep);
		}
	...
}

After that, ccm_rx_work_expired() keeps re-arming the work.

On re-enable, cc_peer_enable()->ccm_rx_timer_start()->queue_delayed_work()
returns false because the work is already pending. The new, shorter delay
is never programmed.

Take the example from the commit message (disable at 1s, re-enable at
100ms). The first check could still fire after about 250ms instead of
25ms, which is the symptom this patch sets out to fix.

The next commit in the series, "bridge: cfm: ignore CCM while CC is
disabled", appears to close this path. It uses disable_delayed_work_sync()
in cc_peer_disable(), disables the work at peer add, calls
enable_delayed_work() in cc_peer_enable(), and checks
READ_ONCE(mep->cc_config.enable) in br_cfm_frame_rx(). With that commit
applied, the remaining window looks only theoretical.

Could the two patches be reordered or squashed, so that this commit fixes
the re-enable case by itself?

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