Fix a callout race condition introduced in TCP timers callouts with r281599.

In TCP timer context, it is not enough to check callout_stop() return value
to decide if a callout is still running or not, previous callout_reset()
return values have also to be checked.

Differential Revision:	https://reviews.freebsd.org/D2763
Reviewed by:		hiren
Approved by:		hiren
MFC after:		1 day
Sponsored by:		Verisign, Inc.

2 files changed, 45 insertions, 16 deletions

diff --git a/sys/netinet/tcp_timer.c b/sys/netinet/tcp_timer.c
index a64e403..d992492 100644
--- a/sys/netinet/tcp_timer.c
+++ b/sys/netinet/tcp_timer.c
@@ -347,11 +347,12 @@ tcp_timer_2msl(void *xtp)
 		tp = tcp_close(tp);             
 	} else {
 		if (tp->t_state != TCPS_TIME_WAIT &&
-		   ticks - tp->t_rcvtime <= TP_MAXIDLE(tp))
-		       callout_reset_on(&tp->t_timers->tt_2msl,
-			   TP_KEEPINTVL(tp), tcp_timer_2msl, tp,
-			   inp_to_cpuid(inp));
-	       else
+		   ticks - tp->t_rcvtime <= TP_MAXIDLE(tp)) {
+			if (!callout_reset(&tp->t_timers->tt_2msl,
+			   TP_KEEPINTVL(tp), tcp_timer_2msl, tp)) {
+				tp->t_timers->tt_flags &= ~TT_2MSL_RST;
+			}
+		} else
 		       tp = tcp_close(tp);
        }
 
@@ -431,11 +432,14 @@ tcp_timer_keep(void *xtp)
 				    tp->rcv_nxt, tp->snd_una - 1, 0);
 			free(t_template, M_TEMP);
 		}
-		callout_reset_on(&tp->t_timers->tt_keep, TP_KEEPINTVL(tp),
-		    tcp_timer_keep, tp, inp_to_cpuid(inp));
-	} else
-		callout_reset_on(&tp->t_timers->tt_keep, TP_KEEPIDLE(tp),
-		    tcp_timer_keep, tp, inp_to_cpuid(inp));
+		if (!callout_reset(&tp->t_timers->tt_keep, TP_KEEPINTVL(tp),
+		    tcp_timer_keep, tp)) {
+			tp->t_timers->tt_flags &= ~TT_KEEP_RST;
+		}
+	} else if (!callout_reset(&tp->t_timers->tt_keep, TP_KEEPIDLE(tp),
+		    tcp_timer_keep, tp)) {
+			tp->t_timers->tt_flags &= ~TT_KEEP_RST;
+		}
 
 #ifdef TCPDEBUG
 	if (inp->inp_socket->so_options & SO_DEBUG)
@@ -810,6 +814,7 @@ tcp_timer_activate(struct tcpcb *tp, uint32_t timer_type, u_int delta)
 	timeout_t *f_callout;
 	struct inpcb *inp = tp->t_inpcb;
 	int cpu = inp_to_cpuid(inp);
+	uint32_t f_reset;
 
 #ifdef TCP_OFFLOAD
 	if (tp->t_flags & TF_TOE)
@@ -823,38 +828,49 @@ tcp_timer_activate(struct tcpcb *tp, uint32_t timer_type, u_int delta)
 		case TT_DELACK:
 			t_callout = &tp->t_timers->tt_delack;
 			f_callout = tcp_timer_delack;
+			f_reset = TT_DELACK_RST;
 			break;
 		case TT_REXMT:
 			t_callout = &tp->t_timers->tt_rexmt;
 			f_callout = tcp_timer_rexmt;
+			f_reset = TT_REXMT_RST;
 			break;
 		case TT_PERSIST:
 			t_callout = &tp->t_timers->tt_persist;
 			f_callout = tcp_timer_persist;
+			f_reset = TT_PERSIST_RST;
 			break;
 		case TT_KEEP:
 			t_callout = &tp->t_timers->tt_keep;
 			f_callout = tcp_timer_keep;
+			f_reset = TT_KEEP_RST;
 			break;
 		case TT_2MSL:
 			t_callout = &tp->t_timers->tt_2msl;
 			f_callout = tcp_timer_2msl;
+			f_reset = TT_2MSL_RST;
 			break;
 		default:
 			panic("tp %p bad timer_type %#x", tp, timer_type);
 		}
 	if (delta == 0) {
 		if ((tp->t_timers->tt_flags & timer_type) &&
-		    callout_stop(t_callout)) {
-			tp->t_timers->tt_flags &= ~timer_type;
+		    callout_stop(t_callout) &&
+		    (tp->t_timers->tt_flags & f_reset)) {
+			tp->t_timers->tt_flags &= ~(timer_type | f_reset);
 		}
 	} else {
 		if ((tp->t_timers->tt_flags & timer_type) == 0) {
-			tp->t_timers->tt_flags |= timer_type;
+			tp->t_timers->tt_flags |= (timer_type | f_reset);
 			callout_reset_on(t_callout, delta, f_callout, tp, cpu);
 		} else {
 			/* Reset already running callout on the same CPU. */
-			callout_reset(t_callout, delta, f_callout, tp);
+			if (!callout_reset(t_callout, delta, f_callout, tp)) {
+				/*
+				 * Callout not cancelled, consider it as not
+				 * properly restarted. */
+				tp->t_timers->tt_flags &= ~f_reset;
+			}
 		}
 	}
 }
@@ -891,6 +907,7 @@ tcp_timer_stop(struct tcpcb *tp, uint32_t timer_type)
 {
 	struct callout *t_callout;
 	timeout_t *f_callout;
+	uint32_t f_reset;
 
 	tp->t_timers->tt_flags |= TT_STOPPED;
 
@@ -898,30 +915,36 @@ tcp_timer_stop(struct tcpcb *tp, uint32_t timer_type)
 		case TT_DELACK:
 			t_callout = &tp->t_timers->tt_delack;
 			f_callout = tcp_timer_delack_discard;
+			f_reset = TT_DELACK_RST;
 			break;
 		case TT_REXMT:
 			t_callout = &tp->t_timers->tt_rexmt;
 			f_callout = tcp_timer_rexmt_discard;
+			f_reset = TT_REXMT_RST;
 			break;
 		case TT_PERSIST:
 			t_callout = &tp->t_timers->tt_persist;
 			f_callout = tcp_timer_persist_discard;
+			f_reset = TT_PERSIST_RST;
 			break;
 		case TT_KEEP:
 			t_callout = &tp->t_timers->tt_keep;
 			f_callout = tcp_timer_keep_discard;
+			f_reset = TT_KEEP_RST;
 			break;
 		case TT_2MSL:
 			t_callout = &tp->t_timers->tt_2msl;
 			f_callout = tcp_timer_2msl_discard;
+			f_reset = TT_2MSL_RST;
 			break;
 		default:
 			panic("tp %p bad timer_type %#x", tp, timer_type);
 		}
 
 	if (tp->t_timers->tt_flags & timer_type) {
-		if (callout_stop(t_callout)) {
-			tp->t_timers->tt_flags &= ~timer_type;
+		if (callout_stop(t_callout) &&
+		    (tp->t_timers->tt_flags & f_reset)) {
+			tp->t_timers->tt_flags &= ~(timer_type | f_reset);
 		} else {
 			/*
 			 * Can't stop the callout, defer tcpcb actual deletion
diff --git a/sys/netinet/tcp_timer.h b/sys/netinet/tcp_timer.h
index 02a7782..dbb8aee 100644
--- a/sys/netinet/tcp_timer.h
+++ b/sys/netinet/tcp_timer.h
@@ -160,6 +160,12 @@ struct tcp_timer {
 #define TT_2MSL		0x0010
 #define TT_MASK		(TT_DELACK|TT_REXMT|TT_PERSIST|TT_KEEP|TT_2MSL)
 
+#define TT_DELACK_RST	0x0100
+#define TT_REXMT_RST	0x0200
+#define TT_PERSIST_RST	0x0400
+#define TT_KEEP_RST	0x0800
+#define TT_2MSL_RST	0x1000
+
 #define TT_STOPPED	0x00010000
 
 #define	TP_KEEPINIT(tp)	((tp)->t_keepinit ? (tp)->t_keepinit : tcp_keepinit)