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

On Sun, 10 May 2026 at 04:40, Nikolay Aleksandrov <[email protected]> wrote:
>
> On 10/05/2026 13:05, Nikolay Aleksandrov wrote:
> > On 10/05/2026 12:15, Ren Wei wrote:
> >> From: Yilin Zhu <[email protected]>
> >>
> >> 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: [email protected]
> >> Reported-by: Yuan Tan <[email protected]>
> >> Reported-by: Yifan Wu <[email protected]>
> >> Reported-by: Juefei Pu <[email protected]>
> >> Reported-by: Xin Liu <[email protected]>
> >> Co-developed-by: Peihan Liu <[email protected]>
> >> Signed-off-by: Peihan Liu <[email protected]>
> >> Signed-off-by: Yilin Zhu <[email protected]>
> >> Signed-off-by: Ren Wei <[email protected]>
> >> ---
> >>   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.
> >
>
> Actually I have a better idea since these handlers can be added only once per
> bridge, we can do away with a simple bitmask (e.g. use net_bridge's options
> which is in a Rx hot cache line) and even reduce net_bridge size while fixing
> these bugs, and also remove a conditional from the fast-path when CFM/MRP are
> not compiled in. Would you like me to prepare it or do you want to give it a go?
>

Hi Nik,

Thanks for the suggestion.

I'll give it a go. IIUC, the idea is to remove the shared
br_frame_type list entries for CFM/MRP, track their per-bridge enable
state with bridge option bits, and dispatch the CFM/MRP handlers directly
from the receive path when the corresponding EtherType and option bit
match.

I'll include MRP in v2.

Thanks,
Yilin

> > 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);
> >
>