kern/55664: rump race condition

[nruslan_devel%yahoo.com@localhost]

>Number:         55664
>Category:       kern
>Synopsis:       rump race condition
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Sep 16 22:10:00 +0000 2020
>Originator:     Ruslan Nikolaev
>Release:        master
>Organization:
Virginia Tech
>Environment:
>Description:
While working on the SMP version of rumprun (https://github.com/ssrg-vt/rumprun-smp), we found several issues:

1. A race condition (bug) in sys/rump/librump/rumpkern/intr.c since rumpuser_cv_signal() is called without holding a mutex

2. sleepq is implemented using a single (global) conditional variable; that should be done per each sleepq separately

Below I attach a patch (relocated) for the master branch. Please let us know what you think. We also have some other fixes for rump but they should probably be considered separately.
>How-To-Repeat:

>Fix:
diff --git a/sys/kern/kern_condvar.c b/sys/kern/kern_condvar.c
index cb58236fcb05..460e077b526f 100644
--- a/sys/kern/kern_condvar.c
+++ b/sys/kern/kern_condvar.c
@@ -102,6 +102,7 @@ void
 cv_destroy(kcondvar_t *cv)
 {
 
+	sleepq_destroy(CV_SLEEPQ(cv));
 #ifdef DIAGNOSTIC
 	KASSERT(cv_is_valid(cv));
 	KASSERT(!cv_has_waiters(cv));
diff --git a/sys/rump/include/rump-sys/kern.h b/sys/rump/include/rump-sys/kern.h
index cac0b96d0698..a54012df9d01 100644
--- a/sys/rump/include/rump-sys/kern.h
+++ b/sys/rump/include/rump-sys/kern.h
@@ -174,6 +174,7 @@ void	rump_syscall_boot_establish(const struct rump_onesyscall *, size_t);
 void	rump_schedlock_cv_wait(struct rumpuser_cv *);
 int	rump_schedlock_cv_timedwait(struct rumpuser_cv *,
 				    const struct timespec *);
+void	rump_schedlock_cv_signal(struct cpu_info *, struct rumpuser_cv *);
 
 void	rump_user_schedule(int, void *);
 void	rump_user_unschedule(int, int *, void *);
diff --git a/sys/rump/librump/rumpkern/intr.c b/sys/rump/librump/rumpkern/intr.c
index 651665ff11bb..829154f187b2 100644
--- a/sys/rump/librump/rumpkern/intr.c
+++ b/sys/rump/librump/rumpkern/intr.c
@@ -464,7 +464,7 @@ rump_softint_run(struct cpu_info *ci)
 
 	for (i = 0; i < SOFTINT_COUNT; i++) {
 		if (!TAILQ_EMPTY(&si_lvl[i].si_pending))
-			rumpuser_cv_signal(si_lvl[i].si_cv);
+			rump_schedlock_cv_signal(ci, si_lvl[i].si_cv);
 	}
 }
 
diff --git a/sys/rump/librump/rumpkern/scheduler.c b/sys/rump/librump/rumpkern/scheduler.c
index 8b4a519c8cda..231cacba3ac9 100644
--- a/sys/rump/librump/rumpkern/scheduler.c
+++ b/sys/rump/librump/rumpkern/scheduler.c
@@ -179,6 +179,16 @@ rump_scheduler_init(int numcpu)
 	mutex_init(&unruntime_lock, MUTEX_DEFAULT, IPL_SCHED);
 }
 
+void
+rump_schedlock_cv_signal(struct cpu_info *ci, struct rumpuser_cv *cv)
+{
+	struct rumpcpu *rcpu = cpuinfo_to_rumpcpu(ci);
+
+	rumpuser_mutex_enter_nowrap(rcpu->rcpu_mtx);
+	rumpuser_cv_signal(cv);
+	rumpuser_mutex_exit(rcpu->rcpu_mtx);
+}
+
 /*
  * condvar ops using scheduler lock as the rumpuser interlock.
  */
diff --git a/sys/rump/librump/rumpkern/sleepq.c b/sys/rump/librump/rumpkern/sleepq.c
index 4ce5142226cb..b047a03166b8 100644
--- a/sys/rump/librump/rumpkern/sleepq.c
+++ b/sys/rump/librump/rumpkern/sleepq.c
@@ -40,25 +40,20 @@ __KERNEL_RCSID(0, "$NetBSD: sleepq.c,v 1.20 2020/04/25 15:42:15 bouyer Exp $");
 #include <rump-sys/kern.h>
 
 syncobj_t sleep_syncobj;
-static kcondvar_t sq_cv;
 
-static int
-sqinit1(void)
+void
+sleepq_init(sleepq_t *sq)
 {
 
-	cv_init(&sq_cv, "sleepq");
-
-	return 0;
+	LIST_INIT(sq);
+	cv_init(&sq->sq_cv, "sleepq");
 }
 
 void
-sleepq_init(sleepq_t *sq)
+sleepq_destroy(sleepq_t *sq)
 {
-	static ONCE_DECL(sqctl);
-
-	RUN_ONCE(&sqctl, sqinit1);
 
-	LIST_INIT(sq);
+	cv_destroy(&sq->sq_cv);
 }
 
 void
@@ -83,7 +78,7 @@ sleepq_block(int timo, bool catch)
 
 	while (l->l_wchan) {
 		l->l_mutex = mp; /* keep sleepq lock until woken up */
-		error = cv_timedwait(&sq_cv, mp, timo);
+		error = cv_timedwait(&l->l_sleepq->sq_cv, mp, timo);
 		if (error == EWOULDBLOCK || error == EINTR) {
 			if (l->l_wchan) {
 				LIST_REMOVE(l, l_sleepchain);
@@ -118,7 +113,7 @@ sleepq_wake(sleepq_t *sq, wchan_t wchan, u_int expected, kmutex_t *mp)
 		}
 	}
 	if (found)
-		cv_broadcast(&sq_cv);
+		cv_broadcast(&sq->sq_cv);
 
 	mutex_spin_exit(mp);
 }
@@ -130,7 +125,7 @@ sleepq_unsleep(struct lwp *l, bool cleanup)
 	l->l_wchan = NULL;
 	l->l_wmesg = NULL;
 	LIST_REMOVE(l, l_sleepchain);
-	cv_broadcast(&sq_cv);
+	cv_broadcast(&l->l_sleepq->sq_cv);
 
 	if (cleanup) {
 		mutex_spin_exit(l->l_mutex);
diff --git a/sys/sys/sleepq.h b/sys/sys/sleepq.h
index c721837bb635..c3f502cf0246 100644
--- a/sys/sys/sleepq.h
+++ b/sys/sys/sleepq.h
@@ -45,13 +45,21 @@
  * Generic sleep queues.
  */
 
+#ifdef _RUMPKERNEL
+# include <sys/condvar.h>
+struct sleepq {
+	LIST_HEAD(, lwp);	/* anonymous struct */
+	kcondvar_t sq_cv;
+};
+#else
+LIST_HEAD(sleepq, lwp);
+#endif
+
 #define	SLEEPTAB_HASH_SHIFT	7
 #define	SLEEPTAB_HASH_SIZE	(1 << SLEEPTAB_HASH_SHIFT)
 #define	SLEEPTAB_HASH_MASK	(SLEEPTAB_HASH_SIZE - 1)
 #define	SLEEPTAB_HASH(wchan)	(((uintptr_t)(wchan) >> 8) & SLEEPTAB_HASH_MASK)
 
-LIST_HEAD(sleepq, lwp);
-
 typedef struct sleepq sleepq_t;
 
 typedef struct sleeptab {
@@ -72,6 +80,16 @@ void	sleepq_changepri(lwp_t *, pri_t);
 void	sleepq_lendpri(lwp_t *, pri_t);
 int	sleepq_block(int, bool);
 
+#ifdef _RUMPKERNEL
+void
+sleepq_destroy(sleepq_t *);
+#else
+static inline void
+sleepq_destroy(sleepq_t *sq)
+{
+}
+#endif
+
 void	sleeptab_init(sleeptab_t *);
 
 extern sleeptab_t	sleeptab;