Checksumming bug in bridge multicast snooping for IPv6?

Checksumming bug in bridge multicast snooping for IPv6?

[Linus Lüssing]

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

Hi everyone,

Somehow I'm having trouble with the IPv6 bridge snooping again:
MLDv2 Reports are dropped by the multicast snooping feature, looks
like it has something to do with checksums. Wireshark does not
display any weirdness, it at least reports the MLD reports
checksum as correct.


The setup is the following: The VM is running a current Linux
version of torvalds branch with no other additions then the
printk-debug patch attached (2.6.38+ #4 SMP PREEMPT Sat Mar 26
22:59:11 GMT 2011 i686 GNU/Linux):
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=tree;h=16c29dafcc86024048f1dbb8349d31cb22c7c55a;hb=16c29dafcc86024048f1dbb8349d31cb22c7c55a
The host machine which is joining a multicast group is doing an
explicit join on the KVM instances provided tap interface:
IPv6: vlc -vvv "udp://@[ff12::124%vmtap1]"
(IPv4: vlc -vvv "udp://@224.0.1.123")
The host machine is running a kernel from Debian unstable:
2.6.37-2-amd64 #1 SMP Sun Feb 27 12:32:01 UTC 2011 x86_64
GNU/Linux

See the attached debug patch and the according output for some
more details where it fails (the bridge is basically ignoring the
MLDv2 report due to the goto in this line:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=net/bridge/br_multicast.c;h=f61eb2eff3fdd387b83d9fab642bb610dde1ad69;hb=HEAD#l1530)

I'm also attaching both a wireshark capture of the ignored IPv6
MLDv2 report and the working IGMPv3 report, which correspond
directly to the attached printk debug output.


I'm a little bit startled because I definitely had that part
working a couple of weeks ago and I'm still trying to figure out
what I might have changed in the setup. I definitely have updated
the VMs kernel, but the same issue is present for the 2.6.38 and
also 2.6.37 release versions with my fixes backported 
(the latter one was the one I had been using back then).
I probably have updated the multicast listener host's kernel, too,
I might have been running 2.6.32 or something more earlier... I've
also tried having the listener host in another VM with the same
2.6.38+ kernel as the bridge-snooping host, but also that did not
make a difference.


Anyways, skb_checksum_complete() is calculating the checksum from
skb's data to tail pointer, right?
RFC3810 for MLDv2, section 5.1.2 says:
"The standard ICMPv6 checksum; it covers the entire MLDv2 message,
plus a "pseudo-header" of IPv6 header fields [RFC2463]."
Could it be that this "pseudo-header" is not included in the
checksumming? Is there a function in the kernel which could
already provide that?
I guess that could also explain why it's working fine for IPv4,
there it's just the IGMP message being checksummed according to
RFC 3376, section 4.1.2.


Cheers, Linus


PS: There also seems to be another offset bug in the same
function, see comment in debug patch file, though seemingly
unrelated to the issue described above. Correcting that
len-variable does to help for the above issue. 

[-- Attachment #2: br-mcast-snoop-printk-debug.log --]
[-- Type: text/plain, Size: 1839 bytes --]

IPv6:
[ 2460.557303] +++ br_multicast_ipv6_rcv()
[ 2460.558114] +++ br_multicast_ipv6_rcv() here 0.5
[ 2460.558114] +++ br_multicast_ipv6_rcv() len: 36, offset: 48, skb_network_offset(skb2): 0
[ 2460.558114] +++ br_multicast_ipv6_rcv() new len: -12
[ 2460.558114] +++ br_multicast_ipv6_rcv() skb2->len: 28 len: -12 skb->len: 76
[ 2460.558114] +++ br_multicast_ipv6_rcv() here 1.5
[ 2460.558114] +++ br_multicast_ipv6_rcv() here 1.7, skb2->csum is 0
[ 2460.558114] +++ br_multicast_ipv6_rcv() here 1.8, skb_checksum_complete(skb2): 81e9 (skb): 2e60
[ 2464.981808] +++ br_multicast_ipv6_rcv()
[ 2464.982388] +++ br_multicast_ipv6_rcv() here 0.5
[ 2465.062634] +++ br_multicast_ipv6_rcv() len: 36, offset: 48, skb_network_offset(skb2): 0
[ 2465.066698] +++ br_multicast_ipv6_rcv() new len: -12
[ 2465.069183] +++ br_multicast_ipv6_rcv() skb2->len: 28 len: -12 skb->len: 76
[ 2465.072013] +++ br_multicast_ipv6_rcv() here 1.5
[ 2465.074217] +++ br_multicast_ipv6_rcv() here 1.7, skb2->csum is 0
[ 2465.076785] +++ br_multicast_ipv6_rcv() here 1.8, skb_checksum_complete(skb2): 81e9 (skb): 2e60

IPv4:
[ 2325.265830] +++ br_multicast_ipv4_rcv() skb2->len: 40 len: 40 skb->len: 40
[ 2325.266567] +++ br_multicast_ipv4_rcv() 2) skb2->len: 16 len: 16 skb->len: 16
[ 2325.266567] +++ br_multicast_ipv4_rcv() here 1.7, skb2->csum is 0
[ 2325.266567] +++ br_multicast_ipv4_rcv() here 1.8, skb_checksum_complete: 0
[ 2325.266567] +++ br_ip4_multicast_add_group() eth1 224.0.1.123
[ 2327.326179] +++ br_multicast_ipv4_rcv() skb2->len: 40 len: 40 skb->len: 40
[ 2327.326674] +++ br_multicast_ipv4_rcv() 2) skb2->len: 16 len: 16 skb->len: 16
[ 2327.326674] +++ br_multicast_ipv4_rcv() here 1.7, skb2->csum is 0
[ 2327.326674] +++ br_multicast_ipv4_rcv() here 1.8, skb_checksum_complete: 0
[ 2327.326674] +++ br_ip4_multicast_add_group() eth1 224.0.1.123

[-- Attachment #3: bridge-snoop-debug.patch --]
[-- Type: text/x-diff, Size: 4954 bytes --]

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index f61eb2e..3d4c5d2 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -764,8 +764,11 @@ static int br_ip4_multicast_add_group(struct net_bridge *br,
 {
 	struct br_ip br_group;
 
-	if (ipv4_is_local_multicast(group))
+printk("+++ br_ip4_multicast_add_group() %s %pI4\n", port->dev->name, &group);
+	if (ipv4_is_local_multicast(group)) {
+printk("+++ br_ip4_multicast_add_group() %s %pI4, is link local\n", port->dev->name, &group);
 		return 0;
+	}
 
 	br_group.u.ip4 = group;
 	br_group.proto = htons(ETH_P_IP);
@@ -780,8 +783,11 @@ static int br_ip6_multicast_add_group(struct net_bridge *br,
 {
 	struct br_ip br_group;
 
-	if (!ipv6_is_transient_multicast(group))
+printk("+++ br_ip6_multicast_add_group() %s %pI6\n", port->dev->name, group);
+	if (!ipv6_is_transient_multicast(group)) {
+printk("+++ br_ip6_multicast_add_group() %s %pI6, is not transient\n", port->dev->name, group);
 		return 0;
+	}
 
 	ipv6_addr_copy(&br_group.u.ip6, group);
 	br_group.proto = htons(ETH_P_IPV6);
@@ -1001,6 +1007,7 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
 	int num;
 	int err = 0;
 
+printk("+++ br_ip6_multicast_mld2_report()\n");
 	if (!pskb_may_pull(skb, sizeof(*icmp6h)))
 		return -EINVAL;
 
@@ -1386,11 +1393,14 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
 	if (skb->len < len || len < ip_hdrlen(skb))
 		return -EINVAL;
 
+printk("+++ br_multicast_ipv4_rcv() skb2->len: %i len: %i skb->len: %i\n", skb2->len, len, skb->len);
 	if (skb->len > len) {
+printk("+++ br_multicast_ipv4_rcv() doing clone\n");
 		skb2 = skb_clone(skb, GFP_ATOMIC);
 		if (!skb2)
 			return -ENOMEM;
 
+printk("+++ br_multicast_ipv4_rcv() and pskb_trim_rcsum\n");
 		err = pskb_trim_rcsum(skb2, len);
 		if (err)
 			goto err_out;
@@ -1405,14 +1415,20 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
 	if (!pskb_may_pull(skb2, sizeof(*ih)))
 		goto out;
 
+printk("+++ br_multicast_ipv4_rcv() 2) skb2->len: %i len: %i skb->len: %i\n", skb2->len, len, skb->len);
 	switch (skb2->ip_summed) {
 	case CHECKSUM_COMPLETE:
+printk("+++ br_multicast_ipv4_rcv() here 1.6\n");
 		if (!csum_fold(skb2->csum))
 			break;
 		/* fall through */
 	case CHECKSUM_NONE:
+printk("+++ br_multicast_ipv4_rcv() here 1.7, skb2->csum is %x\n", skb2->csum);
+		__sum16 foobar;
 		skb2->csum = 0;
-		if (skb_checksum_complete(skb2))
+		foobar = skb_checksum_complete(skb2);
+printk("+++ br_multicast_ipv4_rcv() here 1.8, skb_checksum_complete: %x\n", foobar);
+		if (foobar)
 			goto out;
 	}
 
@@ -1459,6 +1475,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 	int offset;
 	int err;
 
+printk("+++ br_multicast_ipv6_rcv()\n");
 	if (!pskb_may_pull(skb, sizeof(*ip6h)))
 		return -EINVAL;
 
@@ -1476,6 +1493,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 		return 0;
 
 	len = ntohs(ip6h->payload_len);
+//	len = ntohs(ip6h->payload_len) + sizeof(*ip6h); <- should probably be this?
 	if (skb->len < len)
 		return -EINVAL;
 
@@ -1485,6 +1503,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 	if (offset < 0 || nexthdr != IPPROTO_ICMPV6)
 		return 0;
 
+printk("+++ br_multicast_ipv6_rcv() here 0.5\n");
 	/* Okay, we found ICMPv6 header */
 	skb2 = skb_clone(skb, GFP_ATOMIC);
 	if (!skb2)
@@ -1494,7 +1513,9 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 	if (!pskb_may_pull(skb2, offset + sizeof(struct icmp6hdr)))
 		goto out;
 
+printk("+++ br_multicast_ipv6_rcv() len: %i, offset: %i, skb_network_offset(skb2): %i\n", len, offset, skb_network_offset(skb2));
 	len -= offset - skb_network_offset(skb2);
+printk("+++ br_multicast_ipv6_rcv() new len: %i\n", len);
 
 	__skb_pull(skb2, offset);
 	skb_reset_transport_header(skb2);
@@ -1513,27 +1534,37 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 	}
 
 	/* Okay, we found MLD message. Check further. */
+printk("+++ br_multicast_ipv6_rcv() skb2->len: %i len: %i skb->len: %i\n", skb2->len, len, skb->len);
 	if (skb2->len > len) {
+printk("+++ br_multicast_ipv6_rcv() doing pskb_trim_rcsum\n");
 		err = pskb_trim_rcsum(skb2, len);
 		if (err)
 			goto out;
 	}
 
+printk("+++ br_multicast_ipv6_rcv() here 1.5\n");
 	switch (skb2->ip_summed) {
 	case CHECKSUM_COMPLETE:
+printk("+++ br_multicast_ipv6_rcv() here 1.6\n");
 		if (!csum_fold(skb2->csum))
 			break;
 		/*FALLTHROUGH*/
 	case CHECKSUM_NONE:
+printk("+++ br_multicast_ipv6_rcv() here 1.7, skb2->csum is %x\n", skb2->csum);
+		__sum16 foobar;
 		skb2->csum = 0;
-		if (skb_checksum_complete(skb2))
+		foobar = skb_checksum_complete(skb2);
+		if (foobar) {
+printk("+++ br_multicast_ipv6_rcv() here 1.8, skb_checksum_complete(skb2): %x (skb): %x\n", foobar, skb_checksum_complete(skb));
 			goto out;
+		}
 	}
 
 	err = 0;
 
 	BR_INPUT_SKB_CB(skb)->igmp = 1;
 
+printk("+++ br_multicast_ipv6_rcv() here 2\n");
 	switch (icmp6h->icmp6_type) {
 	case ICMPV6_MGM_REPORT:
 	    {

[-- Attachment #4: ipv4-group-join.cap --]
[-- Type: application/cap, Size: 164 bytes --]

[-- Attachment #5: ipv6-group-join.cap --]
[-- Type: application/cap, Size: 236 bytes --]

bridge: mcast snooping, fixes for IPv6 MLDv1/2 parsing

[Linus Lüssing]

Hi everyone,

The following two patches are fixing two issues, related to the parsing of
IPv6 MLD messages.

The first one fixes an observed issue which lead to ignored MLD messages.
In the tests this patch fixes the issue in my scenario. However I'm not
so familiar with the checksumming functions in the kernel, so would be
great if someone could double-check whether the new checksum calculations
make sense like this.

The second patch fixes a potential issue. Bogus values of the 'len' variable
had been observed during tests and this patch successfully fixed this
here. However this patch is untested for the potential issues named
in the patch's description that could have occured without it.

Both patches together have been applied and tested and no more
issues with the parsing and adding of new IPv6 multicast listeners to
the bridge snooping database could be observed (for now ;) ).

Cheers, Linus

[PATCH] bridge: mcast snooping, fix IPv6 MLD checksum calculation

[Linus Lüssing]

In contrast to IGMP, the MLDv1/2 message checksum needs to include an
IPv6 "pseudo-header" in the calculations (see RFC2710, section 3.3;
RFC3810, section 5.1.2).

The multicast snooping feature of the bridge code however did not take
this "pseudo-header" into consideration for the checksum validation when
parsing a snooped IPv6 MLDv1/2 message of another host, leading to
possibly ignored, though valid MLDv1/2 messages. This commit shall fix
this issue.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 net/bridge/br_multicast.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index f61eb2e..47fae4f 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1525,7 +1525,10 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 			break;
 		/*FALLTHROUGH*/
 	case CHECKSUM_NONE:
-		skb2->csum = 0;
+		skb2->csum = ~csum_unfold(csum_ipv6_magic(&ip6h->saddr,
+							  &ip6h->daddr,
+							  skb2->len,
+							  nexthdr, 0));
 		if (skb_checksum_complete(skb2))
 			goto out;
 	}
-- 
1.5.6.5

[PATCH] bridge: mcast snooping, fix length check of snooped MLDv1/2

[Linus Lüssing]

"len = ntohs(ip6h->payload_len)" does not include the length of the ipv6
header itself, which the rest of this function assumes, though.

This leads to a length check less restrictive as it should be in the
following line for one thing. For another, it very likely leads to an
integer underrun when substracting the offset and therefore to a very
high new value of 'len' due to its unsignedness. This will ultimately
lead to the pskb_trim_rcsum() practically never being called, even in
the cases where it should.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 net/bridge/br_multicast.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 47fae4f..3793264 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1475,7 +1475,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 	    ip6h->payload_len == 0)
 		return 0;
 
-	len = ntohs(ip6h->payload_len);
+	len = ntohs(ip6h->payload_len) + sizeof(*ip6h);
 	if (skb->len < len)
 		return -EINVAL;
 
-- 
1.5.6.5

Re: [PATCH] bridge: mcast snooping, fix IPv6 MLD checksum calculation

[Herbert Xu]

On Sun, Mar 27, 2011 at 08:27:23AM +0200, Linus Lüssing wrote:
> In contrast to IGMP, the MLDv1/2 message checksum needs to include an
> IPv6 "pseudo-header" in the calculations (see RFC2710, section 3.3;
> RFC3810, section 5.1.2).
> 
> The multicast snooping feature of the bridge code however did not take
> this "pseudo-header" into consideration for the checksum validation when
> parsing a snooped IPv6 MLDv1/2 message of another host, leading to
> possibly ignored, though valid MLDv1/2 messages. This commit shall fix
> this issue.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> ---
>  net/bridge/br_multicast.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index f61eb2e..47fae4f 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1525,7 +1525,10 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
>  			break;
>  		/*FALLTHROUGH*/
>  	case CHECKSUM_NONE:
> -		skb2->csum = 0;
> +		skb2->csum = ~csum_unfold(csum_ipv6_magic(&ip6h->saddr,
> +							  &ip6h->daddr,
> +							  skb2->len,
> +							  nexthdr, 0));

You also need to include the pseudo-header for the CHECKSUM_COMPLETE
case, before we've fallen through (which only happens if the
hardware checksum doesn't match).

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] bridge: mcast snooping, fix IPv6 MLD checksum calculation

[Linus Lüssing]

So instead of 'if (!csum_fold(skb2->csum))' it should be this?
---
if (!csum_ipv6_magic(&ip6h->saddr, &ip6h->daddr,
                     skb2->len, nexthdr, skb2->csum))
---
(I'm peeking a little bit at
http://lxr.linux.no/linux+*/net/ipv6/netfilter.c#L98)

Hmm, if so, then I don't know how to test and verify that now though.

Cheers, Linus

On Sun, Mar 27, 2011 at 02:37:49PM +0800, Herbert Xu wrote:
> On Sun, Mar 27, 2011 at 08:27:23AM +0200, Linus Lüssing wrote:
> > In contrast to IGMP, the MLDv1/2 message checksum needs to include an
> > IPv6 "pseudo-header" in the calculations (see RFC2710, section 3.3;
> > RFC3810, section 5.1.2).
> > 
> > The multicast snooping feature of the bridge code however did not take
> > this "pseudo-header" into consideration for the checksum validation when
> > parsing a snooped IPv6 MLDv1/2 message of another host, leading to
> > possibly ignored, though valid MLDv1/2 messages. This commit shall fix
> > this issue.
> > 
> > Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> > ---
> >  net/bridge/br_multicast.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> > index f61eb2e..47fae4f 100644
> > --- a/net/bridge/br_multicast.c
> > +++ b/net/bridge/br_multicast.c
> > @@ -1525,7 +1525,10 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
> >  			break;
> >  		/*FALLTHROUGH*/
> >  	case CHECKSUM_NONE:
> > -		skb2->csum = 0;
> > +		skb2->csum = ~csum_unfold(csum_ipv6_magic(&ip6h->saddr,
> > +							  &ip6h->daddr,
> > +							  skb2->len,
> > +							  nexthdr, 0));
> 
> You also need to include the pseudo-header for the CHECKSUM_COMPLETE
> case, before we've fallen through (which only happens if the
> hardware checksum doesn't match).
> 
> Thanks,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> 

Re: [PATCH] bridge: mcast snooping, fix IPv6 MLD checksum calculation

[Herbert Xu]

On Sun, Mar 27, 2011 at 09:06:45AM +0200, Linus Lüssing wrote:
> So instead of 'if (!csum_fold(skb2->csum))' it should be this?
> ---
> if (!csum_ipv6_magic(&ip6h->saddr, &ip6h->daddr,
>                      skb2->len, nexthdr, skb2->csum))
> ---
> (I'm peeking a little bit at
> http://lxr.linux.no/linux+*/net/ipv6/netfilter.c#L98)

Yes that looks about right.

> Hmm, if so, then I don't know how to test and verify that now though.

You'd need a NIC that supported this to test.  Although I wouldn't
worry about it too much as the same code patterns appears over and
over again in our stack.  Perhaps we should put it in a helper.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] bridge: mcast snooping, fix IPv6 MLD checksum calculation

[David Miller]

From: Linus Lüssing <linus.luessing@web.de>
Date: Sun, 27 Mar 2011 08:27:23 +0200

> In contrast to IGMP, the MLDv1/2 message checksum needs to include an
> IPv6 "pseudo-header" in the calculations (see RFC2710, section 3.3;
> RFC3810, section 5.1.2).
> 
> The multicast snooping feature of the bridge code however did not take
> this "pseudo-header" into consideration for the checksum validation when
> parsing a snooped IPv6 MLDv1/2 message of another host, leading to
> possibly ignored, though valid MLDv1/2 messages. This commit shall fix
> this issue.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>

I'm waiting for am updated version of this patch which addresses Herbert
Xu's feedback.

Re: [PATCH] bridge: mcast snooping, fix length check of snooped MLDv1/2

[David Miller]

From: Linus Lüssing <linus.luessing@web.de>
Date: Sun, 27 Mar 2011 08:27:24 +0200

> "len = ntohs(ip6h->payload_len)" does not include the length of the ipv6
> header itself, which the rest of this function assumes, though.
> 
> This leads to a length check less restrictive as it should be in the
> following line for one thing. For another, it very likely leads to an
> integer underrun when substracting the offset and therefore to a very
> high new value of 'len' due to its unsignedness. This will ultimately
> lead to the pskb_trim_rcsum() practically never being called, even in
> the cases where it should.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>

Applied.