[PATCH net] bridge: cfm: keep RX work disabled while CC is disabled

[PATCH net] bridge: cfm: keep RX work disabled while CC is disabled

[Baek Kyu, Kim]

Found by reviewing the peer deletion workqueue fix. After CC is
turned off, a valid CCM can still reach br_cfm_frame_rx(), clear the
peer defect and restart its RX timer. cancel_delayed_work_sync()
only cancels existing work, so a later receive can queue it again.
The peer may then report a new loss defect with CC still disabled.

With a bridge/veth setup under ARM64 QEMU/KVM, disabling CC after a
loss defect and injecting a valid CCM reproduced the restart. The
original kernel reported another loss defect; the fixed kernel did
not. A test-only gate paused an in-flight receive until after the
disable ACK and reproduced the same difference. The gate is absent
from this patch. The reproducer passed with the fix; bridge selftests
were not run.

Initialize each peer's RX work disabled, enable it only when CC is
explicitly enabled, and disable it synchronously when CC is turned
off. This also prevents an in-flight receive from queuing work after
the disable completes.

Fixes: dc32cbb3dbd7 ("bridge: cfm: Kernel space implementation of CFM. CCM frame RX added.")
Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>
---
Tested under ARM64 QEMU/KVM using bridge/veth, netlink CC settings,
and valid CCM frames received through AF_PACKET. After a disable ACK,
the baseline queued RX work and reported another loss defect; the
patched kernel rejected the queue attempt and reported no new defect.
The same difference was observed when an in-flight receive resumed
after the ACK. Explicit re-enable and peer deletion were also checked.
No physical NIC was tested.

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

diff --git a/net/bridge/br_cfm.c b/net/bridge/br_cfm.c
index 9dcc97d63a6f..8576f387d037 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,
@@ -695,6 +696,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);

Re: [PATCH net] bridge: cfm: keep RX work disabled while CC is disabled

[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] The patch only partly closes the post-disable behaviour its
  own commit message describes.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but it sits on
  the cc_peer_enable() path the patch modifies.

--

bridge: cfm: keep RX work disabled while CC is disabled

This patch initializes each peer MEP's ccm_rx_dwork in the disabled
state. It enables the work in cc_peer_enable() and switches
cc_peer_disable() to disable_delayed_work_sync(). The goal is to stop
br_cfm_frame_rx() from requeueing the RX work, and reporting a new loss
defect, after CC has been turned off.

> diff --git a/net/bridge/br_cfm.c b/net/bridge/br_cfm.c
> index 9dcc97d63a6fc..8576f387d037e 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);
>  }

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but it
is on the cc_peer_enable() path changed here. When br_cfm_cc_config_set()
calls ccm_rx_timer_start(), which exp_interval does it see?

br_cfm_cc_config_set() enables the peers before it stores the new config:

    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);
    ...
    mep->cc_config = *config;

ccm_rx_timer_start() then reads the old value:

    interval_us = interval_to_us(peer_mep->mep->cc_config.exp_interval);

On the first enable of a newly created MEP, cc_config is all zeroes. That
makes exp_interval BR_CFM_CCM_INTERVAL_NONE, so interval_to_us() returns 0
and the RX work is queued with a 0 jiffy delay.

If the kworker runs before mep->cc_config is updated, can
ccm_rx_work_expired() keep requeueing itself with a 0 delay? If so, it
could reach the miss limit and latch a spurious ccm_defect, with a
notification.

The same ordering matters when enable and an interval change arrive in
one request. For example, CC was disabled with a 10 min interval and is
now enabled with 3.3 ms. The first tick would then fire after a quarter
of the old interval, which delays loss detection by up to about 2.5
minutes.

Could the new config be stored before the peers are enabled?

>  
>  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]
Does this cover everything the commit message describes? The commit
message says:

    After CC is turned off, a valid CCM can still reach br_cfm_frame_rx(),
    clear the peer defect and restart its RX timer.

This change only prevents the timer restart. The CCM branch of
br_cfm_frame_rx() still does not check mep->cc_config.enable:

    /* A valid CCM frame is received */
    if (peer_mep->cc_status.ccm_defect) {
        peer_mep->cc_status.ccm_defect = false;

        /* Change in CCM defect status - notify */
        br_cfm_notify(RTM_NEWLINK, port);

        /* Start CCM RX timer */
        ccm_rx_timer_start(peer_mep);
    }

    peer_mep->cc_status.seen = true;
    peer_mep->ccm_rx_count_miss = 0;

cc_peer_disable() does not touch cc_status, so a ccm_defect latched
before CC was disabled stays set.

When a valid CCM arrives after that, won't br_cfm_frame_rx() still clear
the defect and send an RTM_NEWLINK notification for a MEP whose CC is
disabled? The only difference is that ccm_rx_timer_start() now does
nothing, because queue_delayed_work() refuses the disabled work.

A receive that is already in flight when br_cfm_cc_config_set() disables
CC seems to behave the same way. disable_delayed_work_sync() returns and
the request is acked, but the in-flight br_cfm_frame_rx() can still clear
the defect and notify.

The same path also keeps updating seen, ccm_rx_count_miss, rdi,
seq_unexp_seen, the TLV status and mep->ccm_rx_snumber.

Would it close the remaining gap to also gate the CCM processing branch
of br_cfm_frame_rx() on mep->cc_config.enable? Disabling the work would
still be needed for the in-flight requeue race.

[ ... ]

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