Skip to contentSkip to conversation

something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

DISCUSSION Krzysztof Kolasa 5 participants 25 messages Oct 12, 2014 – Oct 17, 2014 1 message not in the archive

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

Krzysztof Kolasa

author
#0 lore raw

This commit is the result of bisect my problem with window decoration in gnome shell ( button, icons, colors etc.)

after revert this commit I don't have any problem with themes windows

a little impossible, commit to the network and affect the graphics might be a problem with the allocation and release of memory

my system: 64bit Ubuntu 12.04.5 LTS, kernel next

latest AMD fglrx driver 14.301 ( problem tested and exist in 14.200 also )

Regards,

Krzysztof

#1 lore raw
Quoted 8 lines · Krzysztof Kolasa

This commit is the result of bisect my problem with window decoration in gnome shell ( button, icons, colors etc.)

after revert this commit I don't have any problem with themes windows

a little impossible, commit to the network and affect the graphics might be a problem with the allocation and release of memory

Quoted 7 lines

my system: 64bit Ubuntu 12.04.5 LTS, kernel next

latest AMD fglrx driver 14.301 ( problem tested and exist in 14.200 also )

Regards,

Krzysztof

Hi Krzysztof

This sounds quite strange to me. Some memory corruption might happen regardless of networking changes, and this commit could uncover an existing bug.

You might try different SLUB/SLAB choice, and try different debugging SLUB/SLAB facilities.

With SLUB, skbuff_fclone_cache shares :t-0000512 kmem cache. You could try slub_nomerge=1 for a start.

#2 lore raw

Probably not related with this bug, but with regarding to the offending commit, what's the point of the memmove() in tcp_v4_rcv() since ip_rcv() already clears IPCB()?

#3 lore raw
Quoted 3 lines · Cong Wang

Probably not related with this bug, but with regarding to the offending commit, what's the point of the memmove() in tcp_v4_rcv() since ip_rcv() already clears IPCB()?

Oh, ip options are actually saved in ip_rcv_finish()... Hmm, looks scary to play with variable-length array with memmove()....

author
#4 lore raw

W dniu 14.10.2014 o 02:09, Cong Wang pisze:

Quoted 7 lines · Cong Wang
Quoted 3 lines

Probably not related with this bug, but with regarding to the offending commit, what's the point of the memmove() in tcp_v4_rcv() since ip_rcv() already clears IPCB()?

Oh, ip options are actually saved in ip_rcv_finish()... Hmm, looks scary to play with variable-length array with memmove()....

On my other old laptop with 32bit kernel next and graphics card Intel 945GM just after the revert commit working OK, before, after login to gnome shell in some seconds decorations disappear

32 bit Ubuntu 12.04.5 LTS, gnome shell, kernel source next 14-10-2014

Can anyone confirm this ?

#5 lore raw
Quoted 11 lines · Krzysztof Kolasa

W dniu 14.10.2014 o 02:09, Cong Wang pisze:

Quoted 9 lines · Cong Wang
Quoted 4 lines

Probably not related with this bug, but with regarding to the offending commit, what's the point of the memmove() in tcp_v4_rcv() since ip_rcv() already clears IPCB()?

Oh, ip options are actually saved in ip_rcv_finish()... Hmm, looks scary to play with variable-length array with memmove()....

Quoted 8 lines

On my other old laptop with 32bit kernel next and graphics card Intel 945GM just after the revert commit working OK,
before, after login to gnome shell in some seconds decorations disappear

32 bit Ubuntu 12.04.5 LTS, gnome shell, kernel source next 14-10-2014

Can anyone confirm this ?

Sorry, believe it or not, for me it is hard to find a 32bit machine even VM. :)

Could the attached patch by any chance help? I noticed cookie_v4_check() still uses IPCB() instead of TCPCB() at TCP layer.

Thanks.

include/net/tcp.h +2 -2
index 74efeda..5d11012 100644
@@ -468,8 +468,7 @@ void inet_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb);
 /* From syncookies.c */
 int __cookie_v4_check(const struct iphdr *iph, const struct tcphdr *th,
 		      u32 cookie);
-struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
-			     struct ip_options *opt);
+struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb);
 #ifdef CONFIG_SYN_COOKIES
 
 /* Syncookies use a monotonic timer which increments every 60 seconds.

@@ -530,6 +529,7 @@ void tcp_send_active_reset(struct sock *sk, gfp_t priority);
 int tcp_send_synack(struct sock *);
 bool tcp_syn_flood_action(struct sock *sk, const struct sk_buff *skb,
 			  const char *proto);
+struct ip_options_rcu *tcp_v4_save_options(struct sk_buff *skb);
 void tcp_push_one(struct sock *, unsigned int mss_now);
 void tcp_send_ack(struct sock *sk);
 void tcp_send_delayed_ack(struct sock *sk);
net/ipv4/syncookies.c +4 -12
index 0431a8f..d346303 100644
@@ -255,9 +255,9 @@ bool cookie_check_timestamp(struct tcp_options_received *tcp_opt,
 }
 EXPORT_SYMBOL(cookie_check_timestamp);
 
-struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
-			     struct ip_options *opt)
+struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 {
+	struct ip_options *opt = &TCP_SKB_CB(skb)->header.h4.opt;
 	struct tcp_options_received tcp_opt;
 	struct inet_request_sock *ireq;
 	struct tcp_request_sock *treq;

@@ -317,15 +317,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
 	/* We throwed the options of the initial SYN away, so we hope
 	 * the ACK carries the same options again (see RFC1122 4.2.3.8)
 	 */
-	if (opt && opt->optlen) {
-		int opt_size = sizeof(struct ip_options_rcu) + opt->optlen;
-
-		ireq->opt = kmalloc(opt_size, GFP_ATOMIC);
-		if (ireq->opt != NULL && ip_options_echo(&ireq->opt->opt, skb)) {
-			kfree(ireq->opt);
-			ireq->opt = NULL;
-		}
-	}
+	ireq->opt = tcp_v4_save_options(skb);
 
 	if (security_inet_conn_request(sk, skb, req)) {
 		reqsk_free(req);

@@ -344,7 +336,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
 	flowi4_init_output(&fl4, sk->sk_bound_dev_if, ireq->ir_mark,
 			   RT_CONN_FLAGS(sk), RT_SCOPE_UNIVERSE, IPPROTO_TCP,
 			   inet_sk_flowi_flags(sk),
-			   (opt && opt->srr) ? opt->faddr : ireq->ir_rmt_addr,
+			   opt->srr ? opt->faddr : ireq->ir_rmt_addr,
 			   ireq->ir_loc_addr, th->source, th->dest);
 	security_req_classify_flow(req, flowi4_to_flowi(&fl4));
 	rt = ip_route_output_key(sock_net(sk), &fl4);
net/ipv4/tcp_ipv4.c +2 -2
index 552e87e..e02d586 100644
@@ -883,7 +883,7 @@ EXPORT_SYMBOL(tcp_syn_flood_action);
 /*
  * Save and compile IPv4 options into the request_sock if needed.
  */
-static struct ip_options_rcu *tcp_v4_save_options(struct sk_buff *skb)
+struct ip_options_rcu *tcp_v4_save_options(struct sk_buff *skb)
 {
 	const struct ip_options *opt = &TCP_SKB_CB(skb)->header.h4.opt;
 	struct ip_options_rcu *dopt = NULL;

@@ -1428,7 +1428,7 @@ static struct sock *tcp_v4_hnd_req(struct sock *sk, struct sk_buff *skb)
 
 #ifdef CONFIG_SYN_COOKIES
 	if (!th->syn)
-		sk = cookie_v4_check(sk, skb, &TCP_SKB_CB(skb)->header.h4.opt);
+		sk = cookie_v4_check(sk, skb);
 #endif
 	return sk;
 }
maintainer
#6 lore raw

From: Cong Wang <[email protected]>
Date: Tue, 14 Oct 2014 11:59:25 -0700

Quoted 18 lines · Krzysztof Kolasa
Quoted 17 lines

W dniu 14.10.2014 o 02:09, Cong Wang pisze:

Quoted 9 lines · Cong Wang
Quoted 4 lines

Probably not related with this bug, but with regarding to the offending commit, what's the point of the memmove() in tcp_v4_rcv() since ip_rcv() already clears IPCB()?

Oh, ip options are actually saved in ip_rcv_finish()... Hmm, looks scary to play with variable-length array with memmove()....

On my other old laptop with 32bit kernel next and graphics card Intel 945GM just after the revert commit working OK,
before, after login to gnome shell in some seconds decorations disappear

32 bit Ubuntu 12.04.5 LTS, gnome shell, kernel source next 14-10-2014

Quoted 7 lines
Quoted 2 lines

Can anyone confirm this ?

Sorry, believe it or not, for me it is hard to find a 32bit machine even VM. :)

Could the attached patch by any chance help? I noticed cookie_v4_check() still uses IPCB() instead of TCPCB() at TCP layer.

Eric, please review.

maintainer
#8 lore raw
In reply to a message not in the archive

From: Eric Dumazet <[email protected]>
Date: Tue, 14 Oct 2014 13:42:01 -0700

Quoted 36 lines

2014-10-14 12:03 GMT-07:00 David Miller <[email protected]>:

Quoted 34 lines

From: Cong Wang <[email protected]>
Date: Tue, 14 Oct 2014 11:59:25 -0700

Quoted 1 line

On Tue, Oct 14, 2014 at 6:25 AM, Krzysztof Kolasa <[email protected]>

Quoted 3 lines · wrote:
Quoted 3 lines

W dniu 14.10.2014 o 02:09, Cong Wang pisze:

Quoted 1 line

On Mon, Oct 13, 2014 at 4:59 PM, Cong Wang <[email protected]>

Quoted 6 lines · wrote:
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines

Probably not related with this bug, but with regarding to the offending commit, what's the point of the memmove() in tcp_v4_rcv() since ip_rcv() already clears IPCB()?

Oh, ip options are actually saved in ip_rcv_finish()... Hmm, looks

scary

Quoted 3 lines
Quoted 3 lines
Quoted 2 lines

to play with variable-length array with memmove()....

On my other old laptop with 32bit kernel next and graphics card Intel

945GM

Quoted 9 lines
Quoted 7 lines

just after the revert commit working OK,
before, after login to gnome shell in some seconds decorations disappear

32 bit Ubuntu 12.04.5 LTS, gnome shell, kernel source next 14-10-2014

Can anyone confirm this ?

Sorry, believe it or not, for me it is hard to find a 32bit machine even

VM. :)

Quoted 3 lines

Could the attached patch by any chance help? I noticed cookie_v4_check() still uses IPCB() instead of TCPCB() at TCP layer.

Quoted 8 lines
Quoted 2 lines

Eric, please review.

For an unknown reason I do not see this patch on my [email protected]

It seems my migration to Trusty was not a good move :(

Patch looks good, but seems a net-next candidate to me.

I thought it is a potential fix for this corruption report?

#9 lore raw

2014-10-14 13:47 GMT-07:00 David Miller <[email protected]>:

Quoted 5 lines
Quoted 3 lines

Patch looks good, but seems a net-next candidate to me.

I thought it is a potential fix for this corruption report?

Sure, but a one liner fix seemed more obvious, as it points to the problem.

I do not particularly care, as long as we fixed the bug.

Thanks !

maintainer
#10 lore raw

From: Eric Dumazet <[email protected]>
Date: Tue, 14 Oct 2014 14:03:51 -0700

Quoted 11 lines

2014-10-14 13:47 GMT-07:00 David Miller <[email protected]>:

Quoted 5 lines
Quoted 3 lines

Patch looks good, but seems a net-next candidate to me.

I thought it is a potential fix for this corruption report?

Sure, but a one liner fix seemed more obvious, as it points to the problem.

I do not particularly care, as long as we fixed the bug.

Ok, let's get testing feedback and then I'll look for a formal submission.

#11 lore raw
Quoted 14 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Tue, 14 Oct 2014 14:03:51 -0700

Quoted 11 lines

2014-10-14 13:47 GMT-07:00 David Miller <[email protected]>:

Quoted 5 lines
Quoted 3 lines

Patch looks good, but seems a net-next candidate to me.

I thought it is a potential fix for this corruption report?

Sure, but a one liner fix seemed more obvious, as it points to the problem.

I do not particularly care, as long as we fixed the bug.

Since we are still in merge window, I don't think we have to use a one-line fix for a bug introduced in this merge window.

Quoted 2 lines

Ok, let's get testing feedback and then I'll look for a formal submission.

Yeah, there is no evidence to say my patch fixes the bug Krzysztof reported before he tests it, I found it during code review.

Thanks!

#12 lore raw
Quoted 3 lines · Cong Wang

Since we are still in merge window, I don't think we have to use a one-line fix for a bug introduced in this merge window.

You are clearly refactoring here. Its a nice cleanup.

If I was the maintainer, I would prefer the one line fix.

Then when net-next is open, you refactor.

As I said, I wont argue, do whatever you want.

Thanks

author
#13 lore raw

W dniu 14.10.2014 o 23:43, Eric Dumazet pisze:

Quoted 6 lines · Cong Wang
Quoted 3 lines

Since we are still in merge window, I don't think we have to use a one-line fix for a bug introduced in this merge window.

You are clearly refactoring here. Its a nice cleanup.

Quoted 10 lines

If I was the maintainer, I would prefer the one line fix.

Then when net-next is open, you refactor.

As I said, I wont argue, do whatever you want.

Thanks

one-line patch not resolve problem, fix created by Cong Wang resolves problem !!!

tested on 64bit system and working OK

#14 lore raw
Quoted 13 lines · Krzysztof Kolasa

W dniu 14.10.2014 o 23:43, Eric Dumazet pisze:

Quoted 12 lines · Cong Wang
Quoted 3 lines

Since we are still in merge window, I don't think we have to use a one-line fix for a bug introduced in this merge window.

You are clearly refactoring here. Its a nice cleanup.

If I was the maintainer, I would prefer the one line fix.

Then when net-next is open, you refactor.

As I said, I wont argue, do whatever you want.

Quoted 7 lines
Quoted 4 lines

Thanks

one-line patch not resolve problem, fix created by Cong Wang resolves problem !!!

Hmm, there should be no difference with either patch.

tcp_v4_rcv()
...
memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),

        sizeof(struct inet_skb_parm));
...
-> tcp_v4_do_rcv() 
   ->  tcp_v4_hnd_req()
      -> cookie_v4_check(... , &TCP_SKB_CB(skb)->header.h4.opt)

Hmm...

author
#15 lore raw

W dniu 15.10.2014 o 13:27, Eric Dumazet pisze:

Quoted 25 lines · Krzysztof Kolasa
Quoted 11 lines

W dniu 14.10.2014 o 23:43, Eric Dumazet pisze:

Quoted 10 lines · Cong Wang
Quoted 3 lines

Since we are still in merge window, I don't think we have to use a one-line fix for a bug introduced in this merge window.

You are clearly refactoring here. Its a nice cleanup.

If I was the maintainer, I would prefer the one line fix.

Then when net-next is open, you refactor.

Quoted 8 lines
Quoted 6 lines

As I said, I wont argue, do whatever you want.

Thanks

one-line patch not resolve problem, fix created by Cong Wang resolves problem !!!

Hmm, there should be no difference with either patch.

tcp_v4_rcv()
...
memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),

Quoted 9 lines
         sizeof(struct inet_skb_parm));
...
-> tcp_v4_do_rcv()
    ->  tcp_v4_hnd_req()
       -> cookie_v4_check(... , &TCP_SKB_CB(skb)->header.h4.opt)

Hmm...

on a 32bit system, the patch did not solve the problem :( I have exactly the same problem as before the patch

I do not understand this, perhaps the problem is hidden somewhere else, one thing is certain after revert commit 971f10eca1 everything works correctly

#16 lore raw
Quoted 2 lines · Krzysztof Kolasa

on a 32bit system, the patch did not solve the problem :( I have exactly the same problem as before the patch

You mean Cong patch ?

Quoted 5 lines

I do not understand this, perhaps the problem is hidden somewhere else, one thing is certain after revert commit 971f10eca1 everything works correctly

Can you privately send me the vmlinux file ?

It might be a compiler issue...

author
#17 lore raw

W dniu 15.10.2014 o 17:32, Eric Dumazet pisze:

Quoted 6 lines · Krzysztof Kolasa
Quoted 3 lines

on a 32bit system, the patch did not solve the problem :( I have exactly the same problem as before the patch

You mean Cong patch ?

Yes

Quoted 11 lines
Quoted 5 lines

I do not understand this, perhaps the problem is hidden somewhere else, one thing is certain after revert commit 971f10eca1 everything works correctly

Can you privately send me the vmlinux file ?

It might be a compiler issue...

I sent the file

#18 lore raw
In reply to Krzysztof Kolasa
Quoted 6 lines · Krzysztof Kolasa

one-line patch not resolve problem, fix created by Cong Wang resolves problem !!!

tested on 64bit system and working OK

So my patch fixes your problem on 64bit but not on 32bit, is this correct?

I will send out the patch. And as Eric said, the problem on 32bit might be a different issue, we can definitely fix it separately.

Thanks for testing!

author
#19 lore raw

W dniu 15.10.2014 o 22:36, Cong Wang pisze:

Quoted 8 lines · Krzysztof Kolasa
Quoted 5 lines

one-line patch not resolve problem, fix created by Cong Wang resolves problem !!!

tested on 64bit system and working OK

So my patch fixes your problem on 64bit but not on 32bit, is this correct?

Yes

#20 lore raw
Quoted 8 lines · Krzysztof Kolasa

W dniu 15.10.2014 o 22:36, Cong Wang pisze:

Quoted 7 lines
Quoted 4 lines · Krzysztof Kolasa

one-line patch not resolve problem, fix created by Cong Wang resolves problem !!!

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

tested on 64bit system and working OK

So my patch fixes your problem on 64bit but not on 32bit, is this correct?

Yes

Cool! Patches are coming.

Meanwhile Eric is debugging it, do you mind to try a followup quick fix on your 32bit system?

net/ipv4/tcp_output.c +0 -0
index e13d778..12bd3f6 100644
@@ -1006,8 +1006,7 @@ static int tcp_transmit_skb(struct sock *sk,

struct sk_buff *skb, int clone_it,

        skb->tstamp.tv64 = 0;

        /* Cleanup our debris for IP stacks */

- memset(skb->cb, 0, max(sizeof(struct inet_skb_parm),
- sizeof(struct inet6_skb_parm)));
+ memset(TCPCB(skb), 0, sizeof(*TCPCB(skb)));

        err = icsk->icsk_af_ops->queue_xmit(sk, skb, &inet->cork.fl);
#21 lore raw
Quoted 9 lines · Cong Wang

Meanwhile Eric is debugging it, do you mind to try a followup quick fix on your 32bit system?

net/ipv4/tcp_output.c +0 -0
index e13d778..12bd3f6 100644

net/ipv4/tcp_output.c static int tcp_transmit_skb(struct sock *sk, · line 1010

Quoted 7 lines
net/ipv4/tcp_output.c +1 -2
@@ -1006,8 +1006,7 @@ static int tcp_transmit_skb(struct sock *sk,
 struct sk_buff *skb, int clone_it,
        skb->tstamp.tv64 = 0;
 
        /* Cleanup our debris for IP stacks */
-       memset(skb->cb, 0, max(sizeof(struct inet_skb_parm),
-                              sizeof(struct inet6_skb_parm)));
+       memset(TCPCB(skb), 0, sizeof(*TCPCB(skb)));

Of course, s/TCPCB/TCP_SKB_CB/. :)

Quoted 2 lines
net/ipv4/tcp_output.c +0 -0
        err = icsk->icsk_af_ops->queue_xmit(sk, skb, &inet->cork.fl);
#22 lore raw
In reply to Cong Wang
Quoted 8 lines · Cong Wang
Quoted 6 lines · Krzysztof Kolasa

one-line patch not resolve problem, fix created by Cong Wang resolves problem !!!

tested on 64bit system and working OK

Quoted 7 lines

So my patch fixes your problem on 64bit but not on 32bit, is this correct?

I will send out the patch. And as Eric said, the problem on 32bit might be a different issue, we can definitely fix it separately.

Thanks for testing!

I think more fixes are needed.

This is most probably an IPv6 issue.

#23 lore raw
Quoted 12 lines · Eric Dumazet
Quoted 11 lines · Cong Wang
Quoted 6 lines · Krzysztof Kolasa

one-line patch not resolve problem, fix created by Cong Wang resolves problem !!!

tested on 64bit system and working OK

So my patch fixes your problem on 64bit but not on 32bit, is this correct?

Quoted 9 lines
Quoted 4 lines

I will send out the patch. And as Eric said, the problem on 32bit might be a different issue, we can definitely fix it separately.

Thanks for testing!

I think more fixes are needed.

This is most probably an IPv6 issue.

Yes, ipv6 uses inet6_iif() a lot. We need to use a different accessor.

maintainer
#24 lore raw

From: Eric Dumazet <[email protected]>
Date: Wed, 15 Oct 2014 14:42:34 -0700

Quoted 16 lines · Eric Dumazet
Quoted 15 lines · Cong Wang
Quoted 14 lines · Krzysztof Kolasa
Quoted 6 lines

one-line patch not resolve problem, fix created by Cong Wang resolves problem !!!

tested on 64bit system and working OK

So my patch fixes your problem on 64bit but not on 32bit, is this correct?

I will send out the patch. And as Eric said, the problem on 32bit might be a different issue, we can definitely fix it separately.

Quoted 8 lines
Quoted 6 lines
Quoted 1 line

Thanks for testing!

I think more fixes are needed.

This is most probably an IPv6 issue.

Yes, ipv6 uses inet6_iif() a lot. We need to use a different accessor.

So I applied Cong's patches, but if I am to understand the testing feedback so far only the 64-bit case of the reporter's problems is fixed, and that the 32-bit graphics corruption still happens.

Can I get some clarification of where we stand after Cong's fixes which are already merged into 'net'?

Thanks.

#25 lore raw
Quoted 6 lines · David Miller

So I applied Cong's patches, but if I am to understand the testing feedback so far only the 64-bit case of the reporter's problems is fixed, and that the 32-bit graphics corruption still happens.

Can I get some clarification of where we stand after Cong's fixes which are already merged into 'net'?

I spent some time on the (awful) memmove() implementation on x86_32, and could not find a bug in it.

It would be worth trying doing 2 memcpy() instead just in case...