Date: Wed, 20 Aug 2003 18:41:49 -0400 From: Ed Maste <emaste@sandvine.com> To: 'Mike Silbersack' <silby@silby.com> Cc: freebsd-net@freebsd.org Subject: RE: TCP socket shutdown race condition Message-ID: <FE045D4D9F7AED4CBFF1B3B813C8533701BD3CBF@mail.sandvine.com>
next in thread | raw e-mail | index | archive | help
>Well, I guess the spl() fix is probably going to be the quickest here
>then, please send it to me once you've pounded on it, Ed.
So my spl() fix seems to eliminate the problem for me but while I'm looking
at this stuff I want to make sure there aren't any related cases left in.
My current patch is at the end of this message.
One potential problem is a crfree() interrupting code that's incrementing a
ucred reference. For example, uipc_socket.c:socreate():
so->so_cred = p->p_ucred;
crhold(so->so_cred);
However, I don't think a crfree() interrupting between these should cause a
problem. The refcount would always be at least 2 to begin with, so the
ucred wouldn't be free()d.
Another possible problem is a crfree() interrupting a crfree() for the same
ucred. This would result in a double free, leading to who knows what
corruption. I did try the test against a kernel with invariants on, but
nothing happened; presumably the timing changed enough that the race
condition wouldn't occur.
I protected the if (--cr->cr_ref == 0) in crfree() with splhigh() but left
the actuall free() out of splhigh.
The cr->cr_ref++ in crhold() assembles to an incl (%eax) which will be
atomic on a single processor. However I don't think anything guarantees gcc
will emit that code for that source instruction, so I put a splhigh around
it too. Probably atomic_add_int would be better -- I'll try that out too.
Our stress test is running against the patch below, and I'll report any
findings. If you have any comments on this, please let me know.
Index: kern_prot.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_prot.c,v
retrieving revision 1.53.2.9
diff -c -3 -r1.53.2.9 kern_prot.c
*** kern_prot.c 9 Mar 2002 05:20:26 -0000 1.53.2.9
--- kern_prot.c 20 Aug 2003 22:12:29 -0000
***************
*** 997,1003 ****
--- 997,1005 ----
crhold(cr)
struct ucred *cr;
{
+ int s = splhigh();
cr->cr_ref++;
+ splx(s);
}
/*
***************
*** 1008,1017 ****
--- 1010,1021 ----
crfree(cr)
struct ucred *cr;
{
+ int s = splhigh();
if (cr->cr_ref == 0)
panic("Freeing already free credential! %p", cr);
if (--cr->cr_ref == 0) {
+ splx(s);
/*
* Some callers of crget(), such as nfs_statfs(),
* allocate a temporary credential, but don't
***************
*** 1020,1026 ****
--- 1024,1032 ----
if (cr->cr_uidinfo != NULL)
uifree(cr->cr_uidinfo);
FREE((caddr_t)cr, M_CRED);
+ return;
}
+ splx(s);
}
/*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?FE045D4D9F7AED4CBFF1B3B813C8533701BD3CBF>