cvs commit: src/sys/net bpf.c bpfdesc.h
Robert Watson
rwatson at FreeBSD.org
Sun Jul 24 23:23:47 GMT 2005
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
More information about the cvs-src
mailing list