On Fri, 2020-09-04 at 09:15 +0000, Henrik Bjoernlund wrote:
> This patch extends the processing of frames in the bridge. Currently MRP
> frames needs special processing and the current implementation doesn't
> allow a nice way to process different frame types. Therefore try to
> improve this by adding a list that contains frame types that need
> special processing. This list is iterated for each input frame and if
> there is a match based on frame type then these functions will be called
> and decide what to do with the frame. It can process the frame then the
> bridge doesn't need to do anything or don't process so then the bridge
> will do normal forwarding.
>
> Signed-off-by: Henrik Bjoernlund <henrik.bjoernl...@microchip.com>
> ---
> net/bridge/br_device.c | 1 +
> net/bridge/br_input.c | 31 ++++++++++++++++++++++++++++++-
> net/bridge/br_mrp.c | 19 +++++++++++++++----
> net/bridge/br_private.h | 18 ++++++++++++------
> 4 files changed, 58 insertions(+), 11 deletions(-)
>
Hi,
First I must say I do like this approach, thanks for generalizing it.
You can switch to a hlist so that there's just 1 pointer in the head.
I don't think you need list when you're walking only in one direction.
A few more minor comments below.
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 9a2fb4aa1a10..a9232db03108 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -473,6 +473,7 @@ 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_LIST_HEAD(&br->ftype_list);
> #if IS_ENABLED(CONFIG_BRIDGE_MRP)
> INIT_LIST_HEAD(&br->mrp_list);
> #endif
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 59a318b9f646..0f475b21094c 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -254,6 +254,21 @@ static int nf_hook_bridge_pre(struct sk_buff *skb,
> struct sk_buff **pskb)
> return RX_HANDLER_CONSUMED;
> }
>
> +/* 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;
> +
> + list_for_each_entry_rcu(tmp, &p->br->ftype_list, list) {
> + if (unlikely(tmp->type == skb->protocol))
> + return tmp->func(p, skb);
> + }
Nit: you can drop the {}.
> + return 0;
> +}
> +
> /*
> * Return NULL if skb is handled
> * note: already called with rcu_read_lock
> @@ -343,7 +358,7 @@ static rx_handler_result_t br_handle_frame(struct sk_buff
> **pskb)
> }
> }
>
> - if (unlikely(br_mrp_process(p, skb)))
> + if (unlikely(br_process_frame_type(p, skb)))
> return RX_HANDLER_PASS;
>
> forward:
> @@ -380,3 +395,17 @@ 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)
> +{
> + list_add_rcu(&ft->list, &br->ftype_list);
> +}
> +
> +void br_del_frame(struct net_bridge *br, struct br_frame_type *ft)
> +{
> + struct br_frame_type *tmp;
> +
> + list_for_each_entry(tmp, &br->ftype_list, list)
> + if (ft == tmp)
> + list_del_rcu(&ft->list);
> +}
> diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
> index b36689e6e7cb..0428e1785041 100644
> --- a/net/bridge/br_mrp.c
> +++ b/net/bridge/br_mrp.c
> @@ -6,6 +6,13 @@
> 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),
> + .func = 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)
> @@ -445,6 +452,9 @@ static void br_mrp_del_impl(struct net_bridge *br, struct
> br_mrp *mrp)
>
> list_del_rcu(&mrp->list);
> kfree_rcu(mrp, rcu);
> +
> + if (list_empty(&br->mrp_list))
> + br_del_frame(br, &mrp_frame_type);
> }
>
> /* Adds a new MRP instance.
> @@ -493,6 +503,9 @@ int br_mrp_add(struct net_bridge *br, struct
> br_mrp_instance *instance)
> spin_unlock_bh(&br->lock);
> rcu_assign_pointer(mrp->s_port, p);
>
> + if (list_empty(&br->mrp_list))
> + br_add_frame(br, &mrp_frame_type);
> +
> INIT_DELAYED_WORK(&mrp->test_work, br_mrp_test_work_expired);
> INIT_DELAYED_WORK(&mrp->in_test_work, br_mrp_in_test_work_expired);
> list_add_tail_rcu(&mrp->list, &br->mrp_list);
> @@ -1172,15 +1185,13 @@ static int br_mrp_rcv(struct net_bridge_port *p,
> * normal forwarding.
> * note: already called with rcu_read_lock
> */
> -int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
> +static int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
> {
> /* If there is no MRP instance do normal forwarding */
> if (likely(!(p->flags & BR_MRP_AWARE)))
> goto out;
>
> - if (unlikely(skb->protocol == htons(ETH_P_MRP)))
> - return br_mrp_rcv(p, skb, p->dev);
> -
> + return br_mrp_rcv(p, skb, p->dev);
> out:
> return 0;
> }
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index baa1500f384f..e67c6d9e8bea 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -89,6 +89,13 @@ struct bridge_mcast_stats {
> };
> #endif
>
> +struct br_frame_type {
> + __be16 type;
> + int (*func)(struct net_bridge_port *port,
> + struct sk_buff *skb);
Perhaps frame_handler or something similar would be a better name for the
callback ?
> + struct list_head list;
> +};
> +
> struct br_vlan_stats {
> u64 rx_bytes;
> u64 rx_packets;
> @@ -433,6 +440,8 @@ struct net_bridge {
> #endif
> struct hlist_head fdb_list;
>
> + struct list_head ftype_list;
Could you please expand this to frame_type_list ?
> +
> #if IS_ENABLED(CONFIG_BRIDGE_MRP)
> struct list_head mrp_list;
> #endif
> @@ -708,6 +717,9 @@ 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);
>
> +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);
> @@ -1320,7 +1332,6 @@ extern int (*br_fdb_test_addr_hook)(struct net_device
> *dev, unsigned char *addr)
> #if IS_ENABLED(CONFIG_BRIDGE_MRP)
> int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
> struct nlattr *attr, int cmd, struct netlink_ext_ack *extack);
> -int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb);
> 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);
> @@ -1332,11 +1343,6 @@ static inline int br_mrp_parse(struct net_bridge *br,
> struct net_bridge_port *p,
> return -EOPNOTSUPP;
> }
>
> -static inline int br_mrp_process(struct net_bridge_port *p, struct sk_buff
> *skb)
> -{
> - return 0;
> -}
> -
> static inline bool br_mrp_enabled(struct net_bridge *br)
> {
> return false;
