netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next] mcast: ensure L-L IPv6 packets are accepted by bridge
@ 2019-08-13 14:18 Patrick Ruddy
  2019-08-13 19:53 ` Ido Schimmel
  0 siblings, 1 reply; 8+ messages in thread
From: Patrick Ruddy @ 2019-08-13 14:18 UTC (permalink / raw)
  To: netdev

At present only all-nodes IPv6 multicast packets are accepted by
a bridge interface that is not in multicast router mode. Since
other protocols can be running in the absense of multicast
forwarding e.g. OSPFv3 IPv6 ND. Change the test to allow
all of the FFx2::/16 range to be accepted when not in multicast
router mode. This aligns the code with IPv4 link-local reception
and RFC4291

Signed-off-by: Patrick Ruddy <pruddy@vyatta.att-mail.com>
---
 include/net/addrconf.h    | 15 +++++++++++++++
 net/bridge/br_multicast.c |  2 +-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index becdad576859..05b42867e969 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -434,6 +434,21 @@ static inline void addrconf_addr_solict_mult(const struct in6_addr *addr,
 		      htonl(0xFF000000) | addr->s6_addr32[3]);
 }
 
+/*
+ *      link local multicast address range ffx2::/16 rfc4291
+ */
+static inline bool ipv6_addr_is_ll_mcast(const struct in6_addr *addr)
+{
+#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
+	__be64 *p = (__be64 *)addr;
+	return ((p[0] & cpu_to_be64(0xff0f000000000000UL))
+		^ cpu_to_be64(0xff02000000000000UL)) == 0UL;
+#else
+	return ((addr->s6_addr32[0] & htonl(0xff0f0000)) ^
+		htonl(0xff020000)) == 0;
+#endif
+}
+
 static inline bool ipv6_addr_is_ll_all_nodes(const struct in6_addr *addr)
 {
 #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 9b379e110129..ed3957381fa2 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1664,7 +1664,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 	err = ipv6_mc_check_mld(skb);
 
 	if (err == -ENOMSG) {
-		if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
+		if (!ipv6_addr_is_ll_mcast(&ipv6_hdr(skb)->daddr))
 			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
 
 		if (ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr)) {
-- 
2.20.1


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

* Re: [PATCH net-next] mcast: ensure L-L IPv6 packets are accepted by bridge
  2019-08-13 14:18 [PATCH net-next] mcast: ensure L-L IPv6 packets are accepted by bridge Patrick Ruddy
@ 2019-08-13 19:53 ` Ido Schimmel
  2019-08-13 23:55   ` Nikolay Aleksandrov
  0 siblings, 1 reply; 8+ messages in thread
From: Ido Schimmel @ 2019-08-13 19:53 UTC (permalink / raw)
  To: Patrick Ruddy; +Cc: netdev, roopa, nikolay, linus.luessing

+ Bridge maintainers, Linus

On Tue, Aug 13, 2019 at 03:18:04PM +0100, Patrick Ruddy wrote:
> At present only all-nodes IPv6 multicast packets are accepted by
> a bridge interface that is not in multicast router mode. Since
> other protocols can be running in the absense of multicast
> forwarding e.g. OSPFv3 IPv6 ND. Change the test to allow
> all of the FFx2::/16 range to be accepted when not in multicast
> router mode. This aligns the code with IPv4 link-local reception
> and RFC4291

Can you please quote the relevant part from RFC 4291?

> 
> Signed-off-by: Patrick Ruddy <pruddy@vyatta.att-mail.com>
> ---
>  include/net/addrconf.h    | 15 +++++++++++++++
>  net/bridge/br_multicast.c |  2 +-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> index becdad576859..05b42867e969 100644
> --- a/include/net/addrconf.h
> +++ b/include/net/addrconf.h
> @@ -434,6 +434,21 @@ static inline void addrconf_addr_solict_mult(const struct in6_addr *addr,
>  		      htonl(0xFF000000) | addr->s6_addr32[3]);
>  }
>  
> +/*
> + *      link local multicast address range ffx2::/16 rfc4291
> + */
> +static inline bool ipv6_addr_is_ll_mcast(const struct in6_addr *addr)
> +{
> +#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
> +	__be64 *p = (__be64 *)addr;
> +	return ((p[0] & cpu_to_be64(0xff0f000000000000UL))
> +		^ cpu_to_be64(0xff02000000000000UL)) == 0UL;
> +#else
> +	return ((addr->s6_addr32[0] & htonl(0xff0f0000)) ^
> +		htonl(0xff020000)) == 0;
> +#endif
> +}
> +
>  static inline bool ipv6_addr_is_ll_all_nodes(const struct in6_addr *addr)
>  {
>  #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 9b379e110129..ed3957381fa2 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1664,7 +1664,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
>  	err = ipv6_mc_check_mld(skb);
>  
>  	if (err == -ENOMSG) {
> -		if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
> +		if (!ipv6_addr_is_ll_mcast(&ipv6_hdr(skb)->daddr))
>  			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;

IIUC, you want IPv6 link-local packets to be locally received, but this
also changes how these packets are flooded. RFC 4541 says that packets
addressed to the all hosts address are a special case and should be
forwarded to all ports:

"In IPv6, the data forwarding rules are more straight forward because MLD is
mandated for addresses with scope 2 (link-scope) or greater. The only exception
is the address FF02::1 which is the all hosts link-scope address for which MLD
messages are never sent. Packets with the all hosts link-scope address should
be forwarded on all ports."

Maybe you want something like:

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 09b1dd8cd853..9f312a73f61c 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -132,7 +132,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
 		    br_multicast_querier_exists(br, eth_hdr(skb))) {
 			if ((mdst && mdst->host_joined) ||
-			    br_multicast_is_router(br)) {
+			    br_multicast_is_router(br) ||
+			    BR_INPUT_SKB_CB_LOCAL_RECEIVE(skb)) {
 				local_rcv = true;
 				br->dev->stats.multicast++;
 			}
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 9b379e110129..f03cecf6174e 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1667,6 +1667,9 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 		if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
 			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
 
+		if (ipv6_addr_is_ll_mcast(&ipv6_hdr(skb)->daddr))
+			BR_INPUT_SKB_CB(skb)->local_receive = 1;
+
 		if (ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr)) {
 			err = br_ip6_multicast_mrd_rcv(br, port, skb);
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index b7a4942ff1b3..d76394ca4059 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -426,6 +426,7 @@ struct br_input_skb_cb {
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	u8 igmp;
 	u8 mrouters_only:1;
+	u8 local_receive:1;
 #endif
 	u8 proxyarp_replied:1;
 	u8 src_port_isolated:1;
@@ -445,8 +446,10 @@ struct br_input_skb_cb {
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 # define BR_INPUT_SKB_CB_MROUTERS_ONLY(__skb)	(BR_INPUT_SKB_CB(__skb)->mrouters_only)
+# define BR_INPUT_SKB_CB_LOCAL_RECEIVE(__skb)	(BR_INPUT_SKB_CB(__skb)->local_receive)
 #else
 # define BR_INPUT_SKB_CB_MROUTERS_ONLY(__skb)	(0)
+# define BR_INPUT_SKB_CB_LOCAL_RECEIVE(__skb)	(0)
 #endif
 
 #define br_printk(level, br, format, args...)	\

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

* Re: [PATCH net-next] mcast: ensure L-L IPv6 packets are accepted by bridge
  2019-08-13 19:53 ` Ido Schimmel
@ 2019-08-13 23:55   ` Nikolay Aleksandrov
  2019-08-14 16:40     ` Patrick Ruddy
  0 siblings, 1 reply; 8+ messages in thread
From: Nikolay Aleksandrov @ 2019-08-13 23:55 UTC (permalink / raw)
  To: Ido Schimmel, Patrick Ruddy; +Cc: netdev, roopa, linus.luessing

On 8/13/19 10:53 PM, Ido Schimmel wrote:
> + Bridge maintainers, Linus
> 

Good catch Ido, thanks!
First I'd say the subject needs to reflect that this is a bridge change
better, please rearrange it like so - bridge: mcast: ...
More below,

> On Tue, Aug 13, 2019 at 03:18:04PM +0100, Patrick Ruddy wrote:
>> At present only all-nodes IPv6 multicast packets are accepted by
>> a bridge interface that is not in multicast router mode. Since
>> other protocols can be running in the absense of multicast
>> forwarding e.g. OSPFv3 IPv6 ND. Change the test to allow
>> all of the FFx2::/16 range to be accepted when not in multicast
>> router mode. This aligns the code with IPv4 link-local reception
>> and RFC4291
> 
> Can you please quote the relevant part from RFC 4291?
>
>>
>> Signed-off-by: Patrick Ruddy <pruddy@vyatta.att-mail.com>
>> ---
>>  include/net/addrconf.h    | 15 +++++++++++++++
>>  net/bridge/br_multicast.c |  2 +-
>>  2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
>> index becdad576859..05b42867e969 100644
>> --- a/include/net/addrconf.h
>> +++ b/include/net/addrconf.h
>> @@ -434,6 +434,21 @@ static inline void addrconf_addr_solict_mult(const struct in6_addr *addr,
>>  		      htonl(0xFF000000) | addr->s6_addr32[3]);
>>  }
>>  
>> +/*
>> + *      link local multicast address range ffx2::/16 rfc4291
>> + */
>> +static inline bool ipv6_addr_is_ll_mcast(const struct in6_addr *addr)
>> +{
>> +#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
>> +	__be64 *p = (__be64 *)addr;
>> +	return ((p[0] & cpu_to_be64(0xff0f000000000000UL))
>> +		^ cpu_to_be64(0xff02000000000000UL)) == 0UL;
>> +#else
>> +	return ((addr->s6_addr32[0] & htonl(0xff0f0000)) ^
>> +		htonl(0xff020000)) == 0;
>> +#endif
>> +}
>> +
>>  static inline bool ipv6_addr_is_ll_all_nodes(const struct in6_addr *addr)
>>  {
>>  #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>> index 9b379e110129..ed3957381fa2 100644
>> --- a/net/bridge/br_multicast.c
>> +++ b/net/bridge/br_multicast.c
>> @@ -1664,7 +1664,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
>>  	err = ipv6_mc_check_mld(skb);
>>  
>>  	if (err == -ENOMSG) {
>> -		if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
>> +		if (!ipv6_addr_is_ll_mcast(&ipv6_hdr(skb)->daddr))
>>  			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
> 
> IIUC, you want IPv6 link-local packets to be locally received, but this
> also changes how these packets are flooded. RFC 4541 says that packets

Indeed, we'll start flooding them all, not just the all hosts address.
If that is at all required it'll definitely have to be optional.

> addressed to the all hosts address are a special case and should be
> forwarded to all ports:
> 
> "In IPv6, the data forwarding rules are more straight forward because MLD is
> mandated for addresses with scope 2 (link-scope) or greater. The only exception
> is the address FF02::1 which is the all hosts link-scope address for which MLD
> messages are never sent. Packets with the all hosts link-scope address should
> be forwarded on all ports."
>

I wonder what is the problem for the host to join such group on behalf of the bridge ?
Then you'll receive the traffic at least locally and the RFC says it itself - MLD is mandated
for the other link-local addresses.
It's very late here and maybe I'm missing something.. :)
 
> Maybe you want something like:
> 

I think we can do without the new field, either pass local_rcv into br_multicast_rcv() or
set it based on return value. The extra test will have to remain unfortunately, but we
can reduce the tests by one if carefully done.

> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 09b1dd8cd853..9f312a73f61c 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -132,7 +132,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
>  		    br_multicast_querier_exists(br, eth_hdr(skb))) {
>  			if ((mdst && mdst->host_joined) ||
> -			    br_multicast_is_router(br)) {
> +			    br_multicast_is_router(br) ||
> +			    BR_INPUT_SKB_CB_LOCAL_RECEIVE(skb)) {
>  				local_rcv = true;
>  				br->dev->stats.multicast++;
>  			}
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 9b379e110129..f03cecf6174e 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1667,6 +1667,9 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
>  		if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
>  			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
>  
> +		if (ipv6_addr_is_ll_mcast(&ipv6_hdr(skb)->daddr))
> +			BR_INPUT_SKB_CB(skb)->local_receive = 1;
> +
>  		if (ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr)) {
>  			err = br_ip6_multicast_mrd_rcv(br, port, skb);
>  
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index b7a4942ff1b3..d76394ca4059 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -426,6 +426,7 @@ struct br_input_skb_cb {
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>  	u8 igmp;
>  	u8 mrouters_only:1;
> +	u8 local_receive:1;
>  #endif
>  	u8 proxyarp_replied:1;
>  	u8 src_port_isolated:1;
> @@ -445,8 +446,10 @@ struct br_input_skb_cb {
>  
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>  # define BR_INPUT_SKB_CB_MROUTERS_ONLY(__skb)	(BR_INPUT_SKB_CB(__skb)->mrouters_only)
> +# define BR_INPUT_SKB_CB_LOCAL_RECEIVE(__skb)	(BR_INPUT_SKB_CB(__skb)->local_receive)
>  #else
>  # define BR_INPUT_SKB_CB_MROUTERS_ONLY(__skb)	(0)
> +# define BR_INPUT_SKB_CB_LOCAL_RECEIVE(__skb)	(0)
>  #endif
>  
>  #define br_printk(level, br, format, args...)	\
> 


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

* Re: [PATCH net-next] mcast: ensure L-L IPv6 packets are accepted by bridge
  2019-08-13 23:55   ` Nikolay Aleksandrov
@ 2019-08-14 16:40     ` Patrick Ruddy
  2019-08-14 16:58       ` Nikolay Aleksandrov
  2019-08-14 20:11       ` Linus Lüssing
  0 siblings, 2 replies; 8+ messages in thread
From: Patrick Ruddy @ 2019-08-14 16:40 UTC (permalink / raw)
  To: Nikolay Aleksandrov, Ido Schimmel; +Cc: netdev, roopa, linus.luessing

Thanks both for the quick replies, answers inline...

On Wed, 2019-08-14 at 02:55 +0300, Nikolay Aleksandrov wrote:
> On 8/13/19 10:53 PM, Ido Schimmel wrote:
> > + Bridge maintainers, Linus
> > 
> 
> Good catch Ido, thanks!
> First I'd say the subject needs to reflect that this is a bridge change
> better, please rearrange it like so - bridge: mcast: ...
> More below,
> 
> > On Tue, Aug 13, 2019 at 03:18:04PM +0100, Patrick Ruddy wrote:
> > > At present only all-nodes IPv6 multicast packets are accepted by
> > > a bridge interface that is not in multicast router mode. Since
> > > other protocols can be running in the absense of multicast
> > > forwarding e.g. OSPFv3 IPv6 ND. Change the test to allow
> > > all of the FFx2::/16 range to be accepted when not in multicast
> > > router mode. This aligns the code with IPv4 link-local reception
> > > and RFC4291
> > 
> > Can you please quote the relevant part from RFC 4291?
> > 
> > > Signed-off-by: Patrick Ruddy <pruddy@vyatta.att-mail.com>
> > > ---
> > >  include/net/addrconf.h    | 15 +++++++++++++++
> > >  net/bridge/br_multicast.c |  2 +-
> > >  2 files changed, 16 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> > > index becdad576859..05b42867e969 100644
> > > --- a/include/net/addrconf.h
> > > +++ b/include/net/addrconf.h
> > > @@ -434,6 +434,21 @@ static inline void addrconf_addr_solict_mult(const struct in6_addr *addr,
> > >  		      htonl(0xFF000000) | addr->s6_addr32[3]);
> > >  }
> > >  
> > > +/*
> > > + *      link local multicast address range ffx2::/16 rfc4291
> > > + */
> > > +static inline bool ipv6_addr_is_ll_mcast(const struct in6_addr *addr)
> > > +{
> > > +#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
> > > +	__be64 *p = (__be64 *)addr;
> > > +	return ((p[0] & cpu_to_be64(0xff0f000000000000UL))
> > > +		^ cpu_to_be64(0xff02000000000000UL)) == 0UL;
> > > +#else
> > > +	return ((addr->s6_addr32[0] & htonl(0xff0f0000)) ^
> > > +		htonl(0xff020000)) == 0;
> > > +#endif
> > > +}
> > > +
> > >  static inline bool ipv6_addr_is_ll_all_nodes(const struct in6_addr *addr)
> > >  {
> > >  #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
> > > diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> > > index 9b379e110129..ed3957381fa2 100644
> > > --- a/net/bridge/br_multicast.c
> > > +++ b/net/bridge/br_multicast.c
> > > @@ -1664,7 +1664,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
> > >  	err = ipv6_mc_check_mld(skb);
> > >  
> > >  	if (err == -ENOMSG) {
> > > -		if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
> > > +		if (!ipv6_addr_is_ll_mcast(&ipv6_hdr(skb)->daddr))
> > >  			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
> > 
> > IIUC, you want IPv6 link-local packets to be locally received, but this
> > also changes how these packets are flooded. RFC 4541 says that packets
> 
> Indeed, we'll start flooding them all, not just the all hosts address.
> If that is at all required it'll definitely have to be optional.
> 
> > addressed to the all hosts address are a special case and should be
> > forwarded to all ports:
> > 
> > "In IPv6, the data forwarding rules are more straight forward because MLD is
> > mandated for addresses with scope 2 (link-scope) or greater. The only exception
> > is the address FF02::1 which is the all hosts link-scope address for which MLD
> > messages are never sent. Packets with the all hosts link-scope address should
> > be forwarded on all ports."
> > 
> 
> I wonder what is the problem for the host to join such group on behalf of the bridge ?
> Then you'll receive the traffic at least locally and the RFC says it itself - MLD is mandated
> for the other link-local addresses.
> It's very late here and maybe I'm missing something.. :)
> 
The group is being joined by MLD at the L3 level but the packets are
not being passed up to the l3 interface becasue there is a MLD querier
on the network

snippet from /proc/net/igmp6
...
40   sw1             ff0200000000000000000001ff008700     1 00000004 0
40   sw1             ff020000000000000000000000000002     1 00000004 0
40   sw1             ff020000000000000000000000000001     1 0000000C 0
40   sw1             ff010000000000000000000000000001     1 00000008 0
41   lo1             ff020000000000000000000000000001     1 0000000C 0
41   lo1             ff010000000000000000000000000001     1 00000008 0
42   sw1.1           ff020000000000000000000000000006     1 00000004 0
42   sw1.1           ff020000000000000000000000000005     1 00000004 0
42   sw1.1           ff0200000000000000000001ff000000     2 00000004 0
42   sw1.1           ff0200000000000000000001ff008700     1 00000004 0
42   sw1.1           ff0200000000000000000001ff000099     1 00000004 0
42   sw1.1           ff020000000000000000000000000002     1 00000004 0
42   sw1.1           ff020000000000000000000000000001     1 0000000C 0
42   sw1.1           ff010000000000000000000000000001     1 00000008 0
...

the bridge is sw1 and the l3 intervace is sw1.1

Ido is correct about the flooding - I will update the patch with the
comments and reissue.

Thanks again

-pr
>  
> > Maybe you want something like:
> > 
> 
> I think we can do without the new field, either pass local_rcv into br_multicast_rcv() or
> set it based on return value. The extra test will have to remain unfortunately, but we
> can reduce the tests by one if carefully done.
> 
> > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> > index 09b1dd8cd853..9f312a73f61c 100644
> > --- a/net/bridge/br_input.c
> > +++ b/net/bridge/br_input.c
> > @@ -132,7 +132,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> >  		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
> >  		    br_multicast_querier_exists(br, eth_hdr(skb))) {
> >  			if ((mdst && mdst->host_joined) ||
> > -			    br_multicast_is_router(br)) {
> > +			    br_multicast_is_router(br) ||
> > +			    BR_INPUT_SKB_CB_LOCAL_RECEIVE(skb)) {
> >  				local_rcv = true;
> >  				br->dev->stats.multicast++;
> >  			}
> > diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> > index 9b379e110129..f03cecf6174e 100644
> > --- a/net/bridge/br_multicast.c
> > +++ b/net/bridge/br_multicast.c
> > @@ -1667,6 +1667,9 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
> >  		if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
> >  			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
> >  
> > +		if (ipv6_addr_is_ll_mcast(&ipv6_hdr(skb)->daddr))
> > +			BR_INPUT_SKB_CB(skb)->local_receive = 1;
> > +
> >  		if (ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr)) {
> >  			err = br_ip6_multicast_mrd_rcv(br, port, skb);
> >  
> > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> > index b7a4942ff1b3..d76394ca4059 100644
> > --- a/net/bridge/br_private.h
> > +++ b/net/bridge/br_private.h
> > @@ -426,6 +426,7 @@ struct br_input_skb_cb {
> >  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> >  	u8 igmp;
> >  	u8 mrouters_only:1;
> > +	u8 local_receive:1;
> >  #endif
> >  	u8 proxyarp_replied:1;
> >  	u8 src_port_isolated:1;
> > @@ -445,8 +446,10 @@ struct br_input_skb_cb {
> >  
> >  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> >  # define BR_INPUT_SKB_CB_MROUTERS_ONLY(__skb)	(BR_INPUT_SKB_CB(__skb)->mrouters_only)
> > +# define BR_INPUT_SKB_CB_LOCAL_RECEIVE(__skb)	(BR_INPUT_SKB_CB(__skb)->local_receive)
> >  #else
> >  # define BR_INPUT_SKB_CB_MROUTERS_ONLY(__skb)	(0)
> > +# define BR_INPUT_SKB_CB_LOCAL_RECEIVE(__skb)	(0)
> >  #endif
> >  
> >  #define br_printk(level, br, format, args...)	\
> > 


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

* Re: [PATCH net-next] mcast: ensure L-L IPv6 packets are accepted by bridge
  2019-08-14 16:40     ` Patrick Ruddy
@ 2019-08-14 16:58       ` Nikolay Aleksandrov
  2019-08-14 20:11       ` Linus Lüssing
  1 sibling, 0 replies; 8+ messages in thread
From: Nikolay Aleksandrov @ 2019-08-14 16:58 UTC (permalink / raw)
  To: pruddy, Ido Schimmel; +Cc: netdev, roopa, linus.luessing

On 8/14/19 7:40 PM, Patrick Ruddy wrote:
> Thanks both for the quick replies, answers inline...
> 
> On Wed, 2019-08-14 at 02:55 +0300, Nikolay Aleksandrov wrote:
>> On 8/13/19 10:53 PM, Ido Schimmel wrote:
>>> + Bridge maintainers, Linus
>>>
>>
>> Good catch Ido, thanks!
>> First I'd say the subject needs to reflect that this is a bridge change
>> better, please rearrange it like so - bridge: mcast: ...
>> More below,
>>
>>> On Tue, Aug 13, 2019 at 03:18:04PM +0100, Patrick Ruddy wrote:
>>>> At present only all-nodes IPv6 multicast packets are accepted by
>>>> a bridge interface that is not in multicast router mode. Since
>>>> other protocols can be running in the absense of multicast
>>>> forwarding e.g. OSPFv3 IPv6 ND. Change the test to allow
>>>> all of the FFx2::/16 range to be accepted when not in multicast
>>>> router mode. This aligns the code with IPv4 link-local reception
>>>> and RFC4291
>>>
>>> Can you please quote the relevant part from RFC 4291?
>>>
>>>> Signed-off-by: Patrick Ruddy <pruddy@vyatta.att-mail.com>
>>>> ---
>>>>  include/net/addrconf.h    | 15 +++++++++++++++
>>>>  net/bridge/br_multicast.c |  2 +-
>>>>  2 files changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
>>>> index becdad576859..05b42867e969 100644
>>>> --- a/include/net/addrconf.h
>>>> +++ b/include/net/addrconf.h
>>>> @@ -434,6 +434,21 @@ static inline void addrconf_addr_solict_mult(const struct in6_addr *addr,
>>>>  		      htonl(0xFF000000) | addr->s6_addr32[3]);
>>>>  }
>>>>  
>>>> +/*
>>>> + *      link local multicast address range ffx2::/16 rfc4291
>>>> + */
>>>> +static inline bool ipv6_addr_is_ll_mcast(const struct in6_addr *addr)
>>>> +{
>>>> +#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
>>>> +	__be64 *p = (__be64 *)addr;
>>>> +	return ((p[0] & cpu_to_be64(0xff0f000000000000UL))
>>>> +		^ cpu_to_be64(0xff02000000000000UL)) == 0UL;
>>>> +#else
>>>> +	return ((addr->s6_addr32[0] & htonl(0xff0f0000)) ^
>>>> +		htonl(0xff020000)) == 0;
>>>> +#endif
>>>> +}
>>>> +
>>>>  static inline bool ipv6_addr_is_ll_all_nodes(const struct in6_addr *addr)
>>>>  {
>>>>  #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
>>>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>>>> index 9b379e110129..ed3957381fa2 100644
>>>> --- a/net/bridge/br_multicast.c
>>>> +++ b/net/bridge/br_multicast.c
>>>> @@ -1664,7 +1664,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
>>>>  	err = ipv6_mc_check_mld(skb);
>>>>  
>>>>  	if (err == -ENOMSG) {
>>>> -		if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
>>>> +		if (!ipv6_addr_is_ll_mcast(&ipv6_hdr(skb)->daddr))
>>>>  			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
>>>
>>> IIUC, you want IPv6 link-local packets to be locally received, but this
>>> also changes how these packets are flooded. RFC 4541 says that packets
>>
>> Indeed, we'll start flooding them all, not just the all hosts address.
>> If that is at all required it'll definitely have to be optional.
>>
>>> addressed to the all hosts address are a special case and should be
>>> forwarded to all ports:
>>>
>>> "In IPv6, the data forwarding rules are more straight forward because MLD is
>>> mandated for addresses with scope 2 (link-scope) or greater. The only exception
>>> is the address FF02::1 which is the all hosts link-scope address for which MLD
>>> messages are never sent. Packets with the all hosts link-scope address should
>>> be forwarded on all ports."
>>>
>>
>> I wonder what is the problem for the host to join such group on behalf of the bridge ?
>> Then you'll receive the traffic at least locally and the RFC says it itself - MLD is mandated
>> for the other link-local addresses.
>> It's very late here and maybe I'm missing something.. :)
>>
> The group is being joined by MLD at the L3 level but the packets are
> not being passed up to the l3 interface becasue there is a MLD querier
> on the network
> 

That shouldn't matter if the host has joined the group, there is a specific
check for that. If the host has joined the group and we have an mdst then
we'll hit this code:
                mdst = br_mdb_get(br, skb, vid);
                if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
                    br_multicast_querier_exists(br, eth_hdr(skb))) {
                        if ((mdst && mdst->host_joined) ||
                            br_multicast_is_router(br)) {
                                local_rcv = true;
                                br->dev->stats.multicast++;
                        }
                        mcast_hit = true;
                } else {

local_rcv become true and the packet is passed up, so what is the problem ?
Have you missed to refresh the group and it has expired in the bridge perhaps ?


> snippet from /proc/net/igmp6
> ...
> 40   sw1             ff0200000000000000000001ff008700     1 00000004 0
> 40   sw1             ff020000000000000000000000000002     1 00000004 0
> 40   sw1             ff020000000000000000000000000001     1 0000000C 0
> 40   sw1             ff010000000000000000000000000001     1 00000008 0
> 41   lo1             ff020000000000000000000000000001     1 0000000C 0
> 41   lo1             ff010000000000000000000000000001     1 00000008 0
> 42   sw1.1           ff020000000000000000000000000006     1 00000004 0
> 42   sw1.1           ff020000000000000000000000000005     1 00000004 0
> 42   sw1.1           ff0200000000000000000001ff000000     2 00000004 0
> 42   sw1.1           ff0200000000000000000001ff008700     1 00000004 0
> 42   sw1.1           ff0200000000000000000001ff000099     1 00000004 0
> 42   sw1.1           ff020000000000000000000000000002     1 00000004 0
> 42   sw1.1           ff020000000000000000000000000001     1 0000000C 0
> 42   sw1.1           ff010000000000000000000000000001     1 00000008 0
> ...
> 
> the bridge is sw1 and the l3 intervace is sw1.1
> 
> Ido is correct about the flooding - I will update the patch with the
> comments and reissue.
> 
> Thanks again
> 
> -pr
>>  
>>> Maybe you want something like:
>>>
>>
>> I think we can do without the new field, either pass local_rcv into br_multicast_rcv() or
>> set it based on return value. The extra test will have to remain unfortunately, but we
>> can reduce the tests by one if carefully done.
>>
>>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>>> index 09b1dd8cd853..9f312a73f61c 100644
>>> --- a/net/bridge/br_input.c
>>> +++ b/net/bridge/br_input.c
>>> @@ -132,7 +132,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>>  		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
>>>  		    br_multicast_querier_exists(br, eth_hdr(skb))) {
>>>  			if ((mdst && mdst->host_joined) ||
>>> -			    br_multicast_is_router(br)) {
>>> +			    br_multicast_is_router(br) ||
>>> +			    BR_INPUT_SKB_CB_LOCAL_RECEIVE(skb)) {
>>>  				local_rcv = true;
>>>  				br->dev->stats.multicast++;
>>>  			}
>>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>>> index 9b379e110129..f03cecf6174e 100644
>>> --- a/net/bridge/br_multicast.c
>>> +++ b/net/bridge/br_multicast.c
>>> @@ -1667,6 +1667,9 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
>>>  		if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
>>>  			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
>>>  
>>> +		if (ipv6_addr_is_ll_mcast(&ipv6_hdr(skb)->daddr))
>>> +			BR_INPUT_SKB_CB(skb)->local_receive = 1;
>>> +
>>>  		if (ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr)) {
>>>  			err = br_ip6_multicast_mrd_rcv(br, port, skb);
>>>  
>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>> index b7a4942ff1b3..d76394ca4059 100644
>>> --- a/net/bridge/br_private.h
>>> +++ b/net/bridge/br_private.h
>>> @@ -426,6 +426,7 @@ struct br_input_skb_cb {
>>>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>>>  	u8 igmp;
>>>  	u8 mrouters_only:1;
>>> +	u8 local_receive:1;
>>>  #endif
>>>  	u8 proxyarp_replied:1;
>>>  	u8 src_port_isolated:1;
>>> @@ -445,8 +446,10 @@ struct br_input_skb_cb {
>>>  
>>>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>>>  # define BR_INPUT_SKB_CB_MROUTERS_ONLY(__skb)	(BR_INPUT_SKB_CB(__skb)->mrouters_only)
>>> +# define BR_INPUT_SKB_CB_LOCAL_RECEIVE(__skb)	(BR_INPUT_SKB_CB(__skb)->local_receive)
>>>  #else
>>>  # define BR_INPUT_SKB_CB_MROUTERS_ONLY(__skb)	(0)
>>> +# define BR_INPUT_SKB_CB_LOCAL_RECEIVE(__skb)	(0)
>>>  #endif
>>>  
>>>  #define br_printk(level, br, format, args...)	\
>>>
> 


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

* Re: [PATCH net-next] mcast: ensure L-L IPv6 packets are accepted by bridge
  2019-08-14 16:40     ` Patrick Ruddy
  2019-08-14 16:58       ` Nikolay Aleksandrov
@ 2019-08-14 20:11       ` Linus Lüssing
  2019-08-14 20:34         ` Nikolay Aleksandrov
  1 sibling, 1 reply; 8+ messages in thread
From: Linus Lüssing @ 2019-08-14 20:11 UTC (permalink / raw)
  To: Patrick Ruddy; +Cc: bridge, Nikolay Aleksandrov, Ido Schimmel, netdev, roopa

On Wed, Aug 14, 2019 at 05:40:58PM +0100, Patrick Ruddy wrote:
> The group is being joined by MLD at the L3 level but the packets are
> not being passed up to the l3 interface becasue there is a MLD querier
> on the network
> 
> snippet from /proc/net/igmp6
> ...
> 40   sw1             ff0200000000000000000001ff008700     1 00000004 0
> 40   sw1             ff020000000000000000000000000002     1 00000004 0
> 40   sw1             ff020000000000000000000000000001     1 0000000C 0
> 40   sw1             ff010000000000000000000000000001     1 00000008 0
> 41   lo1             ff020000000000000000000000000001     1 0000000C 0
> 41   lo1             ff010000000000000000000000000001     1 00000008 0
> 42   sw1.1           ff020000000000000000000000000006     1 00000004 0
> 42   sw1.1           ff020000000000000000000000000005     1 00000004 0
> 42   sw1.1           ff0200000000000000000001ff000000     2 00000004 0
> 42   sw1.1           ff0200000000000000000001ff008700     1 00000004 0
> 42   sw1.1           ff0200000000000000000001ff000099     1 00000004 0
> 42   sw1.1           ff020000000000000000000000000002     1 00000004 0
> 42   sw1.1           ff020000000000000000000000000001     1 0000000C 0
> 42   sw1.1           ff010000000000000000000000000001     1 00000008 0
> ...
> 
> the bridge is sw1 and the l3 intervace is sw1.1

What kind of interface is sw1.1 exactly? Is it a VLAN or a VRF
interface? Something else?

Could you also post the output of bridge mdb show?

Regards, Linus


PS: Also please include the bridge mailinglist in the future.

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

* Re: [PATCH net-next] mcast: ensure L-L IPv6 packets are accepted by bridge
  2019-08-14 20:11       ` Linus Lüssing
@ 2019-08-14 20:34         ` Nikolay Aleksandrov
  2019-08-15 13:36           ` Patrick Ruddy
  0 siblings, 1 reply; 8+ messages in thread
From: Nikolay Aleksandrov @ 2019-08-14 20:34 UTC (permalink / raw)
  To: Linus Lüssing, Patrick Ruddy; +Cc: bridge, Ido Schimmel, netdev, roopa

On 8/14/19 11:11 PM, Linus Lüssing wrote:
> On Wed, Aug 14, 2019 at 05:40:58PM +0100, Patrick Ruddy wrote:
>> The group is being joined by MLD at the L3 level but the packets are
>> not being passed up to the l3 interface becasue there is a MLD querier
>> on the network
>>
>> snippet from /proc/net/igmp6
>> ...
>> 40   sw1             ff0200000000000000000001ff008700     1 00000004 0
>> 40   sw1             ff020000000000000000000000000002     1 00000004 0
>> 40   sw1             ff020000000000000000000000000001     1 0000000C 0
>> 40   sw1             ff010000000000000000000000000001     1 00000008 0
>> 41   lo1             ff020000000000000000000000000001     1 0000000C 0
>> 41   lo1             ff010000000000000000000000000001     1 00000008 0
>> 42   sw1.1           ff020000000000000000000000000006     1 00000004 0
>> 42   sw1.1           ff020000000000000000000000000005     1 00000004 0
>> 42   sw1.1           ff0200000000000000000001ff000000     2 00000004 0
>> 42   sw1.1           ff0200000000000000000001ff008700     1 00000004 0
>> 42   sw1.1           ff0200000000000000000001ff000099     1 00000004 0
>> 42   sw1.1           ff020000000000000000000000000002     1 00000004 0
>> 42   sw1.1           ff020000000000000000000000000001     1 0000000C 0
>> 42   sw1.1           ff010000000000000000000000000001     1 00000008 0
>> ...
>>
>> the bridge is sw1 and the l3 intervace is sw1.1
> 
> What kind of interface is sw1.1 exactly? Is it a VLAN or a VRF
> interface? Something else?
> 
+1

> Could you also post the output of bridge mdb show?
> 
> Regards, Linus
> 
> 
> PS: Also please include the bridge mailinglist in the future.
> 

Note that if you'd like to debug a host joined group currently bridge mdb show
will not dump it and if the group is host-joined only it
can even be empty. You can use my latest set (not applied yet):
http://patchwork.ozlabs.org/project/netdev/list/?series=125169

Alternatively you could monitor the mdb events, it will show up there even
today without any changes (bridge monitor mdb) and you can check if it's
getting deleted.

Cheers,
 Nik

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

* Re: [PATCH net-next] mcast: ensure L-L IPv6 packets are accepted by bridge
  2019-08-14 20:34         ` Nikolay Aleksandrov
@ 2019-08-15 13:36           ` Patrick Ruddy
  0 siblings, 0 replies; 8+ messages in thread
From: Patrick Ruddy @ 2019-08-15 13:36 UTC (permalink / raw)
  To: Nikolay Aleksandrov, Linus Lüssing
  Cc: bridge, Ido Schimmel, netdev, roopa

On Wed, 2019-08-14 at 23:34 +0300, Nikolay Aleksandrov wrote:
> On 8/14/19 11:11 PM, Linus Lüssing wrote:
> > On Wed, Aug 14, 2019 at 05:40:58PM +0100, Patrick Ruddy wrote:
> > > The group is being joined by MLD at the L3 level but the packets are
> > > not being passed up to the l3 interface becasue there is a MLD querier
> > > on the network
> > > 
> > > snippet from /proc/net/igmp6
> > > ...
> > > 40   sw1             ff0200000000000000000001ff008700     1 00000004 0
> > > 40   sw1             ff020000000000000000000000000002     1 00000004 0
> > > 40   sw1             ff020000000000000000000000000001     1 0000000C 0
> > > 40   sw1             ff010000000000000000000000000001     1 00000008 0
> > > 41   lo1             ff020000000000000000000000000001     1 0000000C 0
> > > 41   lo1             ff010000000000000000000000000001     1 00000008 0
> > > 42   sw1.1           ff020000000000000000000000000006     1 00000004 0
> > > 42   sw1.1           ff020000000000000000000000000005     1 00000004 0
> > > 42   sw1.1           ff0200000000000000000001ff000000     2 00000004 0
> > > 42   sw1.1           ff0200000000000000000001ff008700     1 00000004 0
> > > 42   sw1.1           ff0200000000000000000001ff000099     1 00000004 0
> > > 42   sw1.1           ff020000000000000000000000000002     1 00000004 0
> > > 42   sw1.1           ff020000000000000000000000000001     1 0000000C 0
> > > 42   sw1.1           ff010000000000000000000000000001     1 00000008 0
> > > ...
> > > 
> > > the bridge is sw1 and the l3 intervace is sw1.1
> > 
> > What kind of interface is sw1.1 exactly? Is it a VLAN or a VRF
> > interface? Something else?
> > 
> +1
> 
> > Could you also post the output of bridge mdb show?
> > 
> > Regards, Linus
> > 
> > 
> > PS: Also please include the bridge mailinglist in the future.
> > 
> 
> Note that if you'd like to debug a host joined group currently bridge mdb show
> will not dump it and if the group is host-joined only it
> can even be empty. You can use my latest set (not applied yet):
> https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_project_netdev_list_-3Fseries-3D125169&d=DwIDaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=au3D9TlUU6OvFpWOU9cuIHeNeV2fw-AOF1ZqCRqsILc&m=KsdarH0MAMMoKZ4PuvHrEC57uEluTGK-XSL4uUxu9MY&s=jyoK6VVmFh1KpKZirrtUYwq9nLy8fz-GigFFLjaLsoE&e=
> 
> Alternatively you could monitor the mdb events, it will show up there even
> today without any changes (bridge monitor mdb) and you can check if it's
> getting deleted.
> 
> Cheers,
>  Nik

The sw1.1 interface is a .1q vlan

The output of "bridge monitor mdb" is empty

I can see the incoming query and the outging report on tshark:
29002 72654.887739 fe80::4041:1ff:fe00:101 → ff02::1      ICMPv6 94
Multicast Listener Query
29003 72655.502035 fe80::eac5:7aff:fe00:8700 → ff02::16     ICMPv6 194
Multicast Listener Report Message v2

debugging shows that bridge code sees the incoming query but not the
outgoing report.

Thanks for all the pointers - I will pursue what is happening.

-pr 


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

end of thread, other threads:[~2019-08-15 13:37 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-13 14:18 [PATCH net-next] mcast: ensure L-L IPv6 packets are accepted by bridge Patrick Ruddy
2019-08-13 19:53 ` Ido Schimmel
2019-08-13 23:55   ` Nikolay Aleksandrov
2019-08-14 16:40     ` Patrick Ruddy
2019-08-14 16:58       ` Nikolay Aleksandrov
2019-08-14 20:11       ` Linus Lüssing
2019-08-14 20:34         ` Nikolay Aleksandrov
2019-08-15 13:36           ` Patrick Ruddy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).