| Message ID | 1467306922-7086-1-git-send-email-xiyou.wangcong@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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. ;)
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
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 ;)
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.)
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.)
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
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.
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 */
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(-)