From owner-freebsd-net@FreeBSD.ORG Tue Nov 13 21:41:07 2012 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 2E9A43E3 for ; Tue, 13 Nov 2012 21:41:07 +0000 (UTC) (envelope-from guy.helmer@gmail.com) Received: from mail-ye0-f182.google.com (mail-ye0-f182.google.com [209.85.213.182]) by mx1.freebsd.org (Postfix) with ESMTP id D8E468FC12 for ; Tue, 13 Nov 2012 21:41:06 +0000 (UTC) Received: by mail-ye0-f182.google.com with SMTP id q9so292434yen.13 for ; Tue, 13 Nov 2012 13:41:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:content-type:content-transfer-encoding:subject:message-id:date :to:mime-version:x-mailer; bh=XQuJdMukwlqNaS+LgrxwRdbsDAM3TUPoccMxxxND89o=; b=PkzcEBja5NGB/K7Kk0svMtRulyV9+dIBQPoKXUhQ7ZMR1remcYvoonnPbUlEj2x59/ PyGAzYfCpbNA/n8/tMq91JiEVdFetVN9/FRYgGjPu10Lz2RDWiNWNc5Exc3mxtMyavNe Wct6Iqh+lfyJGPpMWcfzPSr4t51RW2kQSl8Kbi0hnDvDYLsAI0TXs3LLt74oopGESXVV tXE3+bCbcle3FDNcShNsNxy/v6EkntV3DkZVADKml/JOqRYXJYZlZdk030Ths3n4zZZy T6wU+Wxgc7SIE2H83cshw5G3Z7LIToAo8fGi/Iuveje+GU+MEoDIwyxyAxpTBAynoebQ yVNA== Received: by 10.100.250.2 with SMTP id x2mr6729629anh.14.1352842860747; Tue, 13 Nov 2012 13:41:00 -0800 (PST) Received: from guysmbp.dyn.palisadesys.com ([216.81.189.10]) by mx.google.com with ESMTPS id u22sm10999516yhl.2.2012.11.13.13.40.59 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 13 Nov 2012 13:41:00 -0800 (PST) From: Guy Helmer Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Subject: bpf hold buffer in-use flag Message-Id: <9C928117-2230-4F01-9B95-B6D945AF4416@gmail.com> Date: Tue, 13 Nov 2012 15:40:57 -0600 To: "freebsd-net@freebsd.org" Mime-Version: 1.0 (Mac OS X Mail 6.2 \(1499\)) X-Mailer: Apple Mail (2.1499) X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Nov 2012 21:41:07 -0000 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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- 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 =3D 0; d->bd_bufmode =3D BPF_BUFMODE_BUFFER; d->bd_sig =3D SIGIO; d->bd_direction =3D BPF_D_INOUT; @@ -872,6 +873,9 @@ bpfread(struct cdev *dev, struct uio *uio, int iof callout_stop(&d->bd_callout); timed_out =3D (d->bd_state =3D=3D BPF_TIMED_OUT); d->bd_state =3D 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 =3D 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 =3D bpf_uiomove(d, d->bd_hbuf, d->bd_hlen, uio); BPFD_LOCK(d); - if (d->bd_hbuf !=3D NULL) { - /* Free the hold buffer only if it is still valid. */ - d->bd_fbuf =3D d->bd_hbuf; - d->bd_hbuf =3D NULL; - d->bd_hlen =3D 0; - bpf_buf_reclaimed(d); - } + KASSERT(d->bd_hbuf !=3D NULL, ("bpfread: lost bd_hbuf")); + d->bd_fbuf =3D d->bd_hbuf; + d->bd_hbuf =3D NULL; + d->bd_hlen =3D 0; + bpf_buf_reclaimed(d); + d->bd_hbuf_in_use =3D 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 !=3D NULL) && (d->bd_bufmode !=3D 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 =3D 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 +=3D d->bd_hlen; BPFD_UNLOCK(d); @@ -1967,6 +1977,9 @@ filt_bpfread(struct knote *kn, long hint) ready =3D bpf_ready(d); if (ready) { kn->kn_data =3D 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 +=3D d->bd_hlen; } else if (d->bd_rtout > 0 && d->bd_state =3D=3D 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 =3D=3D 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 =3D d->bd_hbuf; d->bd_hbuf =3D NULL; d->bd_hlen =3D 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 =3D 1; curlen =3D 0; Index: bpf.h =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- 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 =3D (d)->bd_sbuf; = \ Index: bpf_buffer.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- 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 !=3D NULL) free(d->bd_fbuf, M_BPF); Index: bpfdesc.h =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- 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 = */