[RFC] net: bridge: multicast querier per VLAN support

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 <troglobit@gmail.com>
---
 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(-)

On 18/04/18 15:07, Joachim Nilsson wrote:
> 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

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

> - 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.

> - 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.

> - 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 ?
Take a look at br_multicast_set_port_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

I knew this looked familiar when I glanced through it :)

> ²) 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 <troglobit@gmail.com>

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.

Cheers,
  Nik

> ---
>   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(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 02f9f8aab047..ba35485032d8 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -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);
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 56bb9189c374..13d48489e0e1 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -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;
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 277ecd077dc4..72e47d500972 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -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
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 6e31be61d2c6..00dac1bbfaba 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -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;
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index a1ba52d247d8..d1d6c4fb39dd 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -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;
>   	}
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index bb9cbad4bad6..3b8fb28e9ab4 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -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,
>

On Wed, Apr 18, 2018 at 03:31:57PM +0300, Nikolay Aleksandrov wrote:
> On 18/04/18 15:07, Joachim Nilsson wrote:
> > - 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! :)

> > - 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!

> > - 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 ...

> > - 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.

> 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.

> 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

On 18/04/18 16:07, Joachim Nilsson wrote:
> On Wed, Apr 18, 2018 at 03:31:57PM +0300, Nikolay Aleksandrov wrote:
>> On 18/04/18 15:07, Joachim Nilsson wrote:
>>> - 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! :)
> 
>>> - 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!
> 
>>> - 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.

> 
>>> - 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.

Sure, that can be done easily via netlink. br_afspec() can decode any
additional per-vlan attributes and can be fairly easily extended.
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.

> 
>> 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.>
>> 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
>

On Wed, Apr 18, 2018 at 04:14:26PM +0300, Nikolay Aleksandrov wrote:
> 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.

Aha, didn't know that. Thanks! :)

> 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.

Yup, I did some ugly sysfs patches to read queriers per VLAN like that, just
for some basic feedback.  Really awful, although easy to debug because of it
being a simple file ... (I guess I'll have to make friends withe Netlink.)

> [..]
> 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.

Interesting, this I'll have to look at in more detail!

On Wed, 18 Apr 2018 16:14:26 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> On 18/04/18 16:07, Joachim Nilsson wrote:
> > On Wed, Apr 18, 2018 at 03:31:57PM +0300, Nikolay Aleksandrov wrote:  
> >> On 18/04/18 15:07, Joachim Nilsson wrote:  
> >>> - 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! :)
> >   
> >>> - 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!
> >   
> >>> - 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.

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

On April 18, 2018 6:54:07 PM GMT+03:00, Stephen Hemminger <stephen@networkplumber.org> wrote:
>On Wed, 18 Apr 2018 16:14:26 +0300
>Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>
>> On 18/04/18 16:07, Joachim Nilsson wrote:
>> > On Wed, Apr 18, 2018 at 03:31:57PM +0300, Nikolay Aleksandrov
>wrote:  
>> >> On 18/04/18 15:07, Joachim Nilsson wrote:  
>> >>> - 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! :)
>> >   
>> >>> - 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!
>> >   
>> >>> - 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.
>
>In general, all bridge attributes need to show in netlink and sysfs.
>Sysfs is easier for scripting from languages.

True, but vlans and per-vlan settings have never been exposed via sysfs, only through netlink.
I'd like to avoid adding a directory with potentially 4k multiplied by the attr number for each vlan entries.

There is already vlan config infrastructure via netlink.