svn commit: r239983 - stable/8/sys/netinet

[Robert Watson]

On Wed, 5 Sep 2012, Bjoern A. Zeeb wrote:

>> Log:
>>  MFC r239075:
>>
>>  In tcp timers, check INP_DROPPED flag a little later, after
>>  callout_deactivate(), so if INP_DROPPED is set we return with the
>>  timer active flag cleared.
>>
>>  For me this fixes negative keep timer values reported by `netstat -x'
>>  for connections in CLOSE state.
>
> panic: Lock tcp not read locked @ /w/src/sys/netinet/tcp_timer.c:497
>
> reproducable on the cluster.  Probably wrong all the way up to HEAD?

This looks like a mis-merge -- in 8.x, read-locking of the inpcbinfo is not 
used in the TCP timer code, whereas in 9.x and later it is.  There are 
important and subtle differences between inpcb/inpcb/inpcbhash locking across 
all supported major FreeBSD versions, as we have substantially refined our 
approach to improve performance and stability over the years.  As a result, it 
is generally not safe to MFC TCP/UDP locking changes without extreme care.  I 
would recommend seeking at least one, if not multiple, reviewers for changes 
and MFCs along these lines.  Even with you and others doing line-by-line 
reviews of much of the original work, we ran into quite a few bugs due to the 
complexity and difficulty in reviewing the code.

Robert

>
>
>> Modified:
>>  stable/8/sys/netinet/tcp_timer.c
>> Directory Properties:
>>  stable/8/sys/   (props changed)
>> 
>> Modified: stable/8/sys/netinet/tcp_timer.c
>> ==============================================================================
>> --- stable/8/sys/netinet/tcp_timer.c	Sat Sep  1 10:32:40 2012 
>> (r239982)
>> +++ stable/8/sys/netinet/tcp_timer.c	Sat Sep  1 10:33:53 2012 
>> (r239983)
>> @@ -176,13 +176,18 @@ tcp_timer_delack(void *xtp)
>> 		return;
>> 	}
>> 	INP_WLOCK(inp);
>> -	if ((inp->inp_flags & INP_DROPPED) || 
>> callout_pending(&tp->t_timers->tt_delack)
>> -	    || !callout_active(&tp->t_timers->tt_delack)) {
>> +	if (callout_pending(&tp->t_timers->tt_delack) ||
>> +	    !callout_active(&tp->t_timers->tt_delack)) {
>> 		INP_WUNLOCK(inp);
>> 		CURVNET_RESTORE();
>> 		return;
>> 	}
>> 	callout_deactivate(&tp->t_timers->tt_delack);
>> +	if ((inp->inp_flags & INP_DROPPED) != 0) {
>> +		INP_WUNLOCK(inp);
>> +		CURVNET_RESTORE();
>> +		return;
>> +	}
>>
>> 	tp->t_flags |= TF_ACKNOW;
>> 	TCPSTAT_INC(tcps_delack);
>> @@ -222,7 +227,7 @@ tcp_timer_2msl(void *xtp)
>> 	}
>> 	INP_WLOCK(inp);
>> 	tcp_free_sackholes(tp);
>> -	if ((inp->inp_flags & INP_DROPPED) || 
>> callout_pending(&tp->t_timers->tt_2msl) ||
>> +	if (callout_pending(&tp->t_timers->tt_2msl) ||
>> 	    !callout_active(&tp->t_timers->tt_2msl)) {
>> 		INP_WUNLOCK(tp->t_inpcb);
>> 		INP_INFO_WUNLOCK(&V_tcbinfo);
>> @@ -230,6 +235,12 @@ tcp_timer_2msl(void *xtp)
>> 		return;
>> 	}
>> 	callout_deactivate(&tp->t_timers->tt_2msl);
>> +	if ((inp->inp_flags & INP_DROPPED) != 0) {
>> +		INP_WUNLOCK(inp);
>> +		INP_INFO_WUNLOCK(&V_tcbinfo);
>> +		CURVNET_RESTORE();
>> +		return;
>> +	}
>> 	/*
>> 	 * 2 MSL timeout in shutdown went off.  If we're closed but
>> 	 * still waiting for peer to close and connection has been idle
>> @@ -293,14 +304,20 @@ tcp_timer_keep(void *xtp)
>> 		return;
>> 	}
>> 	INP_WLOCK(inp);
>> -	if ((inp->inp_flags & INP_DROPPED) || 
>> callout_pending(&tp->t_timers->tt_keep)
>> -	    || !callout_active(&tp->t_timers->tt_keep)) {
>> +	if (callout_pending(&tp->t_timers->tt_keep) ||
>> +	    !callout_active(&tp->t_timers->tt_keep)) {
>> 		INP_WUNLOCK(inp);
>> 		INP_INFO_WUNLOCK(&V_tcbinfo);
>> 		CURVNET_RESTORE();
>> 		return;
>> 	}
>> 	callout_deactivate(&tp->t_timers->tt_keep);
>> +	if ((inp->inp_flags & INP_DROPPED) != 0) {
>> +		INP_WUNLOCK(inp);
>> +		INP_INFO_WUNLOCK(&V_tcbinfo);
>> +		CURVNET_RESTORE();
>> +		return;
>> +	}
>> 	/*
>> 	 * Keep-alive timer went off; send something
>> 	 * or drop connection if idle for too long.
>> @@ -388,14 +405,20 @@ tcp_timer_persist(void *xtp)
>> 		return;
>> 	}
>> 	INP_WLOCK(inp);
>> -	if ((inp->inp_flags & INP_DROPPED) || 
>> callout_pending(&tp->t_timers->tt_persist)
>> -	    || !callout_active(&tp->t_timers->tt_persist)) {
>> +	if (callout_pending(&tp->t_timers->tt_persist) ||
>> +	    !callout_active(&tp->t_timers->tt_persist)) {
>> 		INP_WUNLOCK(inp);
>> 		INP_INFO_WUNLOCK(&V_tcbinfo);
>> 		CURVNET_RESTORE();
>> 		return;
>> 	}
>> 	callout_deactivate(&tp->t_timers->tt_persist);
>> +	if ((inp->inp_flags & INP_DROPPED) != 0) {
>> +		INP_WUNLOCK(inp);
>> +		INP_INFO_WUNLOCK(&V_tcbinfo);
>> +		CURVNET_RESTORE();
>> +		return;
>> +	}
>> 	/*
>> 	 * Persistance timer into zero window.
>> 	 * Force a byte to be output, if possible.
>> @@ -461,14 +484,20 @@ tcp_timer_rexmt(void * xtp)
>> 		return;
>> 	}
>> 	INP_WLOCK(inp);
>> -	if ((inp->inp_flags & INP_DROPPED) || 
>> callout_pending(&tp->t_timers->tt_rexmt)
>> -	    || !callout_active(&tp->t_timers->tt_rexmt)) {
>> +	if (callout_pending(&tp->t_timers->tt_rexmt) ||
>> +	    !callout_active(&tp->t_timers->tt_rexmt)) {
>> 		INP_WUNLOCK(inp);
>> 		INP_INFO_WUNLOCK(&V_tcbinfo);
>> 		CURVNET_RESTORE();
>> 		return;
>> 	}
>> 	callout_deactivate(&tp->t_timers->tt_rexmt);
>> +	if ((inp->inp_flags & INP_DROPPED) != 0) {
>> +		INP_WUNLOCK(inp);
>> +		INP_INFO_RUNLOCK(&V_tcbinfo);
>> +		CURVNET_RESTORE();
>> +		return;
>> +	}
>> 	tcp_free_sackholes(tp);
>> 	/*
>> 	 * Retransmission timer went off.  Message has not
>> 
>
> -- 
> Bjoern A. Zeeb                                 You have to have visions!
>         Stop bit received. Insert coin for new address family.
>