[PATCH net v2 0/2] bridge: cfm: correct CC receive monitoring transitions

[PATCH net v2 0/2] bridge: cfm: correct CC receive monitoring transitions

[Baek Kyu, Kim]

Two CC transition bugs affect CFM receive monitoring. When CC is
re-enabled with a new interval, the first RX check uses the previous
interval. After CC is disabled, a received CCM can still change peer
status even when its RX work cannot be queued.

Patch 1 stores the new CC configuration before enabling peer RX work.
Patch 2 keeps RX work disabled while CC is off and skips CCM status
processing during that time.

The v1 submission covered the RX work restart. This revision also
addresses the two review findings. The interval fix is a separate
patch because it has a distinct failure condition.

The combined series passed 8 CC interval/transition checks and 3 CCM
status checks under ARM64 QEMU/KVM with bridge/veth. The CCM status
test failed 2 of its 3 checks on the v1 image as expected.

Baek Kyu, Kim (2):
  bridge: cfm: Store the new CC configuration before enabling peers
  bridge: cfm: ignore CCM while CC is disabled

 net/bridge/br_cfm.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

---
v1: https://lkml.iu.edu/2610.3/01364.html
v2:
  - Add the CC enable interval fix as patch 1/2.
  - Skip CCM status updates while CC is disabled in patch 2/2.
  - Retain the RX work disable/enable fix from v1 in patch 2/2.

-- 
2.55.0

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

[Baek Kyu, Kim]

br_cfm_cc_config_set() enables existing peers before storing the new
CC configuration. The receive timer therefore starts with the previous
expected interval, delaying CCM loss detection when enabling CC with a
shorter interval. For example, disabling CC at 1s and re-enabling it at
100ms schedules the first check after 250ms instead of 25ms, before
rounding to jiffies.

Save the disabled-to-enabled transition and store the new configuration
before enabling peers. Keep cancellation on disable before replacing
the configuration.

Fixes: dc32cbb3dbd7 ("bridge: cfm: Kernel space implementation of CFM. CCM frame RX added.")
Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>
---
On the combined v2 kernel under ARM64 QEMU/KVM, all 8 CC interval and
transition checks passed with bridge/veth and netlink configuration.

 net/bridge/br_cfm.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/bridge/br_cfm.c b/net/bridge/br_cfm.c
index 9dcc97d63a6f..28b40dbf4073 100644
--- a/net/bridge/br_cfm.c
+++ b/net/bridge/br_cfm.c
@@ -634,6 +634,7 @@ int br_cfm_cc_config_set(struct net_bridge *br,
 {
 	struct br_cfm_peer_mep *peer_mep;
 	struct br_cfm_mep *mep;
+	bool enable;
 
 	ASSERT_RTNL();
 
@@ -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;
+
+	if (enable)
+		/* CC is enabled */
+		hlist_for_each_entry(peer_mep, &mep->peer_mep_list, head)
+			cc_peer_enable(peer_mep);
+
 	mep->ccm_rx_snumber = 0;
 	mep->ccm_tx_snumber = 1;
 
-- 
2.55.0

[PATCH net v2 2/2] bridge: cfm: ignore CCM while CC is disabled

[Baek Kyu, Kim]

A valid CCM received after CC is disabled can clear the peer defect
and restart its RX timer. Cancelling pending work does not stop a later
receive from queuing it again. Even with RX work disabled, that receive
can still update peer status and notify userspace.

Keep RX work disabled until CC is explicitly enabled, and skip CCM
processing while CC is disabled. This prevents a later CCM from changing
peer status or restarting RX monitoring.

Fixes: dc32cbb3dbd7 ("bridge: cfm: Kernel space implementation of CFM. CCM frame RX added.")
Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>
---
On ARM64 QEMU/KVM with bridge/veth, a valid CCM after the disable ACK
cleared the peer defect on v1 but left it set on the combined v2 kernel.
Re-enabling CC allowed a valid CCM to clear the defect. All 3 status
checks passed on v2; 2 failed on v1 as expected.

 net/bridge/br_cfm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_cfm.c b/net/bridge/br_cfm.c
index 28b40dbf4073..115cccf1d327 100644
--- a/net/bridge/br_cfm.c
+++ b/net/bridge/br_cfm.c
@@ -150,12 +150,13 @@ static void cc_peer_enable(struct br_cfm_peer_mep *peer_mep)
 	memset(&peer_mep->cc_status, 0, sizeof(peer_mep->cc_status));
 	peer_mep->ccm_rx_count_miss = 0;
 
+	enable_delayed_work(&peer_mep->ccm_rx_dwork);
 	ccm_rx_timer_start(peer_mep);
 }
 
 static void cc_peer_disable(struct br_cfm_peer_mep *peer_mep)
 {
-	cancel_delayed_work_sync(&peer_mep->ccm_rx_dwork);
+	disable_delayed_work_sync(&peer_mep->ccm_rx_dwork);
 }
 
 static struct sk_buff *ccm_frame_build(struct br_cfm_mep *mep,
@@ -414,6 +415,9 @@ int br_cfm_frame_rx(struct net_bridge_port *port, struct sk_buff *skb)
 
 	if (hdr->opcode == BR_CFM_OPCODE_CCM) {
 		/* CCM PDU received. */
+		if (!READ_ONCE(mep->cc_config.enable))
+			return 1;
+
 		/* MA ID is after common header + sequence number + MEP ID */
 		maid = skb_header_pointer(skb,
 					  CFM_CCM_PDU_MAID_OFFSET,
@@ -699,6 +703,8 @@ int br_cfm_cc_peer_mep_add(struct net_bridge *br, const u32 instance,
 	peer_mep->mepid = mepid;
 	peer_mep->mep = mep;
 	INIT_DELAYED_WORK(&peer_mep->ccm_rx_dwork, ccm_rx_work_expired);
+	/* Keep RX work disabled until CC is enabled. */
+	disable_delayed_work(&peer_mep->ccm_rx_dwork);
 
 	if (mep->cc_config.enable)
 		cc_peer_enable(peer_mep);
-- 
2.55.0

Re: [PATCH net v2 0/2] bridge: cfm: correct CC receive monitoring transitions

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v2 0/2] bridge: cfm: correct CC receive monitoring transitions

[Baek Kyu, Kim]

Both issues were raised in Sashiko's review of v1. I had also identified the stale interval ordering during an earlier manual inspection of the CC enable path. I validated both changes under ARM64 QEMU/KVM while preparing the two-patch v2 series.

Re: [PATCH net v2 0/2] bridge: cfm: correct CC receive monitoring transitions

[Nikolay Aleksandrov]

On 28/10/2026 18:47, Baek Kyu, Kim wrote:
> Two CC transition bugs affect CFM receive monitoring. When CC is
> re-enabled with a new interval, the first RX check uses the previous
> interval. After CC is disabled, a received CCM can still change peer
> status even when its RX work cannot be queued.
> 
> Patch 1 stores the new CC configuration before enabling peer RX work.
> Patch 2 keeps RX work disabled while CC is off and skips CCM status
> processing during that time.
> 
> The v1 submission covered the RX work restart. This revision also
> addresses the two review findings. The interval fix is a separate
> patch because it has a distinct failure condition.
> 
> The combined series passed 8 CC interval/transition checks and 3 CCM
> status checks under ARM64 QEMU/KVM with bridge/veth. The CCM status
> test failed 2 of its 3 checks on the v1 image as expected.
> 
> Baek Kyu, Kim (2):
>    bridge: cfm: Store the new CC configuration before enabling peers
>    bridge: cfm: ignore CCM while CC is disabled
> 
>   net/bridge/br_cfm.c | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
> 
> ---
> v1: https://lkml.iu.edu/2610.3/01364.html
> v2:
>    - Add the CC enable interval fix as patch 1/2.
>    - Skip CCM status updates while CC is disabled in patch 2/2.
>    - Retain the RX work disable/enable fix from v1 in patch 2/2.
> 

Hi,
Are you actually using CFM anywhere in production? We have been discussing
deprecating cfm and mrp in the bridge as noone seems interested and they have
proven as sources of bugs over time. These don't seem like critical fixes
so I would target net-next when it opens up.

Cheers,
  Nik

Re: [PATCH net v2 0/2] bridge: cfm: correct CC receive monitoring transitions

[Baek Kyu, Kim]

Hi Nik,

I am not currently using bridge CFM in production. I found these issues while inspecting the code, and I agree that the fixes can target net-next when it opens.

Thanks,
Baek Kyu

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

Re: [PATCH net v2 2/2] bridge: cfm: ignore CCM while CC is disabled

[netdev-bot+sashiko]

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

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

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 CC-disable path in br_cfm_cc_config_set()
  (net/bridge/br_cfm.c) does things in the wrong order and does not wait…
- [Low] The patch adds a lockless reader,
  READ_ONCE(mep->cc_config.enable) in br_cfm_frame_rx()…

--

bridge: cfm: ignore CCM while CC is disabled

This patch keeps the per-peer CCM RX delayed work disabled from creation
until CC is enabled. It switches cc_peer_disable() to
disable_delayed_work_sync(), and br_cfm_frame_rx() now drops CCM PDUs
while mep->cc_config.enable is false.

> diff --git a/net/bridge/br_cfm.c b/net/bridge/br_cfm.c
> index 28b40dbf40734..115cccf1d327d 100644
> --- a/net/bridge/br_cfm.c
> +++ b/net/bridge/br_cfm.c
> @@ -150,12 +150,13 @@ static void cc_peer_enable(struct br_cfm_peer_mep *peer_mep)
>  	memset(&peer_mep->cc_status, 0, sizeof(peer_mep->cc_status));
>  	peer_mep->ccm_rx_count_miss = 0;
>  
> +	enable_delayed_work(&peer_mep->ccm_rx_dwork);
>  	ccm_rx_timer_start(peer_mep);
>  }
>  
>  static void cc_peer_disable(struct br_cfm_peer_mep *peer_mep)
>  {
> -	cancel_delayed_work_sync(&peer_mep->ccm_rx_dwork);
> +	disable_delayed_work_sync(&peer_mep->ccm_rx_dwork);
>  }

[Severity: Medium]
Can a CCM still change peer status after the RX work has been disabled?

br_cfm_cc_config_set() disables the RX work for every peer first and
clears the enable flag only after that:

net/bridge/br_cfm.c:br_cfm_cc_config_set() {
    ...
	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;
    ...
}

disable_delayed_work_sync() can sleep while a running
ccm_rx_work_expired() finishes. For that whole loop,
mep->cc_config.enable is still true.

br_cfm_frame_rx() runs in softirq under RCU without RTNL, and it checks
the flag only once. A CCM handled on another CPU during the loop, or one
that read enable just before the store, passes the new check and then
does this:

	if (peer_mep->cc_status.ccm_defect) {
		peer_mep->cc_status.ccm_defect = false;
		br_cfm_notify(RTM_NEWLINK, port);
		ccm_rx_timer_start(peer_mep);
	}

It also updates seen, rdi, seq_unexp_seen and mep->ccm_rx_snumber.

The RX work is already disabled at that point, so ccm_rx_timer_start()
does nothing and the defect stays cleared while CC is off. Nothing like
synchronize_net() follows the store, so such a receiver can also finish
after the netlink request has returned.

This seems to be the case the commit message says it prevents:

    This prevents a later CCM from changing
    peer status or restarting RX monitoring.

Would it work to clear the flag first with
WRITE_ONCE(mep->cc_config.enable, false), then call synchronize_net() so
in-flight RX handlers drain, and only then disable the per-peer RX work?

[ ... ]

> @@ -414,6 +415,9 @@ int br_cfm_frame_rx(struct net_bridge_port *port, struct sk_buff *skb)
>  
>  	if (hdr->opcode == BR_CFM_OPCODE_CCM) {
>  		/* CCM PDU received. */
> +		if (!READ_ONCE(mep->cc_config.enable))
> +			return 1;
> +

[Severity: Low]
Should the writer side of this flag be annotated too?

This READ_ONCE() is a new lockless reader. The only writer is still the
plain struct copy in br_cfm_cc_config_set():

	mep->cc_config = *config;

The writer runs under RTNL and the reader runs in softirq under RCU, so
no common lock serializes them. The copy covers exp_interval, the 48 byte
exp_maid and enable, and the compiler may emit it as a memcpy. KCSAN can
then report a data race against this marked read.

Would a separate WRITE_ONCE(mep->cc_config.enable, ...) work here, placed
at the right point in the disable and enable sequences?

The same RX path also reads exp_maid and exp_interval without annotation
while they are being rewritten. Those reads were there before this patch.

>  		/* MA ID is after common header + sequence number + MEP ID */
>  		maid = skb_header_pointer(skb,
>  					  CFM_CCM_PDU_MAID_OFFSET,

[ ... ]

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