Message ID | 1452874892.1223.175.camel@edumazet-glaptop2.roam.corp.google.com |
---|---|
State | Awaiting Upstream, archived |
Delegated to: | David Miller |
Headers | show |
On Fri, Jan 15, 2016 at 8:21 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > In case MSS option is added in TCP options, skb length increases by 4. > IPv6 needs to update skb->csum if skb has CHECKSUM_COMPLETE, > otherwise kernel complains loudly in netdev_rx_csum_fault() with a > stack dump. > > Signed-off-by: Eric Dumazet <edumazet@google.com> > --- > net/netfilter/xt_TCPMSS.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c > index b7c43def0dc6..e118397254af 100644 > --- a/net/netfilter/xt_TCPMSS.c > +++ b/net/netfilter/xt_TCPMSS.c > @@ -228,7 +228,7 @@ tcpmss_tg6(struct sk_buff *skb, const struct xt_action_param *par) > { > struct ipv6hdr *ipv6h = ipv6_hdr(skb); > u8 nexthdr; > - __be16 frag_off; > + __be16 frag_off, oldlen, newlen; > int tcphoff; > int ret; > > @@ -244,7 +244,12 @@ tcpmss_tg6(struct sk_buff *skb, const struct xt_action_param *par) > return NF_DROP; > if (ret > 0) { > ipv6h = ipv6_hdr(skb); > - ipv6h->payload_len = htons(ntohs(ipv6h->payload_len) + ret); > + oldlen = ipv6h->payload_len; > + newlen = htons(ntohs(oldlen) + ret); > + if (skb->ip_summed == CHECKSUM_COMPLETE) > + skb->csum = csum_add(csum_sub(skb->csum, oldlen), > + newlen); > + ipv6h->payload_len = newlen; Probably should have a utility function do this. Maybe something like update_hdr_field(skb, old, new)? Could return new value so above could just be: ipv6h->payload_len = update_hdr_field(skb, ipv6h->payload_len, htons(ntohs(oldlen) + ret); > } > return XT_CONTINUE; > } > >
On Fri, 2016-01-15 at 08:43 -0800, Tom Herbert wrote: > Probably should have a utility function do this. Maybe something like > update_hdr_field(skb, old, new)? Could return new value so above could > just be: > > ipv6h->payload_len = update_hdr_field(skb, ipv6h->payload_len, > htons(ntohs(oldlen) + ret); Well, oldlen would still need to be ipv6h->payload_len Not sure a helper will be nice for a stable submission. Probably better for net-next ?
On Fri, Jan 15, 2016 at 9:18 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Fri, 2016-01-15 at 08:43 -0800, Tom Herbert wrote: > >> Probably should have a utility function do this. Maybe something like >> update_hdr_field(skb, old, new)? Could return new value so above could >> just be: >> >> ipv6h->payload_len = update_hdr_field(skb, ipv6h->payload_len, >> htons(ntohs(oldlen) + ret); > > Well, oldlen would still need to be ipv6h->payload_len > That comes from the second argument. > Not sure a helper will be nice for a stable submission. > > Probably better for net-next ? > I think this would apply to ipv6: update skb->csum when CE mark is propagated also, maybe others? Seems safer to implement the logic only in one place if possible... No big deal though. >
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Fri, 15 Jan 2016 09:18:44 -0800 > On Fri, 2016-01-15 at 08:43 -0800, Tom Herbert wrote: > >> Probably should have a utility function do this. Maybe something like >> update_hdr_field(skb, old, new)? Could return new value so above could >> just be: >> >> ipv6h->payload_len = update_hdr_field(skb, ipv6h->payload_len, >> htons(ntohs(oldlen) + ret); > > Well, oldlen would still need to be ipv6h->payload_len > > Not sure a helper will be nice for a stable submission. > > Probably better for net-next ? Agreed. We already have those csum_replace{2,4}() helpers in net/checksum.h which we could augment with ones that can handle length changes too.
On Fri, Jan 15, 2016 at 02:04:49PM -0500, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Fri, 15 Jan 2016 09:18:44 -0800 > > > On Fri, 2016-01-15 at 08:43 -0800, Tom Herbert wrote: > > > >> Probably should have a utility function do this. Maybe something like > >> update_hdr_field(skb, old, new)? Could return new value so above could > >> just be: > >> > >> ipv6h->payload_len = update_hdr_field(skb, ipv6h->payload_len, > >> htons(ntohs(oldlen) + ret); > > > > Well, oldlen would still need to be ipv6h->payload_len > > > > Not sure a helper will be nice for a stable submission. > > > > Probably better for net-next ? > > Agreed. We already have those csum_replace{2,4}() helpers in net/checksum.h > which we could augment with ones that can handle length changes too. I'm going to place this into the nf tree, thanks!
diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c index b7c43def0dc6..e118397254af 100644 --- a/net/netfilter/xt_TCPMSS.c +++ b/net/netfilter/xt_TCPMSS.c @@ -228,7 +228,7 @@ tcpmss_tg6(struct sk_buff *skb, const struct xt_action_param *par) { struct ipv6hdr *ipv6h = ipv6_hdr(skb); u8 nexthdr; - __be16 frag_off; + __be16 frag_off, oldlen, newlen; int tcphoff; int ret; @@ -244,7 +244,12 @@ tcpmss_tg6(struct sk_buff *skb, const struct xt_action_param *par) return NF_DROP; if (ret > 0) { ipv6h = ipv6_hdr(skb); - ipv6h->payload_len = htons(ntohs(ipv6h->payload_len) + ret); + oldlen = ipv6h->payload_len; + newlen = htons(ntohs(oldlen) + ret); + if (skb->ip_summed == CHECKSUM_COMPLETE) + skb->csum = csum_add(csum_sub(skb->csum, oldlen), + newlen); + ipv6h->payload_len = newlen; } return XT_CONTINUE; }