All of lore.kernel.org
 help / color / mirror / Atom feed
* IPv6 multicast snooping behaviour on 2.6.39-rc2 and later
@ 2011-08-23  8:41 Ang Way Chuang
  2011-08-23  9:57 ` Eric Dumazet
  0 siblings, 1 reply; 13+ messages in thread
From: Ang Way Chuang @ 2011-08-23  8:41 UTC (permalink / raw)
  To: netdev

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



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

* Re: IPv6 multicast snooping behaviour on 2.6.39-rc2 and later
  2011-08-23  8:41 IPv6 multicast snooping behaviour on 2.6.39-rc2 and later Ang Way Chuang
@ 2011-08-23  9:57 ` Eric Dumazet
  2011-08-23 12:31   ` Ang Way Chuang
  0 siblings, 1 reply; 13+ messages in thread
From: Eric Dumazet @ 2011-08-23  9:57 UTC (permalink / raw)
  To: Ang Way Chuang; +Cc: netdev, Linus Lüssing, Herbert Xu

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) {



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

* Re: IPv6 multicast snooping behaviour on 2.6.39-rc2 and later
  2011-08-23  9:57 ` Eric Dumazet
@ 2011-08-23 12:31   ` Ang Way Chuang
  2011-08-23 13:10     ` Eric Dumazet
  0 siblings, 1 reply; 13+ messages in thread
From: Ang Way Chuang @ 2011-08-23 12:31 UTC (permalink / raw)
  To: Eric Dumazet, netdev; +Cc: Linus Lüssing, Herbert Xu

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 à 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) {
>
>
> --
> 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
>


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

* Re: IPv6 multicast snooping behaviour on 2.6.39-rc2 and later
  2011-08-23 12:31   ` Ang Way Chuang
@ 2011-08-23 13:10     ` Eric Dumazet
  2011-08-23 14:04       ` Ang Way Chuang
  0 siblings, 1 reply; 13+ messages in thread
From: Eric Dumazet @ 2011-08-23 13:10 UTC (permalink / raw)
  To: Ang Way Chuang; +Cc: netdev, Linus Lüssing, Herbert Xu

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) {



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

* Re: IPv6 multicast snooping behaviour on 2.6.39-rc2 and later
  2011-08-23 13:10     ` Eric Dumazet
@ 2011-08-23 14:04       ` Ang Way Chuang
  2011-08-23 17:24         ` Ang Way Chuang
  0 siblings, 1 reply; 13+ messages in thread
From: Ang Way Chuang @ 2011-08-23 14:04 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, Linus Lüssing, Herbert Xu

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:
> 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) {
>
>
>


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

* Re: IPv6 multicast snooping behaviour on 2.6.39-rc2 and later
  2011-08-23 14:04       ` Ang Way Chuang
@ 2011-08-23 17:24         ` Ang Way Chuang
  2011-08-24  4:56           ` [PATCH] bridge: Pseudo-header required for the checksum of ICMP6 header of MLD Ang Way Chuang
  2011-08-24  7:04           ` IPv6 multicast snooping behaviour on 2.6.39-rc2 and later Yan, Zheng 
  0 siblings, 2 replies; 13+ messages in thread
From: Ang Way Chuang @ 2011-08-23 17:24 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, Linus Lüssing, Herbert Xu

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:
>> 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) {
>>
>>
>>
>
> -- 
> 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
>


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

* [PATCH] bridge: Pseudo-header required for the checksum of ICMP6 header of MLD
  2011-08-23 17:24         ` Ang Way Chuang
@ 2011-08-24  4:56           ` Ang Way Chuang
  2011-08-24  5:06             ` David Miller
  2011-08-24  7:04           ` IPv6 multicast snooping behaviour on 2.6.39-rc2 and later Yan, Zheng 
  1 sibling, 1 reply; 13+ messages in thread
From: Ang Way Chuang @ 2011-08-24  4:56 UTC (permalink / raw)
  To: netdev
  Cc: Eric Dumazet, Linus Lüssing, Herbert Xu, yoshfuji,
	Stephen Hemminger

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 <wcang@sfc.wide.ad.jp>
---
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;
---

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

* Re: [PATCH] bridge: Pseudo-header required for the checksum of ICMP6 header of MLD
  2011-08-24  4:56           ` [PATCH] bridge: Pseudo-header required for the checksum of ICMP6 header of MLD Ang Way Chuang
@ 2011-08-24  5:06             ` David Miller
  0 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2011-08-24  5:06 UTC (permalink / raw)
  To: wcang; +Cc: netdev, eric.dumazet, linus.luessing, herbert, yoshfuji, shemminger

From: Ang Way Chuang <wcang@sfc.wide.ad.jp>
Date: Wed, 24 Aug 2011 13:56:13 +0900

> @@ -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,

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.

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

* Re: IPv6 multicast snooping behaviour on 2.6.39-rc2 and later
  2011-08-23 17:24         ` Ang Way Chuang
  2011-08-24  4:56           ` [PATCH] bridge: Pseudo-header required for the checksum of ICMP6 header of MLD Ang Way Chuang
@ 2011-08-24  7:04           ` Yan, Zheng 
  2011-08-24  7:22             ` Eric Dumazet
  2011-08-24  7:38             ` Ang Way Chuang
  1 sibling, 2 replies; 13+ messages in thread
From: Yan, Zheng  @ 2011-08-24  7:04 UTC (permalink / raw)
  To: Ang Way Chuang; +Cc: Eric Dumazet, netdev, Linus Lüssing, Herbert Xu

On Wed, Aug 24, 2011 at 1:24 AM, Ang Way Chuang <wcang@sfc.wide.ad.jp> 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;
 	}

---

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

* Re: IPv6 multicast snooping behaviour on 2.6.39-rc2 and later
  2011-08-24  7:04           ` IPv6 multicast snooping behaviour on 2.6.39-rc2 and later Yan, Zheng 
@ 2011-08-24  7:22             ` Eric Dumazet
  2011-08-24  7:38             ` Ang Way Chuang
  1 sibling, 0 replies; 13+ messages in thread
From: Eric Dumazet @ 2011-08-24  7:22 UTC (permalink / raw)
  To: Yan, Zheng; +Cc: Ang Way Chuang, netdev, Linus Lüssing, Herbert Xu

Le mercredi 24 août 2011 à 15:04 +0800, Yan, Zheng a écrit :
> On Wed, Aug 24, 2011 at 1:24 AM, Ang Way Chuang <wcang@sfc.wide.ad.jp> 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;
>  	}
> 
> ---


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

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

* Re: IPv6 multicast snooping behaviour on 2.6.39-rc2 and later
  2011-08-24  7:04           ` IPv6 multicast snooping behaviour on 2.6.39-rc2 and later Yan, Zheng 
  2011-08-24  7:22             ` Eric Dumazet
@ 2011-08-24  7:38             ` Ang Way Chuang
  1 sibling, 0 replies; 13+ messages in thread
From: Ang Way Chuang @ 2011-08-24  7:38 UTC (permalink / raw)
  To: Yan, Zheng ; +Cc: Eric Dumazet, netdev, Linus Lüssing, Herbert Xu

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:
> On Wed, Aug 24, 2011 at 1:24 AM, Ang Way Chuang <wcang@sfc.wide.ad.jp> 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;
>  	}
>
> ---
> --
> 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
>

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

* Re: [PATCH] bridge: Pseudo-header required for the checksum of ICMP6 header of MLD
  2011-08-24  6:10 [PATCH] bridge: Pseudo-header required for the checksum of ICMP6 header of MLD Ang Way Chuang
@ 2011-08-24  7:18 ` Eric Dumazet
  0 siblings, 0 replies; 13+ messages in thread
From: Eric Dumazet @ 2011-08-24  7:18 UTC (permalink / raw)
  To: Ang Way Chuang; +Cc: netdev

Le mercredi 24 août 2011 à 15:10 +0900, Ang Way Chuang a écrit :
> Just saw Eric's patch for icmp6_type. Suspect this patch may have issue for reference
> of ip6h after pskb_trim_rcsum()?
> 

I dont think so, because ip6h is against skb header, not skb2 one.


> 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 <wcang@sfc.wide.ad.jp>
> ---
> 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;
> ---


Sorry, I cannot comment on this, checksum games are hard :(

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

* [PATCH] bridge: Pseudo-header required for the checksum of ICMP6 header of MLD
@ 2011-08-24  6:10 Ang Way Chuang
  2011-08-24  7:18 ` Eric Dumazet
  0 siblings, 1 reply; 13+ messages in thread
From: Ang Way Chuang @ 2011-08-24  6:10 UTC (permalink / raw)
  To: netdev; +Cc: Eric Dumazet

Just saw Eric's patch for icmp6_type. Suspect this patch may have issue for reference
of ip6h after pskb_trim_rcsum()?

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 <wcang@sfc.wide.ad.jp>
---
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;
---

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

end of thread, other threads:[~2011-08-24  7:38 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-23  8:41 IPv6 multicast snooping behaviour on 2.6.39-rc2 and later Ang Way Chuang
2011-08-23  9:57 ` Eric Dumazet
2011-08-23 12:31   ` Ang Way Chuang
2011-08-23 13:10     ` Eric Dumazet
2011-08-23 14:04       ` Ang Way Chuang
2011-08-23 17:24         ` Ang Way Chuang
2011-08-24  4:56           ` [PATCH] bridge: Pseudo-header required for the checksum of ICMP6 header of MLD Ang Way Chuang
2011-08-24  5:06             ` David Miller
2011-08-24  7:04           ` IPv6 multicast snooping behaviour on 2.6.39-rc2 and later Yan, Zheng 
2011-08-24  7:22             ` Eric Dumazet
2011-08-24  7:38             ` Ang Way Chuang
2011-08-24  6:10 [PATCH] bridge: Pseudo-header required for the checksum of ICMP6 header of MLD Ang Way Chuang
2011-08-24  7:18 ` Eric Dumazet

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.