Re: [PATCH net 1/1] net: bridge: cfm: use a per-bridge frame type

[Nikolay Aleksandrov <razor@blackwall.org>]

On 10/05/2026 12:15, Ren Wei wrote:
> From: Yilin Zhu <zylzyl2333@gmail.com>
> 
> CFM registers a bridge frame handler when the first MEP is created and
> unregisters it when the last MEP is deleted. The registered object also
> contains the hlist_node used by the bridge-local frame_type_list.
> 
> The CFM frame type is currently global, so enabling CFM on multiple
> bridges links the same hlist_node into multiple bridge-local lists. A
> later unregister on one bridge can then operate on list state belonging
> to another bridge.
> 
> Move the CFM frame type into struct net_bridge and register/unregister
> the bridge-owned object. This keeps frame handler list membership local
> to the bridge while preserving the existing first-MEP/last-MEP lifetime.
> 
> Fixes: dc32cbb3dbd7 ("bridge: cfm: Kernel space implementation of CFM. CCM frame RX added.")
> Cc: stable@kernel.org
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Co-developed-by: Peihan Liu <ronbogo@outlook.com>
> Signed-off-by: Peihan Liu <ronbogo@outlook.com>
> Signed-off-by: Yilin Zhu <zylzyl2333@gmail.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> ---
>   net/bridge/br_cfm.c     | 14 ++++++--------
>   net/bridge/br_private.h | 18 +++++++++++-------
>   2 files changed, 17 insertions(+), 15 deletions(-)
> 

I think MRP suffers from the same bug, but I also think we can contain the
fix within the packet type structure instead of making the already huge
struct net_bridge even bigger. Also, linking a struct within net_bridge
to a list within the same net_bridge looks weird.

IMO br_add_frame should take a type & a frame_handler, allocate a br_frame_type
dynamically and link it, then br_del_frame should take a type instead of a ptr
and remove that frame type and free it with kfree_rcu. That would require
br_add_frame return value to be checked in the respective cfm/mrp functions.
Warnings for already existing types on add or missing types on del should be
added.

Would you please take care of MRP as well?

Cheers,
  Nik

> diff --git a/net/bridge/br_cfm.c b/net/bridge/br_cfm.c
> index 118c7ea48c35..547c7415c0ea 100644
> --- a/net/bridge/br_cfm.c
> +++ b/net/bridge/br_cfm.c
> @@ -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,
> @@ -558,8 +553,11 @@ int br_cfm_mep_create(struct net_bridge *br,
>   	INIT_HLIST_HEAD(&mep->peer_mep_list);
>   	INIT_DELAYED_WORK(&mep->ccm_tx_dwork, ccm_tx_work_expired);
>   
> -	if (hlist_empty(&br->mep_list))
> -		br_add_frame(br, &cfm_frame_type);
> +	if (hlist_empty(&br->mep_list)) {
> +		br->cfm_frame_type.type = cpu_to_be16(ETH_P_CFM);
> +		br->cfm_frame_type.frame_handler = br_cfm_frame_rx;
> +		br_add_frame(br, &br->cfm_frame_type);
> +	}
>   
>   	hlist_add_tail_rcu(&mep->head, &br->mep_list);
>   
> @@ -588,7 +586,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_del_frame(br, &br->cfm_frame_type);
>   }
>   
>   int br_cfm_mep_delete(struct net_bridge *br,
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index bed1b1d9b282..a3ed9ee826f5 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -61,6 +61,9 @@ typedef struct bridge_id bridge_id;
>   typedef struct mac_addr mac_addr;
>   typedef __u16 port_id;
>   
> +struct net_bridge_port;
> +struct sk_buff;
> +
>   struct bridge_id {
>   	unsigned char	prio[2];
>   	unsigned char	addr[ETH_ALEN];
> @@ -70,6 +73,13 @@ struct mac_addr {
>   	unsigned char	addr[ETH_ALEN];
>   };
>   
> +struct br_frame_type {
> +	__be16			type;
> +	int			(*frame_handler)(struct net_bridge_port *port,
> +						 struct sk_buff *skb);
> +	struct hlist_node	list;
> +};
> +
>   #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>   /* our own querier */
>   struct bridge_mcast_own_query {
> @@ -585,6 +595,7 @@ struct net_bridge {
>   	struct hlist_head		mrp_list;
>   #endif
>   #if IS_ENABLED(CONFIG_BRIDGE_CFM)
> +	struct br_frame_type	cfm_frame_type;
>   	struct hlist_head		mep_list;
>   #endif
>   };
> @@ -926,13 +937,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);
>