From owner-svn-src-all@FreeBSD.ORG Mon Dec 10 16:14:45 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 74EAB207; Mon, 10 Dec 2012 16:14:45 +0000 (UTC) (envelope-from ghelmer@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:1900:2254:2068::e6a:0]) by mx1.freebsd.org (Postfix) with ESMTP id 599438FC08; Mon, 10 Dec 2012 16:14:45 +0000 (UTC) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.5/8.14.5) with ESMTP id qBAGEjDp053844; Mon, 10 Dec 2012 16:14:45 GMT (envelope-from ghelmer@svn.freebsd.org) Received: (from ghelmer@localhost) by svn.freebsd.org (8.14.5/8.14.5/Submit) id qBAGEixe053838; Mon, 10 Dec 2012 16:14:44 GMT (envelope-from ghelmer@svn.freebsd.org) Message-Id: <201212101614.qBAGEixe053838@svn.freebsd.org> From: Guy Helmer Date: Mon, 10 Dec 2012 16:14:44 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r244090 - head/sys/net X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Dec 2012 16:14:45 -0000 Author: ghelmer Date: Mon Dec 10 16:14:44 2012 New Revision: 244090 URL: http://svnweb.freebsd.org/changeset/base/244090 Log: Changes to resolve races in bpfread() and catchpacket() that, at worst, cause kernel panics. Add a flag to the bpf descriptor to indicate whether the hold buffer is in use. In bpfread(), set the "hold buffer in use" flag before dropping the descriptor lock during the call to bpf_uiomove(). Everywhere else the hold buffer is used or changed, wait while the hold buffer is in use by bpfread(). Add a KASSERT in bpfread() after re-acquiring the descriptor lock to assist uncovering any additional hold buffer races. Modified: head/sys/net/bpf.c head/sys/net/bpf.h head/sys/net/bpf_buffer.c head/sys/net/bpfdesc.h Modified: head/sys/net/bpf.c ============================================================================== --- head/sys/net/bpf.c Mon Dec 10 15:29:56 2012 (r244089) +++ head/sys/net/bpf.c Mon Dec 10 16:14:44 2012 (r244090) @@ -819,6 +819,7 @@ bpfopen(struct cdev *dev, int flags, int * 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 *ui 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 *ui /* * 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, c 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 * 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 ++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; Modified: head/sys/net/bpf.h ============================================================================== --- head/sys/net/bpf.h Mon Dec 10 15:29:56 2012 (r244089) +++ head/sys/net/bpf.h Mon Dec 10 16:14:44 2012 (r244090) @@ -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; \ Modified: head/sys/net/bpf_buffer.c ============================================================================== --- head/sys/net/bpf_buffer.c Mon Dec 10 15:29:56 2012 (r244089) +++ head/sys/net/bpf_buffer.c Mon Dec 10 16:14:44 2012 (r244090) @@ -79,6 +79,8 @@ __FBSDID("$FreeBSD$"); #include #include +#define PRINET 26 /* interruptible */ + /* * Implement historical kernel memory buffering model for BPF: two malloc(9) * kernel buffers are hung off of the descriptor. The size is fixed prior to @@ -189,6 +191,9 @@ bpf_buffer_ioctl_sblen(struct bpf_d *d, 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); Modified: head/sys/net/bpfdesc.h ============================================================================== --- head/sys/net/bpfdesc.h Mon Dec 10 15:29:56 2012 (r244089) +++ head/sys/net/bpfdesc.h Mon Dec 10 16:14:44 2012 (r244090) @@ -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 */