Date: Mon, 25 Jul 2005 00:24:44 +0100 (BST) From: Robert Watson <rwatson@FreeBSD.org> To: "Christian S.J. Peron" <csjp@FreeBSD.org> Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/net bpf.c bpfdesc.h Message-ID: <20050725001935.B48825@fledge.watson.org> In-Reply-To: <200507241721.j6OHLImZ032073@repoman.freebsd.org> References: <200507241721.j6OHLImZ032073@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050725001935.B48825>