bpf hold buffer in-use flag

[Christian Peron]

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