Fix a deficiency in the selinfo interface:

If a selinfo object is recorded (via selrecord()) and then it is
quickly destroyed, with the waiters missing the opportunity to awake,
at the next iteration they will find the selinfo object destroyed,
causing a PF#.

That happens because the selinfo interface has no way to drain the
waiters before to destroy the registered selinfo object. Also this
race is quite rare to get in practice, because it would require a
selrecord(), a poll request by another thread and a quick destruction
of the selrecord()'ed selinfo object.

Fix this by adding the seldrain() routine which should be called
before to destroy the selinfo objects (in order to avoid such case),
and fix the present cases where it might have already been called.
Sometimes, the context is safe enough to prevent this type of race,
like it happens in device drivers which installs selinfo objects on
poll callbacks. There, the destruction of the selinfo object happens
at driver detach time, when all the filedescriptors should be already
closed, thus there cannot be a race.
For this case, mfi(4) device driver can be set as an example, as it
implements a full correct logic for preventing this from happening.

Sponsored by:	Sandvine Incorporated
Reported by:	rstone
Tested by:	pluknet
Reviewed by:	jhb, kib
Approved by:	re (bz)
MFC after:	3 weeks

14 files changed, 34 insertions, 1 deletions

diff --git a/sys/kern/kern_event.c b/sys/kern/kern_event.c
index c512b0a..dc11411 100644
--- a/sys/kern/kern_event.c
+++ b/sys/kern/kern_event.c
@@ -1704,6 +1704,7 @@ kqueue_close(struct file *fp, struct thread *td)
 	SLIST_REMOVE(&fdp->fd_kqlist, kq, kqueue, kq_list);
 	FILEDESC_XUNLOCK(fdp);
 
+	seldrain(&kq->kq_sel);
 	knlist_destroy(&kq->kq_sel.si_note);
 	mtx_destroy(&kq->kq_lock);
 	kq->kq_fdp = NULL;
diff --git a/sys/kern/sys_generic.c b/sys/kern/sys_generic.c
index 6edd4fb..7b45efa 100644
--- a/sys/kern/sys_generic.c
+++ b/sys/kern/sys_generic.c
@@ -1490,6 +1490,23 @@ selfdfree(struct seltd *stp, struct selfd *sfp)
 	uma_zfree(selfd_zone, sfp);
 }
 
+/* Drain the waiters tied to all the selfd belonging the specified selinfo. */
+void
+seldrain(sip)
+        struct selinfo *sip;
+{
+
+	/*
+	 * This feature is already provided by doselwakeup(), thus it is
+	 * enough to go for it.
+	 * Eventually, the context, should take care to avoid races
+	 * between thread calling select()/poll() and file descriptor
+	 * detaching, but, again, the races are just the same as
+	 * selwakeup().
+	 */
+        doselwakeup(sip, -1);
+}
+
 /*
  * Record a select request.
  */
diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
index 14e1207..c44a2c9 100644
--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
@@ -1517,6 +1517,7 @@ pipeclose(cpipe)
 	 */
 	knlist_clear(&cpipe->pipe_sel.si_note, 1);
 	cpipe->pipe_present = PIPE_FINALIZED;
+	seldrain(&cpipe->pipe_sel);
 	knlist_destroy(&cpipe->pipe_sel.si_note);
 
 	/*
diff --git a/sys/kern/tty.c b/sys/kern/tty.c
index 77c02dd..ce49f972 100644
--- a/sys/kern/tty.c
+++ b/sys/kern/tty.c
@@ -1022,6 +1022,8 @@ tty_dealloc(void *arg)
 	MPASS(ttyinq_getsize(&tp->t_inq) == 0);
 	MPASS(ttyoutq_getsize(&tp->t_outq) == 0);
 
+	seldrain(&tp->t_inpoll);
+	seldrain(&tp->t_outpoll);
 	knlist_destroy(&tp->t_inpoll.si_note);
 	knlist_destroy(&tp->t_outpoll.si_note);
 
diff --git a/sys/kern/tty_pts.c b/sys/kern/tty_pts.c
index cf9f94d..f2f5c4e 100644
--- a/sys/kern/tty_pts.c
+++ b/sys/kern/tty_pts.c
@@ -688,6 +688,8 @@ ptsdrv_free(void *softc)
 	racct_sub_cred(psc->pts_cred, RACCT_NPTS, 1);
 	crfree(psc->pts_cred);
 
+	seldrain(&psc->pts_inpoll);
+	seldrain(&psc->pts_outpoll);
 	knlist_destroy(&psc->pts_inpoll.si_note);
 	knlist_destroy(&psc->pts_outpoll.si_note);
 
diff --git a/sys/kern/uipc_mqueue.c b/sys/kern/uipc_mqueue.c
index fbd78c1..b91b890 100644
--- a/sys/kern/uipc_mqueue.c
+++ b/sys/kern/uipc_mqueue.c
@@ -1562,6 +1562,8 @@ mqueue_free(struct mqueue *mq)
 	}
 
 	mtx_destroy(&mq->mq_mutex);
+	seldrain(&mq->mq_rsel);
+	seldrain(&mq->mq_wsel);
 	knlist_destroy(&mq->mq_rsel.si_note);
 	knlist_destroy(&mq->mq_wsel.si_note);
 	uma_zfree(mqueue_zone, mq);
diff --git a/sys/kern/uipc_socket.c b/sys/kern/uipc_socket.c
index 990c6ba..bbd4fad 100644
--- a/sys/kern/uipc_socket.c
+++ b/sys/kern/uipc_socket.c
@@ -661,6 +661,8 @@ sofree(struct socket *so)
 	 */
 	sbdestroy(&so->so_snd, so);
 	sbdestroy(&so->so_rcv, so);
+	seldrain(&so->so_snd.sb_sel);
+	seldrain(&so->so_rcv.sb_sel);
 	knlist_destroy(&so->so_rcv.sb_sel.si_note);
 	knlist_destroy(&so->so_snd.sb_sel.si_note);
 	sodealloc(so);
diff --git a/sys/kern/vfs_subr.c b/sys/kern/vfs_subr.c
index a9fe8d1..325ca99 100644
--- a/sys/kern/vfs_subr.c
+++ b/sys/kern/vfs_subr.c
@@ -3312,6 +3312,7 @@ vbusy(struct vnode *vp)
 static void
 destroy_vpollinfo(struct vpollinfo *vi)
 {
+	seldrain(&vi->vpi_selinfo);
 	knlist_destroy(&vi->vpi_selinfo.si_note);
 	mtx_destroy(&vi->vpi_lock);
 	uma_zfree(vnodepoll_zone, vi);
diff --git a/sys/net/bpf.c b/sys/net/bpf.c
index e516573..79c77a9 100644
--- a/sys/net/bpf.c
+++ b/sys/net/bpf.c
@@ -652,10 +652,10 @@ bpf_dtor(void *data)
 	if (d->bd_bif)
 		bpf_detachd(d);
 	mtx_unlock(&bpf_mtx);
-	selwakeuppri(&d->bd_sel, PRINET);
 #ifdef MAC
 	mac_bpfdesc_destroy(d);
 #endif /* MAC */
+	seldrain(&d->bd_sel);
 	knlist_destroy(&d->bd_sel.si_note);
 	callout_drain(&d->bd_callout);
 	bpf_freed(d);
diff --git a/sys/net/if_tap.c b/sys/net/if_tap.c
index ad29da0..08c669a 100644
--- a/sys/net/if_tap.c
+++ b/sys/net/if_tap.c
@@ -214,6 +214,7 @@ tap_destroy(struct tap_softc *tp)
 	KASSERT(!(tp->tap_flags & TAP_OPEN),
 		("%s flags is out of sync", ifp->if_xname));
 
+	seldrain(&tp->tap_rsel);
 	knlist_destroy(&tp->tap_rsel.si_note);
 	destroy_dev(tp->tap_dev);
 	ether_ifdetach(ifp);
diff --git a/sys/net/if_tun.c b/sys/net/if_tun.c
index d74c9fe..c532884 100644
--- a/sys/net/if_tun.c
+++ b/sys/net/if_tun.c
@@ -259,6 +259,7 @@ tun_destroy(struct tun_softc *tp)
 	if_detach(TUN2IFP(tp));
 	if_free(TUN2IFP(tp));
 	destroy_dev(dev);
+	seldrain(&tp->tun_rsel);
 	knlist_destroy(&tp->tun_rsel.si_note);
 	mtx_destroy(&tp->tun_mtx);
 	cv_destroy(&tp->tun_cv);
diff --git a/sys/security/audit/audit_pipe.c b/sys/security/audit/audit_pipe.c
index a8db113..a953eb0 100644
--- a/sys/security/audit/audit_pipe.c
+++ b/sys/security/audit/audit_pipe.c
@@ -646,6 +646,7 @@ audit_pipe_free(struct audit_pipe *ap)
 	cv_destroy(&ap->ap_cv);
 	AUDIT_PIPE_SX_LOCK_DESTROY(ap);
 	AUDIT_PIPE_LOCK_DESTROY(ap);
+	seldrain(&ap->ap_selinfo);
 	knlist_destroy(&ap->ap_selinfo.si_note);
 	TAILQ_REMOVE(&audit_pipe_list, ap, ap_list);
 	free(ap, M_AUDIT_PIPE);
diff --git a/sys/sys/selinfo.h b/sys/sys/selinfo.h
index 2d2f848..590d184 100644
--- a/sys/sys/selinfo.h
+++ b/sys/sys/selinfo.h
@@ -51,6 +51,7 @@ struct selinfo {
 #define	SEL_WAITING(si)		(!TAILQ_EMPTY(&(si)->si_tdlist))
 
 #ifdef _KERNEL
+void	seldrain(struct selinfo *sip);
 void	selrecord(struct thread *selector, struct selinfo *sip);
 void	selwakeup(struct selinfo *sip);
 void	selwakeuppri(struct selinfo *sip, int pri);
diff --git a/sys/x86/acpica/acpi_apm.c b/sys/x86/acpica/acpi_apm.c
index 02be6e0..776b1be 100644
--- a/sys/x86/acpica/acpi_apm.c
+++ b/sys/x86/acpica/acpi_apm.c
@@ -297,6 +297,7 @@ apmclose(struct cdev *dev, int flag, int fmt, struct thread *td)
 	/* Remove this clone's data from the list and free it. */
 	ACPI_LOCK(acpi);
 	STAILQ_REMOVE(&acpi_sc->apm_cdevs, clone, apm_clone_data, entries);
+	seldrain(&clone->sel_read);
 	knlist_destroy(&clone->sel_read.si_note);
 	ACPI_UNLOCK(acpi);
 	free(clone, M_APMDEV);