svn commit: r354659 - head/usr.sbin/bhyve

[Vincenzo Maffione]

Author: vmaffione
Date: Tue Nov 12 21:07:51 2019
New Revision: 354659
URL: https://svnweb.freebsd.org/changeset/base/354659

Log:
  bhyve: rework mevent processing to fix a race condition
  
  At the end of both mevent_add() and mevent_update(), mevent_notify()
  is called to wakeup the I/O thread, that will call kevent(changelist)
  to update the kernel.
  A race condition is possible where the client calls mevent_add() and
  mevent_update(EV_ENABLE) before the I/O thread has the chance to wake
  up and call mevent_build()+kevent(changelist) in response to mevent_add().
  The mevent_add() is therefore ignored by the I/O thread, and
  kevent(fd, EV_ENABLE) is called before kevent(fd, EV_ADD), resuliting
  in a failure of the kevent(fd, EV_ENABLE) call.
  
  PR:	241808
  Reviewed by:	jhb, markj
  MFC with:	r354288
  Differential Revision:	https://reviews.freebsd.org/D22286

Modified:
  head/usr.sbin/bhyve/mevent.c

Modified: head/usr.sbin/bhyve/mevent.c
==============================================================================
--- head/usr.sbin/bhyve/mevent.c	Tue Nov 12 19:35:46 2019	(r354658)
+++ head/usr.sbin/bhyve/mevent.c	Tue Nov 12 21:07:51 2019	(r354659)
@@ -42,6 +42,7 @@ __FBSDID("$FreeBSD$");
 #endif
 #include <err.h>
 #include <errno.h>
+#include <stdbool.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
@@ -62,12 +63,6 @@ __FBSDID("$FreeBSD$");
 
 #define	MEVENT_MAX	64
 
-#define	MEV_ADD			1
-#define	MEV_ENABLE		2
-#define	MEV_DISABLE		3
-#define	MEV_DEL_PENDING		4
-#define	MEV_ADD_DISABLED	5
-
 extern char *vmname;
 
 static pthread_t mevent_tid;
@@ -83,7 +78,7 @@ struct mevent {
 	enum ev_type me_type;
 	void    *me_param;
 	int	me_cq;
-	int	me_state;
+	int	me_state; /* Desired kevent flags. */
 	int	me_closefd;
 	LIST_ENTRY(mevent) me_list;
 };
@@ -156,30 +151,7 @@ mevent_kq_filter(struct mevent *mevp)
 static int
 mevent_kq_flags(struct mevent *mevp)
 {
-	int ret;
-
-	switch (mevp->me_state) {
-	case MEV_ADD:
-		ret = EV_ADD;		/* implicitly enabled */
-		break;
-	case MEV_ADD_DISABLED:
-		ret = EV_ADD | EV_DISABLE;
-		break;
-	case MEV_ENABLE:
-		ret = EV_ENABLE;
-		break;
-	case MEV_DISABLE:
-		ret = EV_DISABLE;
-		break;
-	case MEV_DEL_PENDING:
-		ret = EV_DELETE;
-		break;
-	default:
-		assert(0);
-		break;
-	}
-
-	return (ret);
+	return (mevp->me_state);
 }
 
 static int
@@ -224,9 +196,15 @@ mevent_build(int mfd, struct kevent *kev)
 		mevp->me_cq = 0;
 		LIST_REMOVE(mevp, me_list);
 
-		if (mevp->me_state == MEV_DEL_PENDING) {
+		if (mevp->me_state & EV_DELETE) {
 			free(mevp);
 		} else {
+			/*
+			 * We need to add the event only once, so we can
+			 * reset the EV_ADD bit after it has been propagated
+			 * to the kevent() arguments the first time.
+			 */
+			mevp->me_state &= ~EV_ADD;
 			LIST_INSERT_HEAD(&global_head, mevp, me_list);
 		}
 
@@ -318,7 +296,7 @@ mevent_add(int tfd, enum ev_type type,
 	   void (*func)(int, enum ev_type, void *), void *param)
 {
 
-	return mevent_add_state(tfd, type, func, param, MEV_ADD);
+	return (mevent_add_state(tfd, type, func, param, EV_ADD));
 }
 
 struct mevent *
@@ -326,36 +304,46 @@ mevent_add_disabled(int tfd, enum ev_type type,
 		    void (*func)(int, enum ev_type, void *), void *param)
 {
 
-	return mevent_add_state(tfd, type, func, param, MEV_ADD_DISABLED);
+	return (mevent_add_state(tfd, type, func, param, EV_ADD | EV_DISABLE));
 }
 
 static int
-mevent_update(struct mevent *evp, int newstate)
+mevent_update(struct mevent *evp, bool enable)
 {
+	int newstate;
+
+	mevent_qlock();
+
 	/*
 	 * It's not possible to enable/disable a deleted event
 	 */
-	if (evp->me_state == MEV_DEL_PENDING)
-		return (EINVAL);
+	assert((evp->me_state & EV_DELETE) == 0);
 
+	newstate = evp->me_state;
+	if (enable) {
+		newstate |= EV_ENABLE;
+		newstate &= ~EV_DISABLE;
+	} else {
+		newstate |= EV_DISABLE;
+		newstate &= ~EV_ENABLE;
+	}
+
 	/*
 	 * No update needed if state isn't changing
 	 */
-	if (evp->me_state == newstate)
-		return (0);
-	
-	mevent_qlock();
+	if (evp->me_state != newstate) {
+		evp->me_state = newstate;
 
-	evp->me_state = newstate;
-
-	/*
-	 * Place the entry onto the changed list if not already there.
-	 */
-	if (evp->me_cq == 0) {
-		evp->me_cq = 1;
-		LIST_REMOVE(evp, me_list);
-		LIST_INSERT_HEAD(&change_head, evp, me_list);
-		mevent_notify();
+		/*
+		 * Place the entry onto the changed list if not
+		 * already there.
+		 */
+		if (evp->me_cq == 0) {
+			evp->me_cq = 1;
+			LIST_REMOVE(evp, me_list);
+			LIST_INSERT_HEAD(&change_head, evp, me_list);
+			mevent_notify();
+		}
 	}
 
 	mevent_qunlock();
@@ -367,14 +355,14 @@ int
 mevent_enable(struct mevent *evp)
 {
 
-	return (mevent_update(evp, MEV_ENABLE));
+	return (mevent_update(evp, true));
 }
 
 int
 mevent_disable(struct mevent *evp)
 {
 
-	return (mevent_update(evp, MEV_DISABLE));
+	return (mevent_update(evp, false));
 }
 
 static int
@@ -392,7 +380,7 @@ mevent_delete_event(struct mevent *evp, int closefd)
 		LIST_INSERT_HEAD(&change_head, evp, me_list);
 		mevent_notify();
         }
-	evp->me_state = MEV_DEL_PENDING;
+	evp->me_state = EV_DELETE;
 
 	if (closefd)
 		evp->me_closefd = 1;