All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] net: bridge: multicast: add support for L2 entries
@ 2020-10-17 18:41 ` Vladimir Oltean
  0 siblings, 0 replies; 13+ messages in thread
From: Vladimir Oltean @ 2020-10-17 18:41 UTC (permalink / raw)
  To: Roopa Prabhu, Nikolay Aleksandrov, David S. Miller,
	Jakub Kicinski, bridge, netdev, linux-kernel
  Cc: andrew, f.fainelli, vivien.didelot, jiri, idosch

From: Nikolay Aleksandrov <nikolay@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 <nikolay@nvidia.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@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.

 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)
 	__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;
 	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(&eth, 0, sizeof(eth));
 	eth.h_proto = htons(proto);
 
-	ret = br_multicast_querier_exists(br, &eth);
+	ret = br_multicast_querier_exists(br, &eth, 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);
 	}
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Bridge] [RFC PATCH] net: bridge: multicast: add support for L2 entries
@ 2020-10-17 18:41 ` Vladimir Oltean
  0 siblings, 0 replies; 13+ messages in thread
From: Vladimir Oltean @ 2020-10-17 18:41 UTC (permalink / raw)
  To: Roopa Prabhu, Nikolay Aleksandrov, David S. Miller,
	Jakub Kicinski, bridge, netdev, linux-kernel
  Cc: andrew, jiri, f.fainelli, vivien.didelot, idosch

From: Nikolay Aleksandrov <nikolay@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 <nikolay@nvidia.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@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.

 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)
 	__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;
 	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(&eth, 0, sizeof(eth));
 	eth.h_proto = htons(proto);
 
-	ret = br_multicast_querier_exists(br, &eth);
+	ret = br_multicast_querier_exists(br, &eth, 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);
 	}
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH] net: bridge: multicast: add support for L2 entries
  2020-10-17 18:41 ` [Bridge] " Vladimir Oltean
  (?)
@ 2020-10-20  5:08 ` kernel test robot
  -1 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2020-10-20  5:08 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 5743 bytes --]

Hi Vladimir,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on linus/master]
[also build test ERROR on next-20201016]
[cannot apply to v5.9]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vladimir-Oltean/net-bridge-multicast-add-support-for-L2-entries/20201018-024416
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 071a0578b0ce0b0e543d1e38ee6926b9cc21c198
config: x86_64-randconfig-a005-20201019 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/1de352fe0a06780ac744ba8642c518dccd1cc3d1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vladimir-Oltean/net-bridge-multicast-add-support-for-L2-entries/20201018-024416
        git checkout 1de352fe0a06780ac744ba8642c518dccd1cc3d1
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/bridge/br_device.c: In function 'br_dev_xmit':
>> net/bridge/br_device.c:96:7: error: too many arguments to function 'br_multicast_querier_exists'
      96 |       br_multicast_querier_exists(br, eth_hdr(skb), mdst))
         |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from net/bridge/br_device.c:19:
   net/bridge/br_private.h:998:20: note: declared here
     998 | static inline bool br_multicast_querier_exists(struct net_bridge *br,
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
--
   net/bridge/br_input.c: In function 'br_handle_frame_finish':
>> net/bridge/br_input.c:137:7: error: too many arguments to function 'br_multicast_querier_exists'
     137 |       br_multicast_querier_exists(br, eth_hdr(skb), mdst)) {
         |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from net/bridge/br_input.c:23:
   net/bridge/br_private.h:998:20: note: declared here
     998 | static inline bool br_multicast_querier_exists(struct net_bridge *br,
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/br_multicast_querier_exists +96 net/bridge/br_device.c

    26	
    27	/* net device transmit always called with BH disabled */
    28	netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
    29	{
    30		struct net_bridge *br = netdev_priv(dev);
    31		struct net_bridge_fdb_entry *dst;
    32		struct net_bridge_mdb_entry *mdst;
    33		struct pcpu_sw_netstats *brstats = this_cpu_ptr(br->stats);
    34		const struct nf_br_ops *nf_ops;
    35		u8 state = BR_STATE_FORWARDING;
    36		const unsigned char *dest;
    37		u16 vid = 0;
    38	
    39		memset(skb->cb, 0, sizeof(struct br_input_skb_cb));
    40	
    41		rcu_read_lock();
    42		nf_ops = rcu_dereference(nf_br_ops);
    43		if (nf_ops && nf_ops->br_dev_xmit_hook(skb)) {
    44			rcu_read_unlock();
    45			return NETDEV_TX_OK;
    46		}
    47	
    48		u64_stats_update_begin(&brstats->syncp);
    49		brstats->tx_packets++;
    50		brstats->tx_bytes += skb->len;
    51		u64_stats_update_end(&brstats->syncp);
    52	
    53		br_switchdev_frame_unmark(skb);
    54		BR_INPUT_SKB_CB(skb)->brdev = dev;
    55		BR_INPUT_SKB_CB(skb)->frag_max_size = 0;
    56	
    57		skb_reset_mac_header(skb);
    58		skb_pull(skb, ETH_HLEN);
    59	
    60		if (!br_allowed_ingress(br, br_vlan_group_rcu(br), skb, &vid, &state))
    61			goto out;
    62	
    63		if (IS_ENABLED(CONFIG_INET) &&
    64		    (eth_hdr(skb)->h_proto == htons(ETH_P_ARP) ||
    65		     eth_hdr(skb)->h_proto == htons(ETH_P_RARP)) &&
    66		    br_opt_get(br, BROPT_NEIGH_SUPPRESS_ENABLED)) {
    67			br_do_proxy_suppress_arp(skb, br, vid, NULL);
    68		} else if (IS_ENABLED(CONFIG_IPV6) &&
    69			   skb->protocol == htons(ETH_P_IPV6) &&
    70			   br_opt_get(br, BROPT_NEIGH_SUPPRESS_ENABLED) &&
    71			   pskb_may_pull(skb, sizeof(struct ipv6hdr) +
    72					 sizeof(struct nd_msg)) &&
    73			   ipv6_hdr(skb)->nexthdr == IPPROTO_ICMPV6) {
    74				struct nd_msg *msg, _msg;
    75	
    76				msg = br_is_nd_neigh_msg(skb, &_msg);
    77				if (msg)
    78					br_do_suppress_nd(skb, br, vid, NULL, msg);
    79		}
    80	
    81		dest = eth_hdr(skb)->h_dest;
    82		if (is_broadcast_ether_addr(dest)) {
    83			br_flood(br, skb, BR_PKT_BROADCAST, false, true);
    84		} else if (is_multicast_ether_addr(dest)) {
    85			if (unlikely(netpoll_tx_running(dev))) {
    86				br_flood(br, skb, BR_PKT_MULTICAST, false, true);
    87				goto out;
    88			}
    89			if (br_multicast_rcv(br, NULL, skb, vid)) {
    90				kfree_skb(skb);
    91				goto out;
    92			}
    93	
    94			mdst = br_mdb_get(br, skb, vid);
    95			if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
  > 96			    br_multicast_querier_exists(br, eth_hdr(skb), mdst))
    97				br_multicast_flood(mdst, skb, false, true);
    98			else
    99				br_flood(br, skb, BR_PKT_MULTICAST, false, true);
   100		} else if ((dst = br_fdb_find_rcu(br, dest, vid)) != NULL) {
   101			br_forward(dst->dst, skb, false, true);
   102		} else {
   103			br_flood(br, skb, BR_PKT_UNICAST, false, true);
   104		}
   105	out:
   106		rcu_read_unlock();
   107		return NETDEV_TX_OK;
   108	}
   109	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 39158 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH] net: bridge: multicast: add support for L2 entries
  2020-10-17 18:41 ` [Bridge] " Vladimir Oltean
@ 2020-10-21  9:17   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 13+ messages in thread
From: Nikolay Aleksandrov @ 2020-10-21  9:17 UTC (permalink / raw)
  To: linux-kernel, bridge, netdev, Roopa Prabhu, davem, vladimir.oltean, kuba
  Cc: vivien.didelot, jiri, idosch, f.fainelli, andrew

On Sat, 2020-10-17 at 21:41 +0300, Vladimir Oltean wrote:
> From: Nikolay Aleksandrov <nikolay@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 <nikolay@nvidia.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@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(&eth, 0, sizeof(eth));
>  	eth.h_proto = htons(proto);
>  
> -	ret = br_multicast_querier_exists(br, &eth);
> +	ret = br_multicast_querier_exists(br, &eth, 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);
>  	}
>  }
>  


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Bridge] [RFC PATCH] net: bridge: multicast: add support for L2 entries
@ 2020-10-21  9:17   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 13+ messages in thread
From: Nikolay Aleksandrov @ 2020-10-21  9:17 UTC (permalink / raw)
  To: linux-kernel, bridge, netdev, Roopa Prabhu, davem, vladimir.oltean, kuba
  Cc: idosch, jiri, f.fainelli, vivien.didelot, andrew

On Sat, 2020-10-17 at 21:41 +0300, Vladimir Oltean wrote:
> From: Nikolay Aleksandrov <nikolay@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 <nikolay@nvidia.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@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(&eth, 0, sizeof(eth));
>  	eth.h_proto = htons(proto);
>  
> -	ret = br_multicast_querier_exists(br, &eth);
> +	ret = br_multicast_querier_exists(br, &eth, 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);
>  	}
>  }
>  


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH] net: bridge: multicast: add support for L2 entries
  2020-10-21  9:17   ` [Bridge] " Nikolay Aleksandrov
@ 2020-10-21  9:25     ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 13+ messages in thread
From: Nikolay Aleksandrov @ 2020-10-21  9:25 UTC (permalink / raw)
  To: vladimir.oltean, bridge, kuba, linux-kernel, netdev, davem, Roopa Prabhu
  Cc: vivien.didelot, jiri, idosch, f.fainelli, andrew

On Wed, 2020-10-21 at 09:17 +0000, Nikolay Aleksandrov wrote:
> On Sat, 2020-10-17 at 21:41 +0300, Vladimir Oltean wrote:
> > From: Nikolay Aleksandrov <nikolay@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 <nikolay@nvidia.com>
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@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.
> 

Oh, one more thing, I don't think we validate that the dst mac that's being
added is actually a multicast one.

> >  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(&eth, 0, sizeof(eth));
> >  	eth.h_proto = htons(proto);
> >  
> > -	ret = br_multicast_querier_exists(br, &eth);
> > +	ret = br_multicast_querier_exists(br, &eth, 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);
> >  	}
> >  }
> >  


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Bridge] [RFC PATCH] net: bridge: multicast: add support for L2 entries
@ 2020-10-21  9:25     ` Nikolay Aleksandrov
  0 siblings, 0 replies; 13+ messages in thread
From: Nikolay Aleksandrov @ 2020-10-21  9:25 UTC (permalink / raw)
  To: vladimir.oltean, bridge, kuba, linux-kernel, netdev, davem, Roopa Prabhu
  Cc: idosch, jiri, f.fainelli, vivien.didelot, andrew

On Wed, 2020-10-21 at 09:17 +0000, Nikolay Aleksandrov wrote:
> On Sat, 2020-10-17 at 21:41 +0300, Vladimir Oltean wrote:
> > From: Nikolay Aleksandrov <nikolay@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 <nikolay@nvidia.com>
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@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.
> 

Oh, one more thing, I don't think we validate that the dst mac that's being
added is actually a multicast one.

> >  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(&eth, 0, sizeof(eth));
> >  	eth.h_proto = htons(proto);
> >  
> > -	ret = br_multicast_querier_exists(br, &eth);
> > +	ret = br_multicast_querier_exists(br, &eth, 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);
> >  	}
> >  }
> >  


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH] net: bridge: multicast: add support for L2 entries
  2020-10-21  9:17   ` [Bridge] " Nikolay Aleksandrov
@ 2020-10-25  6:59     ` Vladimir Oltean
  -1 siblings, 0 replies; 13+ messages in thread
From: Vladimir Oltean @ 2020-10-25  6:59 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: linux-kernel, bridge, netdev, Roopa Prabhu, davem, kuba,
	vivien.didelot, jiri, idosch, f.fainelli, andrew

On Wed, Oct 21, 2020 at 09:17:07AM +0000, Nikolay Aleksandrov wrote:
> > 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.
> 

Shouldn't this be in sync with MDB_PG_FLAGS_L2 though? We also have
MDB_PG_FLAGS_BLOCKED which is BIT(4).

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Bridge] [RFC PATCH] net: bridge: multicast: add support for L2 entries
@ 2020-10-25  6:59     ` Vladimir Oltean
  0 siblings, 0 replies; 13+ messages in thread
From: Vladimir Oltean @ 2020-10-25  6:59 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: andrew, f.fainelli, netdev, bridge, linux-kernel, vivien.didelot,
	idosch, jiri, Roopa Prabhu, kuba, davem

On Wed, Oct 21, 2020 at 09:17:07AM +0000, Nikolay Aleksandrov wrote:
> > 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.
> 

Shouldn't this be in sync with MDB_PG_FLAGS_L2 though? We also have
MDB_PG_FLAGS_BLOCKED which is BIT(4).

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH] net: bridge: multicast: add support for L2 entries
  2020-10-25  6:59     ` [Bridge] " Vladimir Oltean
@ 2020-10-25  7:42       ` Vladimir Oltean
  -1 siblings, 0 replies; 13+ messages in thread
From: Vladimir Oltean @ 2020-10-25  7:42 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: linux-kernel, bridge, netdev, Roopa Prabhu, davem, kuba,
	vivien.didelot, jiri, idosch, f.fainelli, andrew

On Sun, Oct 25, 2020 at 08:59:57AM +0200, Vladimir Oltean wrote:
> On Wed, Oct 21, 2020 at 09:17:07AM +0000, Nikolay Aleksandrov wrote:
> > > 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.
> > 
> 
> Shouldn't this be in sync with MDB_PG_FLAGS_L2 though? We also have
> MDB_PG_FLAGS_BLOCKED which is BIT(4).

Never mind, I'll make it 4.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Bridge] [RFC PATCH] net: bridge: multicast: add support for L2 entries
@ 2020-10-25  7:42       ` Vladimir Oltean
  0 siblings, 0 replies; 13+ messages in thread
From: Vladimir Oltean @ 2020-10-25  7:42 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: andrew, f.fainelli, netdev, bridge, linux-kernel, vivien.didelot,
	idosch, jiri, Roopa Prabhu, kuba, davem

On Sun, Oct 25, 2020 at 08:59:57AM +0200, Vladimir Oltean wrote:
> On Wed, Oct 21, 2020 at 09:17:07AM +0000, Nikolay Aleksandrov wrote:
> > > 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.
> > 
> 
> Shouldn't this be in sync with MDB_PG_FLAGS_L2 though? We also have
> MDB_PG_FLAGS_BLOCKED which is BIT(4).

Never mind, I'll make it 4.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH] net: bridge: multicast: add support for L2 entries
  2020-10-25  6:59     ` [Bridge] " Vladimir Oltean
@ 2020-10-25 10:49       ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 13+ messages in thread
From: Nikolay Aleksandrov @ 2020-10-25 10:49 UTC (permalink / raw)
  To: vladimir.oltean
  Cc: idosch, bridge, davem, andrew, vivien.didelot, linux-kernel,
	jiri, kuba, Roopa Prabhu, netdev, f.fainelli

On Sun, 2020-10-25 at 06:59 +0000, Vladimir Oltean wrote:
> On Wed, Oct 21, 2020 at 09:17:07AM +0000, Nikolay Aleksandrov wrote:
> > > 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.
> > 
> 
> Shouldn't this be in sync with MDB_PG_FLAGS_L2 though? We also have
> MDB_PG_FLAGS_BLOCKED which is BIT(4).

Unfortunately they haven't been in sync from the start. MDB_FLAGS bit
0 is offload, while MDB_PG_FLAGS bit 0 is permanent. As you can see
here blocked is bit 3, while internally it's 4 due to the same reason.
We can't afford to skip 1 bit since this is uAPI and we only got 8 
available bits. I wonder if we need these L2 bits at all, why not use
only proto == 0 to denote it's a L2 entry? I can't remember why I added
the bits back then, but until now proto == 0 wasn't allowed and the
kernel couldn't export it as such, so it seems possible to use it.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Bridge] [RFC PATCH] net: bridge: multicast: add support for L2 entries
@ 2020-10-25 10:49       ` Nikolay Aleksandrov
  0 siblings, 0 replies; 13+ messages in thread
From: Nikolay Aleksandrov @ 2020-10-25 10:49 UTC (permalink / raw)
  To: vladimir.oltean
  Cc: andrew, f.fainelli, netdev, bridge, linux-kernel, vivien.didelot,
	idosch, jiri, Roopa Prabhu, kuba, davem

On Sun, 2020-10-25 at 06:59 +0000, Vladimir Oltean wrote:
> On Wed, Oct 21, 2020 at 09:17:07AM +0000, Nikolay Aleksandrov wrote:
> > > 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.
> > 
> 
> Shouldn't this be in sync with MDB_PG_FLAGS_L2 though? We also have
> MDB_PG_FLAGS_BLOCKED which is BIT(4).

Unfortunately they haven't been in sync from the start. MDB_FLAGS bit
0 is offload, while MDB_PG_FLAGS bit 0 is permanent. As you can see
here blocked is bit 3, while internally it's 4 due to the same reason.
We can't afford to skip 1 bit since this is uAPI and we only got 8 
available bits. I wonder if we need these L2 bits at all, why not use
only proto == 0 to denote it's a L2 entry? I can't remember why I added
the bits back then, but until now proto == 0 wasn't allowed and the
kernel couldn't export it as such, so it seems possible to use it.





^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-10-25 10:49 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-17 18:41 [RFC PATCH] net: bridge: multicast: add support for L2 entries Vladimir Oltean
2020-10-17 18:41 ` [Bridge] " Vladimir Oltean
2020-10-20  5:08 ` kernel test robot
2020-10-21  9:17 ` Nikolay Aleksandrov
2020-10-21  9:17   ` [Bridge] " Nikolay Aleksandrov
2020-10-21  9:25   ` Nikolay Aleksandrov
2020-10-21  9:25     ` [Bridge] " Nikolay Aleksandrov
2020-10-25  6:59   ` Vladimir Oltean
2020-10-25  6:59     ` [Bridge] " Vladimir Oltean
2020-10-25  7:42     ` Vladimir Oltean
2020-10-25  7:42       ` [Bridge] " Vladimir Oltean
2020-10-25 10:49     ` Nikolay Aleksandrov
2020-10-25 10:49       ` [Bridge] " Nikolay Aleksandrov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.