[PATCH] net: fix bridge multicast packet checksum validation

We need to update the skb->csum after pulling the skb, otherwise
checksum validation will fail. This fixes multicast packet loss for
bridges and splats like the following:

[...]
[   43.986968] eth0: hw csum failure
[   43.990344] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0 #2
[   43.996193] Hardware name: BCM2709
[   43.999647] [<800204e0>] (unwind_backtrace) from [<8001cf14>] (show_stack+0x10/0x14)
[   44.007432] [<8001cf14>] (show_stack) from [<801ab614>] (dump_stack+0x80/0x90)
[   44.014695] [<801ab614>] (dump_stack) from [<802e4548>] (__skb_checksum_complete+0x6c/0xac)
[   44.023090] [<802e4548>] (__skb_checksum_complete) from [<803a055c>] (ipv6_mc_validate_checksum+0x104/0x178)
[   44.032959] [<803a055c>] (ipv6_mc_validate_checksum) from [<802e111c>] (skb_checksum_trimmed+0x130/0x188)
[   44.042565] [<802e111c>] (skb_checksum_trimmed) from [<803a06e8>] (ipv6_mc_check_mld+0x118/0x338)
[   44.051501] [<803a06e8>] (ipv6_mc_check_mld) from [<803b2c98>] (br_multicast_rcv+0x5dc/0xd00)
[   44.060077] [<803b2c98>] (br_multicast_rcv) from [<803aa510>] (br_handle_frame_finish+0xac/0x51c)
[...]

Fixes: 9afd85c9e455 ("net: Export IGMP/MLD message validation code")
Reported-by: Álvaro Fernández Rojas <redacted>
Signed-off-by: Linus Lüssing <redacted>
---

Steinar, can you check whether this fixes the bridge issues you reported on
bugzilla #99081? Not quite sure whether it is the same as yours as you
do not seem to have any such call traces.


I am not super happy with how this patch looks, but there is no "skb_push_rcsum"
available and skb_pull_rcsum() seems non-reversible as is. Alternative suggestions
always welcome.


 net/core/skbuff.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5bf88f5..6c34ef6 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -4076,6 +4076,11 @@ struct sk_buff *skb_checksum_trimmed(struct sk_buff *skb,
 	struct sk_buff *skb_chk;
 	unsigned int offset = skb_transport_offset(skb);
 	__sum16 ret;
+	int ip_summed;
+	int csum_valid;
+	int csum_level;
+	int csum_bad;
+	__wsum csum;
 
 	skb_chk = skb_checksum_maybe_trim(skb, transport_len);
 	if (!skb_chk)

@@ -4084,10 +4089,22 @@ struct sk_buff *skb_checksum_trimmed(struct sk_buff *skb,
 	if (!pskb_may_pull(skb_chk, offset))
 		goto err;
 
-	__skb_pull(skb_chk, offset);
+	ip_summed = skb->ip_summed;
+	csum_valid = skb->csum_valid;
+	csum_level = skb->csum_level;
+	csum_bad = skb->csum_bad;
+	csum = skb->csum;
+
+	skb_pull_rcsum(skb_chk, offset);
 	ret = skb_chkf(skb_chk);
 	__skb_push(skb_chk, offset);
 
+	skb->ip_summed = ip_summed;
+	skb->csum_valid = csum_valid;
+	skb->csum_level = csum_level;
+	skb->csum_bad = csum_bad;
+	skb->csum = csum;
+
 	if (ret)
 		goto err;
 

-- 
1.7.10.4

On Mon, Feb 15, 2016 at 03:07:06AM +0100, Linus Lüssing wrote:

Steinar, can you check whether this fixes the bridge issues you reported on
bugzilla #99081? Not quite sure whether it is the same as yours as you
do not seem to have any such call traces.

It doesn't immediately sound like the same problem; why would promisc change
anything if the problem is the checksumming?

I don't have any reboots scheduled for this machine right now, but I'll see
what I can do wrt. testing.

/* Steinar */
-- 
Homepage: https://www.sesse.net/

On Thu, Feb 18, 2016 at 01:51:34PM +0100, Steinar H. Gunderson wrote:

On Mon, Feb 15, 2016 at 03:07:06AM +0100, Linus Lüssing wrote:

quoted

Steinar, can you check whether this fixes the bridge issues you reported on
bugzilla #99081? Not quite sure whether it is the same as yours as you
do not seem to have any such call traces.

It doesn't immediately sound like the same problem; why would promisc change
anything if the problem is the checksumming?

The mdb you provided in the bugzilla ticket misses reports, so it
was unable to parse reports. Which could point to a checksumming
problem.

Enabling promisc probably did not fix the parsing for you, but instead
promisc forces to forward packets upstream on your interface independent
of the mdb. I would assume that even with promisc, your output
from "bridge mdb show" looks rather empty. Can you check?

I don't have any reboots scheduled for this machine right now, but I'll see
what I can do wrt. testing.

Thanks :).

From: Linus Lüssing <redacted>
Date: Mon, 15 Feb 2016 03:07:06 +0100

There really has to be a better way to fix this :-/

On Thu, 18 Feb 2016 15:35:42 -0500 (EST)
David Miller [off-list ref] wrote:

From: Linus Lüssing <redacted>
Date: Mon, 15 Feb 2016 03:07:06 +0100

quoted

@@ -4084,10 +4089,22 @@ struct sk_buff *skb_checksum_trimmed(struct sk_buff *skb,
 	if (!pskb_may_pull(skb_chk, offset))
 		goto err;
 
-	__skb_pull(skb_chk, offset);
+	ip_summed = skb->ip_summed;
+	csum_valid = skb->csum_valid;
+	csum_level = skb->csum_level;
+	csum_bad = skb->csum_bad;
+	csum = skb->csum;
+
+	skb_pull_rcsum(skb_chk, offset);
 	ret = skb_chkf(skb_chk);
 	__skb_push(skb_chk, offset);
 
+	skb->ip_summed = ip_summed;
+	skb->csum_valid = csum_valid;
+	skb->csum_level = csum_level;
+	skb->csum_bad = csum_bad;
+	skb->csum = csum;
+

There really has to be a better way to fix this :-/

Agreed, this is gross.