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-all mailing list