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?