From owner-freebsd-net@FreeBSD.ORG Wed Aug 20 15:41:52 2003 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 6525016A4BF for ; Wed, 20 Aug 2003 15:41:52 -0700 (PDT) Received: from mail.sandvine.com (sandvine.com [199.243.201.138]) by mx1.FreeBSD.org (Postfix) with ESMTP id ACF5043FD7 for ; Wed, 20 Aug 2003 15:41:51 -0700 (PDT) (envelope-from emaste@sandvine.com) Received: by mail.sandvine.com with Internet Mail Service (5.5.2653.19) id ; Wed, 20 Aug 2003 18:41:51 -0400 Message-ID: From: Ed Maste To: 'Mike Silbersack' Date: Wed, 20 Aug 2003 18:41:49 -0400 MIME-Version: 1.0 X-Mailer: Internet Mail Service (5.5.2653.19) Content-Type: text/plain; charset="iso-8859-1" cc: freebsd-net@freebsd.org Subject: RE: TCP socket shutdown race condition X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Aug 2003 22:41:52 -0000 >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); } /*