Re: [PATCH net V3 2/2] net: fix tcp reset packet flowlabel for ipv6

2017-07-31 Thread David Miller 
From: Shaohua Li 
Date: Mon, 31 Jul 2017 12:30:27 -0700

> On Mon, Jul 31, 2017 at 11:10:38AM -0700, Cong Wang wrote:
>> On Mon, Jul 31, 2017 at 10:08 AM, Shaohua Li  wrote:
>> > +/* Like ip6_make_flowlabel, but already has hash */
>> > +static inline __be32 ip6_make_flowlabel_from_hash(struct net *net,
>> > + bool autolabel, u32 hash)
>> > +{
>> > +   __be32 flowlabel;
>> > +
>> > +   if (net->ipv6.sysctl.auto_flowlabels == IP6_AUTO_FLOW_LABEL_OFF ||
>> > +   (!autolabel &&
>> > +net->ipv6.sysctl.auto_flowlabels != 
>> > IP6_AUTO_FLOW_LABEL_FORCED))
>> > +   return 0;
>> > +
>> > +   flowlabel = (__force __be32)hash & IPV6_FLOWLABEL_MASK;
>> > +
>> > +   if (net->ipv6.sysctl.flowlabel_state_ranges)
>> > +   flowlabel |= IPV6_FLOWLABEL_STATELESS_FLAG;
>> > +
>> > +   return flowlabel;
>> > +}
>> 
>> I still don't see why you have to duplicate the code,
>> for me you can just refactor ip6_make_flowlabel()
>> and pass the hash as a parameter and pass
>> 'flowlabel' as 0, and no run-time overhead.
> 
> Still need extra check. Ok, I updated the patch.

This is not how you post a new version of a patch.

It is especially not the way to post a new version of a patch which is
part of a series.

You always must make a clean, fresh, patch posting.  Not as a reply to
a discussion email.

And when the patch is part of a series, you must repost the entire
series along with the "[PATCH ... 0/N] " header posting.

Re: [PATCH net V3 2/2] net: fix tcp reset packet flowlabel for ipv6

2017-07-31 Thread Shaohua Li 
On Mon, Jul 31, 2017 at 11:10:38AM -0700, Cong Wang wrote:
> On Mon, Jul 31, 2017 at 10:08 AM, Shaohua Li  wrote:
> > +/* Like ip6_make_flowlabel, but already has hash */
> > +static inline __be32 ip6_make_flowlabel_from_hash(struct net *net,
> > + bool autolabel, u32 hash)
> > +{
> > +   __be32 flowlabel;
> > +
> > +   if (net->ipv6.sysctl.auto_flowlabels == IP6_AUTO_FLOW_LABEL_OFF ||
> > +   (!autolabel &&
> > +net->ipv6.sysctl.auto_flowlabels != 
> > IP6_AUTO_FLOW_LABEL_FORCED))
> > +   return 0;
> > +
> > +   flowlabel = (__force __be32)hash & IPV6_FLOWLABEL_MASK;
> > +
> > +   if (net->ipv6.sysctl.flowlabel_state_ranges)
> > +   flowlabel |= IPV6_FLOWLABEL_STATELESS_FLAG;
> > +
> > +   return flowlabel;
> > +}
> 
> I still don't see why you have to duplicate the code,
> for me you can just refactor ip6_make_flowlabel()
> and pass the hash as a parameter and pass
> 'flowlabel' as 0, and no run-time overhead.

Still need extra check. Ok, I updated the patch.

Thanks,
Shaohua


>From 373e23f5295ee4cb725109a9e58152451a9fb4cc Mon Sep 17 00:00:00 2001
Message-Id: 
<373e23f5295ee4cb725109a9e58152451a9fb4cc.1501529088.git.s...@fb.com>
From: Shaohua Li 
Date: Tue, 11 Jul 2017 21:09:48 -0700
Subject: [PATCH] net: fix tcp reset packet flowlabel for ipv6

Please see below tcpdump output:
21:00:48.109122 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload 
length: 40) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.: Flags 
[S], cksum 0x0529 (incorrect -> 0xf56c), seq 3282214508, win 43690, options 
[mss 65476,sackOK,TS val 2500903437 ecr 0,nop,wscale 7], length 0
21:00:48.109381 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload 
length: 40) fec0::5054:ff:fe12:3456. > fec0::5054:ff:fe12:3456.55804: Flags 
[S.], cksum 0x0529 (incorrect -> 0x49ad), seq 1923801573, ack 3282214509, win 
43690, options [mss 65476,sackOK,TS val 2500903437 ecr 2500903437,nop,wscale 
7], length 0
21:00:48.109548 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload 
length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.: Flags 
[.], cksum 0x0521 (incorrect -> 0x1bdf), seq 1, ack 1, win 342, options 
[nop,nop,TS val 2500903437 ecr 2500903437], length 0
21:00:48.109823 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload 
length: 62) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.: Flags 
[P.], cksum 0x053f (incorrect -> 0xb8b1), seq 1:31, ack 1, win 342, options 
[nop,nop,TS val 2500903437 ecr 2500903437], length 30
21:00:48.109910 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload 
length: 32) fec0::5054:ff:fe12:3456. > fec0::5054:ff:fe12:3456.55804: Flags 
[.], cksum 0x0521 (incorrect -> 0x1bc1), seq 1, ack 31, win 342, options 
[nop,nop,TS val 2500903437 ecr 2500903437], length 0
21:00:48.110043 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload 
length: 56) fec0::5054:ff:fe12:3456. > fec0::5054:ff:fe12:3456.55804: Flags 
[P.], cksum 0x0539 (incorrect -> 0xb726), seq 1:25, ack 31, win 342, options 
[nop,nop,TS val 2500903438 ecr 2500903437], length 24
21:00:48.110173 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload 
length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.: Flags 
[.], cksum 0x0521 (incorrect -> 0x1ba7), seq 31, ack 25, win 342, options 
[nop,nop,TS val 2500903438 ecr 2500903438], length 0
21:00:48.110211 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload 
length: 32) fec0::5054:ff:fe12:3456. > fec0::5054:ff:fe12:3456.55804: Flags 
[F.], cksum 0x0521 (incorrect -> 0x1ba7), seq 25, ack 31, win 342, options 
[nop,nop,TS val 2500903438 ecr 2500903437], length 0
21:00:48.151099 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload 
length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.: Flags 
[.], cksum 0x0521 (incorrect -> 0x1ba6), seq 31, ack 26, win 342, options 
[nop,nop,TS val 2500903438 ecr 2500903438], length 0
21:00:49.110524 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload 
length: 56) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.: Flags 
[P.], cksum 0x0539 (incorrect -> 0xb324), seq 31:55, ack 26, win 342, options 
[nop,nop,TS val 2500904438 ecr 2500903438], length 24
21:00:49.110637 IP6 (flowlabel 0xb34d5, hlim 64, next-header TCP (6) payload 
length: 20) fec0::5054:ff:fe12:3456. > fec0::5054:ff:fe12:3456.55804: Flags 
[R], cksum 0x0515 (incorrect -> 0x668c), seq 1923801599, win 0, length 0

The tcp reset packet has a different flowlabel, which causes our router
doesn't correctly close tcp connection. The reason is the normal packet
gets the skb->hash from sk->sk_txhash, which is generated randomly.
ip6_make_flowlabel then uses the hash to create a flowlabel. The reset
packet doesn't get assigned a hash, so the flowlabel is calculated with
flowi6.

Since user can't change timewait sock flowlabel, we

Re: [PATCH net V3 2/2] net: fix tcp reset packet flowlabel for ipv6

2017-07-31 Thread Cong Wang 
On Mon, Jul 31, 2017 at 10:08 AM, Shaohua Li  wrote:
> +/* Like ip6_make_flowlabel, but already has hash */
> +static inline __be32 ip6_make_flowlabel_from_hash(struct net *net,
> + bool autolabel, u32 hash)
> +{
> +   __be32 flowlabel;
> +
> +   if (net->ipv6.sysctl.auto_flowlabels == IP6_AUTO_FLOW_LABEL_OFF ||
> +   (!autolabel &&
> +net->ipv6.sysctl.auto_flowlabels != IP6_AUTO_FLOW_LABEL_FORCED))
> +   return 0;
> +
> +   flowlabel = (__force __be32)hash & IPV6_FLOWLABEL_MASK;
> +
> +   if (net->ipv6.sysctl.flowlabel_state_ranges)
> +   flowlabel |= IPV6_FLOWLABEL_STATELESS_FLAG;
> +
> +   return flowlabel;
> +}

I still don't see why you have to duplicate the code,
for me you can just refactor ip6_make_flowlabel()
and pass the hash as a parameter and pass
'flowlabel' as 0, and no run-time overhead.

Or I am missing anything?

[PATCH net V3 2/2] net: fix tcp reset packet flowlabel for ipv6

2017-07-31 Thread Shaohua Li 
From: Shaohua Li 

Please see below tcpdump output:
21:00:48.109122 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload 
length: 40) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.: Flags 
[S], cksum 0x0529 (incorrect -> 0xf56c), seq 3282214508, win 43690, options 
[mss 65476,sackOK,TS val 2500903437 ecr 0,nop,wscale 7], length 0
21:00:48.109381 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload 
length: 40) fec0::5054:ff:fe12:3456. > fec0::5054:ff:fe12:3456.55804: Flags 
[S.], cksum 0x0529 (incorrect -> 0x49ad), seq 1923801573, ack 3282214509, win 
43690, options [mss 65476,sackOK,TS val 2500903437 ecr 2500903437,nop,wscale 
7], length 0
21:00:48.109548 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload 
length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.: Flags 
[.], cksum 0x0521 (incorrect -> 0x1bdf), seq 1, ack 1, win 342, options 
[nop,nop,TS val 2500903437 ecr 2500903437], length 0
21:00:48.109823 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload 
length: 62) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.: Flags 
[P.], cksum 0x053f (incorrect -> 0xb8b1), seq 1:31, ack 1, win 342, options 
[nop,nop,TS val 2500903437 ecr 2500903437], length 30
21:00:48.109910 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload 
length: 32) fec0::5054:ff:fe12:3456. > fec0::5054:ff:fe12:3456.55804: Flags 
[.], cksum 0x0521 (incorrect -> 0x1bc1), seq 1, ack 31, win 342, options 
[nop,nop,TS val 2500903437 ecr 2500903437], length 0
21:00:48.110043 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload 
length: 56) fec0::5054:ff:fe12:3456. > fec0::5054:ff:fe12:3456.55804: Flags 
[P.], cksum 0x0539 (incorrect -> 0xb726), seq 1:25, ack 31, win 342, options 
[nop,nop,TS val 2500903438 ecr 2500903437], length 24
21:00:48.110173 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload 
length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.: Flags 
[.], cksum 0x0521 (incorrect -> 0x1ba7), seq 31, ack 25, win 342, options 
[nop,nop,TS val 2500903438 ecr 2500903438], length 0
21:00:48.110211 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload 
length: 32) fec0::5054:ff:fe12:3456. > fec0::5054:ff:fe12:3456.55804: Flags 
[F.], cksum 0x0521 (incorrect -> 0x1ba7), seq 25, ack 31, win 342, options 
[nop,nop,TS val 2500903438 ecr 2500903437], length 0
21:00:48.151099 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload 
length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.: Flags 
[.], cksum 0x0521 (incorrect -> 0x1ba6), seq 31, ack 26, win 342, options 
[nop,nop,TS val 2500903438 ecr 2500903438], length 0
21:00:49.110524 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload 
length: 56) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.: Flags 
[P.], cksum 0x0539 (incorrect -> 0xb324), seq 31:55, ack 26, win 342, options 
[nop,nop,TS val 2500904438 ecr 2500903438], length 24
21:00:49.110637 IP6 (flowlabel 0xb34d5, hlim 64, next-header TCP (6) payload 
length: 20) fec0::5054:ff:fe12:3456. > fec0::5054:ff:fe12:3456.55804: Flags 
[R], cksum 0x0515 (incorrect -> 0x668c), seq 1923801599, win 0, length 0

The tcp reset packet has a different flowlabel, which causes our router
doesn't correctly close tcp connection. The reason is the normal packet
gets the skb->hash from sk->sk_txhash, which is generated randomly.
ip6_make_flowlabel then uses the hash to create a flowlabel. The reset
packet doesn't get assigned a hash, so the flowlabel is calculated with
flowi6.

Since user can't change timewait sock flowlabel, we create a flowlabel
for timewait socket with the random generated hash (sk->sk_txhash), then
use it in reset packet. In this way, the reset packet will have the same
flowlabel as normal packets.

This also fixes the flowlabel issue for reset packet if user configures
flowlabel, which is ignored previously.

Cc: Eric Dumazet 
Cc: Florent Fourcot 
Cc: Cong Wang 
Signed-off-by: Shaohua Li 
---
 include/net/ipv6.h   | 19 +++
 net/ipv4/tcp_minisocks.c |  8 +++-
 net/ipv6/tcp_ipv6.c  | 18 +-
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 7548367..653d2df 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -798,6 +798,25 @@ static inline __be32 ip6_make_flowlabel(struct net *net, 
struct sk_buff *skb,
return flowlabel;
 }
 
+/* Like ip6_make_flowlabel, but already has hash */
+static inline __be32 ip6_make_flowlabel_from_hash(struct net *net,
+ bool autolabel, u32 hash)
+{
+   __be32 flowlabel;
+
+   if (net->ipv6.sysctl.auto_flowlabels == IP6_AUTO_FLOW_LABEL_OFF ||
+   (!autolabel &&
+net->ipv6.sysctl.auto_flowlabels != IP6_AUTO_FLOW_LABEL_FORCED))
+   return 0;
+
+   flowlabel = (__force __be32)hash & IPV6_FLOWLABEL_MASK;
+
+   i