IPv6 multicast snooping behaviour on 2.6.39-rc2 and later

Hi all,

   Sorry for cross-posting. I have a testbed setup on my VM environment that needs to use MLDv2 report message. My PIM router which is connected to a port on the bridge relies upon MLDv2 message to work. However, the setup failed to work after 2.6.39-rc2 or later. MLDv2 won't be forwarded to any slave ports bound to the bridge interface. Specifically, I track the cause down to this commit id:

     ff9a57a62afbbe2d0f3a09af321f1fd7645f38a5

   However, as soon as I disable multicast snooping on sysfs, MLDv2 message can be forwarded again.

   I am not knowledgeable on the subject of multicast snooping, but is this right behaviour for multicast snooping? IPv4 doesn't seem to suffer the same fate though, IGMP can be forwarded even if multicast_snooping is enabled. Thank you in advance.


Regards,
Ang Way Chuang

Le mardi 23 août 2011 à 17:41 +0900, Ang Way Chuang a écrit :

Hi all,

    Sorry for cross-posting. I have a testbed setup on my VM
environment that needs to use MLDv2 report message. My PIM router
which is connected to a port on the bridge relies upon MLDv2 message
to work. However, the setup failed to work after 2.6.39-rc2 or later.
MLDv2 won't be forwarded to any slave ports bound to the bridge
interface. Specifically, I track the cause down to this commit id:

      ff9a57a62afbbe2d0f3a09af321f1fd7645f38a5

    However, as soon as I disable multicast snooping on sysfs, MLDv2
message can be forwarded again.

    I am not knowledgeable on the subject of multicast snooping, but
is this right behaviour for multicast snooping? IPv4 doesn't seem to
suffer the same fate though, IGMP can be forwarded even if
multicast_snooping is enabled. Thank you in advance.

Thanks very much for the report and your tracking.

Added some CC

I wonder if the  ff9a57a62afb commit doesnt uncover a prior bug, could
you try the following patch ?

Thanks !

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2d85ca7..51e3b2d 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c

@@ -1520,6 +1520,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 		err = pskb_trim_rcsum(skb2, len);
 		if (err)
 			goto out;
+		icmp6h = icmp6_hdr(skb2);
 	}
 
 	switch (skb2->ip_summed) {

Sorry for the blurb. I hope I've configured thunderbird properly to send in plain text. Hope you received my reply. I tried your patch on 3.0.3 and it didn't fix the problem.

Regards,
Ang

On 23/08/11 18:57, Eric Dumazet wrote:

Le mardi 23 août 2011 à 21:31 +0900, Ang Way Chuang a écrit :

Sorry for the blurb. I hope I've configured thunderbird properly to
send in plain text. Hope you received my reply. I tried your patch on
3.0.3 and it didn't fix the problem.

Thanks for trying !

Ok could you please try following patch then ?

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2d85ca7..e9b32a3 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c

@@ -1497,7 +1497,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 	if (!pskb_may_pull(skb2, offset + sizeof(struct icmp6hdr)))
 		goto out;
 
-	len -= offset - skb_network_offset(skb2);
+	len -= offset;
 
 	__skb_pull(skb2, offset);
 	skb_reset_transport_header(skb2);

@@ -1520,6 +1520,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 		err = pskb_trim_rcsum(skb2, len);
 		if (err)
 			goto out;
+		icmp6h = icmp6_hdr(skb2);
 	}
 
 	switch (skb2->ip_summed) {

Tough luck :( Got no love from bridge. It still doesn't work with your latest patch. Please keep sending those patches.

On 23/08/11 22:10, Eric Dumazet wrote:

This is what I found so far from debugging.

The packet is not forwarded due to the failed checksum at br_multicast.c:1533

        case CHECKSUM_NONE:
                skb2->csum = 0;
                if (skb_checksum_complete(skb2))
                        goto out;
        }

Contrary to description of commit ff9a57a6, when the patch of commit ff9a57a6 is applied,
pskb_trim_rcsum is never called at all on my testbed. When commit ff9a57a6 is reverted,
pskb_trim_rcsum will be called. The difference is:

with commit ff9a57a6,
   pskb_trim_rcsum is never called, br_multicast_ipv6_rcv returns -EINVAL which causes
   br_handle_frame_finish to drop the packet

without commit ff9a57a6,
   pskb_trim_rcsum is called overwriting err with 0. br_multicast_ipv6_rcv still fails on the
   same line (skb_checksum_complete). But the difference is err is set to 0 this time. Thereby,
   allowing the packet to be forwarded.

Anyway, I don't think the behaviour is correct with or without commit ff9a57a6



On 23/08/11 23:04, Ang Way Chuang wrote:

Tough luck :( Got no love from bridge. It still doesn't work with your latest patch. Please keep sending those patches.

On 23/08/11 22:10, Eric Dumazet wrote:

quoted

Le mardi 23 août 2011 à 21:31 +0900, Ang Way Chuang a écrit :

quoted

Sorry for the blurb. I hope I've configured thunderbird properly to
send in plain text. Hope you received my reply. I tried your patch on
3.0.3 and it didn't fix the problem.

Thanks for trying !

Ok could you please try following patch then ?

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2d85ca7..e9b32a3 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c

@@ -1497,7 +1497,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
      if (!pskb_may_pull(skb2, offset + sizeof(struct icmp6hdr)))
          goto out;

-    len -= offset - skb_network_offset(skb2);
+    len -= offset;

      __skb_pull(skb2, offset);
      skb_reset_transport_header(skb2);

@@ -1520,6 +1520,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
          err = pskb_trim_rcsum(skb2, len);
          if (err)
              goto out;
+        icmp6h = icmp6_hdr(skb2);
      }

      switch (skb2->ip_summed) {

-- 
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Checksum of ICMPv6 is not properly computed because the pseudo header is not used.
Thus, the MLD packet gets dropped by the bridge. This patch fixes the problem for my
testbed. This bug was made visible by commit ff9a57a62afbbe2d0f3a09af321f1fd7645f38a.

This patch has been tested on 3.0.3. Due to lack of understanding on the checksum
optimization and multicast snooping of the kernel stack, can someone please verify the
correctness of this patch?

Signed-off-by: Ang Way Chuang <redacted>
---

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2d85ca7..bbf361b 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c

@@ -1528,9 +1528,12 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
             break;
         /*FALLTHROUGH*/
     case CHECKSUM_NONE:
-        skb2->csum = 0;
-        if (skb_checksum_complete(skb2))
+        if (!skb_csum_unnecessary(skb2) && csum_ipv6_magic(&ip6h->saddr,
+ &ip6h->daddr, skb2->len, IPPROTO_ICMPV6,
+            skb_checksum(skb2, 0, skb2->len, 0))) {
+            err = -EINVAL;
             goto out;
+        }
     }

     err = 0;

---

From: Ang Way Chuang <redacted>
Date: Wed, 24 Aug 2011 13:56:13 +0900

Patch has been significantly corrupted by your email client, making
it unusable for us.

Please read linux/Documentation/email-clients.txt, email a test patch
to yourself, and only resend your patch to this list once you can
successfully apply such a patch yourself.

On Wed, Aug 24, 2011 at 1:24 AM, Ang Way Chuang [off-list ref] wrote:

This is what I found so far from debugging.

The packet is not forwarded due to the failed checksum at
br_multicast.c:1533

       case CHECKSUM_NONE:
               skb2->csum = 0;
               if (skb_checksum_complete(skb2))
                       goto out;
       }

Contrary to description of commit ff9a57a6, when the patch of commit
ff9a57a6 is applied,
pskb_trim_rcsum is never called at all on my testbed. When commit ff9a57a6
is reverted,
pskb_trim_rcsum will be called. The difference is:

with commit ff9a57a6,
  pskb_trim_rcsum is never called, br_multicast_ipv6_rcv returns -EINVAL
which causes
  br_handle_frame_finish to drop the packet

without commit ff9a57a6,
  pskb_trim_rcsum is called overwriting err with 0. br_multicast_ipv6_rcv
still fails on the
  same line (skb_checksum_complete). But the difference is err is set to 0
this time. Thereby,
  allowing the packet to be forwarded.

Anyway, I don't think the behaviour is correct with or without commit
ff9a57a6

Looks like a checksum calculation bug. Please try below patch, Thanks.

---

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2d85ca7..22d2d1a 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c

@@ -1520,16 +1520,23 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 		err = pskb_trim_rcsum(skb2, len);
 		if (err)
 			goto out;
+		err = -EINVAL;
 	}

+	ip6h = ipv6_hdr(skb2);
+
 	switch (skb2->ip_summed) {
 	case CHECKSUM_COMPLETE:
-		if (!csum_fold(skb2->csum))
+		if (!csum_ipv6_magic(&ip6h->saddr, &ip6h->daddr, skb2->len,
+					IPPROTO_ICMPV6, skb2->csum))
 			break;
 		/*FALLTHROUGH*/
 	case CHECKSUM_NONE:
-		skb2->csum = 0;
-		if (skb_checksum_complete(skb2))
+		skb2->csum = ~csum_unfold(csum_ipv6_magic(&ip6h->saddr,
+							&ip6h->daddr,
+							skb2->len,
+							IPPROTO_ICMPV6, 0));
+		if (__skb_checksum_complete(skb2))
 			goto out;
 	}

---

Le mercredi 24 août 2011 à 15:04 +0800, Yan, Zheng a écrit :

We also could reuse/factorize code, say the one from icmpv6_rcv()

Thanks Zheng Yan. Your patch works as well. I trust others' code more than mine,
so please ignore the patch that I sent earlier.

On 24/08/11 16:04, Yan, Zheng wrote: