Skip to contentSkip to conversation

Regression with commit 971f10eca186 ("tcp: better TCP_SKB_CB layout to reduce cache line misses")

PATCH In review Nicolas Dichtel 3 participants 5 messages Dec 22, 2014

Subsystems (3)
Networking Network drivers TCP
lore
Share & tools
lore

Nicolas Dichtel

author
#0 lore raw

One of our engineer (Huaibin Wang <[email protected]>) has reported and analysed this bug:

This commit introduces a regression with IPv6 + IPsec transport + TCP.

In TCP (net/ipv6/tcp_ipv6.c), xfrm6_policy_check() is called and thus, after some intermediate functions, _decode_session6() is also called.

This function uses IP6CB() (u8 nexthdr = nh[IP6CB(skb)->nhoff]), which is wrong becauses it has been moved to the end of TCP_SKB_CB().

Not sure what is the best way to fix this, any suggestion?

Regards,
Nicolas

#1 lore raw
Quoted 5 lines · Nicolas Dichtel

One of our engineer (Huaibin Wang <[email protected]>) has reported and analysed this bug:

This commit introduces a regression with IPv6 + IPsec transport + TCP.

Quoted 7 lines

In TCP (net/ipv6/tcp_ipv6.c), xfrm6_policy_check() is called and thus, after some intermediate functions, _decode_session6() is also called.

This function uses IP6CB() (u8 nexthdr = nh[IP6CB(skb)->nhoff]), which is wrong becauses it has been moved to the end of TCP_SKB_CB().

Not sure what is the best way to fix this, any suggestion?

Thanks for the report

Presumably tcp_v6_rcv() needs to be reordered so that xfrm6_policy_check() calls are done before the CB swap.

swap should probably be done right before bh_lock_sock_nested()

I am currently traveling and I am not sure if I can get Internet access to post a patch soon.

author
#2 lore raw
Quoted 16 lines · Eric Dumazet
Quoted 5 lines · Nicolas Dichtel

One of our engineer (Huaibin Wang <[email protected]>) has reported and analysed this bug:

This commit introduces a regression with IPv6 + IPsec transport + TCP.

Quoted 7 lines

In TCP (net/ipv6/tcp_ipv6.c), xfrm6_policy_check() is called and thus, after some intermediate functions, _decode_session6() is also called.

This function uses IP6CB() (u8 nexthdr = nh[IP6CB(skb)->nhoff]), which is wrong becauses it has been moved to the end of TCP_SKB_CB().

Not sure what is the best way to fix this, any suggestion?

Thanks for the report

Quoted 7 lines

Presumably tcp_v6_rcv() needs to be reordered so that xfrm6_policy_check() calls are done before the CB swap.

swap should probably be done right before bh_lock_sock_nested()

I am currently traveling and I am not sure if I can get Internet access to post a patch soon.

Ok, thank you for the tip. I will try to cook a patch.

author
#3 lore raw

PATCH 1/1 tcp6: don't move IP6CB before xfrm6_policy_check()

In reply to Eric Dumazet

When xfrm6_policy_check() is used, _decode_session6() is called after some intermediate functions. This function uses IP6CB(), thus TCP_SKB_CB() must be prepared after the call of xfrm6_policy_check().

Before this patch, scenarii with IPv6 + TCP + IPsec Transport are broken.

Fixes 971f10eca186 ("tcp: better TCP_SKB_CB layout to reduce cache line misses")
Reported-by Huaibin Wang <[email protected]>
Suggested-by Eric Dumazet <[email protected]>
Signed-off-by Nicolas Dichtel <[email protected]>
---
 net/ipv6/tcp_ipv6.c | 45 +++++++++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 16 deletions(-)
net/ipv6/tcp_ipv6.c +29 -16
index 5ff87805258e..9c0b54e87b47 100644
@@ -1387,6 +1387,28 @@ ipv6_pktoptions:
 	return 0;
 }
 
+static void tcp_v6_fill_cb(struct sk_buff *skb, const struct ipv6hdr *hdr,
+			   const struct tcphdr *th)
+{
+	/* This is tricky: we move IP6CB at its correct location into
+	 * TCP_SKB_CB(). It must be done after xfrm6_policy_check(), because
+	 * _decode_session6() uses IP6CB().
+	 * barrier() makes sure compiler won't play aliasing games.
+	 */
+	memmove(&TCP_SKB_CB(skb)->header.h6, IP6CB(skb),
+		sizeof(struct inet6_skb_parm));
+	barrier();
+
+	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
+	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
+				    skb->len - th->doff*4);
+	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
+	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
+	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
+	TCP_SKB_CB(skb)->ip_dsfield = ipv6_get_dsfield(hdr);
+	TCP_SKB_CB(skb)->sacked = 0;
+}
+
 static int tcp_v6_rcv(struct sk_buff *skb)
 {
 	const struct tcphdr *th;

@@ -1418,24 +1440,9 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 
 	th = tcp_hdr(skb);
 	hdr = ipv6_hdr(skb);
-	/* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
-	 * barrier() makes sure compiler wont play fool^Waliasing games.
-	 */
-	memmove(&TCP_SKB_CB(skb)->header.h6, IP6CB(skb),
-		sizeof(struct inet6_skb_parm));
-	barrier();
-
-	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
-	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
-				    skb->len - th->doff*4);
-	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
-	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
-	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
-	TCP_SKB_CB(skb)->ip_dsfield = ipv6_get_dsfield(hdr);
-	TCP_SKB_CB(skb)->sacked = 0;
 
 	sk = __inet6_lookup_skb(&tcp_hashinfo, skb, th->source, th->dest,
-				tcp_v6_iif(skb));
+				inet6_iif(skb));
 	if (!sk)
 		goto no_tcp_socket;
 

@@ -1451,6 +1458,8 @@ process:
 	if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
 		goto discard_and_relse;
 
+	tcp_v6_fill_cb(skb, hdr, th);
+
 #ifdef CONFIG_TCP_MD5SIG
 	if (tcp_v6_inbound_md5_hash(sk, skb))
 		goto discard_and_relse;

@@ -1482,6 +1491,8 @@ no_tcp_socket:
 	if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb))
 		goto discard_it;
 
+	tcp_v6_fill_cb(skb, hdr, th);
+
 	if (skb->len < (th->doff<<2) || tcp_checksum_complete(skb)) {
 csum_error:
 		TCP_INC_STATS_BH(net, TCP_MIB_CSUMERRORS);

@@ -1505,6 +1516,8 @@ do_time_wait:
 		goto discard_it;
 	}
 
+	tcp_v6_fill_cb(skb, hdr, th);
+
 	if (skb->len < (th->doff<<2)) {
 		inet_twsk_put(inet_twsk(sk));
 		goto bad_packet;
Signature · 1 line
2.1.0
maintainer
#4 lore raw

Re: [PATCH net] tcp6: don't move IP6CB before xfrm6_policy_check()

From: Nicolas Dichtel <[email protected]>
Date: Mon, 22 Dec 2014 18:22:48 +0100

Quoted 10 lines

When xfrm6_policy_check() is used, _decode_session6() is called after some intermediate functions. This function uses IP6CB(), thus TCP_SKB_CB() must be prepared after the call of xfrm6_policy_check().

Before this patch, scenarii with IPv6 + TCP + IPsec Transport are broken.

Fixes 971f10eca186 ("tcp: better TCP_SKB_CB layout to reduce cache line misses")
Reported-by Huaibin Wang <[email protected]>
Suggested-by Eric Dumazet <[email protected]>
Signed-off-by Nicolas Dichtel <[email protected]>

Applied and queued up for -stable, thanks everyone.