Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 30 Aug 2015 13:44:39 +0000 (UTC)
From:      Julien Charbon <jch@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r287304 - head/sys/netinet
Message-ID:  <201508301344.t7UDidiA051397@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: jch
Date: Sun Aug 30 13:44:39 2015
New Revision: 287304
URL: https://svnweb.freebsd.org/changeset/base/287304

Log:
  Put r284245 back in place:  If at first this fix was seen as a temporary
  workaround for a callout(9) issue, it turns out it is instead the right
  way to use callout in mpsafe mode without using callout_drain().
  
  r284245 commit message:
  
  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

Modified:
  head/sys/netinet/tcp_timer.c
  head/sys/netinet/tcp_timer.h

Modified: head/sys/netinet/tcp_timer.c
==============================================================================
--- head/sys/netinet/tcp_timer.c	Sun Aug 30 08:48:31 2015	(r287303)
+++ head/sys/netinet/tcp_timer.c	Sun Aug 30 13:44:39 2015	(r287304)
@@ -355,10 +355,12 @@ tcp_timer_2msl(void *xtp)
 		TCPSTAT_INC(tcps_finwait2_drops);
 		tp = tcp_close(tp);             
 	} else {
-		if (ticks - tp->t_rcvtime <= TP_MAXIDLE(tp))
-		       callout_reset(&tp->t_timers->tt_2msl,
-			   TP_KEEPINTVL(tp), tcp_timer_2msl, tp);
-		else
+		if (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);
        }
 
@@ -438,11 +440,14 @@ tcp_timer_keep(void *xtp)
 				    tp->rcv_nxt, tp->snd_una - 1, 0);
 			free(t_template, M_TEMP);
 		}
-		callout_reset(&tp->t_timers->tt_keep, TP_KEEPINTVL(tp),
-		    tcp_timer_keep, tp);
-	} else
-		callout_reset(&tp->t_timers->tt_keep, TP_KEEPIDLE(tp),
-		    tcp_timer_keep, tp);
+		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)
@@ -801,6 +806,7 @@ tcp_timer_activate(struct tcpcb *tp, uin
 	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)
@@ -814,38 +820,49 @@ tcp_timer_activate(struct tcpcb *tp, uin
 		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;
+			}
 		}
 	}
 }
@@ -882,6 +899,7 @@ tcp_timer_stop(struct tcpcb *tp, uint32_
 {
 	struct callout *t_callout;
 	timeout_t *f_callout;
+	uint32_t f_reset;
 
 	tp->t_timers->tt_flags |= TT_STOPPED;
 
@@ -889,30 +907,36 @@ tcp_timer_stop(struct tcpcb *tp, uint32_
 		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

Modified: head/sys/netinet/tcp_timer.h
==============================================================================
--- head/sys/netinet/tcp_timer.h	Sun Aug 30 08:48:31 2015	(r287303)
+++ head/sys/netinet/tcp_timer.h	Sun Aug 30 13:44:39 2015	(r287304)
@@ -159,6 +159,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)



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201508301344.t7UDidiA051397>