bpf hold buffer in-use flag

[Guy Helmer]

To try to completely resolve the race in bpfread(), I have put together these changes to add a flag to indicate when the hold buffer cannot be modified because it is in use. Since it's my first time using mtx_sleep() and wakeup(), I wanted to run these past the list to see if I can get any feedback on the approach.


Index: bpf.c
===================================================================
--- bpf.c	(revision 242997)
+++ bpf.c	(working copy)
@@ -819,6 +819,7 @@ bpfopen(struct cdev *dev, int flags, int fmt, stru
	 * particular buffer method.
	 */
	bpf_buffer_init(d);
+	d->bd_hbuf_in_use = 0;
	d->bd_bufmode = BPF_BUFMODE_BUFFER;
	d->bd_sig = SIGIO;
	d->bd_direction = BPF_D_INOUT;
@@ -872,6 +873,9 @@ bpfread(struct cdev *dev, struct uio *uio, int iof
		callout_stop(&d->bd_callout);
	timed_out = (d->bd_state == BPF_TIMED_OUT);
	d->bd_state = BPF_IDLE;
+	while (d->bd_hbuf_in_use)
+		mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock,
+		    PRINET|PCATCH, "bd_hbuf", 0);
	/*
	 * If the hold buffer is empty, then do a timed sleep, which
	 * ends when the timeout expires or when enough packets
@@ -940,27 +944,27 @@ bpfread(struct cdev *dev, struct uio *uio, int iof
	/*
	 * At this point, we know we have something in the hold slot.
	 */
+	d->bd_hbuf_in_use = 1;
	BPFD_UNLOCK(d);

	/*
	 * 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.
-	 *
-	 * XXXRW: More synchronization needed here: what if a second thread
-	 * issues a read on the same fd at the same time?  Don't want this
-	 * getting invalidated.
+  	 *
+	 * We do not have to worry about simultaneous reads because
+	 * we waited for sole access to the hold buffer above.
	 */
	error = bpf_uiomove(d, d->bd_hbuf, d->bd_hlen, uio);

	BPFD_LOCK(d);
-	if (d->bd_hbuf != NULL) {
-		/* Free the hold buffer only if it is still valid. */
-		d->bd_fbuf = d->bd_hbuf;
-		d->bd_hbuf = NULL;
-		d->bd_hlen = 0;
-		bpf_buf_reclaimed(d);
-	}
+	KASSERT(d->bd_hbuf != NULL, ("bpfread: lost bd_hbuf"));
+	d->bd_fbuf = d->bd_hbuf;
+	d->bd_hbuf = NULL;
+	d->bd_hlen = 0;
+	bpf_buf_reclaimed(d);
+	d->bd_hbuf_in_use = 0;
+	wakeup(&d->bd_hbuf_in_use);
	BPFD_UNLOCK(d);

	return (error);
@@ -1114,6 +1118,9 @@ reset_d(struct bpf_d *d)

	BPFD_LOCK_ASSERT(d);

+	while (d->bd_hbuf_in_use)
+		mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock, PRINET,
+		    "bd_hbuf", 0);
	if ((d->bd_hbuf != NULL) &&
	    (d->bd_bufmode != BPF_BUFMODE_ZBUF || bpf_canfreebuf(d))) {
		/* Free the hold buffer. */
@@ -1254,6 +1261,9 @@ bpfioctl(struct cdev *dev, u_long cmd, caddr_t add

			BPFD_LOCK(d);
			n = d->bd_slen;
+			while (d->bd_hbuf_in_use)
+				mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock,
+				    PRINET, "bd_hbuf", 0);
			if (d->bd_hbuf)
				n += d->bd_hlen;
			BPFD_UNLOCK(d);
@@ -1967,6 +1977,9 @@ filt_bpfread(struct knote *kn, long hint)
	ready = bpf_ready(d);
	if (ready) {
		kn->kn_data = d->bd_slen;
+		while (d->bd_hbuf_in_use)
+			mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock,
+			    PRINET, "bd_hbuf", 0);
		if (d->bd_hbuf)
			kn->kn_data += d->bd_hlen;
	} else if (d->bd_rtout > 0 && d->bd_state == BPF_IDLE) {
@@ -2299,6 +2312,9 @@ catchpacket(struct bpf_d *d, u_char *pkt, u_int pk
	 * spot to do it.
	 */
	if (d->bd_fbuf == NULL && bpf_canfreebuf(d)) {
+		while (d->bd_hbuf_in_use)
+			mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock,
+			    PRINET, "bd_hbuf", 0);
		d->bd_fbuf = d->bd_hbuf;
		d->bd_hbuf = NULL;
		d->bd_hlen = 0;
@@ -2341,6 +2357,9 @@ catchpacket(struct bpf_d *d, u_char *pkt, u_int pk
			++d->bd_dcount;
			return;
		}
+		while (d->bd_hbuf_in_use)
+			mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock,
+			    PRINET, "bd_hbuf", 0);
		ROTATE_BUFFERS(d);
		do_wakeup = 1;
		curlen = 0;
Index: bpf.h
===================================================================
--- bpf.h	(revision 242997)
+++ bpf.h	(working copy)
@@ -1235,7 +1235,8 @@ SYSCTL_DECL(_net_bpf);
/*
 * Rotate the packet buffers in descriptor d.  Move the store buffer into the
 * hold slot, and the free buffer ino the store slot.  Zero the length of the
- * new store buffer.  Descriptor lock should be held.
+ * new store buffer.  Descriptor lock should be held. Hold buffer must
+ * not be marked "in use".
 */
#define	ROTATE_BUFFERS(d)	do {					\
	(d)->bd_hbuf = (d)->bd_sbuf;					\
Index: bpf_buffer.c
===================================================================
--- bpf_buffer.c	(revision 242997)
+++ bpf_buffer.c	(working copy)
@@ -189,6 +189,9 @@ bpf_buffer_ioctl_sblen(struct bpf_d *d, u_int *i)
		return (EINVAL);
	}

+	while (d->bd_hbuf_in_use)
+		mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock,
+		    PRINET, "bd_hbuf", 0);
	/* Free old buffers if set */
	if (d->bd_fbuf != NULL)
		free(d->bd_fbuf, M_BPF);
Index: bpfdesc.h
===================================================================
--- bpfdesc.h	(revision 242997)
+++ bpfdesc.h	(working copy)
@@ -63,6 +63,7 @@ struct bpf_d {
	caddr_t		bd_sbuf;	/* store slot */
	caddr_t		bd_hbuf;	/* hold slot */
	caddr_t		bd_fbuf;	/* free slot */
+	int		bd_hbuf_in_use;	/* don't rotate buffers */
	int 		bd_slen;	/* current length of store buffer */
	int 		bd_hlen;	/* current length of hold buffer */