[LibOS] Change IPC subsystem to send IPC_PID_KILL without ack

Previously, the IPC subsystem incorrectly sent IPC_PID_KILL message
(generated as part of kill() syscall) as a duplex message, i.e., the
sender thread (the one issuing kill()) was paused until the receiving
child process handled IPC_PID_KILL callback and sent the acknowledgement
reply message back to sender.

This incorrect logic created a data race between the IPC_PID_KILL ack
message and the exiting child process. In particular, the child could
exit and all its resources (including IPC port to communicate with
parent) could be reclaimed by host OS. This could lead to IPC_PID_KILL
ack message being lost (because of the closed IPC port), and the paused
parent thread would wake up with -ECONNRESET instead of the ack message.
This would lead the kill() implementation to believe that child process
never existed in the first place and to return -ESRCH.

The fix to this data race is to send IPC_PID_KILL without waiting for
acknowledgement. Specification of kill() syscall does not require it to
be synchronous (indeed it is not on Linux), so this fix is correct. This
fix also enabled to merge broadcast_signal() into ipc_pid_kill_send().

LibOS/shim/include/shim_ipc.h

@@ -195,9 +195,8 @@ struct shim_ipc_pid_kill {
     int signum;
 } __attribute__((packed));
 
-int ipc_pid_kill_send (IDTYPE sender, IDTYPE id, enum kill_type type,
-                       int signum);
-int ipc_pid_kill_callback (IPC_CALLBACK_ARGS);
+int ipc_pid_kill_send(IDTYPE sender, IDTYPE target, enum kill_type type, int signum);
+int ipc_pid_kill_callback(struct shim_ipc_msg* msg, struct shim_ipc_port* port);
 
 struct pid_status {
     IDTYPE pid, tgid, pgid;

LibOS/shim/include/shim_signal.h

@@ -149,7 +149,6 @@ int do_kill_thread (IDTYPE sender, IDTYPE tgid, IDTYPE tid, int sig,
 int do_kill_proc (IDTYPE sender, IDTYPE tgid, int sig, bool use_ipc);
 int do_kill_pgroup (IDTYPE sender, IDTYPE pgid, int sig, bool use_ipc);
 
-int broadcast_signal (IDTYPE sender, int sig);
 int kill_all_threads (struct shim_thread * cur, IDTYPE sender, int sig);
 
 #endif /* _SHIM_SIGNAL_H_ */

LibOS/shim/src/ipc/shim_ipc_pid.c

@@ -68,57 +68,50 @@ int init_ns_pid (void)
     return 0;
 }
 
-int broadcast_signal (IDTYPE sender, int signum)
-{
+int ipc_pid_kill_send(IDTYPE sender, IDTYPE target, enum kill_type type, int signum) {
     BEGIN_PROFILE_INTERVAL();
     int ret;
 
-    size_t total_msg_size = get_ipc_msg_size(sizeof(struct shim_ipc_pid_kill));
-    struct shim_ipc_msg* msg = __alloca(total_msg_size);
-    init_ipc_msg(msg, IPC_PID_KILL, total_msg_size, 0);
-    struct shim_ipc_pid_kill * msgin =
-                    (struct shim_ipc_pid_kill *) &msg->msg;
-
-    msgin->sender = sender;
-    msgin->id     = 0;
-    msgin->type   = KILL_ALL;
-    msgin->signum = signum;
-
-    debug("ipc send to %u: IPC_PID_KILL(%u, %d, %u, %d)\n", 0,
-          sender, KILL_ALL, 0, signum);
-
-    ret = broadcast_ipc(msg, IPC_PORT_DIRCLD|IPC_PORT_DIRPRT, /*exclude_port*/ NULL);
-    SAVE_PROFILE_INTERVAL(ipc_pid_kill_send);
-    return ret;
-}
+    if (!signum) {
+        /* if sig is 0, then no signal is sent, but error checking on kill()
+         * is still performed (used to check for existence of processes) */
+        ret = 0;
+        goto out;
+    }
 
-int ipc_pid_kill_send (IDTYPE sender, IDTYPE id, enum kill_type type,
-                       int signum)
-{
-    BEGIN_PROFILE_INTERVAL();
     IDTYPE dest;
-    struct shim_ipc_port * port = NULL;
-    int ret;
-
-    if ((ret = connect_owner(id, &port, &dest)) < 0)
-        goto out;
+    struct shim_ipc_port* port;
+    if (type == KILL_ALL) {
+        dest = 0;
+        port = NULL;
+    } else {
+        ret = connect_owner(target, &port, &dest);
+        if (ret < 0)
+            goto out;
+    }
 
-    size_t total_msg_size = get_ipc_msg_duplex_size(sizeof(struct shim_ipc_pid_kill));
-    struct shim_ipc_msg_duplex* msg = __alloca(total_msg_size);
-    init_ipc_msg_duplex(msg, IPC_PID_KILL, total_msg_size, dest);
+    size_t total_msg_size = get_ipc_msg_size(sizeof(struct shim_ipc_pid_kill));
+    struct shim_ipc_msg* msg = __alloca(total_msg_size);
+    init_ipc_msg(msg, IPC_PID_KILL, total_msg_size, dest);
 
-    struct shim_ipc_pid_kill * msgin =
-                    (struct shim_ipc_pid_kill *) &msg->msg.msg;
+    struct shim_ipc_pid_kill* msgin = (struct shim_ipc_pid_kill *) &msg->msg;
     msgin->sender = sender;
-    msgin->id     = id;
     msgin->type   = type;
+    msgin->id     = target;
     msgin->signum = signum;
 
-    debug("ipc send to %u: IPC_PID_KILL(%u, %d, %u, %d)\n", dest,
-          sender, type, id, signum);
+    if (type == KILL_ALL) {
+        debug("IPC broadcast: IPC_PID_KILL(%u, %d, %u, %d)\n",
+              sender, type, target, signum);
+        ret = broadcast_ipc(msg, IPC_PORT_DIRCLD|IPC_PORT_DIRPRT, /*exclude_port*/ NULL);
+    }
+    else {
+        debug("IPC send to %u: IPC_PID_KILL(%u, %d, %u, %d)\n",
+              dest & 0xFFFF, sender, type, target, signum);
+        ret = send_ipc_message(msg, port);
+        put_ipc_port(port);
+    }
 
-    ret = send_ipc_message_duplex(msg, port, NULL, NULL);
-    put_ipc_port(port);
 out:
     SAVE_PROFILE_INTERVAL(ipc_pid_kill_send);
     return ret;
@@ -127,37 +120,33 @@ out:
 DEFINE_PROFILE_INTERVAL(ipc_pid_kill_send, ipc);
 DEFINE_PROFILE_INTERVAL(ipc_pid_kill_callback, ipc);
 
-int ipc_pid_kill_callback (IPC_CALLBACK_ARGS)
-{
+int ipc_pid_kill_callback(struct shim_ipc_msg* msg, struct shim_ipc_port* port) {
     BEGIN_PROFILE_INTERVAL();
-    struct shim_ipc_pid_kill * msgin =
-            (struct shim_ipc_pid_kill *) msg->msg;
+    struct shim_ipc_pid_kill* msgin = (struct shim_ipc_pid_kill *) msg->msg;
 
-    debug("ipc callback from %u: IPC_PID_KILL(%u, %u, %d)\n",
-          msg->src, msgin->sender, msgin->id, msgin->signum);
+    debug("IPC callback from %u: IPC_PID_KILL(%u, %d, %u, %d)\n",
+          msg->src & 0xFFFF, msgin->sender, msgin->type, msgin->id, msgin->signum);
 
     int ret = 0;
 
     switch (msgin->type) {
         case KILL_THREAD:
-            ret = do_kill_thread(msgin->sender, 0, msgin->id, msgin->signum,
-                                 true);
+            ret = do_kill_thread(msgin->sender, 0, msgin->id, msgin->signum, true);
             break;
         case KILL_PROCESS:
             ret = do_kill_proc(msgin->sender, msgin->id, msgin->signum, true);
             break;
         case KILL_PGROUP:
-            ret = do_kill_pgroup(msgin->sender, msgin->id, msgin->signum,
-                                 true);
+            ret = do_kill_pgroup(msgin->sender, msgin->id, msgin->signum, true);
             break;
         case KILL_ALL:
-            broadcast_ipc(msg, IPC_PORT_DIRPRT|IPC_PORT_DIRCLD, port);
+            broadcast_ipc(msg, IPC_PORT_DIRCLD|IPC_PORT_DIRPRT, port);
             kill_all_threads(NULL, msgin->sender, msgin->signum);
             break;
     }
 
     SAVE_PROFILE_INTERVAL(ipc_pid_kill_callback);
-    return ret < 0 ? ret : RESPONSE_CALLBACK;
+    return ret;
 }
 
 DEFINE_PROFILE_INTERVAL(ipc_pid_getstatus_send, ipc);

LibOS/shim/src/sys/shim_sigaction.c

@@ -489,8 +489,6 @@ static int __kill_all_threads (struct shim_thread * thread, void * arg,
     return srched;
 }
 
-int broadcast_signal (IDTYPE sender, int sig);
-
 int kill_all_threads (struct shim_thread * cur, IDTYPE sender, int sig)
 {
     struct walk_arg arg;
@@ -524,7 +522,7 @@ int shim_do_kill (pid_t pid, int sig)
     /* If pid equals -1, then sig is sent to every process for which the
        calling process has permission to send */
     else if (pid == -1) {
-        broadcast_signal(cur->tid, sig);
+        ipc_pid_kill_send(cur->tid, /*target*/ 0, KILL_ALL, sig);
         kill_all_threads(cur, cur->tid, sig);
         send_to_self = true;
     }