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

2017-08-02 Thread Cong Wang 
On Tue, Aug 1, 2017 at 2:42 PM, Shaohua Li  wrote:
> Is this really better? I don't see any point. I'd use my original patch other
> than this one. that said, there are just several lines of code, brutally
> 'abstract' them into a function doesn't make the code better.

The current ip6_make_flowlabel() is already a mess, with your
change you just make it worse to read. I just want to make it suck
less.

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

2017-08-01 Thread Shaohua Li 
On Tue, Aug 01, 2017 at 02:17:58PM -0700, Cong Wang wrote:
> On Mon, Jul 31, 2017 at 4:00 PM, Shaohua Li  wrote:
> > On Mon, Jul 31, 2017 at 03:35:02PM -0700, Cong Wang wrote:
> >> On Mon, Jul 31, 2017 at 3:19 PM, Shaohua Li  wrote:
> >> >  static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff 
> >> > *skb,
> >> > __be32 flowlabel, bool autolabel,
> >> > -   struct flowi6 *fl6)
> >> > +   struct flowi6 *fl6, u32 hash)
> >> >  {
> >> > -   u32 hash;
> >> > -
> >> > /* @flowlabel may include more than a flow label, eg, the 
> >> > traffic class.
> >> >  * Here we want only the flow label value.
> >> >  */
> >> > @@ -788,7 +786,8 @@ static inline __be32 ip6_make_flowlabel(struct net 
> >> > *net, struct sk_buff *skb,
> >> >  net->ipv6.sysctl.auto_flowlabels != 
> >> > IP6_AUTO_FLOW_LABEL_FORCED))
> >> > return flowlabel;
> >> >
> >> > -   hash = skb_get_hash_flowi6(skb, fl6);
> >> > +   if (skb)
> >> > +   hash = skb_get_hash_flowi6(skb, fl6);
> >>
> >>
> >> Why not just move skb_get_hash_flowi6() to its caller?
> >> This check is not necessary. If you don't want to touch
> >> existing callers, you can just introduce a wrapper:
> >>
> >>
> >> static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff 
> >> *skb,
> >> __be32 flowlabel, bool autolabel,
> >> struct flowi6 *fl6)
> >> {
> >>   u32 hash = skb_get_hash_flowi6(skb, fl6);
> >>   return __ip6_make_flowlabel(net, flowlabel, autolabel, hash);
> >> }
> >
> > this will always call skb_get_hash_flowi6 for the fast path even auto 
> > flowlabel
> > is disabled. I thought we should avoid this.
> 
> Yeah, but you can move the check out too,
> something like:

Is this really better? I don't see any point. I'd use my original patch other
than this one. that said, there are just several lines of code, brutally
'abstract' them into a function doesn't make the code better.
 
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index 6eac5cf8f1e6..18ffa824c00a 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -771,31 +771,22 @@ static inline void
> iph_to_flow_copy_v6addrs(struct flow_keys *flow,
> 
>  #define IP6_DEFAULT_AUTO_FLOW_LABELS   IP6_AUTO_FLOW_LABEL_OPTOUT
> 
> -static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
> -   __be32 flowlabel, bool autolabel,
> -   struct flowi6 *fl6)
> +static inline bool ip6_need_flowlabel(struct net *net, __be32
> flowlabel, bool autolabel)
>  {
> -   u32 hash;
> -
> /* @flowlabel may include more than a flow label, eg, the traffic 
> class.
>  * Here we want only the flow label value.
>  */
> -   flowlabel &= IPV6_FLOWLABEL_MASK;
> -
> -   if (flowlabel ||
> +   if ((flowlabel & IPV6_FLOWLABEL_MASK) ||
> net->ipv6.sysctl.auto_flowlabels == IP6_AUTO_FLOW_LABEL_OFF ||
> (!autolabel &&
>  net->ipv6.sysctl.auto_flowlabels != IP6_AUTO_FLOW_LABEL_FORCED))
> -   return flowlabel;
> -
> -   hash = skb_get_hash_flowi6(skb, fl6);
> +   return false;
> 
> -   /* Since this is being sent on the wire obfuscate hash a bit
> -* to minimize possbility that any useful information to an
> -* attacker is leaked. Only lower 20 bits are relevant.
> -*/
> -   rol32(hash, 16);
> +   return true;
> +}
> 
> +static inline __be32 __ip6_make_flowlabel(struct net *net, __be32
> flowlabel, u32 hash)
> +{
> flowlabel = (__force __be32)hash & IPV6_FLOWLABEL_MASK;
> 
> if (net->ipv6.sysctl.flowlabel_state_ranges)
> @@ -804,6 +795,19 @@ static inline __be32 ip6_make_flowlabel(struct
> net *net, struct sk_buff *skb,
> return flowlabel;
>  }
> 
> +static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
> +   __be32 flowlabel, bool autolabel,
> +   struct flowi6 *fl6)
> +{
> +   u32 hash;
> +
> +   if (!ip6_need_flowlabel(net, flowlabel, autolabel))
> +   return flowlabel & IPV6_FLOWLABEL_MASK;
> +
> +   hash = skb_get_hash_flowi6(skb, fl6);
> +   return __ip6_make_flowlabel(net, flowlabel, hash);
> +}
> +
>  static inline int ip6_default_np_autolabel(struct net *net)
>  {
> switch (net->ipv6.sysctl.auto_flowlabels) {

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

2017-08-01 Thread Cong Wang 
On Mon, Jul 31, 2017 at 4:00 PM, Shaohua Li  wrote:
> On Mon, Jul 31, 2017 at 03:35:02PM -0700, Cong Wang wrote:
>> On Mon, Jul 31, 2017 at 3:19 PM, Shaohua Li  wrote:
>> >  static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff 
>> > *skb,
>> > __be32 flowlabel, bool autolabel,
>> > -   struct flowi6 *fl6)
>> > +   struct flowi6 *fl6, u32 hash)
>> >  {
>> > -   u32 hash;
>> > -
>> > /* @flowlabel may include more than a flow label, eg, the traffic 
>> > class.
>> >  * Here we want only the flow label value.
>> >  */
>> > @@ -788,7 +786,8 @@ static inline __be32 ip6_make_flowlabel(struct net 
>> > *net, struct sk_buff *skb,
>> >  net->ipv6.sysctl.auto_flowlabels != 
>> > IP6_AUTO_FLOW_LABEL_FORCED))
>> > return flowlabel;
>> >
>> > -   hash = skb_get_hash_flowi6(skb, fl6);
>> > +   if (skb)
>> > +   hash = skb_get_hash_flowi6(skb, fl6);
>>
>>
>> Why not just move skb_get_hash_flowi6() to its caller?
>> This check is not necessary. If you don't want to touch
>> existing callers, you can just introduce a wrapper:
>>
>>
>> static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
>> __be32 flowlabel, bool autolabel,
>> struct flowi6 *fl6)
>> {
>>   u32 hash = skb_get_hash_flowi6(skb, fl6);
>>   return __ip6_make_flowlabel(net, flowlabel, autolabel, hash);
>> }
>
> this will always call skb_get_hash_flowi6 for the fast path even auto 
> flowlabel
> is disabled. I thought we should avoid this.

Yeah, but you can move the check out too,
something like:

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 6eac5cf8f1e6..18ffa824c00a 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -771,31 +771,22 @@ static inline void
iph_to_flow_copy_v6addrs(struct flow_keys *flow,

 #define IP6_DEFAULT_AUTO_FLOW_LABELS   IP6_AUTO_FLOW_LABEL_OPTOUT

-static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
-   __be32 flowlabel, bool autolabel,
-   struct flowi6 *fl6)
+static inline bool ip6_need_flowlabel(struct net *net, __be32
flowlabel, bool autolabel)
 {
-   u32 hash;
-
/* @flowlabel may include more than a flow label, eg, the traffic class.
 * Here we want only the flow label value.
 */
-   flowlabel &= IPV6_FLOWLABEL_MASK;
-
-   if (flowlabel ||
+   if ((flowlabel & IPV6_FLOWLABEL_MASK) ||
net->ipv6.sysctl.auto_flowlabels == IP6_AUTO_FLOW_LABEL_OFF ||
(!autolabel &&
 net->ipv6.sysctl.auto_flowlabels != IP6_AUTO_FLOW_LABEL_FORCED))
-   return flowlabel;
-
-   hash = skb_get_hash_flowi6(skb, fl6);
+   return false;

-   /* Since this is being sent on the wire obfuscate hash a bit
-* to minimize possbility that any useful information to an
-* attacker is leaked. Only lower 20 bits are relevant.
-*/
-   rol32(hash, 16);
+   return true;
+}

+static inline __be32 __ip6_make_flowlabel(struct net *net, __be32
flowlabel, u32 hash)
+{
flowlabel = (__force __be32)hash & IPV6_FLOWLABEL_MASK;

if (net->ipv6.sysctl.flowlabel_state_ranges)
@@ -804,6 +795,19 @@ static inline __be32 ip6_make_flowlabel(struct
net *net, struct sk_buff *skb,
return flowlabel;
 }

+static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
+   __be32 flowlabel, bool autolabel,
+   struct flowi6 *fl6)
+{
+   u32 hash;
+
+   if (!ip6_need_flowlabel(net, flowlabel, autolabel))
+   return flowlabel & IPV6_FLOWLABEL_MASK;
+
+   hash = skb_get_hash_flowi6(skb, fl6);
+   return __ip6_make_flowlabel(net, flowlabel, hash);
+}
+
 static inline int ip6_default_np_autolabel(struct net *net)
 {
switch (net->ipv6.sysctl.auto_flowlabels) {

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

2017-07-31 Thread Shaohua Li 
On Mon, Jul 31, 2017 at 03:35:02PM -0700, Cong Wang wrote:
> On Mon, Jul 31, 2017 at 3:19 PM, Shaohua Li  wrote:
> >  static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff 
> > *skb,
> > __be32 flowlabel, bool autolabel,
> > -   struct flowi6 *fl6)
> > +   struct flowi6 *fl6, u32 hash)
> >  {
> > -   u32 hash;
> > -
> > /* @flowlabel may include more than a flow label, eg, the traffic 
> > class.
> >  * Here we want only the flow label value.
> >  */
> > @@ -788,7 +786,8 @@ static inline __be32 ip6_make_flowlabel(struct net 
> > *net, struct sk_buff *skb,
> >  net->ipv6.sysctl.auto_flowlabels != 
> > IP6_AUTO_FLOW_LABEL_FORCED))
> > return flowlabel;
> >
> > -   hash = skb_get_hash_flowi6(skb, fl6);
> > +   if (skb)
> > +   hash = skb_get_hash_flowi6(skb, fl6);
> 
> 
> Why not just move skb_get_hash_flowi6() to its caller?
> This check is not necessary. If you don't want to touch
> existing callers, you can just introduce a wrapper:
> 
> 
> static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
> __be32 flowlabel, bool autolabel,
> struct flowi6 *fl6)
> {
>   u32 hash = skb_get_hash_flowi6(skb, fl6);
>   return __ip6_make_flowlabel(net, flowlabel, autolabel, hash);
> }

this will always call skb_get_hash_flowi6 for the fast path even auto flowlabel
is disabled. I thought we should avoid this.

> 
> And your code can just call:
> 
> __ip6_make_flowlabel(net, flowlabel, autolabel, sk->sk_txhash);

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

2017-07-31 Thread Cong Wang 
On Mon, Jul 31, 2017 at 3:19 PM, Shaohua Li  wrote:
>  static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
> __be32 flowlabel, bool autolabel,
> -   struct flowi6 *fl6)
> +   struct flowi6 *fl6, u32 hash)
>  {
> -   u32 hash;
> -
> /* @flowlabel may include more than a flow label, eg, the traffic 
> class.
>  * Here we want only the flow label value.
>  */
> @@ -788,7 +786,8 @@ static inline __be32 ip6_make_flowlabel(struct net *net, 
> struct sk_buff *skb,
>  net->ipv6.sysctl.auto_flowlabels != IP6_AUTO_FLOW_LABEL_FORCED))
> return flowlabel;
>
> -   hash = skb_get_hash_flowi6(skb, fl6);
> +   if (skb)
> +   hash = skb_get_hash_flowi6(skb, fl6);


Why not just move skb_get_hash_flowi6() to its caller?
This check is not necessary. If you don't want to touch
existing callers, you can just introduce a wrapper:


static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
__be32 flowlabel, bool autolabel,
struct flowi6 *fl6)
{
  u32 hash = skb_get_hash_flowi6(skb, fl6);
  return __ip6_make_flowlabel(net, flowlabel, autolabel, hash);
}

And your code can just call:

__ip6_make_flowlabel(net, flowlabel, autolabel, sk->sk_txhash);

[PATCH V4 net 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   |  9 -
 net/ipv4/tcp_minisocks.c |  8 +++-
 net/ipv6/ip6_gre.c   |  2 +-
 net/ipv6/ip6_output.c|  4 ++--
 net/ipv6/ip6_tunnel.c|  2 +-
 net/ipv6/tcp_ipv6.c  | 18 +-
 6 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 7548367..f8713fd 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -773,10 +773,8 @@ static inline void iph_to_flow_copy_v6addrs(struct 
flow_keys *flow,
 
 static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
__be32 flowlabel, bool autolabel,
-   struct flowi6 *fl6)
+   struct flowi6 *fl6, u32 hash)
 {
-   u32 hash;
-
/* @flowlabel may include more than a flow