Re: r194546 amd64: kernel panic in tcp_sack.c

Kamigishi Rei wrote:
> Lawrence Stewart wrote:
>> Ok. I'm working on a patch to address a different TCP/SACK issue, but 
>> it may in fact be partially relevant to the cause of your panic... 
>> can't promise when I'll be able to take a close look at this but I'm 
>> aware of it now so that's a start. If you run into it again or find 
>> the trigger for the panic, please let me know.
> Reproduced. I don't know what was the trigger this time. The system runs 
> lighttpd, fastcgi python and php, mysql and postgresql.
> Also, in this core somehow everything looks quite similar (looks like 
> another page fault but with panic() called prior to getting into fatal 
> trap 12) to my two earlier dumps:
> http://lists.freebsd.org/pipermail/freebsd-current/2009-June/008777.html
> http://lists.freebsd.org/pipermail/freebsd-current/2009-June/008781.html
> so maybe it's not really a problem with tcp_sack.c or netisr.c.
> 

With some poking around and a key insight provided by Robert Watson, I 
believe we've got this sorted.

The SACK code puts a global cap on the amount of memory that can be used 
for SACK accounting. The variable V_tcp_sack_globalholes tracks how many 
SACK holes are currently allocated across all active TCP connections. It 
gets incremented in tcp_sackhole_alloc() and decremented in 
tcp_sackhole_free() in netinet/tcp_sack.c.

It turns out that there is currently no lock synchronising access to the 
variable, and the incrementing/decrementing is not being done 
atomically. In Kamigishi's case, his server had a traffic profile 
consisting of a large number of clients simultaneously connecting over 
cruddy links which was giving the SACK accounting a real workout. The 
inevitable race would strike one or more times, leaving the count of 
holes not in tune with reality, and eventually when traffic died down 
the variable would decrement down below 0, triggering the panic. Note 
that this panic only occurs if INVARIANTS is compiled into the kernel so 
the issue has been around for some time but not noticed.

The attached patch makes use of the atomic(9) KPI to ensure 
incrementing/decrementing the variable is done atomically, which should 
fix the bug.

Reviews/testing would be good so that we can get this into 8.0.

Cheers,
Lawrence

Index: sys/netinet/tcp_sack.c
===================================================================
--- sys/netinet/tcp_sack.c	(revision 195317)
+++ sys/netinet/tcp_sack.c	(working copy)
_at__at_ -273,7 +273,7 _at__at_
 	hole->rxmit = start;
 
 	tp->snd_numholes++;
-	V_tcp_sack_globalholes++;
+	atomic_add_int(&V_tcp_sack_globalholes, 1);
 
 	return hole;
 }
_at__at_ -289,7 +289,7 _at__at_
 	uma_zfree(V_sack_hole_zone, hole);
 
 	tp->snd_numholes--;
-	V_tcp_sack_globalholes--;
+	atomic_subtract_int(&V_tcp_sack_globalholes, 1);
 
 	KASSERT(tp->snd_numholes >= 0, ("tp->snd_numholes >= 0"));
 	KASSERT(V_tcp_sack_globalholes >= 0, ("tcp_sack_globalholes >= 0"));