I reverted the previous version because Hrvoje Popovski reported the
following recursion:
panic: mtx_enter: locking against myself
mtx_enter()
bpf_mtap_ether()
bnx_start()
ifq_serialize()
if_enqueue()
ether_output()
bpfwrite()
Turns out that bpfwrite() doesn't need to grab the mutex which is here
to protect *read* buffers. Updated diff below, Hrvoje confirmed it
works for him.
ok?
Index: net/bpf.c
===================================================================
RCS file: /cvs/src/sys/net/bpf.c,v
retrieving revision 1.157
diff -u -p -r1.157 bpf.c
--- net/bpf.c 3 Jan 2017 19:28:50 -0000 1.157
+++ net/bpf.c 9 Jan 2017 15:01:30 -0000
@@ -116,6 +116,9 @@ int bpf_sysctl_locked(int *, u_int, void
struct bpf_d *bpfilter_lookup(int);
+/*
+ * Called holding ``bd_mtx''.
+ */
void bpf_attachd(struct bpf_d *, struct bpf_if *);
void bpf_detachd(struct bpf_d *);
void bpf_resetd(struct bpf_d *);
@@ -260,11 +263,12 @@ bpf_movein(struct uio *uio, u_int linkty
/*
* Attach file to the bpf interface, i.e. make d listen on bp.
- * Must be called at splnet.
*/
void
bpf_attachd(struct bpf_d *d, struct bpf_if *bp)
{
+ MUTEX_ASSERT_LOCKED(&d->bd_mtx);
+
/*
* Point d at bp, and add d to the interface's list of listeners.
* Finally, point the driver's bpf cookie at the interface so
@@ -285,9 +289,10 @@ bpf_attachd(struct bpf_d *d, struct bpf_
void
bpf_detachd(struct bpf_d *d)
{
- struct bpf_if *bp;
+ struct bpf_if *bp = d->bd_bif;
+
+ MUTEX_ASSERT_LOCKED(&d->bd_mtx);
- bp = d->bd_bif;
/* Not attached. */
if (bp == NULL)
return;
@@ -313,7 +318,13 @@ bpf_detachd(struct bpf_d *d)
int error;
d->bd_promisc = 0;
+
+ bpf_get(d);
+ mtx_leave(&d->bd_mtx);
error = ifpromisc(bp->bif_ifp, 0);
+ mtx_enter(&d->bd_mtx);
+ bpf_put(d);
+
if (error && !(error == EINVAL || error == ENODEV))
/*
* Something is really wrong if we were able to put
@@ -353,6 +364,7 @@ bpfopen(dev_t dev, int flag, int mode, s
bd->bd_unit = unit;
bd->bd_bufsize = bpf_bufsize;
bd->bd_sig = SIGIO;
+ mtx_init(&bd->bd_mtx, IPL_NET);
task_set(&bd->bd_wake_task, bpf_wakeup_cb, bd);
if (flag & FNONBLOCK)
@@ -372,15 +384,14 @@ int
bpfclose(dev_t dev, int flag, int mode, struct proc *p)
{
struct bpf_d *d;
- int s;
d = bpfilter_lookup(minor(dev));
- s = splnet();
+ mtx_enter(&d->bd_mtx);
bpf_detachd(d);
bpf_wakeup(d);
LIST_REMOVE(d, bd_list);
+ mtx_leave(&d->bd_mtx);
bpf_put(d);
- splx(s);
return (0);
}
@@ -391,11 +402,13 @@ bpfclose(dev_t dev, int flag, int mode,
* Zero the length of the new store buffer.
*/
#define ROTATE_BUFFERS(d) \
+ KASSERT(d->bd_in_uiomove == 0); \
+ MUTEX_ASSERT_LOCKED(&d->bd_mtx); \
(d)->bd_hbuf = (d)->bd_sbuf; \
(d)->bd_hlen = (d)->bd_slen; \
(d)->bd_sbuf = (d)->bd_fbuf; \
(d)->bd_slen = 0; \
- (d)->bd_fbuf = 0;
+ (d)->bd_fbuf = NULL;
/*
* bpfread - read next chunk of packets from buffers
*/
@@ -403,15 +416,17 @@ int
bpfread(dev_t dev, struct uio *uio, int ioflag)
{
struct bpf_d *d;
- int error;
- int s;
+ caddr_t hbuf;
+ int hlen, error;
+
+ KERNEL_ASSERT_LOCKED();
d = bpfilter_lookup(minor(dev));
if (d->bd_bif == NULL)
return (ENXIO);
- s = splnet();
bpf_get(d);
+ mtx_enter(&d->bd_mtx);
/*
* Restrict application to use a buffer the same size as
@@ -460,8 +475,8 @@ bpfread(dev_t dev, struct uio *uio, int
error = EWOULDBLOCK;
} else {
if ((d->bd_rdStart + d->bd_rtout) < ticks) {
- error = tsleep((caddr_t)d, PRINET|PCATCH, "bpf",
- d->bd_rtout);
+ error = msleep(d, &d->bd_mtx, PRINET|PCATCH,
+ "bpf", d->bd_rtout);
} else
error = EWOULDBLOCK;
}
@@ -492,22 +507,30 @@ bpfread(dev_t dev, struct uio *uio, int
/*
* At this point, we know we have something in the hold slot.
*/
- splx(s);
+ hbuf = d->bd_hbuf;
+ hlen = d->bd_hlen;
+ d->bd_hbuf = NULL;
+ d->bd_hlen = 0;
+ d->bd_fbuf = NULL;
+ d->bd_in_uiomove = 1;
/*
* Move data from hold buffer into user space.
* We know the entire buffer is transferred since
* we checked above that the read buffer is bpf_bufsize bytes.
*/
- error = uiomove(d->bd_hbuf, d->bd_hlen, uio);
-
- s = splnet();
- d->bd_fbuf = d->bd_hbuf;
- d->bd_hbuf = NULL;
- d->bd_hlen = 0;
+ mtx_leave(&d->bd_mtx);
+ error = uiomove(hbuf, hlen, uio);
+ mtx_enter(&d->bd_mtx);
+
+ /* Ensure that bpf_resetd() or ROTATE_BUFFERS() haven't been called. */
+ KASSERT(d->bd_fbuf == NULL);
+ KASSERT(d->bd_hbuf == NULL);
+ d->bd_fbuf = hbuf;
+ d->bd_in_uiomove = 0;
out:
+ mtx_leave(&d->bd_mtx);
bpf_put(d);
- splx(s);
return (error);
}
@@ -519,6 +542,8 @@ out:
void
bpf_wakeup(struct bpf_d *d)
{
+ MUTEX_ASSERT_LOCKED(&d->bd_mtx);
+
/*
* As long as csignal() and selwakeup() need to be protected
* by the KERNEL_LOCK() we have to delay the wakeup to
@@ -556,6 +581,8 @@ bpfwrite(dev_t dev, struct uio *uio, int
struct sockaddr_storage dst;
u_int dlt;
+ KERNEL_ASSERT_LOCKED();
+
d = bpfilter_lookup(minor(dev));
if (d->bd_bif == NULL)
return (ENXIO);
@@ -607,12 +634,15 @@ out:
/*
* Reset a descriptor by flushing its packet buffer and clearing the
- * receive and drop counts. Should be called at splnet.
+ * receive and drop counts.
*/
void
bpf_resetd(struct bpf_d *d)
{
- if (d->bd_hbuf) {
+ MUTEX_ASSERT_LOCKED(&d->bd_mtx);
+ KASSERT(d->bd_in_uiomove == 0);
+
+ if (d->bd_hbuf != NULL) {
/* Free the hold buffer. */
d->bd_fbuf = d->bd_hbuf;
d->bd_hbuf = NULL;
@@ -646,7 +676,7 @@ int
bpfioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, struct proc *p)
{
struct bpf_d *d;
- int s, error = 0;
+ int error = 0;
d = bpfilter_lookup(minor(dev));
if (d->bd_locked && suser(p, 0) != 0) {
@@ -674,8 +704,9 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t
}
}
- switch (cmd) {
+ bpf_get(d);
+ switch (cmd) {
default:
error = EINVAL;
break;
@@ -687,11 +718,11 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t
{
int n;
- s = splnet();
+ mtx_enter(&d->bd_mtx);
n = d->bd_slen;
- if (d->bd_hbuf)
+ if (d->bd_hbuf != NULL)
n += d->bd_hlen;
- splx(s);
+ mtx_leave(&d->bd_mtx);
*(int *)addr = n;
break;
@@ -717,7 +748,9 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t
*(u_int *)addr = size = bpf_maxbufsize;
else if (size < BPF_MINBUFSIZE)
*(u_int *)addr = size = BPF_MINBUFSIZE;
+ mtx_enter(&d->bd_mtx);
d->bd_bufsize = size;
+ mtx_leave(&d->bd_mtx);
}
break;
@@ -739,9 +772,9 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t
* Flush read packet buffer.
*/
case BIOCFLUSH:
- s = splnet();
+ mtx_enter(&d->bd_mtx);
bpf_resetd(d);
- splx(s);
+ mtx_leave(&d->bd_mtx);
break;
/*
@@ -753,15 +786,14 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t
* No interface attached yet.
*/
error = EINVAL;
- break;
- }
- s = splnet();
- if (d->bd_promisc == 0) {
- error = ifpromisc(d->bd_bif->bif_ifp, 1);
- if (error == 0)
- d->bd_promisc = 1;
+ } else {
+ if (d->bd_promisc == 0) {
+ MUTEX_ASSERT_UNLOCKED(&d->bd_mtx);
+ error = ifpromisc(d->bd_bif->bif_ifp, 1);
+ if (error == 0)
+ d->bd_promisc = 1;
+ }
}
- splx(s);
break;
/*
@@ -790,8 +822,11 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t
case BIOCSDLT:
if (d->bd_bif == NULL)
error = EINVAL;
- else
+ else {
+ mtx_enter(&d->bd_mtx);
error = bpf_setdlt(d, *(u_int *)addr);
+ mtx_leave(&d->bd_mtx);
+ }
break;
/*
@@ -939,6 +974,8 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t
*(u_int *)addr = d->bd_sig;
break;
}
+
+ bpf_put(d);
return (error);
}
@@ -953,7 +990,6 @@ bpf_setf(struct bpf_d *d, struct bpf_pro
struct srp *filter;
struct bpf_insn *fcode;
u_int flen, size;
- int s;
KERNEL_ASSERT_LOCKED();
filter = wf ? &d->bd_wfilter : &d->bd_rfilter;
@@ -962,9 +998,9 @@ bpf_setf(struct bpf_d *d, struct bpf_pro
if (fp->bf_len != 0)
return (EINVAL);
srp_update_locked(&bpf_insn_gc, filter, NULL);
- s = splnet();
+ mtx_enter(&d->bd_mtx);
bpf_resetd(d);
- splx(s);
+ mtx_leave(&d->bd_mtx);
return (0);
}
flen = fp->bf_len;
@@ -989,9 +1025,9 @@ bpf_setf(struct bpf_d *d, struct bpf_pro
srp_update_locked(&bpf_insn_gc, filter, bf);
- s = splnet();
+ mtx_enter(&d->bd_mtx);
bpf_resetd(d);
- splx(s);
+ mtx_leave(&d->bd_mtx);
return (0);
}
@@ -1005,7 +1041,6 @@ bpf_setif(struct bpf_d *d, struct ifreq
{
struct bpf_if *bp, *candidate = NULL;
int error = 0;
- int s;
/*
* Look through attached interfaces for the named one.
@@ -1030,7 +1065,7 @@ bpf_setif(struct bpf_d *d, struct ifreq
* If we're already attached to requested interface,
* just flush the buffer.
*/
- s = splnet();
+ mtx_enter(&d->bd_mtx);
if (d->bd_sbuf == NULL) {
if ((error = bpf_allocbufs(d)))
goto out;
@@ -1044,7 +1079,7 @@ bpf_setif(struct bpf_d *d, struct ifreq
}
bpf_resetd(d);
out:
- splx(s);
+ mtx_leave(&d->bd_mtx);
return (error);
}
@@ -1064,7 +1099,9 @@ int
bpfpoll(dev_t dev, int events, struct proc *p)
{
struct bpf_d *d;
- int s, revents;
+ int revents;
+
+ KERNEL_ASSERT_LOCKED();
/*
* An imitation of the FIONREAD ioctl code.
@@ -1085,7 +1122,7 @@ bpfpoll(dev_t dev, int events, struct pr
revents = events & (POLLOUT | POLLWRNORM);
if (events & (POLLIN | POLLRDNORM)) {
- s = splnet();
+ mtx_enter(&d->bd_mtx);
if (d->bd_hlen != 0 || (d->bd_immediate && d->bd_slen != 0))
revents |= events & (POLLIN | POLLRDNORM);
else {
@@ -1097,7 +1134,7 @@ bpfpoll(dev_t dev, int events, struct pr
d->bd_rdStart = ticks;
selrecord(p, &d->bd_sel);
}
- splx(s);
+ mtx_leave(&d->bd_mtx);
}
return (revents);
}
@@ -1110,9 +1147,11 @@ bpfkqfilter(dev_t dev, struct knote *kn)
{
struct bpf_d *d;
struct klist *klist;
- int s;
+
+ KERNEL_ASSERT_LOCKED();
d = bpfilter_lookup(minor(dev));
+
switch (kn->kn_filter) {
case EVFILT_READ:
klist = &d->bd_sel.si_note;
@@ -1122,14 +1161,14 @@ bpfkqfilter(dev_t dev, struct knote *kn)
return (EINVAL);
}
- kn->kn_hook = d;
-
- s = splnet();
bpf_get(d);
+ kn->kn_hook = d;
SLIST_INSERT_HEAD(klist, kn, kn_selnext);
+
+ mtx_enter(&d->bd_mtx);
if (d->bd_rtout != -1 && d->bd_rdStart == 0)
d->bd_rdStart = ticks;
- splx(s);
+ mtx_leave(&d->bd_mtx);
return (0);
}
@@ -1138,12 +1177,11 @@ void
filt_bpfrdetach(struct knote *kn)
{
struct bpf_d *d = kn->kn_hook;
- int s;
- s = splnet();
+ KERNEL_ASSERT_LOCKED();
+
SLIST_REMOVE(&d->bd_sel.si_note, kn, knote, kn_selnext);
bpf_put(d);
- splx(s);
}
int
@@ -1151,9 +1189,14 @@ filt_bpfread(struct knote *kn, long hint
{
struct bpf_d *d = kn->kn_hook;
+ KERNEL_ASSERT_LOCKED();
+
+ mtx_enter(&d->bd_mtx);
kn->kn_data = d->bd_hlen;
if (d->bd_immediate)
kn->kn_data += d->bd_slen;
+ mtx_leave(&d->bd_mtx);
+
return (kn->kn_data > 0);
}
@@ -1196,7 +1239,6 @@ _bpf_mtap(caddr_t arg, const struct mbuf
struct timeval tv;
int gottime = 0;
int drop = 0;
- int s;
if (m == NULL)
return (0);
@@ -1232,12 +1274,10 @@ _bpf_mtap(caddr_t arg, const struct mbuf
if (!gottime++)
microtime(&tv);
- KERNEL_LOCK();
- s = splnet();
+ mtx_enter(&d->bd_mtx);
bpf_catchpacket(d, (u_char *)m, pktlen, slen, cpfn,
&tv);
- splx(s);
- KERNEL_UNLOCK();
+ mtx_leave(&d->bd_mtx);
if (d->bd_fildrop)
drop = 1;
@@ -1367,6 +1407,7 @@ bpf_catchpacket(struct bpf_d *d, u_char
int totlen, curlen;
int hdrlen, do_wakeup = 0;
+ MUTEX_ASSERT_LOCKED(&d->bd_mtx);
if (d->bd_bif == NULL)
return;
@@ -1450,6 +1491,8 @@ bpf_catchpacket(struct bpf_d *d, u_char
int
bpf_allocbufs(struct bpf_d *d)
{
+ MUTEX_ASSERT_LOCKED(&d->bd_mtx);
+
d->bd_fbuf = malloc(d->bd_bufsize, M_DEVBUF, M_NOWAIT);
if (d->bd_fbuf == NULL)
return (ENOMEM);
@@ -1469,7 +1512,7 @@ bpf_allocbufs(struct bpf_d *d)
void
bpf_get(struct bpf_d *bd)
{
- bd->bd_ref++;
+ atomic_inc_int(&bd->bd_ref);
}
/*
@@ -1479,7 +1522,7 @@ bpf_get(struct bpf_d *bd)
void
bpf_put(struct bpf_d *bd)
{
- if (--bd->bd_ref > 0)
+ if (atomic_dec_int_nv(&bd->bd_ref) > 0)
return;
free(bd->bd_sbuf, M_DEVBUF, 0);
@@ -1614,6 +1657,8 @@ bpfilter_lookup(int unit)
{
struct bpf_d *bd;
+ KERNEL_ASSERT_LOCKED();
+
LIST_FOREACH(bd, &bpf_d_list, bd_list)
if (bd->bd_unit == unit)
return (bd);
@@ -1657,10 +1702,10 @@ bpf_getdltlist(struct bpf_d *d, struct b
int
bpf_setdlt(struct bpf_d *d, u_int dlt)
{
- int s;
struct ifnet *ifp;
struct bpf_if *bp;
+ MUTEX_ASSERT_LOCKED(&d->bd_mtx);
if (d->bd_bif->bif_dlt == dlt)
return (0);
ifp = d->bd_bif->bif_ifp;
@@ -1670,11 +1715,9 @@ bpf_setdlt(struct bpf_d *d, u_int dlt)
}
if (bp == NULL)
return (EINVAL);
- s = splnet();
bpf_detachd(d);
bpf_attachd(d, bp);
bpf_resetd(d);
- splx(s);
return (0);
}
Index: net/bpfdesc.h
===================================================================
RCS file: /cvs/src/sys/net/bpfdesc.h,v
retrieving revision 1.33
diff -u -p -r1.33 bpfdesc.h
--- net/bpfdesc.h 3 Jan 2017 19:28:50 -0000 1.33
+++ net/bpfdesc.h 4 Jan 2017 13:42:10 -0000
@@ -56,15 +56,17 @@ struct bpf_d {
* fbuf (free) - When read is done, put cluster here.
* On receiving, if sbuf is full and fbuf is 0, packet is dropped.
*/
+ struct mutex bd_mtx; /* protect buffer slots below */
caddr_t bd_sbuf; /* store slot */
caddr_t bd_hbuf; /* hold slot */
caddr_t bd_fbuf; /* free slot */
int bd_slen; /* current length of store buffer */
int bd_hlen; /* current length of hold buffer */
-
int bd_bufsize; /* absolute length of buffers */
- struct bpf_if * bd_bif; /* interface descriptor */
+ int bd_in_uiomove; /* for debugging purpose */
+
+ struct bpf_if *bd_bif; /* interface descriptor */
u_long bd_rtout; /* Read timeout in 'ticks' */
u_long bd_rdStart; /* when the read started */
struct srp bd_rfilter; /* read filter code */
