Re: kern/57705: rump doesn't have a way to interrupt rump_sys_* by a signal

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

The attached patch implements a rump signal model,
RUMP_SIGMODEL_RECORD_WAKEUP, for use with rump_boot_setsigmodel, that
makes rump signal delivery record the signal in the process and wake
up any cv_wait_sig or cv_timedwait_sig in progress.  And it adds a
function rump_lwp_kill(l, signo) to simulate signal delivery.

Combined with RUMP_SIGMODEL_RECORD_WAKEUP, rump_lwp_kill will cause
pending cv_wait_sig or cv_timedwait_sig to wake, and cause subsequent
cv_wait_sig or cv_timedwait_sig to fail with EINTR without sleeping.
This is a new signal model so it won't break any existing code
(although in principle the extra logic in cv_wait_sig and
cv_timedwait_sig could break code because it's not conditional on
RUMP_SIGMODEL_RECORD_WAKEUP).

It's a little bodgy:

1. It takes proc_lock in any cv_wait_sig or cv_timedwait_sig call --
   perhaps we can relax this to use a narrower-scoped lock, like
   l->l_mutex (if that works), so it doesn't become a point of
   contention in all rump code.

2. It delivers the signal to all lwps in a process, not just the
   requested one -- should be a simple matter of programming to make
   it work for just the requested one, but this serves my testing
   needs for now.

3. Nothing clears the signal, so subsequent cv_wait_sig or
   cv_timedwait_sig will continue to fail with EINTR without sleeping.
   Either the caller of rump_sys_* will need to do something to clear
   the pending signal (i.e., pretend to `handle' it), or we'll need to
   invent some way for rump_sys_* itself to clear it.

But this serves my needs for the moment, which is to test the fix for
the PR kern/57703 eventfd panic without crashing my development
laptop.

From ec313e6d40f530da7a2fd5a823c1c71e99e36fc0 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sun, 19 Nov 2023 01:14:21 +0000
Subject: [PATCH] rump: New mechanism to interrupt rump_sys_* as if by a
 signal.

PR kern/57705
---
 sys/rump/include/rump/rump.h        |  5 ++-
 sys/rump/librump/rumpkern/locks.c   | 52 ++++++++++++++++++++----
 sys/rump/librump/rumpkern/signals.c | 28 +++++++++++++
 tests/rump/rumpkern/t_signals.c     | 62 +++++++++++++++++++++++++++++
 4 files changed, 139 insertions(+), 8 deletions(-)

diff --git a/sys/rump/include/rump/rump.h b/sys/rump/include/rump/rump.h
index a4679d225fe7..48d1b794c95e 100644
--- a/sys/rump/include/rump/rump.h
+++ b/sys/rump/include/rump/rump.h
@@ -64,7 +64,8 @@ enum rump_sigmodel {
 	RUMP_SIGMODEL_IGNORE,
 	RUMP_SIGMODEL__HOST_NOTANYMORE,
 	RUMP_SIGMODEL_RAISE,
-	RUMP_SIGMODEL_RECORD
+	RUMP_SIGMODEL_RECORD,
+	RUMP_SIGMODEL_RECORD_WAKEUP,
 };
 
 /* flags to rump_lwproc_rfork */
@@ -124,6 +125,8 @@ int	rump_init_server(const char *);
 int	rump_daemonize_done(int);
 #define RUMP_DAEMONIZE_SUCCESS 0
 
+void	rump_lwp_kill(struct lwp *, int);
+
 #ifndef _KERNEL
 #include <rump/rumpkern_if_pub.h>
 #include <rump/rumpvfs_if_pub.h>
diff --git a/sys/rump/librump/rumpkern/locks.c b/sys/rump/librump/rumpkern/locks.c
index 1b60b8e70706..bb3931708102 100644
--- a/sys/rump/librump/rumpkern/locks.c
+++ b/sys/rump/librump/rumpkern/locks.c
@@ -378,7 +378,7 @@ cv_destroy(kcondvar_t *cv)
 }
 
 static int
-docvwait(kcondvar_t *cv, kmutex_t *mtx, struct timespec *ts)
+docvwait(kcondvar_t *cv, kmutex_t *mtx, struct timespec *ts, int catch)
 {
 	struct lwp *l = curlwp;
 	int rv;
@@ -395,9 +395,21 @@ docvwait(kcondvar_t *cv, kmutex_t *mtx, struct timespec *ts)
 
 	UNLOCKED(mtx, false);
 
-	l->l_sched.info = cv;
 	rv = 0;
-	if (ts) {
+
+	if (catch) {
+		if (mtx != &proc_lock)
+			mutex_enter(&proc_lock);
+	}
+	l->l_sched.info = cv;
+	if (catch) {
+		rv = sigispending(curlwp, 0);
+		if (mtx != &proc_lock)
+			mutex_exit(&proc_lock);
+	}
+	if (rv) {
+		/* nothing */
+	} else if (ts) {
 		if (rumpuser_cv_timedwait(RUMPCV(cv), RUMPMTX(mtx),
 		    ts->tv_sec, ts->tv_nsec))
 			rv = EWOULDBLOCK;
@@ -430,7 +442,15 @@ docvwait(kcondvar_t *cv, kmutex_t *mtx, struct timespec *ts)
 		mutex_enter(mtx);
 		rv = EINTR;
 	}
+	if (catch) {
+		if (mtx != &proc_lock)
+			mutex_enter(&proc_lock);
+	}
 	l->l_sched.info = NULL;
+	if (catch) {
+		if (mtx != &proc_lock)
+			mutex_exit(&proc_lock);
+	}
 
 	return rv;
 }
@@ -441,7 +461,7 @@ cv_wait(kcondvar_t *cv, kmutex_t *mtx)
 
 	if (__predict_false(rump_threads == 0))
 		panic("cv_wait without threads");
-	(void) docvwait(cv, mtx, NULL);
+	(void) docvwait(cv, mtx, NULL, /*catch*/0);
 }
 
 int
@@ -450,7 +470,7 @@ cv_wait_sig(kcondvar_t *cv, kmutex_t *mtx)
 
 	if (__predict_false(rump_threads == 0))
 		panic("cv_wait without threads");
-	return docvwait(cv, mtx, NULL);
+	return docvwait(cv, mtx, NULL, /*catch*/1);
 }
 
 int
@@ -460,17 +480,35 @@ cv_timedwait(kcondvar_t *cv, kmutex_t *mtx, int ticks)
 	extern int hz;
 	int rv;
 
+	if (ticks == 0) {
+		cv_wait(cv, mtx);
+		rv = 0;
+	} else {
+		ts.tv_sec = ticks / hz;
+		ts.tv_nsec = (ticks % hz) * (1000000000/hz);
+		rv = docvwait(cv, mtx, &ts, /*catch*/0);
+	}
+
+	return rv;
+}
+
+int
+cv_timedwait_sig(kcondvar_t *cv, kmutex_t *mtx, int ticks)
+{
+	struct timespec ts;
+	extern int hz;
+	int rv;
+
 	if (ticks == 0) {
 		rv = cv_wait_sig(cv, mtx);
 	} else {
 		ts.tv_sec = ticks / hz;
 		ts.tv_nsec = (ticks % hz) * (1000000000/hz);
-		rv = docvwait(cv, mtx, &ts);
+		rv = docvwait(cv, mtx, &ts, /*catch*/1);
 	}
 
 	return rv;
 }
-__strong_alias(cv_timedwait_sig,cv_timedwait);
 
 void
 cv_signal(kcondvar_t *cv)
diff --git a/sys/rump/librump/rumpkern/signals.c b/sys/rump/librump/rumpkern/signals.c
index 2eb06f188bf5..556678e08b4d 100644
--- a/sys/rump/librump/rumpkern/signals.c
+++ b/sys/rump/librump/rumpkern/signals.c
@@ -105,6 +105,22 @@ rumpsig_record(struct proc *p, int signo)
 	}
 }
 
+static void
+rumpsig_record_wakeup(struct proc *p, int signo)
+{
+	struct lwp *l;
+
+	if (!sigismember(&p->p_sigctx.ps_sigignore, signo)) {
+		sigaddset(&p->p_sigpend.sp_set, signo);
+		mutex_enter(p->p_lock);
+		LIST_FOREACH(l, &p->p_lwps, l_sibling) {
+			if (l->l_sched.info)
+				cv_broadcast(l->l_sched.info);
+		}
+		mutex_exit(p->p_lock);
+	}
+}
+
 typedef void (*rumpsig_fn)(struct proc *, int);
 
 static rumpsig_fn rumpsig = rumpsig_raise;
@@ -135,6 +151,9 @@ rump_boot_setsigmodel(enum rump_sigmodel model)
 	case RUMP_SIGMODEL_RECORD:
 		rumpsig = rumpsig_record;
 		break;
+	case RUMP_SIGMODEL_RECORD_WAKEUP:
+		rumpsig = rumpsig_record_wakeup;
+		break;
 
 	/* for compat, though I doubt anyone is using it */
 	case RUMP_SIGMODEL__HOST_NOTANYMORE:
@@ -143,6 +162,15 @@ rump_boot_setsigmodel(enum rump_sigmodel model)
 	}
 }
 
+void
+rump_lwp_kill(struct lwp *l, int signo)
+{
+
+	mutex_enter(&proc_lock);
+	psignal(l->l_proc, signo);
+	mutex_exit(&proc_lock);
+}
+
 void
 psignal(struct proc *p, int sig)
 {
diff --git a/tests/rump/rumpkern/t_signals.c b/tests/rump/rumpkern/t_signals.c
index ba0c0ea36fb2..1f7712d29eee 100644
--- a/tests/rump/rumpkern/t_signals.c
+++ b/tests/rump/rumpkern/t_signals.c
@@ -32,11 +32,13 @@
 
 #include <atf-c.h>
 #include <errno.h>
+#include <pthread.h>
 #include <string.h>
 #include <signal.h>
 #include <unistd.h>
 
 #include <rump/rump.h>
+#include <rump/rump_syscalls.h>
 
 #include "../kernspace/kernspace.h"
 #include "h_macros.h"
@@ -62,6 +64,13 @@ ATF_TC_HEAD(sigpanic, tc)
 	atf_tc_set_md_var(tc, "descr", "RUMP_SIGMODEL_PANIC");
 }
 
+ATF_TC(sigrecordwakeup);
+ATF_TC_HEAD(sigrecordwakeup, tc)
+{
+
+	atf_tc_set_md_var(tc, "descr", "RUMP_SIGMODEL_RECORD_WAKEUP");
+}
+
 static volatile sig_atomic_t sigcnt;
 static void
 thehand(int sig)
@@ -116,12 +125,65 @@ ATF_TC_BODY(sigpanic, tc)
 	}
 }
 
+struct sigrecordwakeupctx {
+	pid_t pid;
+	struct lwp *lwp;
+};
+
+static void *
+signalme(void *cookie)
+{
+	struct sigrecordwakeupctx *C = cookie;
+
+	RZ(rump_pub_lwproc_newlwp(C->pid));
+
+	sleep(1);
+	rump_lwp_kill(C->lwp, SIGUSR1);
+
+	return NULL;
+}
+
+ATF_TC_BODY(sigrecordwakeup, tc)
+{
+	struct sigrecordwakeupctx ctx, *C = &ctx;
+	pthread_t t;
+	int fd[2];
+	char c;
+	ssize_t nwrit;
+	int error;
+
+	rump_boot_setsigmodel(RUMP_SIGMODEL_RECORD_WAKEUP);
+	rump_init();
+
+	memset(C, 0, sizeof(*C));
+
+	RZ(rump_pub_lwproc_rfork(RUMP_RFCFDG));
+	RL(C->pid = rump_sys_getpid());
+	RZ(rump_pub_lwproc_newlwp(C->pid));
+	C->lwp = rump_pub_lwproc_curlwp();
+
+	RL(rump_sys_pipe(fd));
+
+	RZ(pthread_create(&t, NULL, &signalme, C));
+
+	alarm(2);
+	nwrit = rump_sys_read(fd[0], &c, sizeof(c));
+	ATF_REQUIRE_EQ_MSG(nwrit, -1, "rump_sys_read: %zd", nwrit);
+	error = errno;
+	ATF_REQUIRE_EQ_MSG(error, EINTR, "errno=%d (%s)",
+	    error, strerror(error));
+
+	RZ(pthread_join(t, NULL));
+	alarm(0);
+}
+
 ATF_TP_ADD_TCS(tp)
 {
 
 	ATF_TP_ADD_TC(tp, sigraise);
 	ATF_TP_ADD_TC(tp, sigignore);
 	ATF_TP_ADD_TC(tp, sigpanic);
+	ATF_TP_ADD_TC(tp, sigrecordwakeup);
 
 	return atf_no_error();
 }