| Message ID | 1445958135-19805-3-git-send-email-hannes@stressinduktion.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Oct 27, 2015 at 8:02 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > CHECKSUM_PARTIAL skbs should never arrive in ip_fragment. If we get one > of those warn about them once and handle them gracefully by recalculating > the checksum. > > Cc: Eric Dumazet <edumazet@google.com> > Cc: Vlad Yasevich <vyasevich@gmail.com> > Cc: Benjamin Coddington <bcodding@redhat.com> > Cc: Tom Herbert <tom@herbertland.com> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > --- > net/ipv4/ip_output.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c > index 0b02417..3f94a3b 100644 > --- a/net/ipv4/ip_output.c > +++ b/net/ipv4/ip_output.c > @@ -533,6 +533,11 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, > > dev = rt->dst.dev; > > + /* for offloaded checksums cleanup checksum before fragmentation */ > + if (WARN_ON_ONCE(skb->ip_summed == CHECKSUM_PARTIAL) && > + (err = skb_checksum_help(skb))) > + goto fail; > + Why the WARN_ON_ONCE? Is there a prior check somewhere that avoid this condition? > /* > * Point into the IP datagram header. > */ > @@ -657,9 +662,6 @@ slow_path_clean: > } > > slow_path: > - /* for offloaded checksums cleanup checksum before fragmentation */ > - if ((skb->ip_summed == CHECKSUM_PARTIAL) && skb_checksum_help(skb)) > - goto fail; > iph = ip_hdr(skb); > > left = skb->len - hlen; /* Space per frame */ > -- > 2.5.0 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 27, 2015, at 17:06, Tom Herbert wrote: > On Tue, Oct 27, 2015 at 8:02 AM, Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: > > CHECKSUM_PARTIAL skbs should never arrive in ip_fragment. If we get one > > of those warn about them once and handle them gracefully by recalculating > > the checksum. > > > > Cc: Eric Dumazet <edumazet@google.com> > > Cc: Vlad Yasevich <vyasevich@gmail.com> > > Cc: Benjamin Coddington <bcodding@redhat.com> > > Cc: Tom Herbert <tom@herbertland.com> > > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > > --- > > net/ipv4/ip_output.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c > > index 0b02417..3f94a3b 100644 > > --- a/net/ipv4/ip_output.c > > +++ b/net/ipv4/ip_output.c > > @@ -533,6 +533,11 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, > > > > dev = rt->dst.dev; > > > > + /* for offloaded checksums cleanup checksum before fragmentation */ > > + if (WARN_ON_ONCE(skb->ip_summed == CHECKSUM_PARTIAL) && > > + (err = skb_checksum_help(skb))) > > + goto fail; > > + > Why the WARN_ON_ONCE? Is there a prior check somewhere that avoid this > condition? While I am pretty sure we should not hit the condition in IPv6 anymore, I think this could frighten people in IPv4 land. I will repost without the WARN_ON_ONCE. Maybe it makes sense to use the IFF_DEBUG interface flags again? :) I will repost without those WARN_ON_ONCEs. Bye, Hannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 27, 2015 at 8:02 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > CHECKSUM_PARTIAL skbs should never arrive in ip_fragment. If we get one > of those warn about them once and handle them gracefully by recalculating > the checksum. > I believe a UDP sender within the kernel (like an encapsulation) that happens to send using a frag list that exceeds MTU is quite possible and would be a problem with current code. > Cc: Eric Dumazet <edumazet@google.com> > Cc: Vlad Yasevich <vyasevich@gmail.com> > Cc: Benjamin Coddington <bcodding@redhat.com> > Cc: Tom Herbert <tom@herbertland.com> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > --- > net/ipv4/ip_output.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c > index 0b02417..3f94a3b 100644 > --- a/net/ipv4/ip_output.c > +++ b/net/ipv4/ip_output.c > @@ -533,6 +533,11 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, > > dev = rt->dst.dev; > > + /* for offloaded checksums cleanup checksum before fragmentation */ > + if (WARN_ON_ONCE(skb->ip_summed == CHECKSUM_PARTIAL) && > + (err = skb_checksum_help(skb))) > + goto fail; > + > /* > * Point into the IP datagram header. > */ > @@ -657,9 +662,6 @@ slow_path_clean: > } > > slow_path: > - /* for offloaded checksums cleanup checksum before fragmentation */ > - if ((skb->ip_summed == CHECKSUM_PARTIAL) && skb_checksum_help(skb)) > - goto fail; > iph = ip_hdr(skb); > > left = skb->len - hlen; /* Space per frame */ > -- > 2.5.0 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 10/27/2015 06:02 PM, Hannes Frederic Sowa wrote: > CHECKSUM_PARTIAL skbs should never arrive in ip_fragment. If we get one > of those warn about them once and handle them gracefully by recalculating > the checksum. > > Cc: Eric Dumazet <edumazet@google.com> > Cc: Vlad Yasevich <vyasevich@gmail.com> > Cc: Benjamin Coddington <bcodding@redhat.com> > Cc: Tom Herbert <tom@herbertland.com> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > --- > net/ipv4/ip_output.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c > index 0b02417..3f94a3b 100644 > --- a/net/ipv4/ip_output.c > +++ b/net/ipv4/ip_output.c > @@ -533,6 +533,11 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, > > dev = rt->dst.dev; > > + /* for offloaded checksums cleanup checksum before fragmentation */ > + if (WARN_ON_ONCE(skb->ip_summed == CHECKSUM_PARTIAL) && > + (err = skb_checksum_help(skb))) scripts/checkpatch.pl shou;d have complained about using = in the *if* expression. > + goto fail; > + > /* > * Point into the IP datagram header. > */ [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei, On Tue, Oct 27, 2015, at 20:01, Sergei Shtylyov wrote: > On 10/27/2015 06:02 PM, Hannes Frederic Sowa wrote: > > > CHECKSUM_PARTIAL skbs should never arrive in ip_fragment. If we get one > > of those warn about them once and handle them gracefully by recalculating > > the checksum. > > > > Cc: Eric Dumazet <edumazet@google.com> > > Cc: Vlad Yasevich <vyasevich@gmail.com> > > Cc: Benjamin Coddington <bcodding@redhat.com> > > Cc: Tom Herbert <tom@herbertland.com> > > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > > --- > > net/ipv4/ip_output.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c > > index 0b02417..3f94a3b 100644 > > --- a/net/ipv4/ip_output.c > > +++ b/net/ipv4/ip_output.c > > @@ -533,6 +533,11 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, > > > > dev = rt->dst.dev; > > > > + /* for offloaded checksums cleanup checksum before fragmentation */ > > + if (WARN_ON_ONCE(skb->ip_summed == CHECKSUM_PARTIAL) && > > + (err = skb_checksum_help(skb))) > > scripts/checkpatch.pl shou;d have complained about using = in the > *if* > expression. I know and I ignored it deliberately because I found it nicer this way. I made sure gcc does not complain by using extra braces around the assignment. Bye, Hannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c index 0b02417..3f94a3b 100644 --- a/net/ipv4/ip_output.c +++ b/net/ipv4/ip_output.c @@ -533,6 +533,11 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, dev = rt->dst.dev; + /* for offloaded checksums cleanup checksum before fragmentation */ + if (WARN_ON_ONCE(skb->ip_summed == CHECKSUM_PARTIAL) && + (err = skb_checksum_help(skb))) + goto fail; + /* * Point into the IP datagram header. */ @@ -657,9 +662,6 @@ slow_path_clean: } slow_path: - /* for offloaded checksums cleanup checksum before fragmentation */ - if ((skb->ip_summed == CHECKSUM_PARTIAL) && skb_checksum_help(skb)) - goto fail; iph = ip_hdr(skb); left = skb->len - hlen; /* Space per frame */
CHECKSUM_PARTIAL skbs should never arrive in ip_fragment. If we get one of those warn about them once and handle them gracefully by recalculating the checksum. Cc: Eric Dumazet <edumazet@google.com> Cc: Vlad Yasevich <vyasevich@gmail.com> Cc: Benjamin Coddington <bcodding@redhat.com> Cc: Tom Herbert <tom@herbertland.com> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> --- net/ipv4/ip_output.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)