| Message ID | 20221011110126.1798114-1-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Change explicit exit notification to be a SIGUSR1 instead of a SIGTERM signal | expand |
Hi, On Tue, Oct 11, 2022 at 01:01:26PM +0200, Arne Schwabe wrote: > From the implemention and the fact that it is a an OCC message > (basically the rudimentary predecessor to control channel), > this message is very old. > > I think in the past this feature fit nicely to the weird inetd + openvpn mode > that seems to have far to many hacks still left in our code. With inetd, > it made sense that the server instance quits if you press C-c on the client. > > In our current state where inetd is no longer supported, this behaviour > to exit makes little sense and this patch changes the behaviour to SIGUSR1. I am very confused about v2 of this patch - and would be happy to ACK v1. I have applied v1, and tested - p2p without the patch -> client sends EEN -> server exits 2022-10-14 16:56:56 us=334517 Exit message received by peer 2022-10-14 16:56:56 us=334753 TCP/UDP: Closing socket 2022-10-14 16:56:56 us=334814 Closing TUN/TAP interface 2022-10-14 16:56:56 us=334836 net_addr_v4_del: 10.204.11.1 dev tun11 2022-10-14 16:56:56 us=334987 net_addr_v6_del: fd00:abcd:204:11::1/64 dev tun11 2022-10-14 16:56:56 us=342133 SIGTERM[soft,remote-exit] received, process exiting - p2p with the v1 patch -> client sends EEN -> server logs, and restarts 2022-10-14 16:57:30 us=151079 Exit message received by peer 2022-10-14 16:57:30 us=151254 TCP/UDP: Closing socket 2022-10-14 16:57:30 us=151316 Closing TUN/TAP interface 2022-10-14 16:57:30 us=151343 net_addr_v4_del: 10.204.11.1 dev tun11 2022-10-14 16:57:30 us=151465 net_addr_v6_del: fd00:abcd:204:11::1/64 dev tun11 2022-10-14 16:57:30 us=155086 SIGUSR1[soft,remote-exit] received, process restarting 2022-10-14 16:57:30 us=155149 Restart pause, 5 second(s) (and then it rearms itself and is ready to server another p2p client) - p2mp server without patch -> client sends EEN -> server logs, and terminates the instance 2022-10-14 16:58:26 us=544309 cron2-freebsd-tc-amd64/194.97.140.21:17570 Exit message received by peer 2022-10-14 16:58:26 us=544365 cron2-freebsd-tc-amd64/194.97.140.21:17570 SIGTERM[soft,remote-exit] received, client-instance exiting - p2mp server with the v1 patch -> client sends EEN -> server logs, and says it will "restart" the instance 2022-10-14 16:59:02 us=113506 cron2-freebsd-tc-amd64/194.97.140.21:41691 Exit message received by peer 2022-10-14 16:59:02 us=113573 cron2-freebsd-tc-amd64/194.97.140.21:41691 SIGUSR1[soft,remote-exit] received, client-instance restarting - with a 2.4 client, it has no "Exit message recived by peer" (because that is the new control channel thing), and just logs 2022-10-14 17:03:47 us=455929 cron2-freebsd-tc-amd64/194.97.140.21:13152 SIGUSR1[soft,remote-exit] received, client-instance restarting Trying to understand the code is an interesting maze... that message above comes from sig.c::print_signal(), which is called from "multi::multi_close_instance_on_signal()". *That* one is called (mostly) from multi_process_post(), which says " * Also close context on signal." and basically does if (IS_SIG(&mi->context)) { if (flags & MPP_CLOSE_ON_SIGNAL) { multi_close_instance_on_signal(m, mi); ret = false; } } ... so, the nature of the (soft) signal raised does not seem to play any role to the instance - "if signal, then close instance". Just the logging is a bit off now ("restarting" vs. "exiting"). If we care about logging, I think we should do the signal remap in multi_close_instance_on_signal() (... which already remaps SIGUSR1 by means of remap_signal(), but only if c->options.remap_sigusr1 is set). OTOH, maybe it should just *not* call remap_signal(), as the per-instance signals and the global signals do different things, and always remap SIGUSR1-in-instance to SIGTERM... What I do not think will make us happy in future is the extra conditionals in v2... gert
> > If we care about logging, I think we should do the signal remap in > multi_close_instance_on_signal() (... which already remaps SIGUSR1 > by means of remap_signal(), but only if c->options.remap_sigusr1 is > set). > > OTOH, maybe it should just *not* call remap_signal(), as the per-instance > signals and the global signals do different things, and always remap > SIGUSR1-in-instance to SIGTERM... > > > What I do not think will make us happy in future is the extra > conditionals in v2... A completely different effect screwed up my testing and lead to believe that signals are handled different in p2p and p2mp. There is different behaviour in p2p and p2mp but it is triggered by the HMAC cookie approach that only p2mp has. Detailed explaination is in the code in patch v3. Arne
diff --git a/src/openvpn/occ.c b/src/openvpn/occ.c index 1ed0d3771..eb1f2fae7 100644 --- a/src/openvpn/occ.c +++ b/src/openvpn/occ.c @@ -431,7 +431,7 @@ process_received_occ_msg(struct context *c) case OCC_EXIT: dmsg(D_PACKET_CONTENT, "RECEIVED OCC_EXIT"); - c->sig->signal_received = SIGTERM; + c->sig->signal_received = SIGUSR1; c->sig->signal_text = "remote-exit"; break; } diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 26259c6b8..da8c456b1 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -193,7 +193,7 @@ void receive_exit_message(struct context *c) { dmsg(D_STREAM_ERRORS, "Exit message received by peer"); - c->sig->signal_received = SIGTERM; + c->sig->signal_received = SIGUSR1; c->sig->signal_text = "remote-exit"; #ifdef ENABLE_MANAGEMENT if (management)
From the implemention and the fact that it is a an OCC message (basically the rudimentary predecessor to control channel), this message is very old. I think in the past this feature fit nicely to the weird inetd + openvpn mode that seems to have far to many hacks still left in our code. With inetd, it made sense that the server instance quits if you press C-c on the client. In our current state where inetd is no longer supported, this behaviour to exit makes little sense and this patch changes the behaviour to SIGUSR1. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/occ.c | 2 +- src/openvpn/push.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)