[net:master 17/19] net//bridge/br_multicast.c:1432:32: error: 'union <anonymous>' has no member named 'ip6'; did you mean 'ip4'?

[net:master 17/19] net//bridge/br_multicast.c:1432:32: error: 'union <anonymous>' has no member named 'ip6'; did you mean 'ip4'?

[kbuild test robot]

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

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git master
head:   aab456dfa404f3a16d6f1780e62a6a8533c4d008
commit: 5a2de63fd1a59c30c02526d427bc014b98adf508 [17/19] bridge: do not add port to router list when receives query with source 0.0.0.0
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git checkout 5a2de63fd1a59c30c02526d427bc014b98adf508
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   net//bridge/br_multicast.c: In function 'br_multicast_query_received':
>> net//bridge/br_multicast.c:1432:32: error: 'union <anonymous>' has no member named 'ip6'; did you mean 'ip4'?
          !ipv6_addr_any(&saddr->u.ip6)))
                                   ^~~
                                   ip4

vim +1432 net//bridge/br_multicast.c

  1414	
  1415	static void br_multicast_query_received(struct net_bridge *br,
  1416						struct net_bridge_port *port,
  1417						struct bridge_mcast_other_query *query,
  1418						struct br_ip *saddr,
  1419						unsigned long max_delay)
  1420	{
  1421		if (!br_multicast_select_querier(br, port, saddr))
  1422			return;
  1423	
  1424		br_multicast_update_query_timer(br, query, max_delay);
  1425	
  1426		/* Based on RFC4541, section 2.1.1 IGMP Forwarding Rules,
  1427		 * the arrival port for IGMP Queries where the source address
  1428		 * is 0.0.0.0 should not be added to router port list.
  1429		 */
  1430		if ((saddr->proto == htons(ETH_P_IP) && saddr->u.ip4) ||
  1431		    (saddr->proto == htons(ETH_P_IPV6) &&
> 1432		     !ipv6_addr_any(&saddr->u.ip6)))
  1433			br_multicast_mark_router(br, port);
  1434	}
  1435	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

Re: [net:master 17/19] net//bridge/br_multicast.c:1432:32: error: 'union <anonymous>' has no member named 'ip6'; did you mean 'ip4'?

[Nikolay Aleksandrov]

On 27/10/2018 03:50, kbuild test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git master
> head:   aab456dfa404f3a16d6f1780e62a6a8533c4d008
> commit: 5a2de63fd1a59c30c02526d427bc014b98adf508 [17/19] bridge: do not add port to router list when receives query with source 0.0.0.0
> config: powerpc-defconfig (attached as .config)
> compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         git checkout 5a2de63fd1a59c30c02526d427bc014b98adf508
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.2.0 make.cross ARCH=powerpc 
> 
> All errors (new ones prefixed by >>):
> 
>    net//bridge/br_multicast.c: In function 'br_multicast_query_received':
>>> net//bridge/br_multicast.c:1432:32: error: 'union <anonymous>' has no member named 'ip6'; did you mean 'ip4'?
>           !ipv6_addr_any(&saddr->u.ip6)))
>                                    ^~~
>                                    ip4
> 
> vim +1432 net//bridge/br_multicast.c
> 
>   1414	
>   1415	static void br_multicast_query_received(struct net_bridge *br,
>   1416						struct net_bridge_port *port,
>   1417						struct bridge_mcast_other_query *query,
>   1418						struct br_ip *saddr,
>   1419						unsigned long max_delay)
>   1420	{
>   1421		if (!br_multicast_select_querier(br, port, saddr))
>   1422			return;
>   1423	
>   1424		br_multicast_update_query_timer(br, query, max_delay);
>   1425	
>   1426		/* Based on RFC4541, section 2.1.1 IGMP Forwarding Rules,
>   1427		 * the arrival port for IGMP Queries where the source address
>   1428		 * is 0.0.0.0 should not be added to router port list.
>   1429		 */
>   1430		if ((saddr->proto == htons(ETH_P_IP) && saddr->u.ip4) ||
>   1431		    (saddr->proto == htons(ETH_P_IPV6) &&
>> 1432		     !ipv6_addr_any(&saddr->u.ip6)))
>   1433			br_multicast_mark_router(br, port);
>   1434	}
>   1435	
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 

Should've seen this one coming when reviewing the patch, ip6 is defined
only when IPv6 is configured.
I'll send a fix in a minute after running a few tests.

Thanks.

[PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

[Nikolay Aleksandrov]

Recently a check was added which prevents marking of routers with zero
source address, but for IPv6 that cannot happen as the relevant RFCs
actually forbid such packets:
RFC 2710 (MLDv1):
"To be valid, the Query message MUST
 come from a link-local IPv6 Source Address, be at least 24 octets
 long, and have a correct MLD checksum."

Same goes for RFC 3810.

And also it can be seen as a requirement in ipv6_mc_check_mld_query()
which is used by the bridge to validate the message before processing
it. Thus any queries with :: source address won't be processed anyway.
So just remove the check for zero IPv6 source address from the query
processing function.

Fixes: 5a2de63fd1a5 ("bridge: do not add port to router list when receives query with source 0.0.0.0")
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_multicast.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 41cdafbf2ebe..6bac0d6b7b94 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1428,8 +1428,7 @@ static void br_multicast_query_received(struct net_bridge *br,
 	 * is 0.0.0.0 should not be added to router port list.
 	 */
 	if ((saddr->proto == htons(ETH_P_IP) && saddr->u.ip4) ||
-	    (saddr->proto == htons(ETH_P_IPV6) &&
-	     !ipv6_addr_any(&saddr->u.ip6)))
+	    saddr->proto == htons(ETH_P_IPV6))
 		br_multicast_mark_router(br, port);
 }
 
-- 
2.17.2

Re: [Bridge] [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

[Stephen Hemminger]

On Sat, 27 Oct 2018 12:07:47 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> Recently a check was added which prevents marking of routers with zero
> source address, but for IPv6 that cannot happen as the relevant RFCs
> actually forbid such packets:
> RFC 2710 (MLDv1):
> "To be valid, the Query message MUST
>  come from a link-local IPv6 Source Address, be at least 24 octets
>  long, and have a correct MLD checksum."
> 
> Same goes for RFC 3810.
> 
> And also it can be seen as a requirement in ipv6_mc_check_mld_query()
> which is used by the bridge to validate the message before processing
> it. Thus any queries with :: source address won't be processed anyway.
> So just remove the check for zero IPv6 source address from the query
> processing function.
> 
> Fixes: 5a2de63fd1a5 ("bridge: do not add port to router list when receives query with source 0.0.0.0")
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

What about a broken/malicious sender? Could an all zero source be used
to poison the multicast table?

Re: [Bridge] [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

[Nikolay Aleksandrov]

On 28/10/2018 17:20, Stephen Hemminger wrote:
> On Sat, 27 Oct 2018 12:07:47 +0300
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> 
>> Recently a check was added which prevents marking of routers with zero
>> source address, but for IPv6 that cannot happen as the relevant RFCs
>> actually forbid such packets:
>> RFC 2710 (MLDv1):
>> "To be valid, the Query message MUST
>>  come from a link-local IPv6 Source Address, be at least 24 octets
>>  long, and have a correct MLD checksum."
>>
>> Same goes for RFC 3810.
>>
>> And also it can be seen as a requirement in ipv6_mc_check_mld_query()
>> which is used by the bridge to validate the message before processing
>> it. Thus any queries with :: source address won't be processed anyway.
>> So just remove the check for zero IPv6 source address from the query
>> processing function.
>>
>> Fixes: 5a2de63fd1a5 ("bridge: do not add port to router list when receives query with source 0.0.0.0")
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> 
> What about a broken/malicious sender? Could an all zero source be used
> to poison the multicast table?
> 

No, this has nothing to do with the table. This is about marking routers
and we shouldn't consider queries with src 0.0.0.0 (the original fix)
but for IPv6 such query is invalid and in fact doesn't reach that code at all.
As I've written in the commit message, ipv6_mc_check_mld_query() already checks
for that and marks it as invalid thus it isn't processed and we can drop that
check from the bridge mcast code, if you test with such src you can see in
the bridge mcast statistics that the MLD errors are going up showing that these
packets are treated as errors.

Thanks,
 Nik

Re: [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

[Hangbin Liu]

On Sat, Oct 27, 2018 at 12:07:47PM +0300, Nikolay Aleksandrov wrote:
> Recently a check was added which prevents marking of routers with zero
> source address, but for IPv6 that cannot happen as the relevant RFCs
> actually forbid such packets:
> RFC 2710 (MLDv1):
> "To be valid, the Query message MUST
>  come from a link-local IPv6 Source Address, be at least 24 octets
>  long, and have a correct MLD checksum."
> 
> Same goes for RFC 3810.
> 
> And also it can be seen as a requirement in ipv6_mc_check_mld_query()
> which is used by the bridge to validate the message before processing
> it. Thus any queries with :: source address won't be processed anyway.
> So just remove the check for zero IPv6 source address from the query
> processing function.
> 
> Fixes: 5a2de63fd1a5 ("bridge: do not add port to router list when receives query with source 0.0.0.0")
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Opps.. Sorry for the mistake and thank you for your fix.

Regards
Hangbin

> ---
>  net/bridge/br_multicast.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 41cdafbf2ebe..6bac0d6b7b94 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1428,8 +1428,7 @@ static void br_multicast_query_received(struct net_bridge *br,
>  	 * is 0.0.0.0 should not be added to router port list.
>  	 */
>  	if ((saddr->proto == htons(ETH_P_IP) && saddr->u.ip4) ||
> -	    (saddr->proto == htons(ETH_P_IPV6) &&
> -	     !ipv6_addr_any(&saddr->u.ip6)))
> +	    saddr->proto == htons(ETH_P_IPV6))
>  		br_multicast_mark_router(br, port);
>  }
>  
> -- 
> 2.17.2
> 

Re: [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

[David Miller]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Sat, 27 Oct 2018 12:07:47 +0300

> Recently a check was added which prevents marking of routers with zero
> source address, but for IPv6 that cannot happen as the relevant RFCs
> actually forbid such packets:
> RFC 2710 (MLDv1):
> "To be valid, the Query message MUST
>  come from a link-local IPv6 Source Address, be at least 24 octets
>  long, and have a correct MLD checksum."
> 
> Same goes for RFC 3810.
> 
> And also it can be seen as a requirement in ipv6_mc_check_mld_query()
> which is used by the bridge to validate the message before processing
> it. Thus any queries with :: source address won't be processed anyway.
> So just remove the check for zero IPv6 source address from the query
> processing function.
> 
> Fixes: 5a2de63fd1a5 ("bridge: do not add port to router list when receives query with source 0.0.0.0")
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Applied.

Re: [Bridge] [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

[Linus Lüssing]

Even though RFC4541 recommends this, I'm not quite sure whether
this works... even for IGMP.

I think this would lead to multicast packet loss in a scenario
like this:

----------

     [Switch A] -------------- [Switch B]
        /                          /
       /                          /
      /                          /
 (Host A)                   (Host B)


- Snooping Switches: Switch A + Switch B
- Selected Querier: Switch A, with 0.0.0.0 query source
- Multicast Listener: Host A
- Multicast Data Sender: Host B

1) Host A sends IGMP report to Switch A
2) Switch A refrains from forwarding it to Switch B
   (reports are only forwarded to multicast routers according to
    RFC4541)
   => Switch B does not learn about listeners on Host A

Now, with this patch and recommendation in RFC4541 to not add queries
with a 0.0.0.0 source address to the multicast router port list:

3) Host B sends multicast data to Switch B
   => Switch B does not forward it to Switch A as it neither
      detected a multicast listener nor multicast router on
      the according port.
   => Host A does not receive the multicast data it signed up for

(Or with colors:
https://metameute.de/~tux/linux/bridge/query-zero-source-no-mcrouter-port.png)

----------

Alternatively we would need to ignore 0.0.0.0 for the querier
election and "querier present" detection. And by that disable
multicast snooping if there are no queries from a non-zero source
address.

But I'm a little hesitant whether ignoring is a reliable way as
IGMPv3 (RFC3376) and IGMPv2 (RFC2236) make no such restrictions
regarding the query source address.

With no such restrictions according to RFC3376/RFC2236 a 0.0.0.0
would always win the querier election. Meaning any potential
querier with a non-zero source address would remain silent.
Meaning we would always disable multicast snooping then?


Adding queriers with a 0.0.0.0 source address to the multicast
router list, too, seems like a less harmful way then disabling multicast
snooping completely?

----------

However, one of the two options seems to be necessary. Either
reverting the patch for the IGMP part, too. Or Ignoring 0.0.0.0
sources for querier eletcion and presence detection.

The current state seems broken to me unless I'm missing something.

Re: [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

[Ying Xu]

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

 I think the scenario mentioned above is abnormal.
According to rfc 4541, multicast router port means this port is attached to
a real router.

The source of query indicats that is a real router or only a
switch.(0.0.0.0 means switch,non-zero means router).
In the scenario above,the switch A was selected to be a querier that means
A performs as a router,
so switch A should config its query source address to non-zero,and then
Host A can recieve the traffic from B.

On Fri, Dec 14, 2018 at 12:10 AM Linus Lüssing <linus.luessing@c0d3.blue>
wrote:

> Even though RFC4541 recommends this, I'm not quite sure whether
> this works... even for IGMP.
>
> I think this would lead to multicast packet loss in a scenario
> like this:
>
> ----------
>
>      [Switch A] -------------- [Switch B]
>         /                          /
>        /                          /
>       /                          /
>  (Host A)                   (Host B)
>
>
> - Snooping Switches: Switch A + Switch B
> - Selected Querier: Switch A, with 0.0.0.0 query source
> - Multicast Listener: Host A
> - Multicast Data Sender: Host B
>
> 1) Host A sends IGMP report to Switch A
> 2) Switch A refrains from forwarding it to Switch B
>    (reports are only forwarded to multicast routers according to
>     RFC4541)
>    => Switch B does not learn about listeners on Host A
>
> Now, with this patch and recommendation in RFC4541 to not add queries
> with a 0.0.0.0 source address to the multicast router port list:
>
> 3) Host B sends multicast data to Switch B
>    => Switch B does not forward it to Switch A as it neither
>       detected a multicast listener nor multicast router on
>       the according port.
>    => Host A does not receive the multicast data it signed up for
>
> (Or with colors:
>
> https://metameute.de/~tux/linux/bridge/query-zero-source-no-mcrouter-port.png
> )
>
> ----------
>
> Alternatively we would need to ignore 0.0.0.0 for the querier
> election and "querier present" detection. And by that disable
> multicast snooping if there are no queries from a non-zero source
> address.
>
> But I'm a little hesitant whether ignoring is a reliable way as
> IGMPv3 (RFC3376) and IGMPv2 (RFC2236) make no such restrictions
> regarding the query source address.
>
> With no such restrictions according to RFC3376/RFC2236 a 0.0.0.0
> would always win the querier election. Meaning any potential
> querier with a non-zero source address would remain silent.
> Meaning we would always disable multicast snooping then?
>
>
> Adding queriers with a 0.0.0.0 source address to the multicast
> router list, too, seems like a less harmful way then disabling multicast
> snooping completely?
>
> ----------
>
> However, one of the two options seems to be necessary. Either
> reverting the patch for the IGMP part, too. Or Ignoring 0.0.0.0
> sources for querier eletcion and presence detection.
>
> The current state seems broken to me unless I'm missing something.
>

[-- Attachment #2: Type: text/html, Size: 3644 bytes --]

Re: [Bridge] [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

[Linus Lüssing]

Hi and thanks for your reply!

On Fri, Dec 14, 2018 at 10:32:16AM +0800, Ying Xu wrote:
>  I think the scenario mentioned above is abnormal.

Can we agree, that this scenario, if switch A and B were using the
current bridge code, has issues right now which it did
not have before that patch?

I also do not quite understand what you mean with "abnormal". Do
you think that it is unlikely to have two snooping switches and
general queries with a 0.0.0.0 source?

Note that with the current bridge code and according to RFC3376
and RFC2236, as soon as a query with a 0.0.0.0 source is sent somewhere
in the broadcast domain, it will become the selected querier [*].


> The source of query indicats that is a real router or only a switch.(0.0.0.0
> means switch,non-zero means router).
> In the scenario above,the switch A was selected to be a querier that means A
> performs as a router,
> so switch A should config its query source address to non-zero,and then Host A
> can recieve the traffic from B.

Even if in the described scenario switch A were configured to use a a non-zero source
address to become a router, so that switch B would mark the port
to switch A as a multicast router port, switch A would still loose
in the querier election right now, as 0.0.0.0 is lower (RFC3376, RFC2236). So switch
B would then become the selected querier with its 0.0.0.0 source
and switch A would become silent even though it had a non-zero
source address.

And then we would have the same issue again, only swapped between
host+switch A and host+switch B.

Would you agree, does that make sense?

Regards, Linus


[*]: Looking at br_ip4_multicast_select_querier():

If previously selected querier were 0.0.0.0, it would accept any
source as a new querier ("!br->ip4_querier.addr.u.ip4"). However,
if the previously selected querier were non-zero, a query with
0.0.0.0 would win, too
("ntohl(saddr) <= ntohl(br->ip4_querier.addr.u.ip4)").

Re: [Bridge] [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

[Hangbin Liu]

Hi Linus,

Sorry, my mail client somehow droped your message and I didn't see your reply.
I find this mail after Nikolay pointed out yesterday.

> Hi and thanks for your reply!
> 
> On Fri, Dec 14, 2018 at 10:32:16AM +0800, Ying Xu wrote:
> >  I think the scenario mentioned above is abnormal.
> 
> Can we agree, that this scenario, if switch A and B were using the
> current bridge code, has issues right now which it did
> not have before that patch?

Yes, I agree. But this "regression" could be fixed by setting up correct
switch configuration. See more explains below.

> 
> I also do not quite understand what you mean with "abnormal". Do
> you think that it is unlikely to have two snooping switches and
> general queries with a 0.0.0.0 source?

The "abnormal" means we shouldn't use this kind of configuration on
snooping switch.

We could have general queries with a 0.0.0.0 source, but these queries
are only from proxy Querier, not from real Querier.

Based on RFC 4541,
2.1.1.  IGMP Forwarding Rules
      b) The arrival port for IGMP Queries (sent by multicast routers)
         where the source address is not 0.0.0.0.

         The 0.0.0.0 address represents a special case where the switch
         is proxying IGMP Queries for faster network convergence, but is
         not itself the Querier.  The switch does not use its own IP
         address (even if it has one), because this would cause the
         Queries to be seen as coming from a newly elected Querier.  The
         0.0.0.0 address is used to indicate that the Query packets are
         NOT from a multicast router.

This paragraph specified "0.0.0.0" is only for switch proxying queries.
It should not be used as a IGMP Querier.

"because this would cause the Queries to be seen as coming from a newly
 elected Querier" means other address could be elected as a Querier but
"0.0.0.0" should not.

AFAIK, most verdors(Cisco, HW, etc. expect H3C) use none-zero address
as default address on proxying switches. But H3C also respect the
b) section.

>
> Note that with the current bridge code and according to RFC3376
> and RFC2236, as soon as a query with a 0.0.0.0 source is sent somewhere
> in the broadcast domain, it will become the selected querier [*].

Yes, In RFC 3376

6.6.2. Querier Election

   IGMPv3 elects a single querier per subnet using the same querier
   election mechanism as IGMPv2, namely by IP address.  When a router
   receives a query with a lower IP address, it sets the Other-Querier-
   Present timer to Other Querier Present Interval and ceases to send
   queries on the network if it was the previously elected querier.
   After its Other-Querier Present timer expires, it should begin
   sending General Queries.

It only said we should select lower IP address as Querier, but didn't specify
the "0.0.0.0" address.

While in the later RFC 4541

Abstract
   This memo describes the recommendations for Internet Group Management
   Protocol (IGMP) and Multicast Listener Discovery (MLD) snooping
   switches.  These are based on best current practices for IGMPv2, with
   further considerations for IGMPv3- and MLDv2-snooping.

And section 2.1.1.  IGMP Forwarding Rules(I just pasted above). It did
specify the "0.0.0.0" address issue.

I think we should respect the later and accurater standard, shouldn't we?

>
>
> > The source of query indicats that is a real router or only a switch.(0.0.0.0
> > means switch,non-zero means router).
> > In the scenario above,the switch A was selected to be a querier that means A
> > performs as a router,
> > so switch A should config its query source address to non-zero,and then Host A
> > can recieve the traffic from B.
>
> Even if in the described scenario switch A were configured to use a a non-zero
> source address to become a router, so that switch B would mark the port
> to switch A as a multicast router port, switch A would still loose
> in the querier election right now, as 0.0.0.0 is lower (RFC3376, RFC2236). So
> switch B would then become the selected querier with its 0.0.0.0 source
> and switch A would become silent even though it had a non-zero
> source address.
>
> And then we would have the same issue again, only swapped between
> host+switch A and host+switch B.
>
> Would you agree, does that make sense?
>

Yes, that's why Ying Xu said the topology is "abnormal", or illegal. Because
in a correct configured topology, there should has a Querier with none-zero
IP address.

> Regards, Linus
>
>
> [*]: Looking at br_ip4_multicast_select_querier():
>
> If previously selected querier were 0.0.0.0, it would accept any
> source as a new querier ("!br->ip4_querier.addr.u.ip4"). However,
> if the previously selected querier were non-zero, a query with
> 0.0.0.0 would win, too
> ("ntohl(saddr) <= ntohl(br->ip4_querier.addr.u.ip4)").

In RFC 3810

7.6.2.  Querier Election
   When a router starts operating on a subnet, by default it
   considers itself as being the Querier.

   When a router receives a query with a lower IPv6 address than its
   own, it sets the Other Querier Present timer to Other Querier Present
   Timeout; if it was previously in Querier state, it switches to Non-
   Querier state and ceases to send queries on the link.

   All MLDv2 queries MUST be sent with the FE80::/64 link-local source
   address prefix.

MLDv2 also sepcified the default Querier is itself and electe a lower adddress
as new Querier. And the source address should be link-loacl source address
instead of 0("::"). This is another evidence that we should not use "0.0.0.0"
as a Querier. So I think we should fix the IPv4 election in function
br_ip4_multicast_select_querier().

This looks like a little extreme and may have "regression", but the "regression"
should be fixed by setting up correct router/switch configuration.

What do you think?

Regards
Hangbin

Re: [Bridge] [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

[Nikolay Aleksandrov]

On 21/02/2019 10:01, Hangbin Liu wrote:
> Hi Linus,
> 
> Sorry, my mail client somehow droped your message and I didn't see your reply.
> I find this mail after Nikolay pointed out yesterday.
> 
>> Hi and thanks for your reply!
>>
>> On Fri, Dec 14, 2018 at 10:32:16AM +0800, Ying Xu wrote:
>>>  I think the scenario mentioned above is abnormal.
>>
>> Can we agree, that this scenario, if switch A and B were using the
>> current bridge code, has issues right now which it did
>> not have before that patch?
> 
> Yes, I agree. But this "regression" could be fixed by setting up correct
> switch configuration. See more explains below.
> 

That is irrelevant, if the setup once worked we should not break it unless
it's in RFC requirement violation and RFC 4541 is only suggestive, not required.

>>
>> I also do not quite understand what you mean with "abnormal". Do
>> you think that it is unlikely to have two snooping switches and
>> general queries with a 0.0.0.0 source?
> 
> The "abnormal" means we shouldn't use this kind of configuration on
> snooping switch.
> 
> We could have general queries with a 0.0.0.0 source, but these queries
> are only from proxy Querier, not from real Querier.
> 
> Based on RFC 4541,
> 2.1.1.  IGMP Forwarding Rules
>       b) The arrival port for IGMP Queries (sent by multicast routers)
>          where the source address is not 0.0.0.0.
> 
>          The 0.0.0.0 address represents a special case where the switch
>          is proxying IGMP Queries for faster network convergence, but is
>          not itself the Querier.  The switch does not use its own IP
>          address (even if it has one), because this would cause the
>          Queries to be seen as coming from a newly elected Querier.  The
>          0.0.0.0 address is used to indicate that the Query packets are
>          NOT from a multicast router.
> 
> This paragraph specified "0.0.0.0" is only for switch proxying queries.
> It should not be used as a IGMP Querier.
> 
> "because this would cause the Queries to be seen as coming from a newly
>  elected Querier" means other address could be elected as a Querier but
> "0.0.0.0" should not.
> 

But this change hasn't been incorporated, has it ? A 0.0.0.0 address currently
will always win the election and silence all of the rest. Current bridge state
is simply broken for some cases because of that.

> AFAIK, most verdors(Cisco, HW, etc. expect H3C) use none-zero address
> as default address on proxying switches. But H3C also respect the
> b) section.
> 
>>
>> Note that with the current bridge code and according to RFC3376
>> and RFC2236, as soon as a query with a 0.0.0.0 source is sent somewhere
>> in the broadcast domain, it will become the selected querier [*].
> 
> Yes, In RFC 3376
> 
> 6.6.2. Querier Election
> 
>    IGMPv3 elects a single querier per subnet using the same querier
>    election mechanism as IGMPv2, namely by IP address.  When a router
>    receives a query with a lower IP address, it sets the Other-Querier-
>    Present timer to Other Querier Present Interval and ceases to send
>    queries on the network if it was the previously elected querier.
>    After its Other-Querier Present timer expires, it should begin
>    sending General Queries.
> 
> It only said we should select lower IP address as Querier, but didn't specify
> the "0.0.0.0" address.
> 
> While in the later RFC 4541
> 
> Abstract
>    This memo describes the recommendations for Internet Group Management
>    Protocol (IGMP) and Multicast Listener Discovery (MLD) snooping
>    switches.  These are based on best current practices for IGMPv2, with
>    further considerations for IGMPv3- and MLDv2-snooping.
> 
> And section 2.1.1.  IGMP Forwarding Rules(I just pasted above). It did
> specify the "0.0.0.0" address issue.
> 
> I think we should respect the later and accurater standard, shouldn't we?
> 

No if it is breaking valid setups, RFC 4541 is only a suggestion and in this case
the bridge has worked like that for a long time. The current situation is broken
because a 0.0.0.0 querier will win the election always and will silence the rest.

If it's not involved in the election then we'll have snooping effectively disabled
unless a non-zero querier shows up which again sounds wrong and was already pointed
out by Linus. I think the best course of action is to revert this change.

>>
>>
>>> The source of query indicats that is a real router or only a switch.(0.0.0.0
>>> means switch,non-zero means router).
>>> In the scenario above,the switch A was selected to be a querier that means A
>>> performs as a router,
>>> so switch A should config its query source address to non-zero,and then Host A
>>> can recieve the traffic from B.
>>
>> Even if in the described scenario switch A were configured to use a a non-zero
>> source address to become a router, so that switch B would mark the port
>> to switch A as a multicast router port, switch A would still loose
>> in the querier election right now, as 0.0.0.0 is lower (RFC3376, RFC2236). So
>> switch B would then become the selected querier with its 0.0.0.0 source
>> and switch A would become silent even though it had a non-zero
>> source address.
>>
>> And then we would have the same issue again, only swapped between
>> host+switch A and host+switch B.
>>
>> Would you agree, does that make sense?
>>
> 
> Yes, that's why Ying Xu said the topology is "abnormal", or illegal. Because
> in a correct configured topology, there should has a Querier with none-zero
> IP address.
> 

Certainly not illegal.

>> Regards, Linus
>>
>>
>> [*]: Looking at br_ip4_multicast_select_querier():
>>
>> If previously selected querier were 0.0.0.0, it would accept any
>> source as a new querier ("!br->ip4_querier.addr.u.ip4"). However,
>> if the previously selected querier were non-zero, a query with
>> 0.0.0.0 would win, too
>> ("ntohl(saddr) <= ntohl(br->ip4_querier.addr.u.ip4)").
> 
> In RFC 3810
> 
> 7.6.2.  Querier Election
>    When a router starts operating on a subnet, by default it
>    considers itself as being the Querier.
> 
>    When a router receives a query with a lower IPv6 address than its
>    own, it sets the Other Querier Present timer to Other Querier Present
>    Timeout; if it was previously in Querier state, it switches to Non-
>    Querier state and ceases to send queries on the link.
> 
>    All MLDv2 queries MUST be sent with the FE80::/64 link-local source
>    address prefix.
> 
> MLDv2 also sepcified the default Querier is itself and electe a lower adddress
> as new Querier. And the source address should be link-loacl source address
> instead of 0("::"). This is another evidence that we should not use "0.0.0.0"
> as a Querier. So I think we should fix the IPv4 election in function
> br_ip4_multicast_select_querier().
> 
> This looks like a little extreme and may have "regression", but the "regression"
> should be fixed by setting up correct router/switch configuration.
> 
> What do you think?
> 

Removing 0.0.0.0 from the election will effectively disable snooping even if there's
a configured bridge unless it has an address. You can see that this will end up in
people having suddenly their multicast flooded with current setups, right ?
Any big behaviour change like that should be optional, but I don't think we need 
another option as this is not so big of a deal because we're not breaking any
required behaviour.
In case you decide to follow the option path, please use the new boolopt api to avoid
adding new fields to the bridge, this should be an on/off thing. I still vote for a
revert though.

> Regards
> Hangbin
> 

Thanks,
 Nik

Re: [Bridge] [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

[Hangbin Liu]

Hi Nikolay,

On Thu, Feb 21, 2019 at 03:20:14PM +0200, Nikolay Aleksandrov wrote:
> > 
> > Yes, I agree. But this "regression" could be fixed by setting up correct
> > switch configuration. See more explains below.
> > 
> 
> That is irrelevant, if the setup once worked we should not break it unless
> it's in RFC requirement violation and RFC 4541 is only suggestive, not required.

Thanks for your reply. I just noticed the RFC4541 category is informational.

> > "because this would cause the Queries to be seen as coming from a newly
> >  elected Querier" means other address could be elected as a Querier but
> > "0.0.0.0" should not.
> > 
> 
> But this change hasn't been incorporated, has it ? A 0.0.0.0 address currently
> will always win the election and silence all of the rest. Current bridge state
> is simply broken for some cases because of that.

Yes. I agree. I realized linus also said

"""
However, one of the two options seems to be necessary. Either
reverting the patch for the IGMP part, too. Or Ignoring 0.0.0.0
sources for querier eletcion and presence detection.
"""

> 
> Removing 0.0.0.0 from the election will effectively disable snooping even if there's
> a configured bridge unless it has an address. You can see that this will end up in
> people having suddenly their multicast flooded with current setups, right ?

Yes

> Any big behaviour change like that should be optional, but I don't think we need 
> another option as this is not so big of a deal because we're not breaking any
> required behaviour.

Just a little curious, RFC 3376 said the General Queries are sent from multicast
routers. I think a router *should* has a IP address, isn't it?

RFC 4541 also suggested:

      If the switch is not the Querier, it should use the 'all-zeros' IP
      Source Address in these proxy queries (even though some hosts may
      elect to not process queries with a 0.0.0.0 IP Source Address).
      When such proxy queries are received, they must not be included in
      the Querier election process.

And what I got is most vendors apply this suggestion.

> In case you decide to follow the option path, please use the new boolopt api to avoid
> adding new fields to the bridge, this should be an on/off thing. I still vote for a
> revert though.

For consistency with other vendors and rfc, I would prefer to remove zero address election.
For compatibility with previous users, I'm also OK to revert it.

Regards
Hangbin

Re: [Bridge] [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

[Nikolay Aleksandrov]

On 22/02/2019 09:57, Hangbin Liu wrote:
> Hi Nikolay,
> 
> On Thu, Feb 21, 2019 at 03:20:14PM +0200, Nikolay Aleksandrov wrote:
>>>
>>> Yes, I agree. But this "regression" could be fixed by setting up correct
>>> switch configuration. See more explains below.
>>>
>>
>> That is irrelevant, if the setup once worked we should not break it unless
>> it's in RFC requirement violation and RFC 4541 is only suggestive, not required.
> 
> Thanks for your reply. I just noticed the RFC4541 category is informational.
> 
>>> "because this would cause the Queries to be seen as coming from a newly
>>>  elected Querier" means other address could be elected as a Querier but
>>> "0.0.0.0" should not.
>>>
>>
>> But this change hasn't been incorporated, has it ? A 0.0.0.0 address currently
>> will always win the election and silence all of the rest. Current bridge state
>> is simply broken for some cases because of that.
> 
> Yes. I agree. I realized linus also said
> 
> """
> However, one of the two options seems to be necessary. Either
> reverting the patch for the IGMP part, too. Or Ignoring 0.0.0.0
> sources for querier eletcion and presence detection.
> """
> 
>>
>> Removing 0.0.0.0 from the election will effectively disable snooping even if there's
>> a configured bridge unless it has an address. You can see that this will end up in
>> people having suddenly their multicast flooded with current setups, right ?
> 
> Yes
> 
>> Any big behaviour change like that should be optional, but I don't think we need 
>> another option as this is not so big of a deal because we're not breaking any
>> required behaviour.
> 
> Just a little curious, RFC 3376 said the General Queries are sent from multicast
> routers. I think a router *should* has a IP address, isn't it?
> 
> RFC 4541 also suggested:
> 
>       If the switch is not the Querier, it should use the 'all-zeros' IP
>       Source Address in these proxy queries (even though some hosts may
>       elect to not process queries with a 0.0.0.0 IP Source Address).
>       When such proxy queries are received, they must not be included in
>       the Querier election process.
> 
> And what I got is most vendors apply this suggestion.
> 
>> In case you decide to follow the option path, please use the new boolopt api to avoid
>> adding new fields to the bridge, this should be an on/off thing. I still vote for a
>> revert though.
> 
> For consistency with other vendors and rfc, I would prefer to remove zero address election.
> For compatibility with previous users, I'm also OK to revert it.
> > Regards
> Hangbin
> 

Hi,
In this case I'd suggest the following course of action:
 - For -net/-stable revert the change since backporting new options is a
   no-go and we need to restore the bridge state
 - After -net is merged in net-next, for net-next if you'd like add it
   as an option and also exclude it from elections when the option is
   enabled (for example something like multicast_nonzero_src_querier).
   Just please use the boolopt api and don't add new fields/attr ids.
   Obviously by default this option will be off to be backwards
   compatible and avoid surprise mcast flood.

Or just leave it reverted. :)

Thanks,
 Nik

Re: [Bridge] [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

[Hangbin Liu]

On Fri, Feb 22, 2019 at 01:16:53PM +0200, Nikolay Aleksandrov wrote:
> > 
> > For consistency with other vendors and rfc, I would prefer to remove zero address election.
> > For compatibility with previous users, I'm also OK to revert it.
> > > Regards
> > Hangbin
> > 
> 
> Hi,
> In this case I'd suggest the following course of action:
>  - For -net/-stable revert the change since backporting new options is a
>    no-go and we need to restore the bridge state
>  - After -net is merged in net-next, for net-next if you'd like add it
>    as an option and also exclude it from elections when the option is
>    enabled (for example something like multicast_nonzero_src_querier).
>    Just please use the boolopt api and don't add new fields/attr ids.
>    Obviously by default this option will be off to be backwards
>    compatible and avoid surprise mcast flood.
> 
> Or just leave it reverted. :)

This makes sense to me. Thanks for your suggestions.

Regards
Hangbin