Thanks for the feedback, and sorry for the delayed v3. It was sent. Best, Xiang
On Tue, Apr 7, 2026 at 12:07 AM Ido Schimmel <[email protected]> wrote: > > On Sat, Apr 04, 2026 at 05:03:24PM -0700, Xiang Mei wrote: > > ccm_tx_work_expired() re-arms itself via queue_delayed_work() using > > the configured exp_interval converted by interval_to_us(). When > > exp_interval is BR_CFM_CCM_INTERVAL_NONE or out of range, > > interval_to_us() returns 0, causing the worker to fire immediately in > > a tight loop that allocates skbs until OOM. > > > > Fix this by validating exp_interval at configuration time: > > > > - Constrain IFLA_BRIDGE_CFM_CC_CONFIG_EXP_INTERVAL to [1, 7] in the > > netlink policy so userspace cannot set an invalid value. > > > > - Reject starting CCM TX in br_cfm_cc_ccm_tx() when exp_interval has > > not yet been configured (defaults to 0 from kzalloc). > > > > Fixes: a806ad8ee2aa ("bridge: cfm: Kernel space implementation of CFM. CCM > > frame TX added.") > > Nit: Doesn't matter in practice, but let's blame commit 2be665c3940d > ("bridge: cfm: Netlink SET configuration Interface.") instead as I don't > think this bug could be triggered before exposing the netlink API. > > > Reported-by: Weiming Shi <[email protected]> > > Signed-off-by: Xiang Mei <[email protected]> > > --- > > v2: Move validation out of the datapath and into configuration > > > > net/bridge/br_cfm.c | 6 ++++++ > > net/bridge/br_cfm_netlink.c | 2 +- > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/net/bridge/br_cfm.c b/net/bridge/br_cfm.c > > index 118c7ea48c35..dea56fffa1c1 100644 > > --- a/net/bridge/br_cfm.c > > +++ b/net/bridge/br_cfm.c > > @@ -805,6 +805,12 @@ int br_cfm_cc_ccm_tx(struct net_bridge *br, const u32 > > instance, > > goto save; > > } > > > > + if (!interval_to_us(mep->cc_config.exp_interval)) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Invalid CCM interval"); > > + return -EINVAL; > > + } > > + > > /* Start delayed work to transmit CCM frames. It is done with zero > > delay > > * to send first frame immediately > > */ > > diff --git a/net/bridge/br_cfm_netlink.c b/net/bridge/br_cfm_netlink.c > > index 2faab44652e7..1bb33c8f587b 100644 > > --- a/net/bridge/br_cfm_netlink.c > > +++ b/net/bridge/br_cfm_netlink.c > > @@ -34,7 +34,7 @@ br_cfm_cc_config_policy[IFLA_BRIDGE_CFM_CC_CONFIG_MAX + > > 1] = { > > [IFLA_BRIDGE_CFM_CC_CONFIG_UNSPEC] = { .type = NLA_REJECT }, > > [IFLA_BRIDGE_CFM_CC_CONFIG_INSTANCE] = { .type = NLA_U32 }, > > [IFLA_BRIDGE_CFM_CC_CONFIG_ENABLE] = { .type = NLA_U32 }, > > - [IFLA_BRIDGE_CFM_CC_CONFIG_EXP_INTERVAL] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_CC_CONFIG_EXP_INTERVAL] = NLA_POLICY_RANGE(NLA_U32, > > 1, 7), > > Use BR_CFM_CCM_INTERVAL_3_3_MS and BR_CFM_CCM_INTERVAL_10_MIN instead of > the magic numbers? > > The Sashiko review points out that blocking BR_CFM_CCM_INTERVAL_NONE > might break user space, but it seems weird to allow passing a value that > is interpreted the same as an invalid one. Worst case, if someone > complains, we can revert and go back to v1. > > > [IFLA_BRIDGE_CFM_CC_CONFIG_EXP_MAID] = { > > .type = NLA_BINARY, .len = CFM_MAID_LENGTH }, > > }; > > -- > > 2.43.0 > >
