All of lore.kernel.org
 help / color / mirror / Atom feed
* [Patch net] net_sched: fix mirrored packets checksum
@ 2016-06-30 17:15 Cong Wang
  2016-06-30 19:50 ` Daniel Borkmann
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Cong Wang @ 2016-06-30 17:15 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang, Jamal Hadi Salim, Tom Herbert

Similar to commit 9b368814b336 ("net: fix bridge multicast packet checksum validation")
we need to fixup the checksum for CHECKSUM_COMPLETE when
pushing skb on RX path. Otherwise we get similar splats.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Tom Herbert <tom@herbertland.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/linux/skbuff.h | 19 +++++++++++++++++++
 net/core/skbuff.c      | 18 ------------------
 net/sched/act_mirred.c |  2 +-
 3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index ee38a41..61ab566 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2870,6 +2870,25 @@ static inline void skb_postpush_rcsum(struct sk_buff *skb,
 }
 
 /**
+ *	skb_push_rcsum - push skb and update receive checksum
+ *	@skb: buffer to update
+ *	@len: length of data pulled
+ *
+ *	This function performs an skb_push on the packet and updates
+ *	the CHECKSUM_COMPLETE checksum.  It should be used on
+ *	receive path processing instead of skb_push unless you know
+ *	that the checksum difference is zero (e.g., a valid IP header)
+ *	or you are setting ip_summed to CHECKSUM_NONE.
+ */
+static inline unsigned char *skb_push_rcsum(struct sk_buff *skb,
+					    unsigned int len)
+{
+	skb_push(skb, len);
+	skb_postpush_rcsum(skb, skb->data, len);
+	return skb->data;
+}
+
+/**
  *	pskb_trim_rcsum - trim received skb and update checksum
  *	@skb: buffer to trim
  *	@len: new length
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f2b77e5..eb12d21 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3016,24 +3016,6 @@ int skb_append_pagefrags(struct sk_buff *skb, struct page *page,
 EXPORT_SYMBOL_GPL(skb_append_pagefrags);
 
 /**
- *	skb_push_rcsum - push skb and update receive checksum
- *	@skb: buffer to update
- *	@len: length of data pulled
- *
- *	This function performs an skb_push on the packet and updates
- *	the CHECKSUM_COMPLETE checksum.  It should be used on
- *	receive path processing instead of skb_push unless you know
- *	that the checksum difference is zero (e.g., a valid IP header)
- *	or you are setting ip_summed to CHECKSUM_NONE.
- */
-static unsigned char *skb_push_rcsum(struct sk_buff *skb, unsigned len)
-{
-	skb_push(skb, len);
-	skb_postpush_rcsum(skb, skb->data, len);
-	return skb->data;
-}
-
-/**
  *	skb_pull_rcsum - pull skb and update receive checksum
  *	@skb: buffer to update
  *	@len: length of data pulled
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 128942b..1f5bd6c 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -181,7 +181,7 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 
 	if (!(at & AT_EGRESS)) {
 		if (m->tcfm_ok_push)
-			skb_push(skb2, skb->mac_len);
+			skb_push_rcsum(skb2, skb->mac_len);
 	}
 
 	/* mirror is always swallowed */
-- 
2.1.0

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

* Re: [Patch net] net_sched: fix mirrored packets checksum
  2016-06-30 17:15 [Patch net] net_sched: fix mirrored packets checksum Cong Wang
@ 2016-06-30 19:50 ` Daniel Borkmann
  2016-06-30 22:42   ` Cong Wang
  2016-07-01 12:39 ` Jamal Hadi Salim
  2016-07-01 20:20 ` David Miller
  2 siblings, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2016-06-30 19:50 UTC (permalink / raw)
  To: Cong Wang, netdev; +Cc: Jamal Hadi Salim, Tom Herbert

Hi Cong,

On 06/30/2016 07:15 PM, Cong Wang wrote:
> Similar to commit 9b368814b336 ("net: fix bridge multicast packet checksum validation")
> we need to fixup the checksum for CHECKSUM_COMPLETE when
> pushing skb on RX path. Otherwise we get similar splats.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Tom Herbert <tom@herbertland.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>   include/linux/skbuff.h | 19 +++++++++++++++++++
>   net/core/skbuff.c      | 18 ------------------
>   net/sched/act_mirred.c |  2 +-
>   3 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index ee38a41..61ab566 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2870,6 +2870,25 @@ static inline void skb_postpush_rcsum(struct sk_buff *skb,
>   }
>
>   /**
> + *	skb_push_rcsum - push skb and update receive checksum
> + *	@skb: buffer to update
> + *	@len: length of data pulled
> + *
> + *	This function performs an skb_push on the packet and updates
> + *	the CHECKSUM_COMPLETE checksum.  It should be used on
> + *	receive path processing instead of skb_push unless you know
> + *	that the checksum difference is zero (e.g., a valid IP header)
> + *	or you are setting ip_summed to CHECKSUM_NONE.
> + */
> +static inline unsigned char *skb_push_rcsum(struct sk_buff *skb,
> +					    unsigned int len)
> +{
> +	skb_push(skb, len);
> +	skb_postpush_rcsum(skb, skb->data, len);
> +	return skb->data;
> +}
> +
> +/**
>    *	pskb_trim_rcsum - trim received skb and update checksum
>    *	@skb: buffer to trim
>    *	@len: new length
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index f2b77e5..eb12d21 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3016,24 +3016,6 @@ int skb_append_pagefrags(struct sk_buff *skb, struct page *page,
>   EXPORT_SYMBOL_GPL(skb_append_pagefrags);
>
>   /**
> - *	skb_push_rcsum - push skb and update receive checksum
> - *	@skb: buffer to update
> - *	@len: length of data pulled
> - *
> - *	This function performs an skb_push on the packet and updates
> - *	the CHECKSUM_COMPLETE checksum.  It should be used on
> - *	receive path processing instead of skb_push unless you know
> - *	that the checksum difference is zero (e.g., a valid IP header)
> - *	or you are setting ip_summed to CHECKSUM_NONE.
> - */
> -static unsigned char *skb_push_rcsum(struct sk_buff *skb, unsigned len)
> -{
> -	skb_push(skb, len);
> -	skb_postpush_rcsum(skb, skb->data, len);
> -	return skb->data;
> -}
> -
> -/**
>    *	skb_pull_rcsum - pull skb and update receive checksum
>    *	@skb: buffer to update
>    *	@len: length of data pulled

Fix looks good to me, just a minor comment.

Maybe makes sense to move skb_push_rcsum() but /also/ skb_pull_rcsum()
to the header then? Both seem similarly small at least (could be split
f.e into two patches then, first for the move, second for the actual fix).

Thanks,
Daniel

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

* Re: [Patch net] net_sched: fix mirrored packets checksum
  2016-06-30 19:50 ` Daniel Borkmann
@ 2016-06-30 22:42   ` Cong Wang
  2016-06-30 23:11     ` Daniel Borkmann
  0 siblings, 1 reply; 9+ messages in thread
From: Cong Wang @ 2016-06-30 22:42 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Tom Herbert

On Thu, Jun 30, 2016 at 12:50 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Maybe makes sense to move skb_push_rcsum() but /also/ skb_pull_rcsum()
> to the header then? Both seem similarly small at least (could be split
> f.e into two patches then, first for the move, second for the actual fix).

No objection from me. Please feel free to send a patch. ;)

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

* Re: [Patch net] net_sched: fix mirrored packets checksum
  2016-06-30 22:42   ` Cong Wang
@ 2016-06-30 23:11     ` Daniel Borkmann
  2016-06-30 23:26       ` Cong Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2016-06-30 23:11 UTC (permalink / raw)
  To: Cong Wang; +Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Tom Herbert

On 07/01/2016 12:42 AM, Cong Wang wrote:
> On Thu, Jun 30, 2016 at 12:50 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> Maybe makes sense to move skb_push_rcsum() but /also/ skb_pull_rcsum()
>> to the header then? Both seem similarly small at least (could be split
>> f.e into two patches then, first for the move, second for the actual fix).
>
> No objection from me. Please feel free to send a patch. ;)

Shrug, I actually meant this as feedback to your patch, since you move that
helper and not as a note to myself. ;)

Thanks,
Daniel

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

* Re: [Patch net] net_sched: fix mirrored packets checksum
  2016-06-30 23:11     ` Daniel Borkmann
@ 2016-06-30 23:26       ` Cong Wang
  2016-06-30 23:41         ` Cong Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Cong Wang @ 2016-06-30 23:26 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Tom Herbert

On Thu, Jun 30, 2016 at 4:11 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 07/01/2016 12:42 AM, Cong Wang wrote:
>>
>> On Thu, Jun 30, 2016 at 12:50 PM, Daniel Borkmann <daniel@iogearbox.net>
>> wrote:
>>>
>>>
>>> Maybe makes sense to move skb_push_rcsum() but /also/ skb_pull_rcsum()
>>> to the header then? Both seem similarly small at least (could be split
>>> f.e into two patches then, first for the move, second for the actual
>>> fix).
>>
>>
>> No objection from me. Please feel free to send a patch. ;)
>
>
> Shrug, I actually meant this as feedback to your patch, since you move that
> helper and not as a note to myself. ;)

Interesting, my patch only moves what it needs, why does it need
to do more?

Again, I am not against your idea, just 1) it doesn't belong to my patch
2) I am too lazy to create a patch for it, or, I am perfectly fine with not
moving it too ;)

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

* Re: [Patch net] net_sched: fix mirrored packets checksum
  2016-06-30 23:26       ` Cong Wang
@ 2016-06-30 23:41         ` Cong Wang
  2016-07-01  9:13           ` Daniel Borkmann
  0 siblings, 1 reply; 9+ messages in thread
From: Cong Wang @ 2016-06-30 23:41 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Tom Herbert

On Thu, Jun 30, 2016 at 4:26 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Jun 30, 2016 at 4:11 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 07/01/2016 12:42 AM, Cong Wang wrote:
>>>
>>> On Thu, Jun 30, 2016 at 12:50 PM, Daniel Borkmann <daniel@iogearbox.net>
>>> wrote:
>>>>
>>>>
>>>> Maybe makes sense to move skb_push_rcsum() but /also/ skb_pull_rcsum()
>>>> to the header then? Both seem similarly small at least (could be split
>>>> f.e into two patches then, first for the move, second for the actual
>>>> fix).
>>>
>>>
>>> No objection from me. Please feel free to send a patch. ;)
>>
>>
>> Shrug, I actually meant this as feedback to your patch, since you move that
>> helper and not as a note to myself. ;)
>
> Interesting, my patch only moves what it needs, why does it need
> to do more?

In case you miss the context:
http://marc.info/?l=linux-netdev&m=146730654005424&w=2

This patch should be backported to stable too, which is another
reason why we should keep it as small as possible.

Here, at Twitter, we already backported it to 4.1 kernel for testing.

(The reason why I don't have a Fixes: tag is that I don't identify an
offending commit to blame yet.)

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

* Re: [Patch net] net_sched: fix mirrored packets checksum
  2016-06-30 23:41         ` Cong Wang
@ 2016-07-01  9:13           ` Daniel Borkmann
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel Borkmann @ 2016-07-01  9:13 UTC (permalink / raw)
  To: Cong Wang; +Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Tom Herbert

On 07/01/2016 01:41 AM, Cong Wang wrote:
> On Thu, Jun 30, 2016 at 4:26 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Thu, Jun 30, 2016 at 4:11 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>> On 07/01/2016 12:42 AM, Cong Wang wrote:
>>>> On Thu, Jun 30, 2016 at 12:50 PM, Daniel Borkmann <daniel@iogearbox.net>
>>>> wrote:
>>>>>
>>>>> Maybe makes sense to move skb_push_rcsum() but /also/ skb_pull_rcsum()
>>>>> to the header then? Both seem similarly small at least (could be split
>>>>> f.e into two patches then, first for the move, second for the actual
>>>>> fix).
>>>>
>>>> No objection from me. Please feel free to send a patch. ;)
>>>
>>> Shrug, I actually meant this as feedback to your patch, since you move that
>>> helper and not as a note to myself. ;)
>>
>> Interesting, my patch only moves what it needs, why does it need
>> to do more?
>
> In case you miss the context:
> http://marc.info/?l=linux-netdev&m=146730654005424&w=2

I didn't miss it. Btw, recently had a similar issue (f8ffad69c9f8b8dfb0b).

> This patch should be backported to stable too, which is another
> reason why we should keep it as small as possible.

Fair enough.

> Here, at Twitter, we already backported it to 4.1 kernel for testing.
>
> (The reason why I don't have a Fixes: tag is that I don't identify an
> offending commit to blame yet.)

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

* Re: [Patch net] net_sched: fix mirrored packets checksum
  2016-06-30 17:15 [Patch net] net_sched: fix mirrored packets checksum Cong Wang
  2016-06-30 19:50 ` Daniel Borkmann
@ 2016-07-01 12:39 ` Jamal Hadi Salim
  2016-07-01 20:20 ` David Miller
  2 siblings, 0 replies; 9+ messages in thread
From: Jamal Hadi Salim @ 2016-07-01 12:39 UTC (permalink / raw)
  To: Cong Wang, netdev; +Cc: Tom Herbert

On 16-06-30 01:15 PM, Cong Wang wrote:
> Similar to commit 9b368814b336 ("net: fix bridge multicast packet checksum validation")
> we need to fixup the checksum for CHECKSUM_COMPLETE when
> pushing skb on RX path. Otherwise we get similar splats.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Tom Herbert <tom@herbertland.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

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

* Re: [Patch net] net_sched: fix mirrored packets checksum
  2016-06-30 17:15 [Patch net] net_sched: fix mirrored packets checksum Cong Wang
  2016-06-30 19:50 ` Daniel Borkmann
  2016-07-01 12:39 ` Jamal Hadi Salim
@ 2016-07-01 20:20 ` David Miller
  2 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2016-07-01 20:20 UTC (permalink / raw)
  To: xiyou.wangcong; +Cc: netdev, jhs, tom

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 30 Jun 2016 10:15:22 -0700

> Similar to commit 9b368814b336 ("net: fix bridge multicast packet checksum validation")
> we need to fixup the checksum for CHECKSUM_COMPLETE when
> pushing skb on RX path. Otherwise we get similar splats.
> 
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Tom Herbert <tom@herbertland.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied and queued up for -stable, thanks.

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

end of thread, other threads:[~2016-07-01 20:20 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-30 17:15 [Patch net] net_sched: fix mirrored packets checksum Cong Wang
2016-06-30 19:50 ` Daniel Borkmann
2016-06-30 22:42   ` Cong Wang
2016-06-30 23:11     ` Daniel Borkmann
2016-06-30 23:26       ` Cong Wang
2016-06-30 23:41         ` Cong Wang
2016-07-01  9:13           ` Daniel Borkmann
2016-07-01 12:39 ` Jamal Hadi Salim
2016-07-01 20:20 ` David Miller

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.