Skip to contentSkip to conversation

tcp/dccp: block bh before arming time_wait timer

PATCH In review Eric Dumazet 3 participants 4 messages Dec 1, 2017

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

Eric Dumazet

author
#0 lore raw

PATCH 1/1 tcp/dccp: block bh before arming time_wait timer

From: Eric Dumazet <[email protected]>

Maciej Żenczykowski reported some panics in tcp_twsk_destructor() that might be caused by the following bug.

timewait timer is pinned to the cpu, because we want to transition timwewait refcount from 0 to 4 in one go, once everything has been initialized.

At the time commit ed2e92394589 ("tcp/dccp: fix timewait races in timer handling") was merged, TCP was always running from BH habdler.

After commit 5413d1babe8f ("net: do not block BH while processing socket backlog") we definitely can run tcp_time_wait() from process context.

We need to block BH in the critical section so that the pinned timer has still its purpose.

This bug is more likely to happen under stress and when very small RTO are used in datacenter flows.

Fixes 5413d1babe8f ("net: do not block BH while processing socket backlog")
Signed-off-by Eric Dumazet <[email protected]>
Reported-by Maciej Żenczykowski <[email protected]>

---
 net/dccp/minisocks.c     |    6 ++++++
 net/ipv4/tcp_minisocks.c |    6 ++++++

 2 files changed, 12 insertions(+)
net/dccp/minisocks.c +6 -0
index abd07a443219853b022bef41cb072e90ff8f07f0..178bb9833311f83205317b07fe64cb2e45a9f734 100644
@@ -57,10 +57,16 @@ void dccp_time_wait(struct sock *sk, int state, int timeo)
 		if (state == DCCP_TIME_WAIT)
 			timeo = DCCP_TIMEWAIT_LEN;
 
+		/* tw_timer is pinned, so we need to make sure BH are disabled
+		 * in following section, otherwise timer handler could run before
+		 * we complete the initialization.
+		 */
+		local_bh_disable();
 		inet_twsk_schedule(tw, timeo);
 		/* Linkage updates. */
 		__inet_twsk_hashdance(tw, sk, &dccp_hashinfo);
 		inet_twsk_put(tw);
+		local_bh_enable();
 	} else {
 		/* Sorry, if we're out of memory, just CLOSE this
 		 * socket up.  We've got bigger problems than
net/ipv4/tcp_minisocks.c +6 -0
index e36eff0403f4e80c4f7291a70614f40125652133..b079b619b60ca577d5ef20a5065fce87acecd96c 100644
@@ -310,10 +310,16 @@ void tcp_time_wait(struct sock *sk, int state, int timeo)
 		if (state == TCP_TIME_WAIT)
 			timeo = TCP_TIMEWAIT_LEN;
 
+		/* tw_timer is pinned, so we need to make sure BH are disabled
+		 * in following section, otherwise timer handler could run before
+		 * we complete the initialization.
+		 */
+		local_bh_disable();
 		inet_twsk_schedule(tw, timeo);
 		/* Linkage updates. */
 		__inet_twsk_hashdance(tw, sk, &tcp_hashinfo);
 		inet_twsk_put(tw);
+		local_bh_enable();
 	} else {
 		/* Sorry, if we're out of memory, just CLOSE this
 		 * socket up.  We've got bigger problems than
maintainer
#2 lore raw
In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Fri, 01 Dec 2017 10:06:56 -0800

Quoted 19 lines

From: Eric Dumazet <[email protected]>

Maciej Żenczykowski reported some panics in tcp_twsk_destructor() that might be caused by the following bug.

timewait timer is pinned to the cpu, because we want to transition timwewait refcount from 0 to 4 in one go, once everything has been initialized.

At the time commit ed2e92394589 ("tcp/dccp: fix timewait races in timer handling") was merged, TCP was always running from BH habdler.

After commit 5413d1babe8f ("net: do not block BH while processing socket backlog") we definitely can run tcp_time_wait() from process context.

We need to block BH in the critical section so that the pinned timer has still its purpose.

Quoted 6 lines

This bug is more likely to happen under stress and when very small RTO are used in datacenter flows.

Fixes 5413d1babe8f ("net: do not block BH while processing socket backlog")
Signed-off-by Eric Dumazet <[email protected]>
Reported-by Maciej Żenczykowski <[email protected]>

Applied and queued up for -stable, thanks Eric.

author
#3 lore raw
Quoted 26 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Fri, 01 Dec 2017 10:06:56 -0800

Quoted 10 lines

From: Eric Dumazet <[email protected]>

Maciej Żenczykowski reported some panics in tcp_twsk_destructor() that might be caused by the following bug.

timewait timer is pinned to the cpu, because we want to transition timwewait refcount from 0 to 4 in one go, once everything has been initialized.

At the time commit ed2e92394589 ("tcp/dccp: fix timewait races in

timer

Quoted 7 lines

handling") was merged, TCP was always running from BH habdler.
After commit 5413d1babe8f ("net: do not block BH while processing socket backlog") we definitely can run tcp_time_wait() from process context.

We need to block BH in the critical section so that the pinned

timer

Quoted 3 lines

has still its purpose.

This bug is more likely to happen under stress and when very small

RTO

Quoted 8 lines
Quoted 3 lines

are used in datacenter flows.

Fixes 5413d1babe8f ("net: do not block BH while processing socket

backlog")

Quoted 2 lines
Signed-off-by Eric Dumazet <[email protected]>
Reported-by Maciej Żenczykowski <[email protected]>

Applied and queued up for -stable, thanks Eric.

It just occurred to me that we can now revert 614bdd4d6e61d26 ("tcp: must block bh in __inet_twsk_hashdance()")