svn commit: r346671 - head/sys/net

[Kyle Evans]

Author: kevans
Date: Thu Apr 25 13:46:12 2019
New Revision: 346671
URL: https://svnweb.freebsd.org/changeset/base/346671

Log:
  tun(4): Don't allow open of open or dying devices
  
  Previously, a pid check was used to prevent open of the tun(4); this works,
  but may not make the most sense as we don't prevent the owner process from
  opening the tun device multiple times.
  
  The potential race described near tun_pid should not be an issue: if a
  tun(4) is to be handed off, its fd has to have been sent via control message
  or some other mechanism that duplicates the fd to the receiving process so
  that it may set the pid. Otherwise, the pid gets cleared when the original
  process closes it and you have no effective handoff mechanism.
  
  Close up another potential issue with handing a tun(4) off by not clobbering
  state if the closer isn't the controller anymore. If we want some state to
  be cleared, we should do that a little more surgically.
  
  Additionally, nothing prevents a dying tun(4) from being "reopened" in the
  middle of tun_destroy as soon as the mutex is unlocked, quickly leading to a
  bad time. Return EBUSY if we're marked for destruction, as well, and the
  consumer will need to deal with it. The associated character device will be
  destroyed in short order.
  
  MFC after:	2 weeks
  Differential Revision:	https://reviews.freebsd.org/D20033

Modified:
  head/sys/net/if_tun.c

Modified: head/sys/net/if_tun.c
==============================================================================
--- head/sys/net/if_tun.c	Thu Apr 25 12:44:08 2019	(r346670)
+++ head/sys/net/if_tun.c	Thu Apr 25 13:46:12 2019	(r346671)
@@ -81,16 +81,10 @@ struct tun_softc {
 #define	TUN_RWAIT	0x0040
 #define	TUN_ASYNC	0x0080
 #define	TUN_IFHEAD	0x0100
+#define	TUN_DYING	0x0200
 
 #define TUN_READY       (TUN_OPEN | TUN_INITED)
 
-	/*
-	 * XXXRW: tun_pid is used to exclusively lock /dev/tun.  Is this
-	 * actually needed?  Can we just return EBUSY if already open?
-	 * Problem is that this involved inherent races when a tun device
-	 * is handed off from one process to another, as opposed to just
-	 * being slightly stale informationally.
-	 */
 	pid_t	tun_pid;		/* owning pid */
 	struct	ifnet *tun_ifp;		/* the interface */
 	struct  sigio *tun_sigio;	/* information for async I/O */
@@ -277,6 +271,7 @@ tun_destroy(struct tun_softc *tp)
 	struct cdev *dev;
 
 	mtx_lock(&tp->tun_mtx);
+	tp->tun_flags |= TUN_DYING;
 	if ((tp->tun_flags & TUN_OPEN) != 0)
 		cv_wait_unlock(&tp->tun_cv, &tp->tun_mtx);
 	else
@@ -473,19 +468,13 @@ tunopen(struct cdev *dev, int flag, int mode, struct t
 		tp = dev->si_drv1;
 	}
 
-	/*
-	 * XXXRW: This use of tun_pid is subject to error due to the
-	 * fact that a reference to the tunnel can live beyond the
-	 * death of the process that created it.  Can we replace this
-	 * with a simple busy flag?
-	 */
 	mtx_lock(&tp->tun_mtx);
-	if (tp->tun_pid != 0 && tp->tun_pid != td->td_proc->p_pid) {
+	if ((tp->tun_flags & (TUN_OPEN | TUN_DYING)) != 0) {
 		mtx_unlock(&tp->tun_mtx);
 		return (EBUSY);
 	}
-	tp->tun_pid = td->td_proc->p_pid;
 
+	tp->tun_pid = td->td_proc->p_pid;
 	tp->tun_flags |= TUN_OPEN;
 	ifp = TUN2IFP(tp);
 	if_link_state_change(ifp, LINK_STATE_UP);
@@ -509,6 +498,16 @@ tunclose(struct cdev *dev, int foo, int bar, struct th
 	ifp = TUN2IFP(tp);
 
 	mtx_lock(&tp->tun_mtx);
+	/*
+	 * Simply close the device if this isn't the controlling process.  This
+	 * may happen if, for instance, the tunnel has been handed off to
+	 * another process.  The original controller should be able to close it
+	 * without putting us into an inconsistent state.
+	 */
+	if (td->td_proc->p_pid != tp->tun_pid) {
+		mtx_unlock(&tp->tun_mtx);
+		return (0);
+	}
 
 	/*
 	 * junk all pending output