Use a mutex to serialize accesses to buffer slots.

With this change bpf_catchpacket() no longer need the KERNEL_LOCK().

Tested by Hrvoje Popovski who reported a recursion in the previous
attempt.

ok bluhm@

1 files changed, 110 insertions, 66 deletions

diff --git a/sys/net/bpf.c b/sys/net/bpf.c
index e55e4a9ad3c..64b16d53a2c 100644
--- a/sys/net/bpf.c
+++ b/sys/net/bpf.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: bpf.c,v 1.157 2017/01/03 19:28:50 mpi Exp $	*/
+/*	$OpenBSD: bpf.c,v 1.158 2017/01/09 19:15:01 mpi Exp $	*/
 /*	$NetBSD: bpf.c,v 1.33 1997/02/21 23:59:35 thorpej Exp $	*/
 
 /*
@@ -116,6 +116,9 @@ int	bpf_sysctl_locked(int *, u_int, void *, size_t *, void *, size_t);
 
 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 linktype, struct mbuf **mp,
 
 /*
  * 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
@@ -287,6 +291,8 @@ bpf_detachd(struct bpf_d *d)
 {
 	struct bpf_if *bp;
 
+	MUTEX_ASSERT_LOCKED(&d->bd_mtx);
+
 	bp = d->bd_bif;
 	/* Not attached. */
 	if (bp == NULL)
@@ -313,7 +319,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 +365,7 @@ bpfopen(dev_t dev, int flag, int mode, struct proc *p)
 	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 +385,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 +403,13 @@ bpfclose(dev_t dev, int flag, int mode, struct proc *p)
  * 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 +417,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 +476,8 @@ bpfread(dev_t dev, struct uio *uio, int ioflag)
 			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 +508,30 @@ bpfread(dev_t dev, struct uio *uio, int ioflag)
 	/*
 	 * 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 +543,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 +582,8 @@ bpfwrite(dev_t dev, struct uio *uio, int ioflag)
 	struct sockaddr_storage dst;
 	u_int dlt;
 
+	KERNEL_ASSERT_LOCKED();
+
 	d = bpfilter_lookup(minor(dev));
 	if (d->bd_bif == NULL)
 		return (ENXIO);
@@ -607,12 +635,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 +677,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 +705,9 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, struct proc *p)
 		}
 	}
 
-	switch (cmd) {
+	bpf_get(d);
 
+	switch (cmd) {
 	default:
 		error = EINVAL;
 		break;
@@ -687,11 +719,11 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, struct proc *p)
 		{
 			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 +749,9 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, struct proc *p)
 				*(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 +773,9 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, struct proc *p)
 	 * 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 +787,14 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, struct proc *p)
 			 * 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 +823,11 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, struct proc *p)
 	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 +975,8 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, struct proc *p)
 		*(u_int *)addr = d->bd_sig;
 		break;
 	}
+
+	bpf_put(d);
 	return (error);
 }
 
@@ -953,7 +991,6 @@ bpf_setf(struct bpf_d *d, struct bpf_program *fp, int wf)
 	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 +999,9 @@ bpf_setf(struct bpf_d *d, struct bpf_program *fp, int wf)
 		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 +1026,9 @@ bpf_setf(struct bpf_d *d, struct bpf_program *fp, int wf)
 
 	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 +1042,6 @@ bpf_setif(struct bpf_d *d, struct ifreq *ifr)
 {
 	struct bpf_if *bp, *candidate = NULL;
 	int error = 0;
-	int s;
 
 	/*
 	 * Look through attached interfaces for the named one.
@@ -1030,7 +1066,7 @@ bpf_setif(struct bpf_d *d, struct ifreq *ifr)
 	 * 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 +1080,7 @@ bpf_setif(struct bpf_d *d, struct ifreq *ifr)
 	}
 	bpf_resetd(d);
 out:
-	splx(s);
+	mtx_leave(&d->bd_mtx);
 	return (error);
 }
 
@@ -1064,7 +1100,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 +1123,7 @@ bpfpoll(dev_t dev, int events, struct proc *p)
 	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 +1135,7 @@ bpfpoll(dev_t dev, int events, struct proc *p)
 				d->bd_rdStart = ticks;
 			selrecord(p, &d->bd_sel);
 		}
-		splx(s);
+		mtx_leave(&d->bd_mtx);
 	}
 	return (revents);
 }
@@ -1110,9 +1148,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 +1162,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 +1178,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 +1190,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 +1240,6 @@ _bpf_mtap(caddr_t arg, const struct mbuf *m, u_int direction,
 	struct timeval tv;
 	int gottime = 0;
 	int drop = 0;
-	int s;
 
 	if (m == NULL)
 		return (0);
@@ -1232,12 +1275,10 @@ _bpf_mtap(caddr_t arg, const struct mbuf *m, u_int direction,
 			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 +1408,7 @@ bpf_catchpacket(struct bpf_d *d, u_char *pkt, size_t pktlen, size_t snaplen,
 	int totlen, curlen;
 	int hdrlen, do_wakeup = 0;
 
+	MUTEX_ASSERT_LOCKED(&d->bd_mtx);
 	if (d->bd_bif == NULL)
 		return;
 
@@ -1450,6 +1492,8 @@ bpf_catchpacket(struct bpf_d *d, u_char *pkt, size_t pktlen, size_t snaplen,
 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 +1513,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 +1523,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 +1658,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 +1703,10 @@ bpf_getdltlist(struct bpf_d *d, struct bpf_dltlist *bfl)
 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 +1716,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);
 }