Guy,
Can you please describe the race and how to reproduce it? When we introduced
zerocopy-bpf, we also introduced bpf_bufheld() and bpf_bufreclaimed() which are
effectively hops for the regular buffer mode. Maybe we could maintain state
there as well? In any case, I would like to understand the race/and reproduce it
Thanks!
On 2012-11-13, at 3:40 PM, Guy Helmer wrote:
> 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 */
>
> _______________________________________________
> freebsd-net@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
