Skip to contentSkip to conversation

net: fix flowlabel inconsistency in reset packet

PATCH Superseded Shaohua Li 2 participants 4 messages Nov 17, 2017 – Nov 19, 2017

Subsystems (4)
Networking Network drivers TCP Sockets

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Shaohua Li

author
#0 lore raw

cover net: fix flowlabel inconsistency in reset packet

From: Shaohua Li <[email protected]>

Hi,

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.5555: 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.5555 > 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.5555: 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.5555: 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.5555 > 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.5555 > 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.5555: 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.5555 > 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.5555: 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.5555: 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.5555 > 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. We are using flowlabel to do load balance. Routers in the path maintain connection state. So if flow label changes, the packet is routed through a different router. In this case, the old router doesn't get the reset packet to close the 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.

The patches fix the issue.

Thanks,
Shaohua

Shaohua Li (2):
net-next: use five-tuple hash for sk_txhash
net-next: copy user configured flowlabel to reset packet

 include/net/sock.h    | 18 ++++--------------
 include/net/tcp.h     |  2 +-
 net/ipv4/datagram.c   |  2 +-
 net/ipv4/syncookies.c |  4 +++-
 net/ipv4/tcp_input.c  |  1 -
 net/ipv4/tcp_ipv4.c   | 17 ++++++++++++-----
 net/ipv4/tcp_output.c |  1 -
 net/ipv6/datagram.c   |  4 +++-
 net/ipv6/syncookies.c |  3 ++-
 net/ipv6/tcp_ipv6.c   | 36 ++++++++++++++++++++++++++++++------
 10 files changed, 56 insertions(+), 32 deletions(-)
Signature · 1 line
2.9.5
author
#1 lore raw

PATCH 1/2 net-next: use five-tuple hash for sk_txhash

From: Shaohua Li <[email protected]>

We are using sk_txhash to calculate flowlabel, but sk_txhash isn't always available, for example, in inet_timewait_sock. This causes problem for reset packet, which will have a different flowlabel. This causes our router doesn't correctly close tcp connection. We are using flowlabel to do load balance. Routers in the path maintain connection state. So if flow label changes, the packet is routed through a different router. In this case, the old router doesn't get the reset packet to close the tcp connection.

Per Tom's suggestion, we switch back to five-tuple hash, so we can reconstruct correct flowlabel for reset packet.

At most places, we already have the flowi info, so we directly use it build sk_txhash. For synack, we do this after route search. At that time, we have the flowi info ready, so don't need to create the flowi info again.

I don't change sk_rethink_txhash() though, it still uses random hash, which is the whole point to select a different path after a negative routing advise.

Cc Martin KaFai Lau <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Florent Fourcot <[email protected]>
Cc Cong Wang <[email protected]>
Cc Tom Herbert <[email protected]>
Signed-off-by Shaohua Li <[email protected]>
---
 include/net/sock.h    | 18 ++++--------------
 include/net/tcp.h     |  2 +-
 net/ipv4/datagram.c   |  2 +-
 net/ipv4/syncookies.c |  4 +++-
 net/ipv4/tcp_input.c  |  1 -
 net/ipv4/tcp_ipv4.c   | 17 ++++++++++++-----
 net/ipv4/tcp_output.c |  1 -
 net/ipv6/datagram.c   |  4 +++-
 net/ipv6/syncookies.c |  3 ++-
 net/ipv6/tcp_ipv6.c   | 18 +++++++++++++-----
 10 files changed, 39 insertions(+), 31 deletions(-)
include/net/sock.h +4 -14
index f8715c5..85a6192 100644
@@ -1732,22 +1732,12 @@ static inline kuid_t sock_net_uid(const struct net *net, const struct sock *sk)
 	return sk ? sk->sk_uid : make_kuid(net->user_ns, 0);
 }
 
-static inline u32 net_tx_rndhash(void)
-{
-	u32 v = prandom_u32();
-
-	return v ?: 1;
-}
-
-static inline void sk_set_txhash(struct sock *sk)
-{
-	sk->sk_txhash = net_tx_rndhash();
-}
-
 static inline void sk_rethink_txhash(struct sock *sk)
 {
-	if (sk->sk_txhash)
-		sk_set_txhash(sk);
+	if (sk->sk_txhash) {
+		u32 v = prandom_u32();
+		sk->sk_txhash = v ?: 1;
+	}
 }
 
 static inline struct dst_entry *
include/net/tcp.h +1 -1
index 85ea578..8d68fde 100644
@@ -1840,7 +1840,7 @@ struct tcp_request_sock_ops {
 				 __u16 *mss);
 #endif
 	struct dst_entry *(*route_req)(const struct sock *sk, struct flowi *fl,
-				       const struct request_sock *req);
+				       struct request_sock *req);
 	u32 (*init_seq)(const struct sk_buff *skb);
 	u32 (*init_ts_off)(const struct net *net, const struct sk_buff *skb);
 	int (*send_synack)(const struct sock *sk, struct dst_entry *dst,
net/ipv4/datagram.c +1 -1
index f915abf..ed9ccb7 100644
@@ -74,7 +74,7 @@ int __ip4_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len
 	inet->inet_daddr = fl4->daddr;
 	inet->inet_dport = usin->sin_port;
 	sk->sk_state = TCP_ESTABLISHED;
-	sk_set_txhash(sk);
+	sk->sk_txhash = get_hash_from_flowi4(fl4);
 	inet->inet_id = jiffies;
 
 	sk_dst_set(sk, &rt->dst);
net/ipv4/syncookies.c +3 -1
index fda37f2..76f1cf6 100644
@@ -335,7 +335,6 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 	treq->rcv_isn		= ntohl(th->seq) - 1;
 	treq->snt_isn		= cookie;
 	treq->ts_off		= 0;
-	treq->txhash		= net_tx_rndhash();
 	req->mss		= mss;
 	ireq->ir_num		= ntohs(th->dest);
 	ireq->ir_rmt_port	= th->source;

@@ -376,6 +375,9 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 			   opt->srr ? opt->faddr : ireq->ir_rmt_addr,
 			   ireq->ir_loc_addr, th->source, th->dest, sk->sk_uid);
 	security_req_classify_flow(req, flowi4_to_flowi(&fl4));
+
+	treq->txhash = get_hash_from_flowi4(&fl4);
+
 	rt = ip_route_output_key(sock_net(sk), &fl4);
 	if (IS_ERR(rt)) {
 		reqsk_free(req);
net/ipv4/tcp_input.c +0 -1
index dabbf1d..92b4a10 100644
@@ -6289,7 +6289,6 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	}
 
 	tcp_rsk(req)->snt_isn = isn;
-	tcp_rsk(req)->txhash = net_tx_rndhash();
 	tcp_openreq_init_rwin(req, sk, dst);
 	if (!want_cookie) {
 		tcp_reqsk_record_syn(sk, req, skb);
net/ipv4/tcp_ipv4.c +12 -5
index c6bc0c4..0e71b1b 100644
@@ -222,7 +222,7 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 	if (err)
 		goto failure;
 
-	sk_set_txhash(sk);
+	sk->sk_txhash = get_hash_from_flowi4(fl4);
 
 	rt = ip_route_newports(fl4, rt, orig_sport, orig_dport,
 			       inet->inet_sport, inet->inet_dport, sk);

@@ -871,8 +871,12 @@ static int tcp_v4_send_synack(const struct sock *sk, struct dst_entry *dst,
 	struct sk_buff *skb;
 
 	/* First, grab a route. */
-	if (!dst && (dst = inet_csk_route_req(sk, &fl4, req)) == NULL)
-		return -1;
+	if (!dst) {
+		if ((dst = inet_csk_route_req(sk, &fl4, req)) == NULL)
+			return -1;
+
+		tcp_rsk(req)->txhash = get_hash_from_flowi4(&fl4);
+	}
 
 	skb = tcp_make_synack(sk, dst, req, foc, synack_type);
 

@@ -1278,9 +1282,12 @@ static void tcp_v4_init_req(struct request_sock *req,
 
 static struct dst_entry *tcp_v4_route_req(const struct sock *sk,
 					  struct flowi *fl,
-					  const struct request_sock *req)
+					  struct request_sock *req)
 {
-	return inet_csk_route_req(sk, &fl->u.ip4, req);
+	struct dst_entry *dst = inet_csk_route_req(sk, &fl->u.ip4, req);
+	if (dst)
+		tcp_rsk(req)->txhash = get_hash_from_flowi4(&fl->u.ip4);
+	return dst;
 }
 
 struct request_sock_ops tcp_request_sock_ops __read_mostly = {
net/ipv4/tcp_output.c +0 -1
index 540b7d9..f324676 100644
@@ -3750,7 +3750,6 @@ int tcp_rtx_synack(const struct sock *sk, struct request_sock *req)
 	struct flowi fl;
 	int res;
 
-	tcp_rsk(req)->txhash = net_tx_rndhash();
 	res = af_ops->send_synack(sk, NULL, &fl, req, NULL, TCP_SYNACK_NORMAL);
 	if (!res) {
 		__TCP_INC_STATS(sock_net(sk), TCP_MIB_RETRANSSEGS);
net/ipv6/datagram.c +3 -1
index a1f9187..c13286c 100644
@@ -150,6 +150,7 @@ int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
 	int			addr_type;
 	int			err;
 	__be32			fl6_flowlabel = 0;
+	struct flowi6		fl6;
 
 	if (usin->sin6_family == AF_INET) {
 		if (__ipv6_only_sock(sk))

@@ -260,7 +261,8 @@ int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
 	}
 
 	sk->sk_state = TCP_ESTABLISHED;
-	sk_set_txhash(sk);
+	ip6_datagram_flow_key_init(&fl6, sk);
+	sk->sk_txhash = get_hash_from_flowi6(&fl6);
 out:
 	return err;
 }
net/ipv6/syncookies.c +2 -1
index e7a3a6b..3ba9401 100644
@@ -216,7 +216,6 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 	treq->rcv_isn = ntohl(th->seq) - 1;
 	treq->snt_isn = cookie;
 	treq->ts_off = 0;
-	treq->txhash = net_tx_rndhash();
 
 	/*
 	 * We need to lookup the dst_entry to get the correct window size.

@@ -238,6 +237,8 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 		fl6.flowi6_uid = sk->sk_uid;
 		security_req_classify_flow(req, flowi6_to_flowi(&fl6));
 
+		treq->txhash = get_hash_from_flowi6(&fl6);
+
 		dst = ip6_dst_lookup_flow(sk, &fl6, final_p);
 		if (IS_ERR(dst))
 			goto out_free;
net/ipv6/tcp_ipv6.c +13 -5
index 6bb98c9..a1a5802 100644
@@ -286,7 +286,7 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr,
 	if (err)
 		goto late_failure;
 
-	sk_set_txhash(sk);
+	sk->sk_txhash = get_hash_from_flowi6(&fl6);
 
 	if (likely(!tp->repair)) {
 		if (!tp->write_seq)

@@ -470,9 +470,13 @@ static int tcp_v6_send_synack(const struct sock *sk, struct dst_entry *dst,
 	int err = -ENOMEM;
 
 	/* First, grab a route. */
-	if (!dst && (dst = inet6_csk_route_req(sk, fl6, req,
+	if (!dst) {
+		if ((dst = inet6_csk_route_req(sk, fl6, req,
 					       IPPROTO_TCP)) == NULL)
-		goto done;
+			goto done;
+
+		tcp_rsk(req)->txhash = get_hash_from_flowi6(fl6);
+	}
 
 	skb = tcp_make_synack(sk, dst, req, foc, synack_type);
 

@@ -743,9 +747,13 @@ static void tcp_v6_init_req(struct request_sock *req,
 
 static struct dst_entry *tcp_v6_route_req(const struct sock *sk,
 					  struct flowi *fl,
-					  const struct request_sock *req)
+					  struct request_sock *req)
 {
-	return inet6_csk_route_req(sk, &fl->u.ip6, req, IPPROTO_TCP);
+	struct dst_entry *dst;
+	dst = inet6_csk_route_req(sk, &fl->u.ip6, req, IPPROTO_TCP);
+	if (dst)
+		tcp_rsk(req)->txhash = get_hash_from_flowi6(&fl->u.ip6);
+	return dst;
 }
 
 struct request_sock_ops tcp6_request_sock_ops __read_mostly = {
Signature · 1 line
2.9.5
author
#2 lore raw

PATCH 2/2 net-next: copy user configured flowlabel to reset packet

In reply to Shaohua Li

From: Shaohua Li <[email protected]>

Reset packet doesn't use user configured flowlabel, instead, it always uses 0. This will cause inconsistency for flowlabel. tw sock already records flowlabel info, so we can directly use it.

Cc Martin KaFai Lau <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Florent Fourcot <[email protected]>
Cc Cong Wang <[email protected]>
Cc Tom Herbert <[email protected]>
Signed-off-by Shaohua Li <[email protected]>
---
 net/ipv6/tcp_ipv6.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)
net/ipv6/tcp_ipv6.c +17 -1
index a1a5802..9b678cd 100644
@@ -901,6 +901,8 @@ static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb)
 	struct sock *sk1 = NULL;
 #endif
 	int oif = 0;
+	u8 tclass = 0;
+	__be32 flowlabel = 0;
 
 	if (th->rst)
 		return;

@@ -954,7 +956,21 @@ static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb)
 		trace_tcp_send_reset(sk, skb);
 	}
 
-	tcp_v6_send_response(sk, skb, seq, ack_seq, 0, 0, 0, oif, key, 1, 0, 0);
+	if (sk) {
+		if (sk_fullsock(sk)) {
+			struct ipv6_pinfo *np = inet6_sk(sk);
+
+			tclass = np->tclass;
+			flowlabel = np->flow_label & IPV6_FLOWLABEL_MASK;
+		} else {
+			struct inet_timewait_sock *tw = inet_twsk(sk);
+
+			tclass = tw->tw_tclass;
+			flowlabel = cpu_to_be32(tw->tw_flowlabel);
+		}
+	}
+	tcp_v6_send_response(sk, skb, seq, ack_seq, 0, 0, 0, oif, key, 1,
+		tclass, flowlabel);
 
 #ifdef CONFIG_TCP_MD5SIG
 out:
Signature · 1 line
2.9.5
maintainer
#3 lore raw
In reply to Shaohua Li

From: Shaohua Li <[email protected]>
Date: Fri, 17 Nov 2017 10:24:49 -0800

Quoted 5 lines

From: Shaohua Li <[email protected]>

Hi,

Please see below tcpdump output:

...

Quoted 7 lines

The tcp reset packet has a different flowlabel, which causes our router doesn't correctly close tcp connection. We are using flowlabel to do load balance. Routers in the path maintain connection state. So if flow label changes, the packet is routed through a different router. In this case, the old router doesn't get the reset packet to close the tcp connection.

Quoted 6 lines

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.

The patches fix the issue.

Please resubmit this when the net-next tree opens back up as it is closed right now, thank you.