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 */
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[email protected]"
