From e7147f7ad2bfb920d0ea8e4ae51d106743261ee9 Mon Sep 17 00:00:00 2001 From: Dmitrii Kuvaiskii Date: Sun, 7 Jul 2019 19:58:11 -0700 Subject: [PATCH] [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 | 5 +- LibOS/shim/include/shim_signal.h | 1 - LibOS/shim/src/ipc/shim_ipc_pid.c | 93 +++++++++++++---------------- LibOS/shim/src/sys/shim_sigaction.c | 4 +- 4 files changed, 44 insertions(+), 59 deletions(-) diff --git a/LibOS/shim/include/shim_ipc.h b/LibOS/shim/include/shim_ipc.h index c8b39fa5..4419122e 100644 --- a/LibOS/shim/include/shim_ipc.h +++ b/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; diff --git a/LibOS/shim/include/shim_signal.h b/LibOS/shim/include/shim_signal.h index 21ebd156..c1f257ca 100644 --- a/LibOS/shim/include/shim_signal.h +++ b/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_ */ diff --git a/LibOS/shim/src/ipc/shim_ipc_pid.c b/LibOS/shim/src/ipc/shim_ipc_pid.c index 5bb966f7..57b09518 100644 --- a/LibOS/shim/src/ipc/shim_ipc_pid.c +++ b/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; + 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; + } + + IDTYPE dest; + 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_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; + init_ipc_msg(msg, IPC_PID_KILL, total_msg_size, dest); + 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; -} - -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; - - 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); - - struct shim_ipc_pid_kill * msgin = - (struct shim_ipc_pid_kill *) &msg->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); diff --git a/LibOS/shim/src/sys/shim_sigaction.c b/LibOS/shim/src/sys/shim_sigaction.c index 720624f3..e86fd543 100644 --- a/LibOS/shim/src/sys/shim_sigaction.c +++ b/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; }