procd/ujail behaviour

[Daniel Golle]

Hi Hartmut,

yes, this is a known problem affecting jailed services not terminating
on TERM signal (or not terminating fast enough).
As trapping KILL signal is not possible, simply forwarding this signal
to the jailed process is not possible.

I have a patch ready which lets ujail handle the timeout and sending of
SIGKILL (instead of letting procd send SIGKILL to ujail which will not
have the desired effect). However, in this way the termination timeout
can not be adjusted and is hard-coded to 5 seconds for now (passing the
term-timeout as paramter to ujail would be possible, but I haven't
implemented that yet).

Another option would be to hijack another signal (SIGUSR2, for example)
to be sent to ujail by procd which would make ujail then send SIGKILL
to the jailed process. While hat approach is more easy to implement and
allows using the existing timeout logic in procd (which is adjustable),
it then no longer allows forwarding that hijacked signal (SIGUSR2) to
the jailed process which is also not nice.

An even more complicated option would be to let ujail *always* expose
an ubus object and then let procd tell ujail to send signals via ubus.
(this is currently implemented for OCI containers, but not for ujail'ed
services).

It'd be nice if you can try the patch attached (to be put in
package/system/procd/patches) and let me know what you think.


Cheers


Daniel


On Fri, Dec 10, 2021 at 02:12:30PM +0100, e9hack wrote:
> 
> Hi,
> 
> I did update the init script for stubby, that it runs with ujail. Stubby does run, but it isn't possible to stop it via '/etc/init.d/stubby stop'. The ujail process is stopped, but stubby itself not. It looks like that it isn't possible to stop stubby with SIGTERM. Stubby will be ended by SIGKILL only. Procd sends first SIGTERM and than SIGKILL to ujail, but ujail sends SIGTERM only. Either ujail doesn't send SIGKILL if the jailed process doesn't terminate or procd sends SIGKILL to early so that ujail can't send SIGKILL any more. In the log I see the following messages:
> 
> Fri Dec 10 13:35:52 2021 user.err : jail: forwarding signal 15 to the jailed process
> Fri Dec 10 13:35:52 2021 daemon.err stubby[6669]: jail: forwarding signal 15 to the jailed process
> Fri Dec 10 13:35:57 2021 daemon.info procd: Instance stubby::stubby pid 6669 not stopped on SIGTERM, sending SIGKILL instead
> 
> 6669 was the pid of the ujail process.
> 
> In ujail, I changed a few DEBUG() calls to ERROR(). Without this, the first two lines are not shown.
> 
> Any ideas?
> 
> Regards,
> Hartmut
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
-------------- next part --------------
>From 478ef55cdc2d0e0a615a4fb19430d2ca34c21b31 Mon Sep 17 00:00:00 2001
From: Daniel Golle <daniel at makrotopia.org>
Date: Fri, 10 Dec 2021 13:48:59 +0000
Subject: [PATCH] jail: make sure jailed process is terminated

Don't ever send SIGKILL to ujail, as that will kill ujail but not the
jailed process.
Instead, let ujail send SIGKILL in case of SIGTERM not succeeding after
a fixed timeout of 5 seconds.

Signed-off-by: Daniel Golle <daniel at makrotopia.org>
---
 jail/jail.c        | 8 +++++++-
 service/instance.c | 9 ++++++---
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/jail/jail.c b/jail/jail.c
index 6e9306d..bc2c039 100644
--- a/jail/jail.c
+++ b/jail/jail.c
@@ -1093,11 +1093,17 @@ static void jail_handle_signal(int signo)
 	if (hook_running) {
 		DEBUG("forwarding signal %d to the hook process\n", signo);
 		kill(hook_process.pid, signo);
+		/* set timeout to send SIGKILL hook process in case SIGTERM doesn't succeed */
+		if (signo == SIGTERM)
+			uloop_timeout_set(&hook_process_timeout, 5000);
 	}
 
 	if (jail_running) {
 		DEBUG("forwarding signal %d to the jailed process\n", signo);
 		kill(jail_process.pid, signo);
+		/* set timeout to send SIGKILL jail process in case SIGTERM doesn't succeed */
+		if (signo == SIGTERM)
+			uloop_timeout_set(&jail_process_timeout, 5000);
 	}
 }
 
@@ -1112,7 +1118,7 @@ static void signals_init(void)
 
 		if (!sigismember(&sigmask, i))
 			continue;
-		if ((i == SIGCHLD) || (i == SIGPIPE) || (i == SIGSEGV))
+		if ((i == SIGCHLD) || (i == SIGPIPE) || (i == SIGSEGV) || (i == SIGSTOP) || (i == SIGKILL))
 			continue;
 
 		s.sa_handler = jail_handle_signal;
diff --git a/service/instance.c b/service/instance.c
index 748c1e5..07a6239 100644
--- a/service/instance.c
+++ b/service/instance.c
@@ -867,7 +867,8 @@ instance_stop(struct service_instance *in, bool halt)
 	in->halt = halt;
 	in->restart = in->respawn = false;
 	kill(in->proc.pid, SIGTERM);
-	uloop_timeout_set(&in->timeout, in->term_timeout * 1000);
+	if (!in->has_jail)
+		uloop_timeout_set(&in->timeout, in->term_timeout * 1000);
 }
 
 static void
@@ -884,7 +885,8 @@ instance_restart(struct service_instance *in)
 	in->halt = true;
 	in->restart = true;
 	kill(in->proc.pid, SIGTERM);
-	uloop_timeout_set(&in->timeout, in->term_timeout * 1000);
+	if (!in->has_jail)
+		uloop_timeout_set(&in->timeout, in->term_timeout * 1000);
 }
 
 static void
@@ -1662,7 +1664,8 @@ void instance_dump(struct blob_buf *b, struct service_instance *in, int verbose)
 		blobmsg_add_blob(b, in->command);
 	if (in->bundle)
 		blobmsg_add_string(b, "bundle", in->bundle);
-	blobmsg_add_u32(b, "term_timeout", in->term_timeout);
+	if (!in->has_jail)
+		blobmsg_add_u32(b, "term_timeout", in->term_timeout);
 	if (!in->proc.pending)
 		blobmsg_add_u32(b, "exit_code", in->exit_code);
 
-- 
2.34.1