net: bridge: multicast querier per VLAN support

From what I understand, multicast join/report already support per VLAN

This RFC patch¹ is an attempt to add multicast querier per VLAN support to a VLAN aware bridge. I'm posting it as RFC for now since non-VLAN aware bridges are not handled, and one of my questions is if that is complexity we need to continue supporting?

From what I understand, multicast join/report already support per VLAN
operation, and the MDB as well support filtering per VLAN, but queries are currently limited to per-port operation on VLAN-aware bridges.

The naive² approach of this patch relocates query timers from the bridge to operate per VLAN, on timer expiry we send queries to all bridge ports in the same VLAN. Tagged port members have tagged VLAN queries.

Unlike the original patch¹, which uses a sysfs entry to set the querier address of each VLAN, this use the IP address of the VLAN interface when initiating a per VLAN query. A version of inet_select_addr() is used for this, called inet_select_dev_addr(), not included in this patch.

Open questions/TODO:

- First of all, is this patch useful to anyone

- The current br_multicast.c is very complex. The support for both IPv4 and IPv6 is a no-brainer, but it also has #ifdef VLAN_FILTERING and 'br->vlan_enabled' ... this has likely been discussed before, but if we could remove those code paths I believe what's left would be quite a bit easier to read and maintain.

- Many per-bridge specific multicast sysfs settings may need to have a corresponding per-VLAN setting, e.g. snooping, query_interval, etc. How should we go about that? (For status reporting I have a proposal)

- Dito per-port specific multicast sysfs settings, e.g. multicast_router

- The MLD support has been kept in sync with the rest but is completely untested. In particular I suspect the wrong source IP will be used.

¹) Initially based on a patch by Cumulus Networks

    http://repo3.cumulusnetworks.com/repo/pool/cumulus/l/linux/linux-source-4.1_4.1.33-1+cl3u11_all.deb

²) This patch is currently limited to work only on bridges with VLAN
enabled. Care has been taken to support MLD snooping, but it is completely untested.

Thank you for reading this far!

Signed-off-by Joachim Nilsson <[email protected]>

---
  net/bridge/br_device.c    |   2 +-
  net/bridge/br_input.c     |   2 +-
  net/bridge/br_multicast.c | 456 ++++++++++++++++++++++++--------------
  net/bridge/br_private.h   |  38 +++-
  net/bridge/br_stp.c       |   5 +-
  net/bridge/br_vlan.c      |   3 +
  6 files changed, 327 insertions(+), 179 deletions(-)

net/bridge/br_device.c +1 -1

index 02f9f8aab047..ba35485032d8 100644
@@ -98,7 +98,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, vid, eth_hdr(skb)))
  			br_multicast_flood(mdst, skb, false, true);
  		else
  			br_flood(br, skb, BR_PKT_MULTICAST, false, true);

net/bridge/br_input.c +1 -1

index 56bb9189c374..13d48489e0e1 100644
@@ -137,7 +137,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
  		mdst = br_mdb_get(br, skb, vid);
  		if ((mdst && mdst->addr.proto == htons(ETH_P_ALL)) ||
  		    ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
-		     br_multicast_querier_exists(br, eth_hdr(skb)))) {
+		     br_multicast_querier_exists(br, vid, eth_hdr(skb)))) {
  			if ((mdst && mdst->host_joined) ||
  			    br_multicast_is_router(br)) {
  				local_rcv = true;

net/bridge/br_multicast.c +295 -161

index 277ecd077dc4..72e47d500972 100644
@@ -13,6 +13,7 @@
  #include <linux/err.h>
  #include <linux/export.h>
  #include <linux/if_ether.h>
+#include <linux/if_vlan.h>
  #include <linux/igmp.h>
  #include <linux/jhash.h>
  #include <linux/kernel.h>

@@ -37,7 +38,7 @@
 
  #include "br_private.h"
 
-static void br_multicast_start_querier(struct net_bridge *br,
+static void br_multicast_start_querier(struct net_bridge_vlan *vlan,
  				       struct bridge_mcast_own_query *query);
  static void br_multicast_add_router(struct net_bridge *br,
  				    struct net_bridge_port *port);

@@ -46,13 +47,14 @@ static void br_ip4_multicast_leave_group(struct net_bridge *br,
  					 __be32 group,
  					 __u16 vid,
  					 const unsigned char *src);
-
+static void br_ip4_multicast_query_expired(struct timer_list *t);
  static void __del_port_router(struct net_bridge_port *p);
  #if IS_ENABLED(CONFIG_IPV6)
  static void br_ip6_multicast_leave_group(struct net_bridge *br,
  					 struct net_bridge_port *port,
  					 const struct in6_addr *group,
  					 __u16 vid, const unsigned char *src);
+static void br_ip6_multicast_query_expired(struct timer_list *t);
  #endif
  unsigned int br_mdb_rehash_seq;
 

@@ -381,8 +383,30 @@ static int br_mdb_rehash(struct net_bridge_mdb_htable __rcu **mdbp, int max,
  	return 0;
  }
 
+__be32 br_multicast_inet_addr(struct net_bridge *br, u16 vid)
+{
+	struct net_device *dev;
+
+	if (!br->multicast_query_use_ifaddr)
+		return 0;
+
+	if (!vid)
+		return inet_select_addr(br->dev, 0, RT_SCOPE_LINK);
+
+	rcu_read_lock();
+	dev = __vlan_find_dev_deep_rcu(br->dev, htons(ETH_P_8021Q), vid);
+	rcu_read_unlock();
+
+	if (!dev)
+		return 0;
+
+	return inet_select_dev_addr(dev, 0, RT_SCOPE_LINK);
+}
+
  static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
  						    __be32 group,
+						    __u16 vid,
+						    bool tagged,
  						    u8 *igmp_type)
  {
  	struct igmpv3_query *ihv3;

@@ -391,12 +415,17 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
  	struct igmphdr *ih;
  	struct ethhdr *eth;
  	struct iphdr *iph;
+	int vh_size = 0;
+
+	/* if vid is non-zero, insert the 1Q header also */
+	if (vid && tagged)
+		vh_size = sizeof(struct vlan_hdr);
 
  	igmp_hdr_size = sizeof(*ih);
  	if (br->multicast_igmp_version == 3)
  		igmp_hdr_size = sizeof(*ihv3);
  	skb = netdev_alloc_skb_ip_align(br->dev, sizeof(*eth) + sizeof(*iph) +
-						 igmp_hdr_size + 4);
+						 vh_size + igmp_hdr_size + 4);
  	if (!skb)
  		goto out;
 

@@ -415,6 +444,15 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
  	eth->h_proto = htons(ETH_P_IP);
  	skb_put(skb, sizeof(*eth));
 
+	if (vid && tagged) {
+		skb = vlan_insert_tag_set_proto(skb, htons(ETH_P_8021Q), vid);
+		if (!skb) {
+			kfree_skb(skb);
+			br_err(br, "Failed adding VLAN tag to IGMP query, vid:%d\n", vid);
+			return NULL;
+		}
+	}
+
  	skb_set_network_header(skb, skb->len);
  	iph = ip_hdr(skb);
 

@@ -426,8 +464,7 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
  	iph->frag_off = htons(IP_DF);
  	iph->ttl = 1;
  	iph->protocol = IPPROTO_IGMP;
-	iph->saddr = br->multicast_query_use_ifaddr ?
-		     inet_select_addr(br->dev, 0, RT_SCOPE_LINK) : 0;
+	iph->saddr = br_multicast_inet_addr(br, vid);
  	iph->daddr = htonl(INADDR_ALLHOSTS_GROUP);
  	((u8 *)&iph[1])[0] = IPOPT_RA;
  	((u8 *)&iph[1])[1] = 4;

@@ -477,6 +514,8 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
  #if IS_ENABLED(CONFIG_IPV6)
  static struct sk_buff *br_ip6_multicast_alloc_query(struct net_bridge *br,
  						    const struct in6_addr *grp,
+						    __u16 vid,
+						    bool tagged,
  						    u8 *igmp_type)
  {
  	struct mld2_query *mld2q;

@@ -486,13 +525,18 @@ static struct sk_buff *br_ip6_multicast_alloc_query(struct net_bridge *br,
  	size_t mld_hdr_size;
  	struct sk_buff *skb;
  	struct ethhdr *eth;
+	int vh_size = 0;
  	u8 *hopopt;
 
+	/* if vid is non-zero, insert the 1Q header also */
+	if (vid && tagged)
+		vh_size = sizeof(struct vlan_hdr);
+
  	mld_hdr_size = sizeof(*mldq);
  	if (br->multicast_mld_version == 2)
  		mld_hdr_size = sizeof(*mld2q);
  	skb = netdev_alloc_skb_ip_align(br->dev, sizeof(*eth) + sizeof(*ip6h) +
-						 8 + mld_hdr_size);
+						 vh_size + 8 + mld_hdr_size);
  	if (!skb)
  		goto out;
 

@@ -506,6 +550,15 @@ static struct sk_buff *br_ip6_multicast_alloc_query(struct net_bridge *br,
  	eth->h_proto = htons(ETH_P_IPV6);
  	skb_put(skb, sizeof(*eth));
 
+	if (vid && tagged) {
+		skb = vlan_insert_tag_set_proto(skb, htons(ETH_P_8021Q), vid);
+		if (!skb) {
+			kfree_skb(skb);
+			br_err(br, "Failed adding VLAN tag to MLD query, vid:%d\n", vid);
+			return NULL;
+		}
+	}
+
  	/* IPv6 header + HbH option */
  	skb_set_network_header(skb, skb->len);
  	ip6h = ipv6_hdr(skb);

@@ -590,15 +643,17 @@ static struct sk_buff *br_ip6_multicast_alloc_query(struct net_bridge *br,
 
  static struct sk_buff *br_multicast_alloc_query(struct net_bridge *br,
  						struct br_ip *addr,
+						bool tagged,
  						u8 *igmp_type)
  {
  	switch (addr->proto) {
  	case htons(ETH_P_IP):
-		return br_ip4_multicast_alloc_query(br, addr->u.ip4, igmp_type);
+		return br_ip4_multicast_alloc_query(br, addr->u.ip4, addr->vid,
+						    tagged, igmp_type);
  #if IS_ENABLED(CONFIG_IPV6)
  	case htons(ETH_P_IPV6):
-		return br_ip6_multicast_alloc_query(br, &addr->u.ip6,
-						    igmp_type);
+		return br_ip6_multicast_alloc_query(br, &addr->u.ip6, addr->vid,
+						    tagged, igmp_type);
  #endif
  	}
  	return NULL;

@@ -905,14 +960,16 @@ static void br_multicast_local_router_expired(struct timer_list *t)
  	spin_unlock(&br->multicast_lock);
  }
 
-static void br_multicast_querier_expired(struct net_bridge *br,
+static void br_multicast_querier_expired(struct net_bridge_vlan *vlan,
  					 struct bridge_mcast_own_query *query)
  {
+	struct net_bridge *br = vlan->br;
+
  	spin_lock(&br->multicast_lock);
  	if (!netif_running(br->dev) || br->multicast_disabled)
  		goto out;
 
-	br_multicast_start_querier(br, query);
+	br_multicast_start_querier(vlan, query);
 
  out:
  	spin_unlock(&br->multicast_lock);

@@ -920,17 +977,17 @@ static void br_multicast_querier_expired(struct net_bridge *br,
 
  static void br_ip4_multicast_querier_expired(struct timer_list *t)
  {
-	struct net_bridge *br = from_timer(br, t, ip4_other_query.timer);
+	struct net_bridge_vlan *v = from_timer(v, t, ip4_other_query.timer);
 
-	br_multicast_querier_expired(br, &br->ip4_own_query);
+	br_multicast_querier_expired(v, &v->ip4_own_query);
  }
 
  #if IS_ENABLED(CONFIG_IPV6)
  static void br_ip6_multicast_querier_expired(struct timer_list *t)
  {
-	struct net_bridge *br = from_timer(br, t, ip6_other_query.timer);
+	struct net_bridge_vlan *v = from_timer(v, t, ip6_other_query.timer);
 
-	br_multicast_querier_expired(br, &br->ip6_own_query);
+	br_multicast_querier_expired(v, &v->ip6_own_query);
  }
  #endif
 

@@ -938,11 +995,17 @@ static void br_multicast_select_own_querier(struct net_bridge *br,
  					    struct br_ip *ip,
  					    struct sk_buff *skb)
  {
+	struct net_bridge_vlan *v;
+
+	v = br_vlan_find(br_vlan_group(br), ip->vid);
+	if (!v)
+		return;
+
  	if (ip->proto == htons(ETH_P_IP))
-		br->ip4_querier.addr.u.ip4 = ip_hdr(skb)->saddr;
+		v->ip4_querier.addr.u.ip4 = ip_hdr(skb)->saddr;
  #if IS_ENABLED(CONFIG_IPV6)
  	else
-		br->ip6_querier.addr.u.ip6 = ipv6_hdr(skb)->saddr;
+		v->ip6_querier.addr.u.ip6 = ipv6_hdr(skb)->saddr;
  #endif
  }
 

@@ -951,9 +1014,27 @@ static void __br_multicast_send_query(struct net_bridge *br,
  				      struct br_ip *ip)
  {
  	struct sk_buff *skb;
+	bool tagged = false;
  	u8 igmp_type;
 
-	skb = br_multicast_alloc_query(br, ip, &igmp_type);
+	if (port->state == BR_STATE_DISABLED ||
+	    port->state == BR_STATE_BLOCKING)
+		return;
+
+#ifdef CONFIG_BRIDGE_VLAN_FILTERING
+	if (port && ip->vid) {
+		struct net_bridge_vlan *v;
+
+		v = br_vlan_find(nbp_vlan_group_rcu(port), ip->vid);
+		if (!br->vlan_enabled || !v)
+			return;
+
+		if (!(v->flags & BRIDGE_VLAN_INFO_UNTAGGED))
+			tagged = true;
+	}
+#endif
+
+	skb = br_multicast_alloc_query(br, ip, tagged, &igmp_type);
  	if (!skb)
  		return;
 

@@ -972,11 +1053,12 @@ static void __br_multicast_send_query(struct net_bridge *br,
  	}
  }
 
-static void br_multicast_send_query(struct net_bridge *br,
+static void br_multicast_send_query(struct net_bridge_vlan *vlan,
  				    struct net_bridge_port *port,
  				    struct bridge_mcast_own_query *own_query)
  {
  	struct bridge_mcast_other_query *other_query = NULL;
+	struct net_bridge *br = vlan->br;
  	struct br_ip br_group;
  	unsigned long time;
 

@@ -985,22 +1067,27 @@ static void br_multicast_send_query(struct net_bridge *br,
  		return;
 
  	memset(&br_group.u, 0, sizeof(br_group.u));
-
-	if (port ? (own_query == &port->ip4_own_query) :
-		   (own_query == &br->ip4_own_query)) {
-		other_query = &br->ip4_other_query;
+	br_group.vid = vlan->vid;
+	if (own_query == &vlan->ip4_own_query) {
+		other_query = &vlan->ip4_other_query;
  		br_group.proto = htons(ETH_P_IP);
  #if IS_ENABLED(CONFIG_IPV6)
  	} else {
-		other_query = &br->ip6_other_query;
+		other_query = &vlan->ip6_other_query;
  		br_group.proto = htons(ETH_P_IPV6);
  #endif
  	}
 
+	if (port) {
+		__br_multicast_send_query(br, port, &br_group);
+		return;
+	}
+
  	if (!other_query || timer_pending(&other_query->timer))
  		return;
 
-	__br_multicast_send_query(br, port, &br_group);
+	list_for_each_entry(port, &br->port_list, list)
+		__br_multicast_send_query(br, port, &br_group);
 
  	time = jiffies;
  	time += own_query->startup_sent < br->multicast_startup_query_count ?

@@ -1009,42 +1096,6 @@ static void br_multicast_send_query(struct net_bridge *br,
  	mod_timer(&own_query->timer, time);
  }
 
-static void
-br_multicast_port_query_expired(struct net_bridge_port *port,
-				struct bridge_mcast_own_query *query)
-{
-	struct net_bridge *br = port->br;
-
-	spin_lock(&br->multicast_lock);
-	if (port->state == BR_STATE_DISABLED ||
-	    port->state == BR_STATE_BLOCKING)
-		goto out;
-
-	if (query->startup_sent < br->multicast_startup_query_count)
-		query->startup_sent++;
-
-	br_multicast_send_query(port->br, port, query);
-
-out:
-	spin_unlock(&br->multicast_lock);
-}
-
-static void br_ip4_multicast_port_query_expired(struct timer_list *t)
-{
-	struct net_bridge_port *port = from_timer(port, t, ip4_own_query.timer);
-
-	br_multicast_port_query_expired(port, &port->ip4_own_query);
-}
-
-#if IS_ENABLED(CONFIG_IPV6)
-static void br_ip6_multicast_port_query_expired(struct timer_list *t)
-{
-	struct net_bridge_port *port = from_timer(port, t, ip6_own_query.timer);
-
-	br_multicast_port_query_expired(port, &port->ip6_own_query);
-}
-#endif
-
  static void br_mc_disabled_update(struct net_device *dev, bool value)
  {
  	struct switchdev_attr attr = {

@@ -1063,12 +1114,6 @@ int br_multicast_add_port(struct net_bridge_port *port)
 
  	timer_setup(&port->multicast_router_timer,
  		    br_multicast_router_expired, 0);
-	timer_setup(&port->ip4_own_query.timer,
-		    br_ip4_multicast_port_query_expired, 0);
-#if IS_ENABLED(CONFIG_IPV6)
-	timer_setup(&port->ip6_own_query.timer,
-		    br_ip6_multicast_port_query_expired, 0);
-#endif
  	br_mc_disabled_update(port->dev, port->br->multicast_disabled);
 
  	port->mcast_stats = netdev_alloc_pcpu_stats(struct bridge_mcast_stats);

@@ -1109,15 +1154,47 @@ static void __br_multicast_enable_port(struct net_bridge_port *port)
  	if (br->multicast_disabled || !netif_running(br->dev))
  		return;
 
-	br_multicast_enable(&port->ip4_own_query);
-#if IS_ENABLED(CONFIG_IPV6)
-	br_multicast_enable(&port->ip6_own_query);
-#endif
  	if (port->multicast_router == MDB_RTR_TYPE_PERM &&
  	    hlist_unhashed(&port->rlist))
  		br_multicast_add_router(br, port);
  }
 
+static void __br_multicast_vlan_init(struct net_bridge_vlan *vlan)
+{
+	vlan->ip4_querier.port = NULL;
+	vlan->ip4_other_query.delay_time = 0;
+
+	timer_setup(&vlan->ip4_other_query.timer,
+		    br_ip4_multicast_querier_expired, 0);
+	timer_setup(&vlan->ip4_own_query.timer,
+		    br_ip4_multicast_query_expired, 0);
+
+#if IS_ENABLED(CONFIG_IPV6)
+	vlan->ip6_querier.port = NULL;
+	vlan->ip6_other_query.delay_time = 0;
+	timer_setup(&vlan->ip6_other_query.timer,
+		    br_ip6_multicast_querier_expired, 0);
+	timer_setup(&vlan->ip6_own_query.timer,
+		    br_ip6_multicast_query_expired, 0);
+ #endif
+}
+
+void br_multicast_enable_vlan(struct net_bridge *br, u16 vid)
+{
+	struct net_bridge_vlan *v;
+
+	v = br_vlan_find(br_vlan_group(br), vid);
+	if (!v)
+		return;
+
+	__br_multicast_vlan_init(v);
+	br_multicast_enable(&v->ip4_own_query);
+#if IS_ENABLED(CONFIG_IPV6)
+	br_multicast_enable(&v->ip6_own_query);
+#endif
+}
+
+/* called by stp to enable timers, only use it to enable router port? -jnn */
  void br_multicast_enable_port(struct net_bridge_port *port)
  {
  	struct net_bridge *br = port->br;

@@ -1127,6 +1204,7 @@ void br_multicast_enable_port(struct net_bridge_port *port)
  	spin_unlock(&br->multicast_lock);
  }
 
+/* called by stp_if */
  void br_multicast_disable_port(struct net_bridge_port *port)
  {
  	struct net_bridge *br = port->br;

@@ -1139,12 +1217,6 @@ void br_multicast_disable_port(struct net_bridge_port *port)
  			br_multicast_del_pg(br, pg);
 
  	__del_port_router(port);
-
-	del_timer(&port->multicast_router_timer);
-	del_timer(&port->ip4_own_query.timer);
-#if IS_ENABLED(CONFIG_IPV6)
-	del_timer(&port->ip6_own_query.timer);
-#endif
  	spin_unlock(&br->multicast_lock);
  }
 

@@ -1283,65 +1355,66 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
  }
  #endif
 
-static bool br_ip4_multicast_select_querier(struct net_bridge *br,
+static bool br_ip4_multicast_select_querier(struct net_bridge_vlan *vlan,
  					    struct net_bridge_port *port,
  					    __be32 saddr)
  {
-	if (!timer_pending(&br->ip4_own_query.timer) &&
-	    !timer_pending(&br->ip4_other_query.timer))
+
+	if (!timer_pending(&vlan->ip4_own_query.timer) &&
+	    !timer_pending(&vlan->ip4_other_query.timer))
  		goto update;
 
-	if (!br->ip4_querier.addr.u.ip4)
+	if (!vlan->ip4_querier.addr.u.ip4)
  		goto update;
 
-	if (ntohl(saddr) <= ntohl(br->ip4_querier.addr.u.ip4))
+	if (ntohl(saddr) <= ntohl(vlan->ip4_querier.addr.u.ip4))
  		goto update;
 
  	return false;
 
  update:
-	br->ip4_querier.addr.u.ip4 = saddr;
+	vlan->ip4_querier.addr.u.ip4 = saddr;
 
  	/* update protected by general multicast_lock by caller */
-	rcu_assign_pointer(br->ip4_querier.port, port);
+	rcu_assign_pointer(vlan->ip4_querier.port, port);
 
  	return true;
  }
 
  #if IS_ENABLED(CONFIG_IPV6)
-static bool br_ip6_multicast_select_querier(struct net_bridge *br,
+static bool br_ip6_multicast_select_querier(struct net_bridge_vlan *vlan,
  					    struct net_bridge_port *port,
  					    struct in6_addr *saddr)
  {
-	if (!timer_pending(&br->ip6_own_query.timer) &&
-	    !timer_pending(&br->ip6_other_query.timer))
+	if (!timer_pending(&vlan->ip6_own_query.timer) &&
+	    !timer_pending(&vlan->ip6_other_query.timer))
  		goto update;
 
-	if (ipv6_addr_cmp(saddr, &br->ip6_querier.addr.u.ip6) <= 0)
+	if (ipv6_addr_cmp(saddr, &vlan->ip6_querier.addr.u.ip6) <= 0)
  		goto update;
 
  	return false;
 
  update:
-	br->ip6_querier.addr.u.ip6 = *saddr;
+	vlan->ip6_querier.addr.u.ip6 = *saddr;
 
  	/* update protected by general multicast_lock by caller */
-	rcu_assign_pointer(br->ip6_querier.port, port);
+	rcu_assign_pointer(vlan->ip6_querier.port, port);
 
  	return true;
  }
  #endif
 
-static bool br_multicast_select_querier(struct net_bridge *br,
+static bool br_multicast_select_querier(struct net_bridge_vlan *vlan,
  					struct net_bridge_port *port,
  					struct br_ip *saddr)
  {
  	switch (saddr->proto) {
  	case htons(ETH_P_IP):
-		return br_ip4_multicast_select_querier(br, port, saddr->u.ip4);
+		return br_ip4_multicast_select_querier(vlan, port, saddr->u.ip4);
  #if IS_ENABLED(CONFIG_IPV6)
  	case htons(ETH_P_IPV6):
-		return br_ip6_multicast_select_querier(br, port, &saddr->u.ip6);
+		return br_ip6_multicast_select_querier(vlan, port, &saddr->u.ip6);
  #endif
  	}
 

@@ -1425,17 +1498,17 @@ static void br_multicast_mark_router(struct net_bridge *br,
  		  now + br->multicast_querier_interval);
  }
 
-static void br_multicast_query_received(struct net_bridge *br,
+static void br_multicast_query_received(struct net_bridge_vlan *vlan,
  					struct net_bridge_port *port,
  					struct bridge_mcast_other_query *query,
  					struct br_ip *saddr,
  					unsigned long max_delay)
  {
-	if (!br_multicast_select_querier(br, port, saddr))
+	if (!br_multicast_select_querier(vlan, port, saddr))
  		return;
 
-	br_multicast_update_query_timer(br, query, max_delay);
-	br_multicast_mark_router(br, port);
+	br_multicast_update_query_timer(vlan->br, query, max_delay);
+	br_multicast_mark_router(vlan->br, port);
  }
 
  static int br_ip4_multicast_query(struct net_bridge *br,

@@ -1482,10 +1555,17 @@ static int br_ip4_multicast_query(struct net_bridge *br,
  	}
 
  	if (!group) {
+		struct net_bridge_vlan *v;
+
+		v = br_vlan_find(br_vlan_group(br), vid);
+		if (!v)
+			goto out;
+
  		saddr.proto = htons(ETH_P_IP);
+		saddr.vid   = vid;
  		saddr.u.ip4 = iph->saddr;
 
-		br_multicast_query_received(br, port, &br->ip4_other_query,
+		br_multicast_query_received(v, port, &v->ip4_other_query,
  					    &saddr, max_delay);
  		goto out;
  	}

@@ -1565,10 +1645,17 @@ static int br_ip6_multicast_query(struct net_bridge *br,
  	is_general_query = group && ipv6_addr_any(group);
 
  	if (is_general_query) {
+		struct net_bridge_vlan *v;
+
+		v = br_vlan_find(br_vlan_group(br), vid);
+		if (!v)
+			goto out;
+
  		saddr.proto = htons(ETH_P_IPV6);
+		saddr.vid   = vid;
  		saddr.u.ip6 = ip6h->saddr;
 
-		br_multicast_query_received(br, port, &br->ip6_other_query,
+		br_multicast_query_received(v, port, &v->ip6_other_query,
  					    &saddr, max_delay);
  		goto out;
  	} else if (!group) {

@@ -1716,20 +1803,22 @@ static void br_ip4_multicast_leave_group(struct net_bridge *br,
  					 __u16 vid,
  					 const unsigned char *src)
  {
+	struct net_bridge_vlan *v;
  	struct br_ip br_group;
-	struct bridge_mcast_own_query *own_query;
 
  	if (ipv4_is_local_multicast(group))
  		return;
 
-	own_query = port ? &port->ip4_own_query : &br->ip4_own_query;
+	v = br_vlan_find(br_vlan_group(br), vid);
+	if (!v)
+		return;
 
  	br_group.u.ip4 = group;
  	br_group.proto = htons(ETH_P_IP);
  	br_group.vid = vid;
 
-	br_multicast_leave_group(br, port, &br_group, &br->ip4_other_query,
-				 own_query, src);
+	br_multicast_leave_group(br, port, &br_group, &v->ip4_other_query,
+				 &v->ip4_own_query, src);
  }
 
  #if IS_ENABLED(CONFIG_IPV6)

@@ -1739,20 +1828,22 @@ static void br_ip6_multicast_leave_group(struct net_bridge *br,
  					 __u16 vid,
  					 const unsigned char *src)
  {
+	struct net_bridge_vlan *v;
  	struct br_ip br_group;
-	struct bridge_mcast_own_query *own_query;
 
  	if (ipv6_addr_is_ll_all_nodes(group))
  		return;
 
-	own_query = port ? &port->ip6_own_query : &br->ip6_own_query;
+	v = br_vlan_find(br_vlan_group(br), vid);
+	if (!v)
+		return;
 
  	br_group.u.ip6 = *group;
  	br_group.proto = htons(ETH_P_IPV6);
  	br_group.vid = vid;
 
-	br_multicast_leave_group(br, port, &br_group, &br->ip6_other_query,
-				 own_query, src);
+	br_multicast_leave_group(br, port, &br_group, &v->ip6_other_query,
+				 &v->ip6_own_query, src);
  }
  #endif
 

@@ -1938,37 +2029,42 @@ int br_multicast_rcv(struct net_bridge *br, struct net_bridge_port *port,
  	return ret;
  }
 
-static void br_multicast_query_expired(struct net_bridge *br,
+static void br_multicast_query_expired(struct net_bridge_vlan *vlan,
  				       struct bridge_mcast_own_query *query,
  				       struct bridge_mcast_querier *querier)
  {
+	struct net_bridge *br = vlan->br;
+
  	spin_lock(&br->multicast_lock);
  	if (query->startup_sent < br->multicast_startup_query_count)
  		query->startup_sent++;
 
  	RCU_INIT_POINTER(querier->port, NULL);
-	br_multicast_send_query(br, NULL, query);
+	br_multicast_send_query(vlan, NULL, query);
  	spin_unlock(&br->multicast_lock);
  }
 
  static void br_ip4_multicast_query_expired(struct timer_list *t)
  {
-	struct net_bridge *br = from_timer(br, t, ip4_own_query.timer);
+	struct net_bridge_vlan *v = from_timer(v, t, ip4_own_query.timer);
 
-	br_multicast_query_expired(br, &br->ip4_own_query, &br->ip4_querier);
+	br_multicast_query_expired(v, &v->ip4_own_query, &v->ip4_querier);
  }
 
  #if IS_ENABLED(CONFIG_IPV6)
  static void br_ip6_multicast_query_expired(struct timer_list *t)
  {
-	struct net_bridge *br = from_timer(br, t, ip6_own_query.timer);
+	struct net_bridge_vlan *v = from_timer(v, t, ip6_own_query.timer);
 
-	br_multicast_query_expired(br, &br->ip6_own_query, &br->ip6_querier);
+	br_multicast_query_expired(v, &v->ip6_own_query, &v->ip6_querier);
  }
  #endif
 
  void br_multicast_init(struct net_bridge *br)
  {
+	struct net_bridge_vlan_group *vg;
+	struct net_bridge_vlan *v;
+
  	br->hash_elasticity = 4;
  	br->hash_max = 512;
 

@@ -1985,29 +2081,22 @@ void br_multicast_init(struct net_bridge *br)
  	br->multicast_querier_interval = 255 * HZ;
  	br->multicast_membership_interval = 260 * HZ;
 
-	br->ip4_other_query.delay_time = 0;
-	br->ip4_querier.port = NULL;
  	br->multicast_igmp_version = 2;
  #if IS_ENABLED(CONFIG_IPV6)
  	br->multicast_mld_version = 1;
-	br->ip6_other_query.delay_time = 0;
-	br->ip6_querier.port = NULL;
  #endif
  	br->has_ipv6_addr = 1;
 
  	spin_lock_init(&br->multicast_lock);
  	timer_setup(&br->multicast_router_timer,
  		    br_multicast_local_router_expired, 0);
-	timer_setup(&br->ip4_other_query.timer,
-		    br_ip4_multicast_querier_expired, 0);
-	timer_setup(&br->ip4_own_query.timer,
-		    br_ip4_multicast_query_expired, 0);
-#if IS_ENABLED(CONFIG_IPV6)
-	timer_setup(&br->ip6_other_query.timer,
-		    br_ip6_multicast_querier_expired, 0);
-	timer_setup(&br->ip6_own_query.timer,
-		    br_ip6_multicast_query_expired, 0);
-#endif
+
+	vg = br_vlan_group(br);
+	if (!vg || !vg->num_vlans)
+		return;
+
+	list_for_each_entry(v, &vg->vlan_list, vlist)
+		__br_multicast_vlan_init(v);
  }
 
  static void __br_multicast_open(struct net_bridge *br,

@@ -2023,21 +2112,41 @@ static void __br_multicast_open(struct net_bridge *br,
 
  void br_multicast_open(struct net_bridge *br)
  {
-	__br_multicast_open(br, &br->ip4_own_query);
+	struct net_bridge_vlan_group *vg;
+	struct net_bridge_vlan *v;
+
+	vg = br_vlan_group(br);
+	if (!vg || !vg->num_vlans)
+		return;
+
+	list_for_each_entry(v, &vg->vlan_list, vlist) {
+		__br_multicast_vlan_init(v);
+		__br_multicast_open(br, &v->ip4_own_query);
  #if IS_ENABLED(CONFIG_IPV6)
-	__br_multicast_open(br, &br->ip6_own_query);
+		__br_multicast_open(br, &v->ip6_own_query);
  #endif
+	}
  }
 
  void br_multicast_stop(struct net_bridge *br)
  {
+	struct net_bridge_vlan_group *vg;
+	struct net_bridge_vlan *v;
+
  	del_timer_sync(&br->multicast_router_timer);
-	del_timer_sync(&br->ip4_other_query.timer);
-	del_timer_sync(&br->ip4_own_query.timer);
+
+	vg = br_vlan_group(br);
+	if (!vg || !vg->num_vlans)
+		return;
+
+	list_for_each_entry(v, &vg->vlan_list, vlist) {
+		del_timer_sync(&v->ip4_other_query.timer);
+		del_timer_sync(&v->ip4_own_query.timer);
  #if IS_ENABLED(CONFIG_IPV6)
-	del_timer_sync(&br->ip6_other_query.timer);
-	del_timer_sync(&br->ip6_own_query.timer);
+		del_timer_sync(&v->ip6_other_query.timer);
+		del_timer_sync(&v->ip6_own_query.timer);
  #endif
+	}
  }
 
  void br_multicast_dev_del(struct net_bridge *br)

@@ -2162,25 +2271,37 @@ int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
  	return err;
  }
 
-static void br_multicast_start_querier(struct net_bridge *br,
+/* Must be called with multicast_lock */
+static void br_multicast_init_querier(struct net_bridge_vlan *vlan,
+				      struct bridge_mcast_own_query *query,
+				      unsigned long max_delay)
+{
+	struct bridge_mcast_other_query *other_query = NULL;
+
+	if (query == &vlan->ip4_own_query)
+		other_query = &vlan->ip4_other_query;
+	else
+		other_query = &vlan->ip6_other_query;
+
+	if (!timer_pending(&other_query->timer))
+		other_query->delay_time = jiffies + max_delay;
+
+	br_multicast_start_querier(vlan, query);
+}
+
+static void br_multicast_start_querier(struct net_bridge_vlan *vlan,
  				       struct bridge_mcast_own_query *query)
  {
-	struct net_bridge_port *port;
+	struct net_bridge *br = vlan->br;
 
  	__br_multicast_open(br, query);
 
-	list_for_each_entry(port, &br->port_list, list) {
-		if (port->state == BR_STATE_DISABLED ||
-		    port->state == BR_STATE_BLOCKING)
-			continue;
-
-		if (query == &br->ip4_own_query)
-			br_multicast_enable(&port->ip4_own_query);
+	if (query == &vlan->ip4_own_query)
+		br_multicast_enable(&vlan->ip4_own_query);
  #if IS_ENABLED(CONFIG_IPV6)
-		else
-			br_multicast_enable(&port->ip6_own_query);
+	else
+		br_multicast_enable(&vlan->ip6_own_query);
  #endif
-	}
  }
 
  int br_multicast_toggle(struct net_bridge *br, unsigned long val)

@@ -2248,6 +2369,8 @@ EXPORT_SYMBOL_GPL(br_multicast_router);
 
  int br_multicast_set_querier(struct net_bridge *br, unsigned long val)
  {
+	struct net_bridge_vlan_group *vg;
+	struct net_bridge_vlan *v;
  	unsigned long max_delay;
 
  	val = !!val;

@@ -2260,19 +2383,18 @@ int br_multicast_set_querier(struct net_bridge *br, unsigned long val)
  	if (!val)
  		goto unlock;
 
-	max_delay = br->multicast_query_response_interval;
-
-	if (!timer_pending(&br->ip4_other_query.timer))
-		br->ip4_other_query.delay_time = jiffies + max_delay;
+	vg = br_vlan_group(br);
+	if (!vg || !vg->num_vlans)
+		goto unlock;
 
-	br_multicast_start_querier(br, &br->ip4_own_query);
+	max_delay = br->multicast_query_response_interval;
 
+	list_for_each_entry(v, &vg->vlan_list, vlist) {
+		br_multicast_init_querier(v, &v->ip4_own_query, max_delay);
  #if IS_ENABLED(CONFIG_IPV6)
-	if (!timer_pending(&br->ip6_other_query.timer))
-		br->ip6_other_query.delay_time = jiffies + max_delay;
-
-	br_multicast_start_querier(br, &br->ip6_own_query);
+		br_multicast_init_querier(v, &v->ip6_own_query, max_delay);
  #endif
+	}
 
  unlock:
  	spin_unlock_bh(&br->multicast_lock);

@@ -2425,6 +2547,7 @@ EXPORT_SYMBOL_GPL(br_multicast_list_adjacent);
   */
  bool br_multicast_has_querier_anywhere(struct net_device *dev, int proto)
  {
+	struct net_bridge_vlan_group *vg;
  	struct net_bridge *br;
  	struct net_bridge_port *port;
  	struct ethhdr eth;

@@ -2438,12 +2561,16 @@ bool br_multicast_has_querier_anywhere(struct net_device *dev, int proto)
  	if (!port || !port->br)
  		goto unlock;
 
+	vg = nbp_vlan_group_rcu(port);
+	if (!vg)
+		goto unlock;
+
  	br = port->br;
 
  	memset(&eth, 0, sizeof(eth));
  	eth.h_proto = htons(proto);
 
-	ret = br_multicast_querier_exists(br, &eth);
+	ret = br_multicast_querier_exists(br, br_get_pvid(vg), &eth);
 
  unlock:
  	rcu_read_unlock();

@@ -2462,7 +2589,8 @@ EXPORT_SYMBOL_GPL(br_multicast_has_querier_anywhere);
   */
  bool br_multicast_has_querier_adjacent(struct net_device *dev, int proto)
  {
-	struct net_bridge *br;
+	struct net_bridge_vlan_group *vg;
+	struct net_bridge_vlan *v;
  	struct net_bridge_port *port;
  	bool ret = false;
 

@@ -2474,18 +2602,24 @@ bool br_multicast_has_querier_adjacent(struct net_device *dev, int proto)
  	if (!port || !port->br)
  		goto unlock;
 
-	br = port->br;
+	vg = nbp_vlan_group_rcu(port);
+	if (!vg)
+		goto unlock;
+
+	v = br_vlan_find(br_vlan_group(port->br), br_get_pvid(vg));
+	if (!v)
+		goto unlock;
 
  	switch (proto) {
  	case ETH_P_IP:
-		if (!timer_pending(&br->ip4_other_query.timer) ||
-		    rcu_dereference(br->ip4_querier.port) == port)
+		if (!timer_pending(&v->ip4_other_query.timer) ||
+		    rcu_dereference(v->ip4_querier.port) == port)
  			goto unlock;
  		break;
  #if IS_ENABLED(CONFIG_IPV6)
  	case ETH_P_IPV6:
-		if (!timer_pending(&br->ip6_other_query.timer) ||
-		    rcu_dereference(br->ip6_querier.port) == port)
+		if (!timer_pending(&v->ip6_other_query.timer) ||
+		    rcu_dereference(v->ip6_querier.port) == port)
  			goto unlock;
  		break;
  #endif

net/bridge/br_private.h +26 -12

index 6e31be61d2c6..00dac1bbfaba 100644
@@ -140,6 +140,17 @@ struct net_bridge_vlan {
  		struct net_bridge_vlan	*brvlan;
  	};
 
+#ifdef CONFIG_BRIDGE_IGMP_SNOOPING
+	struct bridge_mcast_other_query	ip4_other_query;
+	struct bridge_mcast_own_query	ip4_own_query;
+	struct bridge_mcast_querier	ip4_querier;
+#if IS_ENABLED(CONFIG_IPV6)
+	struct bridge_mcast_other_query	ip6_other_query;
+	struct bridge_mcast_own_query	ip6_own_query;
+	struct bridge_mcast_querier	ip6_querier;
+#endif
+#endif
+
  	struct br_tunnel_info		tinfo;
 
  	struct list_head		vlist;

@@ -261,10 +272,6 @@ struct net_bridge_port {
  	struct rcu_head			rcu;
 
  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
-	struct bridge_mcast_own_query	ip4_own_query;
-#if IS_ENABLED(CONFIG_IPV6)
-	struct bridge_mcast_own_query	ip6_own_query;
-#endif /* IS_ENABLED(CONFIG_IPV6) */
  	unsigned char			multicast_router;
  	struct bridge_mcast_stats	__percpu *mcast_stats;
  	struct timer_list		multicast_router_timer;

@@ -390,14 +397,8 @@ struct net_bridge {
  	struct hlist_head		router_list;
 
  	struct timer_list		multicast_router_timer;
-	struct bridge_mcast_other_query	ip4_other_query;
-	struct bridge_mcast_own_query	ip4_own_query;
-	struct bridge_mcast_querier	ip4_querier;
  	struct bridge_mcast_stats	__percpu *mcast_stats;
  #if IS_ENABLED(CONFIG_IPV6)
-	struct bridge_mcast_other_query	ip6_other_query;
-	struct bridge_mcast_own_query	ip6_own_query;
-	struct bridge_mcast_querier	ip6_querier;
  	u8				multicast_mld_version;
  #endif /* IS_ENABLED(CONFIG_IPV6) */
  #endif

@@ -618,6 +619,7 @@ int br_multicast_add_port(struct net_bridge_port *port);
  void br_multicast_del_port(struct net_bridge_port *port);
  void br_multicast_enable_port(struct net_bridge_port *port);
  void br_multicast_disable_port(struct net_bridge_port *port);
+void br_multicast_enable_vlan(struct net_bridge *br, u16 vid);
  void br_multicast_init(struct net_bridge *br);
  void br_multicast_open(struct net_bridge *br);
  void br_multicast_stop(struct net_bridge *br);

@@ -633,6 +635,7 @@ int br_multicast_set_igmp_version(struct net_bridge *br, unsigned long val);
  #if IS_ENABLED(CONFIG_IPV6)
  int br_multicast_set_mld_version(struct net_bridge *br, unsigned long val);
  #endif
+__be32 br_multicast_inet_addr(struct net_bridge *br, u16 vid);
  struct net_bridge_mdb_entry *
  br_mdb_ip_get(struct net_bridge_mdb_htable *mdb, struct br_ip *dst);
  struct net_bridge_mdb_entry *

@@ -687,17 +690,27 @@ __br_multicast_querier_exists(struct net_bridge *br,
  	       (own_querier_enabled || timer_pending(&querier->timer));
  }
 
+static struct net_bridge_vlan_group *br_vlan_group(const struct net_bridge *br);
+struct net_bridge_vlan *br_vlan_find(struct net_bridge_vlan_group *vg, u16 vid);
+
  static inline bool br_multicast_querier_exists(struct net_bridge *br,
+					       u16 vid,
  					       struct ethhdr *eth)
  {
+	struct net_bridge_vlan *v;
+
+	v = br_vlan_find(br_vlan_group(br), vid);
+	if (!v)
+		return false;
+
  	switch (eth->h_proto) {
  	case (htons(ETH_P_IP)):
  		return __br_multicast_querier_exists(br,
-			&br->ip4_other_query, false);
+			&v->ip4_other_query, false);
  #if IS_ENABLED(CONFIG_IPV6)
  	case (htons(ETH_P_IPV6)):
  		return __br_multicast_querier_exists(br,
-			&br->ip6_other_query, true);
+			&v->ip6_other_query, true);
  #endif
  	default:
  		return false;

@@ -768,6 +781,7 @@ static inline bool br_multicast_is_router(struct net_bridge *br)
  }
 
  static inline bool br_multicast_querier_exists(struct net_bridge *br,
+					       u16 vid,
  					       struct ethhdr *eth)
  {
  	return false;

net/bridge/br_stp.c +1 -4

index a1ba52d247d8..d1d6c4fb39dd 100644
@@ -460,10 +460,7 @@ void br_port_state_selection(struct net_bridge *br)
 
  		if (p->state != BR_STATE_BLOCKING)
  			br_multicast_enable_port(p);
-		/* Multicast is not disabled for the port when it goes in
-		 * blocking state because the timers will expire and stop by
-		 * themselves without sending more queries.
-		 */
+
  		if (p->state == BR_STATE_FORWARDING)
  			++liveports;
  	}

net/bridge/br_vlan.c +3 -0

index bb9cbad4bad6..3b8fb28e9ab4 100644
@@ -270,6 +270,9 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags)
  			goto out_filt;
  		}
  		vg->num_vlans++;
+
+		/* Start per VLAN IGMP/MLD querier timers */
+		br_multicast_enable_vlan(br, v->vid);
  	}
 
  	err = rhashtable_lookup_insert_fast(&vg->vlan_hash, &v->vnode,

Quoted 1 line · Joachim Nilsson

- First of all, is this patch useful to anyone

Obviously to us as it's based on our patch. :-)
We actually recently discussed what will be needed to make it acceptable to upstream.

Quoted 5 lines

- The current br_multicast.c is very complex. The support for both IPv4 and IPv6 is a no-brainer, but it also has #ifdef VLAN_FILTERING and 'br->vlan_enabled' ... this has likely been discussed before, but if we could remove those code paths I believe what's left would be quite a bit easier to read and maintain.

br->vlan_enabled has a wrapper that can be used without ifdefs, as does br_vlan_find() so in short - you can remove the ifdefs and use the wrappers, they'll degrade to always false/null when vlans are disabled.

Quoted 3 lines

- Many per-bridge specific multicast sysfs settings may need to have a corresponding per-VLAN setting, e.g. snooping, query_interval, etc. How should we go about that? (For status reporting I have a proposal)

We'll have to add more to the per-vlan context, but yes it has to happen. It will be only netlink interface for config/retrieval, no sysfs.

Quoted 1 line

- Dito per-port specific multicast sysfs settings, e.g. multicast_router

I'm not sure I follow this one, there is per-port mcast router config now ?

Thanks for the effort, I see that you have done some of the required cleanups for this to be upstreamable, but as you've noted above we need to make it complete (with the per-vlan contexts and all).

I will review this patch in detail later and come back if there's anything.

Quoted 4 lines · Nikolay Aleksandrov

Quoted 1 line · Joachim Nilsson

- First of all, is this patch useful to anyone

Obviously to us as it's based on our patch. :-)
We actually recently discussed what will be needed to make it acceptable to upstream.

Great! :)

Quoted 8 lines

Quoted 5 lines

- The current br_multicast.c is very complex. The support for both IPv4 and IPv6 is a no-brainer, but it also has #ifdef VLAN_FILTERING and 'br->vlan_enabled' ... this has likely been discussed before, but if we could remove those code paths I believe what's left would be quite a bit easier to read and maintain.

br->vlan_enabled has a wrapper that can be used without ifdefs, as does br_vlan_find() so in short - you can remove the ifdefs and use the wrappers, they'll degrade to always false/null when vlans are disabled.

Thanks, I'll have a look at that and prepare an RFC v2!

Quoted 5 lines

Quoted 3 lines

- Many per-bridge specific multicast sysfs settings may need to have a corresponding per-VLAN setting, e.g. snooping, query_interval, etc. How should we go about that? (For status reporting I have a proposal)

We'll have to add more to the per-vlan context, but yes it has to happen. It will be only netlink interface for config/retrieval, no sysfs.

Some settings are possible to do with sysfs, like multicast_query_interval and ...

Quoted 3 lines

Quoted 2 lines

- Dito per-port specific multicast sysfs settings, e.g. multicast_router

I'm not sure I follow this one, there is per-port mcast router config now ?

Sorry no, I meant we may want to add more per-VLAN settings when we get this base patch merged. Like router ports, we may want to be able to set them per VLAN.

Quoted 4 lines

Thanks for the effort, I see that you have done some of the required cleanups for this to be upstreamable, but as you've noted above we need to make it complete (with the per-vlan contexts and all).

There's definitely more work to be done. Agreeing on a base set of changes to start with is maybe the most important, as well as making it complete.>

Quoted 1 line

I will review this patch in detail later and come back if there's anything.

Thank you so much for the quick feedback so far! :)

Cheers
/Joachim

We want to avoid sysfs in general, all of networking config and stats are moving to netlink. It is better controlled and structured for such changes, also provides nice interfaces for automatic type checks etc.

Also (but a minor reason) there is no tree/entity in sysfs for the vlans where to add this. It will either have to be a file which does some format string hack (like us currently) or will need to add new tree for them which I'd really like to avoid for the bridge.

[..]
Also after my vlan rhastable change, we have per-vlan context even today (e.g. per-vlan stats use it) so we'll just extend that.

Quoted 19 lines · Joachim Nilsson

Quoted 4 lines · Nikolay Aleksandrov

Quoted 1 line · Joachim Nilsson

- First of all, is this patch useful to anyone

Obviously to us as it's based on our patch. :-)
We actually recently discussed what will be needed to make it acceptable to upstream.

Great! :)

Quoted 8 lines

Quoted 5 lines

- The current br_multicast.c is very complex. The support for both IPv4 and IPv6 is a no-brainer, but it also has #ifdef VLAN_FILTERING and 'br->vlan_enabled' ... this has likely been discussed before, but if we could remove those code paths I believe what's left would be quite a bit easier to read and maintain.

br->vlan_enabled has a wrapper that can be used without ifdefs, as does br_vlan_find() so in short - you can remove the ifdefs and use the wrappers, they'll degrade to always false/null when vlans are disabled.

Thanks, I'll have a look at that and prepare an RFC v2!

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

- Many per-bridge specific multicast sysfs settings may need to have a corresponding per-VLAN setting, e.g. snooping, query_interval, etc. How should we go about that? (For status reporting I have a proposal)

We'll have to add more to the per-vlan context, but yes it has to happen. It will be only netlink interface for config/retrieval, no sysfs.

Some settings are possible to do with sysfs, like multicast_query_interval and ...

We want to avoid sysfs in general, all of networking config and stats are moving to netlink. It is better controlled and structured for such changes, also provides nice interfaces for automatic type checks etc.

Also (but a minor reason) there is no tree/entity in sysfs for the vlans where to add this. It will either have to be a file which does some format string hack (like us currently) or will need to add new tree for them which I'd really like to avoid for the bridge.

Quoted 2 lines · On Wed, 18 Apr 2018 16:14:26 +0300 Nikolay Aleksandrov…

Quoted 1 line · Joachim Nilsson

On Wed, Apr 18, 2018 at 03:31:57PM +0300, Nikolay Aleksandrov

Quoted 4 lines · wrote:

Quoted 4 lines

Quoted 4 lines · Joachim Nilsson

Quoted 1 line

- First of all, is this patch useful to anyone

Obviously to us as it's based on our patch. :-)
We actually recently discussed what will be needed to make it

acceptable to upstream.

Quoted 4 lines

Quoted 4 lines

Great! :)

Quoted 1 line

Quoted 1 line

- The current br_multicast.c is very complex. The support for

both IPv4

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

and IPv6 is a no-brainer, but it also has #ifdef

VLAN_FILTERING and

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

'br->vlan_enabled' ... this has likely been discussed before,

but if

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

we could remove those code paths I believe what's left would

be quite

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

a bit easier to read and maintain.

br->vlan_enabled has a wrapper that can be used without ifdefs, as

does br_vlan_find()

Quoted 1 line

Quoted 1 line

Quoted 1 line

so in short - you can remove the ifdefs and use the wrappers,

they'll degrade to always

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

false/null when vlans are disabled.

Thanks, I'll have a look at that and prepare an RFC v2!

Quoted 1 line

Quoted 1 line

- Many per-bridge specific multicast sysfs settings may need to

have a

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

corresponding per-VLAN setting, e.g. snooping, query_interval,

etc.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

How should we go about that? (For status reporting I have a

proposal)

Quoted 1 line

Quoted 1 line

Quoted 1 line

We'll have to add more to the per-vlan context, but yes it has to

happen.

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

It will be only netlink interface for config/retrieval, no sysfs.

Some settings are possible to do with sysfs, like

multicast_query_interval

Quoted 4 lines

Quoted 1 line

and ...

We want to avoid sysfs in general, all of networking config and stats are moving to netlink. It is better controlled and structured for

such

Quoted 1 line

changes, also provides nice interfaces for automatic type checks

etc.

Quoted 2 lines

Also (but a minor reason) there is no tree/entity in sysfs for the

vlans

Quoted 2 lines

where to add this. It will either have to be a file which does some format string hack (like us currently) or will need to add new tree

for

Quoted 1 line

them which I'd really like to avoid for the bridge.

In general, all bridge attributes need to show in netlink and sysfs. Sysfs is easier for scripting from languages.