Re: [PATCH net v2 1/1] net: bridge: use option bits for CFM/MRP frame handlers

[Nikolay Aleksandrov <razor@blackwall.org>]

On 31/08/2026 14:20, Zhiling Zou wrote:
> CFM and MRP register a global br_frame_type whose hlist_node is linked
> into the per-bridge frame_type_list when the first MEP/MRP instance is
> created. Enabling the protocol on multiple bridges therefore inserts the
> same node into multiple lists. Unregistering it on one bridge then
> corrupts list state belonging to another.
> 
> These handlers can only be installed once per bridge, and they are
> uncommon. Track their per-bridge enable state with net_bridge option
> bits, which already live on the Rx hot cache line, and dispatch the
> matching handler directly from the receive path. Check both bits
> together first as an unlikely case.
> 
> Remove the generic frame_type_list and br_frame_type helpers, which
> have had no other users since CFM and MRP were added. That shrinks
> struct net_bridge by 8 bytes and drops the list walk from the fast
> path. When neither protocol is compiled in, the new checks are compiled
> out completely.
> 
> Fixes: 90c628dd47ff ("net: bridge: extend the process of special frames")
> Fixes: dc32cbb3dbd7 ("bridge: cfm: Kernel space implementation of CFM. CCM frame RX added.")
> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>
> Suggested-by: Nikolay Aleksandrov <razor@blackwall.org>
> Co-developed-by: Yilin Zhu <zylzyl2333@gmail.com>
> Signed-off-by: Yilin Zhu <zylzyl2333@gmail.com>
> Signed-off-by: Zhiling Zou <zhilinz@nebusec.ai>
> ---
> changes in v2:
>    - Replace the per-bridge br_frame_type object with net_bridge option
>      bits.
>    - Dispatch CFM/MRP handlers from the receive path. Check both option
>      bits together first as an unlikely case.
>    - Cover MRP, which has the same shared hlist_node bug.
>    - Remove frame_type_list so CFM/MRP do not affect the fast path when
>      they are disabled in .config.
>    - v1 Link: https://lore.kernel.org/all/7198fe2845c30c60c6b3833dd78cead8c5966931.1778378864.git.zylzyl2333@gmail.com/
> 

Hi,
I might've been a bit overzealous on the fast-path comments, so these options will
almost always be enabled by distros, that's what they usually do. So a few minor
changes would make the code more readable below...

>   net/bridge/br_cfm.c     | 11 +++--------
>   net/bridge/br_device.c  |  1 -
>   net/bridge/br_input.c   | 44 ++++++++++++++++++++---------------------
>   net/bridge/br_mrp.c     | 13 +++---------
>   net/bridge/br_private.h | 15 ++++----------
>   5 files changed, 32 insertions(+), 52 deletions(-)
> 
> diff --git a/net/bridge/br_cfm.c b/net/bridge/br_cfm.c
> index dea56fffa1c19..9dcc97d63a6fc 100644
> --- a/net/bridge/br_cfm.c
> +++ b/net/bridge/br_cfm.c
> @@ -367,7 +367,7 @@ static u32 ccm_tlv_extract(struct sk_buff *skb, u32 index,
>   }
>   
>   /* note: already called with rcu_read_lock */
> -static int br_cfm_frame_rx(struct net_bridge_port *port, struct sk_buff *skb)
> +int br_cfm_frame_rx(struct net_bridge_port *port, struct sk_buff *skb)
>   {
>   	u32 mdlevel, interval, size, index, max;
>   	const struct br_cfm_common_hdr *hdr;
> @@ -489,11 +489,6 @@ static int br_cfm_frame_rx(struct net_bridge_port *port, struct sk_buff *skb)
>   	return 1;
>   }
>   
> -static struct br_frame_type cfm_frame_type __read_mostly = {
> -	.type = cpu_to_be16(ETH_P_CFM),
> -	.frame_handler = br_cfm_frame_rx,
> -};
> -
>   int br_cfm_mep_create(struct net_bridge *br,
>   		      const u32 instance,
>   		      struct br_cfm_mep_create *const create,
> @@ -559,7 +554,7 @@ int br_cfm_mep_create(struct net_bridge *br,
>   	INIT_DELAYED_WORK(&mep->ccm_tx_dwork, ccm_tx_work_expired);
>   
>   	if (hlist_empty(&br->mep_list))
> -		br_add_frame(br, &cfm_frame_type);
> +		br_opt_toggle(br, BROPT_CFM_ENABLED, true);
>   
>   	hlist_add_tail_rcu(&mep->head, &br->mep_list);
>   
> @@ -588,7 +583,7 @@ static void mep_delete_implementation(struct net_bridge *br,
>   	kfree_rcu(mep, rcu);
>   
>   	if (hlist_empty(&br->mep_list))
> -		br_del_frame(br, &cfm_frame_type);
> +		br_opt_toggle(br, BROPT_CFM_ENABLED, false);
>   }
>   
>   int br_cfm_mep_delete(struct net_bridge *br,
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index ff55dab736326..e01c44a90d84b 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -503,7 +503,6 @@ void br_dev_setup(struct net_device *dev)
>   	spin_lock_init(&br->lock);
>   	INIT_LIST_HEAD(&br->port_list);
>   	INIT_HLIST_HEAD(&br->fdb_list);
> -	INIT_HLIST_HEAD(&br->frame_type_list);
>   #if IS_ENABLED(CONFIG_BRIDGE_MRP)
>   	INIT_HLIST_HEAD(&br->mrp_list);
>   #endif
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index d87a5f9fa92b7..72892e5b40439 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -317,20 +317,33 @@ static int nf_hook_bridge_pre(struct sk_buff *skb, struct sk_buff **pskb)
>   	return RX_HANDLER_CONSUMED;
>   }
>   
> +#if IS_ENABLED(CONFIG_BRIDGE_CFM) || IS_ENABLED(CONFIG_BRIDGE_MRP)
> +/* CFM/MRP are uncommon; test both enable bits together first. */

drop the #if and the comment

> +#define BR_CFM_MRP_OPTS \
> +	((IS_ENABLED(CONFIG_BRIDGE_CFM) ? BIT(BROPT_CFM_ENABLED) : 0UL) | \
> +	 (IS_ENABLED(CONFIG_BRIDGE_MRP) ? BIT(BROPT_MRP_ENABLED) : 0UL))

always define this

> +
>   /* Return 0 if the frame was not processed otherwise 1
>    * note: already called with rcu_read_lock
>    */
>   static int br_process_frame_type(struct net_bridge_port *p,
>   				 struct sk_buff *skb)
>   {
> -	struct br_frame_type *tmp;
> -
> -	hlist_for_each_entry_rcu(tmp, &p->br->frame_type_list, list)
> -		if (unlikely(tmp->type == skb->protocol))
> -			return tmp->frame_handler(p, skb);
> +	struct net_bridge *br = p->br;
>   
> +#if IS_ENABLED(CONFIG_BRIDGE_CFM)

drop the #if
> +	if (skb->protocol == htons(ETH_P_CFM) &&
> +	    br_opt_get(br, BROPT_CFM_ENABLED))
> +		return br_cfm_frame_rx(p, skb);
> +#endif
> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)

and this one

> +	if (skb->protocol == htons(ETH_P_MRP) &&
> +	    br_opt_get(br, BROPT_MRP_ENABLED))
> +		return br_mrp_process(p, skb);
> +#endif

instead follow what others are doing and make these functions noops in the headers
if these options are not defined, there are plenty of examples in br_private.h
The compiler should prune these when they're noops.

>   	return 0;
>   }
> +#endif
>   
>   /*
>    * Return NULL if skb is handled
> @@ -425,8 +438,11 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
>   		}
>   	}
>   
> -	if (unlikely(br_process_frame_type(p, skb)))
> +#if IS_ENABLED(CONFIG_BRIDGE_CFM) || IS_ENABLED(CONFIG_BRIDGE_MRP)

drop this #if

> +	if (unlikely((p->br->options & BR_CFM_MRP_OPTS) &&

always do the check, but read p->br->options with READ_ONCE because it might change
in case none of the options are defined we get & 0 so the compiler will prune the
branch entirely, otherwise we have a single fast check (we already had that with the list
anyway, but here it's better because this is in a hot cache line)

> +		     br_process_frame_type(p, skb)))
>   		return RX_HANDLER_PASS;
> +#endif
>   
>   forward:
>   	if (br_mst_is_enabled(p))
> @@ -467,19 +483,3 @@ rx_handler_func_t *br_get_rx_handler(const struct net_device *dev)
>   
>   	return br_handle_frame;
>   }
> -
> -void br_add_frame(struct net_bridge *br, struct br_frame_type *ft)
> -{
> -	hlist_add_head_rcu(&ft->list, &br->frame_type_list);
> -}
> -
> -void br_del_frame(struct net_bridge *br, struct br_frame_type *ft)
> -{
> -	struct br_frame_type *tmp;
> -
> -	hlist_for_each_entry(tmp, &br->frame_type_list, list)
> -		if (ft == tmp) {
> -			hlist_del_rcu(&ft->list);
> -			return;
> -		}
> -}
> diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
> index ef16d07039241..dce6efa96c4c6 100644
> --- a/net/bridge/br_mrp.c
> +++ b/net/bridge/br_mrp.c
> @@ -6,13 +6,6 @@
>   static const u8 mrp_test_dmac[ETH_ALEN] = { 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 };
>   static const u8 mrp_in_test_dmac[ETH_ALEN] = { 0x1, 0x15, 0x4e, 0x0, 0x0, 0x3 };
>   
> -static int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb);
> -
> -static struct br_frame_type mrp_frame_type __read_mostly = {
> -	.type = cpu_to_be16(ETH_P_MRP),
> -	.frame_handler = br_mrp_process,
> -};
> -
>   static bool br_mrp_is_ring_port(struct net_bridge_port *p_port,
>   				struct net_bridge_port *s_port,
>   				struct net_bridge_port *port)
> @@ -486,7 +479,7 @@ static void br_mrp_del_impl(struct net_bridge *br, struct br_mrp *mrp)
>   	kfree_rcu(mrp, rcu);
>   
>   	if (hlist_empty(&br->mrp_list))
> -		br_del_frame(br, &mrp_frame_type);
> +		br_opt_toggle(br, BROPT_MRP_ENABLED, false);
>   }
>   
>   /* Adds a new MRP instance.
> @@ -536,7 +529,7 @@ int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
>   	rcu_assign_pointer(mrp->s_port, p);
>   
>   	if (hlist_empty(&br->mrp_list))
> -		br_add_frame(br, &mrp_frame_type);
> +		br_opt_toggle(br, BROPT_MRP_ENABLED, true);
>   
>   	INIT_DELAYED_WORK(&mrp->test_work, br_mrp_test_work_expired);
>   	INIT_DELAYED_WORK(&mrp->in_test_work, br_mrp_in_test_work_expired);
> @@ -1241,7 +1234,7 @@ static int br_mrp_rcv(struct net_bridge_port *p,
>    * normal forwarding.
>    * note: already called with rcu_read_lock
>    */
> -static int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
> +int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
>   {
>   	/* If there is no MRP instance do normal forwarding */
>   	if (likely(!test_bit(BR_MRP_AWARE_BIT, &p->flags)))
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index d337b1cfb980d..afe7c0b4f8fa7 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -495,12 +495,13 @@ enum net_bridge_opts {
>   	BROPT_MST_ENABLED,
>   	BROPT_MDB_OFFLOAD_FAIL_NOTIFICATION,
>   	BROPT_FDB_LOCAL_VLAN_0,
> +	BROPT_CFM_ENABLED,
> +	BROPT_MRP_ENABLED,
>   };
>   
>   struct net_bridge {
>   	spinlock_t			lock;
>   	spinlock_t			hash_lock;
> -	struct hlist_head		frame_type_list;
>   	struct net_device		*dev;
>   	unsigned long			options;
>   	/* These fields are accessed on each packet */
> @@ -932,16 +933,6 @@ int nbp_backup_change(struct net_bridge_port *p, struct net_device *backup_dev);
>   int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb);
>   rx_handler_func_t *br_get_rx_handler(const struct net_device *dev);
>   
> -struct br_frame_type {
> -	__be16			type;
> -	int			(*frame_handler)(struct net_bridge_port *port,
> -						 struct sk_buff *skb);
> -	struct hlist_node	list;
> -};
> -
> -void br_add_frame(struct net_bridge *br, struct br_frame_type *ft);
> -void br_del_frame(struct net_bridge *br, struct br_frame_type *ft);
> -
>   static inline bool br_rx_handler_check_rcu(const struct net_device *dev)
>   {
>   	return rcu_dereference(dev->rx_handler) == br_get_rx_handler(dev);
> @@ -2080,6 +2071,7 @@ int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
>   bool br_mrp_enabled(struct net_bridge *br);
>   void br_mrp_port_del(struct net_bridge *br, struct net_bridge_port *p);
>   int br_mrp_fill_info(struct sk_buff *skb, struct net_bridge *br);
> +int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb);
>   #else
>   static inline int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
>   			       struct nlattr *attr, int cmd,
> @@ -2111,6 +2103,7 @@ int br_cfm_parse(struct net_bridge *br, struct net_bridge_port *p,
>   		 struct nlattr *attr, int cmd, struct netlink_ext_ack *extack);
>   bool br_cfm_created(struct net_bridge *br);
>   void br_cfm_port_del(struct net_bridge *br, struct net_bridge_port *p);
> +int br_cfm_frame_rx(struct net_bridge_port *port, struct sk_buff *skb);
>   int br_cfm_config_fill_info(struct sk_buff *skb, struct net_bridge *br);
>   int br_cfm_status_fill_info(struct sk_buff *skb,
>   			    struct net_bridge *br,