From owner-cvs-src@FreeBSD.ORG Sun Jul 24 23:23:47 2005 Return-Path: X-Original-To: cvs-src@FreeBSD.org Delivered-To: cvs-src@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id A549716A41F; Sun, 24 Jul 2005 23:23:47 +0000 (GMT) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [204.156.12.53]) by mx1.FreeBSD.org (Postfix) with ESMTP id 47C1243D45; Sun, 24 Jul 2005 23:23:47 +0000 (GMT) (envelope-from rwatson@FreeBSD.org) Received: from fledge.watson.org (fledge.watson.org [204.156.12.50]) by cyrus.watson.org (Postfix) with ESMTP id 9DB7C46B2A; Sun, 24 Jul 2005 19:23:46 -0400 (EDT) Date: Mon, 25 Jul 2005 00:24:44 +0100 (BST) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: "Christian S.J. Peron" In-Reply-To: <200507241721.j6OHLImZ032073@repoman.freebsd.org> Message-ID: <20050725001935.B48825@fledge.watson.org> References: <200507241721.j6OHLImZ032073@repoman.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/net bpf.c bpfdesc.h X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 24 Jul 2005 23:23:47 -0000 On Sun, 24 Jul 2005, Christian S.J. Peron wrote: > Revision Changes Path > 1.154 +91 -14 src/sys/net/bpf.c > 1.30 +26 -0 src/sys/net/bpfdesc.h + if (bpf_bpfd_cnt == 0) + return (SYSCTL_OUT(req, 0, 0)); + mtx_lock(&bpf_mtx); + KASSERT(bpf_bpfd_cnt != 0, ("zero bpf descriptors present")); + LIST_FOREACH(bp, &bpf_iflist, bif_next) { + LIST_FOREACH(bd, &bp->bif_dlist, bd_next) { + BPFD_LOCK(bd); + bpfstats_fill_xbpf(&xbd, bd); + BPFD_UNLOCK(bd); + error = SYSCTL_OUT(req, &xbd, sizeof(xbd)); + if (error != 0) { + mtx_unlock(&bpf_mtx); + return (error); + } + } + } + mtx_unlock(&bpf_mtx); Looks like you hold bpf_mtx over calls to SYSCTL_OUT(), which may sleep if it is required to write to a user memory page that is not in memory. This can result in a lot of nasty things, including deadlock, odd lock orders (especially if the page fault results in a signal being delivered to a process), etc. In general, monitoring code of this sort needs to store its output into a temporary kernel buffer and then copy that out, or it needs to drop mutexes and accept race conditions. Generally the former is easier to program, and the latter uses less kernel memory. Also, because the bpf_mtx isn't held between the first and second tests of bpf_bpfd_cnt, a race can occur resulting in a panic when the kassert fails, if the count is elevated before the call to hold the mutex, and not once the mutex is released by the other thread. Does the kassert actually add value here? In the unusual event of a race, you do a slightly more expensive list walk, but only in rare cases. With the incorrect KASSERT(), you panic instead. Robert N M Watson