From owner-freebsd-net@FreeBSD.ORG Tue Jun 13 11:52:52 2006 Return-Path: X-Original-To: freebsd-net@freebsd.org 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 BF9C616A41B for ; Tue, 13 Jun 2006 11:52:52 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [209.31.154.42]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1B8B343D64 for ; Tue, 13 Jun 2006 11:52:52 +0000 (GMT) (envelope-from rwatson@FreeBSD.org) Received: from fledge.watson.org (fledge.watson.org [209.31.154.41]) by cyrus.watson.org (Postfix) with ESMTP id 1A39E46C34; Tue, 13 Jun 2006 07:52:51 -0400 (EDT) Date: Tue, 13 Jun 2006 12:52:51 +0100 (BST) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: Blue In-Reply-To: <448E6635.4010800@zyxel.com.tw> Message-ID: <20060613124552.V74063@fledge.watson.org> References: <448E6635.4010800@zyxel.com.tw> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-net@freebsd.org Subject: Re: [FreeBSD6.1-RELEASE]problem about soisconnected() in uipc_socket2.c X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Jun 2006 11:52:52 -0000 On Tue, 13 Jun 2006, Blue wrote: > I have found a questionable code in soisconnected() function in > uipc_socket2.c. In the very beginning, the SOCK_LOCK() would lock the > socket. However, in line 127, it unlocks socket. I am wondering that > SOCK_UNLOCK() should be called until all the socket's parameters are done. > In my opinion, it should be located right before ACCEPT_UNLOCK(). Some fields of the socket are protected by accept_mtx rather than the socket lock. You can check the field locking key in socketvar.h for details: /*- * Locking key to struct socket: * (a) constant after allocation, no locking required. * (b) locked by SOCK_LOCK(so). * (c) locked by SOCKBUF_LOCK(&so->so_rcv). * (d) locked by SOCKBUF_LOCK(&so->so_snd). * (e) locked by ACCEPT_LOCK(). * (f) not locked since integer reads/writes are atomic. * (g) used only as a sleep/wakeup address, no value. * (h) locked by global mutex so_global_mtx. */ ... int so_qstate; /* (e) internal state flags SQ_* */ TAILQ_HEAD(, socket) so_incomp; /* (e) queue of partial unaccepted connections */ TAILQ_HEAD(, socket) so_comp; /* (e) queue of complete unaccepted connections */ TAILQ_ENTRY(socket) so_list; /* (e) list of unaccepted connections */ u_short so_qlen; /* (e) number of unaccepted connections */ u_short so_incqlen; /* (e) number of unaccepted incomplete connections */ u_short so_qlimit; /* (e) max number queued connections */ The socket lock is dropped at the earliest point in each case after which no further manipulation of the socket lock protected fields take place. The only really dubious thing about what's there right now is the handling of socket upcalls, which has poorly defined locking properties, but I think in the non-accept filter case, the locking looks generally correct. Right now we call so_upcall in different places with different locks, and some upcalls perform activities that may require acquiring further locks, possibly in bad orders. We rely to some extent on the behavior of the protocol to keep certain races from happening, and that reliance is not well documented. I have several works in progress related to socket locking which improve this to some extent. One breaks out the single upcall function pointer into a series of upcalls each with well defined behavior; another coalesces the various socket locks, also resulting in well defined behavior. Robert N M Watson Computer Laboratory Universty of Cambridge