On Sat, 2020-10-17 at 21:41 +0300, Vladimir Oltean wrote:
> From: Nikolay Aleksandrov <niko...@nvidia.com>
>
> Extend the bridge multicast control and data path to configure routes
> for L2 (non-IP) multicast groups.
>
> The uapi struct br_mdb_entry union u is extended with another variant,
> interpretation, mac_addr, which does not change the structure size, and
> which is valid when the MDB_FLAGS_L2 flag is found set.
>
> To be compatible with the forwarding code that is already in place,
> which acts as an IGMP/MLD snooping bridge with querier capabilities, we
> need to declare that for L2 MDB entries (for which there exists no such
> thing as IGMP/MLD snooping/querying), that there is always a querier.
> Otherwise, these entries would be flooded to all bridge ports and not
> just to those that are members of the L2 multicast group.
>
> Needless to say, only permanent L2 multicast groups can be installed on
> a bridge port.
>
> Signed-off-by: Nikolay Aleksandrov <niko...@nvidia.com>
> Signed-off-by: Vladimir Oltean <vladimir.olt...@nxp.com>
> ---
> This patch is adapted from the version that Nikolay posted here:
> https://lore.kernel.org/netdev/20200708090454.zvb6o7jr2woirw3i@skbuf/
>
> There, he marked the patch as "unfinished". I haven't made any major
> modifications to it, but I've tested it and it appears to work ok,
> including with offloading. Hence, I would appreciate some tips regarding
> things that might be missing.
>
Hi,
I almost missed this one, thank you for fixing it up. I was wondering if we
can move br_ip's mac_addr in the "dst" union to save some space and reduce
ops when matching, since we're also matching on the protocol field. In general
do we need the ->l2 field at all, can we use proto == 0 ? In order to make it
more readable it can be in a helper with a descriptive name so we don't wonder
what proto == 0 meant later. A few more minor comments below.
> include/linux/if_bridge.h | 1 +
> include/uapi/linux/if_bridge.h | 2 ++
> net/bridge/br_device.c | 2 +-
> net/bridge/br_input.c | 2 +-
> net/bridge/br_mdb.c | 24 ++++++++++++++++++++----
> net/bridge/br_multicast.c | 12 ++++++++++--
> net/bridge/br_private.h | 7 +++++--
> 7 files changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index 556caed00258..b135ad714383 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -26,6 +26,7 @@ struct br_ip {
> struct in6_addr ip6;
> #endif
> } dst;
> + unsigned char mac_addr[ETH_ALEN];
> __be16 proto;
> __u16 vid;
> };
> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> index 4c687686aa8f..a25f6f9aa8c3 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -520,12 +520,14 @@ struct br_mdb_entry {
> #define MDB_FLAGS_FAST_LEAVE (1 << 1)
> #define MDB_FLAGS_STAR_EXCL (1 << 2)
> #define MDB_FLAGS_BLOCKED (1 << 3)
> +#define MDB_FLAGS_L2 (1 << 5)
I think this should be 4.
> __u8 flags;
> __u16 vid;
> struct {
> union {
> __be32 ip4;
> struct in6_addr ip6;
> + unsigned char mac_addr[ETH_ALEN];
> } u;
> __be16 proto;
> } addr;
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 6f742fee874a..06c28753b911 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -93,7 +93,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct
> net_device *dev)
>
> mdst = br_mdb_get(br, skb, vid);
> if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
> - br_multicast_querier_exists(br, eth_hdr(skb)))
> + br_multicast_querier_exists(br, eth_hdr(skb), mdst))
> br_multicast_flood(mdst, skb, false, true);
> else
> br_flood(br, skb, BR_PKT_MULTICAST, false, true);
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 59a318b9f646..d31b5c18c6a1 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -134,7 +134,7 @@ int br_handle_frame_finish(struct net *net, struct sock
> *sk, struct sk_buff *skb
> case BR_PKT_MULTICAST:
> mdst = br_mdb_get(br, skb, vid);
> if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
> - br_multicast_querier_exists(br, eth_hdr(skb))) {
> + br_multicast_querier_exists(br, eth_hdr(skb), mdst)) {
> if ((mdst && mdst->host_joined) ||
> br_multicast_is_router(br)) {
> local_rcv = true;
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index e15bab19a012..4decf3eb7001 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -66,6 +66,8 @@ static void __mdb_entry_fill_flags(struct br_mdb_entry *e,
> unsigned char flags)
> e->flags |= MDB_FLAGS_STAR_EXCL;
> if (flags & MDB_PG_FLAGS_BLOCKED)
> e->flags |= MDB_FLAGS_BLOCKED;
> + if (flags & MDB_PG_FLAGS_L2)
> + e->flags |= MDB_FLAGS_L2;
> }
>
> static void __mdb_entry_to_br_ip(struct br_mdb_entry *entry, struct br_ip
> *ip,
> @@ -87,6 +89,8 @@ static void __mdb_entry_to_br_ip(struct br_mdb_entry
> *entry, struct br_ip *ip,
> ip->src.ip6 =
> nla_get_in6_addr(mdb_attrs[MDBE_ATTR_SOURCE]);
> break;
> #endif
> + default:
> + ether_addr_copy(ip->mac_addr, entry->addr.u.mac_addr);
> }
>
> }
> @@ -174,9 +178,11 @@ static int __mdb_fill_info(struct sk_buff *skb,
> if (mp->addr.proto == htons(ETH_P_IP))
> e.addr.u.ip4 = mp->addr.dst.ip4;
> #if IS_ENABLED(CONFIG_IPV6)
> - if (mp->addr.proto == htons(ETH_P_IPV6))
> + else if (mp->addr.proto == htons(ETH_P_IPV6))
> e.addr.u.ip6 = mp->addr.dst.ip6;
> #endif
> + else
> + ether_addr_copy(e.addr.u.mac_addr, mp->addr.mac_addr);
> e.addr.proto = mp->addr.proto;
> nest_ent = nla_nest_start_noflag(skb,
> MDBA_MDB_ENTRY_INFO);
> @@ -210,6 +216,8 @@ static int __mdb_fill_info(struct sk_buff *skb,
> }
> break;
> #endif
> + default:
> + ether_addr_copy(e.addr.u.mac_addr, mp->addr.mac_addr);
> }
> if (p) {
> if (nla_put_u8(skb, MDBA_MDB_EATTR_RTPROT, p->rt_protocol))
> @@ -562,9 +570,12 @@ void br_mdb_notify(struct net_device *dev,
> if (mp->addr.proto == htons(ETH_P_IP))
> ip_eth_mc_map(mp->addr.dst.ip4, mdb.addr);
> #if IS_ENABLED(CONFIG_IPV6)
> - else
> + else if (mp->addr.proto == htons(ETH_P_IPV6))
> ipv6_eth_mc_map(&mp->addr.dst.ip6, mdb.addr);
> #endif
> + else
> + ether_addr_copy(mdb.addr, mp->addr.mac_addr);
> +
> mdb.obj.orig_dev = pg->key.port->dev;
> switch (type) {
> case RTM_NEWMDB:
> @@ -693,7 +704,7 @@ static bool is_valid_mdb_entry(struct br_mdb_entry *entry,
> return false;
> }
> #endif
> - } else {
> + } else if (entry->addr.proto != 0) {
> NL_SET_ERR_MSG_MOD(extack, "Unknown entry protocol");
> return false;
> }
> @@ -857,6 +868,11 @@ static int br_mdb_add_group(struct net_bridge *br,
> struct net_bridge_port *port,
> return err;
> }
>
> + if (entry->state != MDB_PERMANENT && mp->l2) {
> + NL_SET_ERR_MSG_MOD(extack, "Only permanent L2 entries allowed");
> + return -EINVAL;
> + }
> +
> /* host join */
> if (!port) {
> if (mp->host_joined) {
> @@ -891,7 +907,7 @@ static int br_mdb_add_group(struct net_bridge *br, struct
> net_bridge_port *port,
> return -ENOMEM;
> }
> rcu_assign_pointer(*pp, p);
> - if (entry->state == MDB_TEMPORARY)
> + if (entry->state == MDB_TEMPORARY && !mp->l2)
> mod_timer(&p->timer, now + br->multicast_membership_interval);
> br_mdb_notify(br->dev, mp, p, RTM_NEWMDB);
> /* if we are adding a new EXCLUDE port group (*,G) it needs to be also
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index eae898c3cff7..bc03057e7caf 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -179,7 +179,8 @@ struct net_bridge_mdb_entry *br_mdb_get(struct net_bridge
> *br,
> break;
> #endif
> default:
> - return NULL;
> + ip.proto = 0;
> + ether_addr_copy(ip.mac_addr, eth_hdr(skb)->h_dest);
> }
>
> return br_mdb_ip_get_rcu(br, &ip);
> @@ -1050,6 +1051,7 @@ struct net_bridge_mdb_entry
> *br_multicast_new_group(struct net_bridge *br,
>
> mp->br = br;
> mp->addr = *group;
> + mp->l2 = !!(group->proto == 0);
> mp->mcast_gc.destroy = br_multicast_destroy_mdb_entry;
> timer_setup(&mp->timer, br_multicast_group_expired, 0);
> err = rhashtable_lookup_insert_fast(&br->mdb_hash_tbl, &mp->rhnode,
> @@ -1169,6 +1171,8 @@ struct net_bridge_port_group
> *br_multicast_new_port_group(
> p->key.addr = *group;
> p->key.port = port;
> p->flags = flags;
> + if (group->proto == htons(0))
> + p->flags |= MDB_PG_FLAGS_L2;
Can we pass the flag from the caller? This kind of beats the purpose of
receiving the flags as an argument. :)
> p->filter_mode = filter_mode;
> p->rt_protocol = rt_protocol;
> p->mcast_gc.destroy = br_multicast_destroy_port_group;
> @@ -1203,6 +1207,10 @@ void br_multicast_host_join(struct
> net_bridge_mdb_entry *mp, bool notify)
> if (notify)
> br_mdb_notify(mp->br->dev, mp, NULL, RTM_NEWMDB);
> }
> +
> + if (mp->l2)
> + return;
> +
> mod_timer(&mp->timer, jiffies + mp->br->multicast_membership_interval);
> }
>
> @@ -3690,7 +3698,7 @@ bool br_multicast_has_querier_anywhere(struct
> net_device *dev, int proto)
> memset(ð, 0, sizeof(eth));
> eth.h_proto = htons(proto);
>
> - ret = br_multicast_querier_exists(br, ð);
> + ret = br_multicast_querier_exists(br, ð, NULL);
>
> unlock:
> rcu_read_unlock();
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 345118e35c42..63a98c1af351 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -215,6 +215,7 @@ struct net_bridge_fdb_entry {
> #define MDB_PG_FLAGS_FAST_LEAVE BIT(2)
> #define MDB_PG_FLAGS_STAR_EXCL BIT(3)
> #define MDB_PG_FLAGS_BLOCKED BIT(4)
> +#define MDB_PG_FLAGS_L2 BIT(5)
>
> #define PG_SRC_ENT_LIMIT 32
>
> @@ -272,6 +273,7 @@ struct net_bridge_mdb_entry {
> struct net_bridge_port_group __rcu *ports;
> struct br_ip addr;
> bool host_joined;
> + bool l2;
>
> struct timer_list timer;
> struct hlist_node mdb_node;
> @@ -871,7 +873,8 @@ __br_multicast_querier_exists(struct net_bridge *br,
> }
>
> static inline bool br_multicast_querier_exists(struct net_bridge *br,
> - struct ethhdr *eth)
> + struct ethhdr *eth,
> + const struct
> net_bridge_mdb_entry *mdb)
> {
> switch (eth->h_proto) {
> case (htons(ETH_P_IP)):
> @@ -883,7 +886,7 @@ static inline bool br_multicast_querier_exists(struct
> net_bridge *br,
> &br->ip6_other_query, true);
> #endif
> default:
> - return false;
> + return !!(mdb && mdb->l2);
> }
> }
>
