Created attachment 2893 [details] add systemd bits Ever since systemd, there are problems with keeping track of the running daemon process [1] and errors [2]. We were trying to avoid this solution, but after several bugs and many failed attempts from various people, it looks like there is no other working way than to use their API to notify them that the daemon is running. Also Debian moved to this solution [3]. Unfortunately Colin didn't manage to send the patch upstream, even though it would be useful for others (adding to CC, sorry if it was intentional) so I am trying to do so. The code is very simple, guarded by the ifdefs and enabled only if --with-systemd switch is added to configure. If it will not get to OpenBSD, I thing carrying it in portable would certainly make a sense. I also added a sample systemd service file to contrib directory to reflect its usage (not particularly needed). [1] https://bugzilla.redhat.com/show_bug.cgi?id=1381997 [2] https://bugzilla.redhat.com/show_bug.cgi?id=1291172 [3] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=778913
Created attachment 2896 [details] do not call daemon again when restarting on sighup I'm not wild about letting that particular camel's nose into the tent. How about we fix the sighup behaviour to not call daemon, keep the same pid and not touch the pidfile?
Sorry for a quite late answer. I got bitten by this while trying to rebase to the OpenSSH 7.4. Your patch got into this release in 7fc4766a, but was partially reverted in f2398eb7. But to my understanding, the daemonized() function returns 1 even for the first call running from systemd service file and therefore prevents call to daemon() and therefore systemd does not keep track of the service again. sshd[12655]: debug3: already daemonized systemd[1]: Starting OpenSSH server daemon... Reverting the chunk with the condition if (!(debug_flag || inetd_flag || no_daemon_flag || already_daemon)) { makes it working again.
Jakub: That's only a problem because for some reason Fedora doesn't use the -D option when running from systemd (if you did, then no_daemon_flag would be true so the value of already_daemon would be irrelevant). When you have a smart service supervisor, IMO -D makes much more sense.
Colin, the -D was omitted (and Type=Forking added) because systemd was unable to report failures (when configuration file had an error), as described in the Red Hat bugzilla [1]. Certainly, the -D makes more sense these days, but with that, typing systemctl restart sshd is not producing any error with -D option. Also no error goes into the journal (as in the description of the linked bug). The one solution is "letting that particular camel's nose into the tent" as used in Ubuntu/Debian, but I still hope, there is a better way to do that. Clearly, what I am trying to point out here is that the applied change is modifying the behavior not only for re-exec (reload), but also for the "start", which was not intentional to my understanding. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1291172
The test if the process is daemon passes for every process that is run as a systemd service so it does not solve the problem for us. As the result with the pushed change the 7.4p1 will not write PID file when running as a systemd service. I would consider a bit different approach. We can remember if we already called daemon() using environment variable and prevent the repeated calls if this variable is already set. This finally made it working for me. The patch can look somehow like this: diff -up openssh-7.4p1/misc.c.daemon openssh-7.4p1/misc.c --- openssh-7.4p1/misc.c.daemon 2017-02-03 13:08:14.751282516 +0100 +++ openssh-7.4p1/misc.c 2017-02-03 13:08:14.778282474 +0100 @@ -1273,6 +1273,9 @@ daemonized(void) return 0; /* parent is not init */ if (getsid(0) != getpid()) return 0; /* not session leader */ + if (getenv("_SSH_DAEMONIZED") == NULL) + return 0; /* already reexeced */ + debug3("already daemonized"); return 1; } diff -up openssh-7.4p1/sshd.c.daemon openssh-7.4p1/sshd.c --- openssh-7.4p1/sshd.c.daemon 2017-02-03 13:08:14.755282510 +0100 +++ openssh-7.4p1/sshd.c 2017-02-03 13:09:29.765164356 +0100 @@ -1866,6 +1866,7 @@ main(int ac, char **av) if (daemon(0, 0) < 0) fatal("daemon() failed: %.200s", strerror(errno)); + setenv("_SSH_DAEMONIZED", "1", 1); disconnect_controlling_tty(); } /* Reinitialize the log (because of the fork above). */
Created attachment 2950 [details] fixed patch Never mind. Nothing from above resolves the race condition between systemd reading PID file and sshd after reexec writing it, except adding SD_NOTIFY code so I gave up. I updated the patch to move the SD_NOTIFY to the place where it will not be called by children for every connection (as currently used in Debian).
(In reply to Jakub Jelen from comment #6) > Created attachment 2950 [details] > fixed patch > > Never mind. Nothing from above resolves the race condition between > systemd reading PID file and sshd after reexec writing it, except > adding SD_NOTIFY code so I gave up. What about reading the pidfile first to see if it has the correct PID before rewriting it? I did something like that to work around a problem in pam_loginuid (LinuxPAM ticket #23, I'd link to it but fedorahosted.org seems to have imploded). Does systemd even need a pidfile?
(In reply to Darren Tucker from comment #7) > (In reply to Jakub Jelen from comment #6) > > Created attachment 2950 [details] > > fixed patch > > > > Never mind. Nothing from above resolves the race condition between > > systemd reading PID file and sshd after reexec writing it, except > > adding SD_NOTIFY code so I gave up. > > What about reading the pidfile first to see if it has the correct > PID before rewriting it? I did something like that to work around a > problem in pam_loginuid (LinuxPAM ticket #23, I'd link to it but > fedorahosted.org seems to have imploded). I don't think that would help the initial race condition, when systemd tries to read the PID file before it is written after the daemon(). > Does systemd even need a pidfile? From man systemd.service: > If this setting [Type=forking] is used, it is recommended to also use the PIDFile= option, so that systemd can identify the main process of the daemon. systemd will proceed with starting follow-up units as soon as the parent process exits. There is also GuessMainPID= option, but from the documentation in the same manual page I am not convinced that it is something that we would like to use in case we can specify the PID file reliably.
In a non-forking type=notify case systemd keeps track of the sshd service as it is a child process. Any termination of the child process is a signal to the system launching process and all operations are handled in this way. I wouldn't worry about the pid file at all.
Created attachment 3099 [details] notify systemd when daemon reloading I observed a race similar to restarts during SSH daemon reloads. Environment: SLES12 SP2 openssh-7.2p2-74.1 systemd-228-150.15.2 Reproducer: ssh $REMOTE_HOST systemctl reload sshd.service ; ssh $REMOTE_HOST Actual behavior: The second SSH connection fails in roughly 50% cases. Expected behavior: First SSH blocks until daemon reloads and second connection succeeds. I propose to extend the suggested patch in comment 6 with another sd_notify() call (attached) so that systemd can properly track reloading state of the daemon. The two patches fixed the behavior in my testcase (I admit I didn't check actual systemd handling of the notification).
I don't understand the problem here. Can you not arrange for systemd to run sshd with the -D flag to begin with? When sshd restarts for SIGHUP it will reexec itself with the same commandline argument and will therefore not daemonise. wrt reporting errors when sshd is run with the -D flag, I don't understand the problem there either: sshd reports config parsing errors to stderr, and this isn't affected by the presence of the -D flag at all.
(In reply to Damien Miller from comment #11) > I don't understand the problem here. > > Can you not arrange for systemd to run sshd with the -D flag to > begin with? When sshd restarts for SIGHUP it will reexec itself with > the same commandline argument and will therefore not daemonise. That is an option, yet it has other drawbacks and one needs to carefully step among all the combinations that have some kind of race condition or similar problems (those were actually the reason for the Debian/RH/SUSE issues referenced above). That said, putting in an ifdeffed init system integration doesn't need to be a bad idea (I intentionally avoided writing 'systemd', as I personally consider this to be a broader issue, which would be present with any other init system that would want to keep track of the state the services are in.) Would it help if the state reporting was init-system agnostic - say split into a separate file(s) the way auditing is? (From my point of view that would be the right thing.)
Created attachment 3798 [details] standalone systemd notifications This implements the equivalent of sd_notify() without bringing in the rest of systemd bloat. It it also signal-handler safe, which is not the case for the originally proposed diffs. Lightly tested.
Thanks for working on that, will be great to have native support for the readiness protocol. One review comment: unless I'm missing it because it's handled outside of the patch context, after a RELOADING=1, when the reload operation is complete, a READY=1 needs to be sent too: https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html#RELOADING=1 While there, it would be really nice if the RELOADING=1 message also included MONOTONIC_USEC=<timestamp> (CLOCK_MONOTONIC in usec as a decimal string), which is used for accurate synchronization. IE, write a string like "RELOADING=1\nMONOTONIC_USEC=1234...". This will enable the unit to be of Type=notify-reload which adds some nice features.
I think the READY=1 will be sent implicitly after sshd restarts
(In reply to Luca Boccassi from comment #14) > While there, it would be really nice if the RELOADING=1 message also > included MONOTONIC_USEC=<timestamp> (CLOCK_MONOTONIC in usec as a > decimal string), which is used for accurate synchronization. IE, > write a string like "RELOADING=1\nMONOTONIC_USEC=1234...". This will > enable the unit to be of Type=notify-reload which adds some nice > features. That's more tricky as the reload is called from signal handler context and we can't use snprint() there to format the usec part of the message. We'd have to refactor how sshd manages SIGHUP restarts. That would make some other things easier, but it's still a bigger change. Anyway, if some of the distro people on this bug can report on whether the patch is okay, then we can move forward with this and finesse it later.
I don't see any problems from eyeballing the patch. I've pushed a version of the Debian packaging with this (and consequent modifications; we also have a socket activation patch from Ubuntu, but reworking that to avoid libsystemd wasn't too hard) to https://salsa.debian.org/ssh-team/openssh/-/tree/without-libsystemd, though so far I've only checked that it passes the regression tests. https://salsa.debian.org/ssh-team/openssh/-/jobs/5521815 has .debs for people who feel comfortable installing things from random CI jobs. Obviously I don't recommend installing those on production, but it's probably OK to do so in a container/VM. I'll look more once I've had some sleep.
I've done some testing and this does seem to basically work. The one thing I'd point out is following on from Luca's comment: RELOADING=1 is ignored if you don't also send MONOTONIC_USEC=. So if you're not going to send that (and I understand the reasons), you might as well not bother sending RELOADING=1 either; we'll just have to stick with Type=notify rather than Type=notify-reload for now, which wouldn't be a regression.
(In reply to Colin Watson from comment #18) > I've done some testing and this does seem to basically work. > > The one thing I'd point out is following on from Luca's comment: > RELOADING=1 is ignored if you don't also send MONOTONIC_USEC=. So > if you're not going to send that (and I understand the reasons), you > might as well not bother sending RELOADING=1 either; we'll just have > to stick with Type=notify rather than Type=notify-reload for now, > which wouldn't be a regression. Mmmh hang on I don't think that should be the case. The MONOTONIC_USEC is for the Type=notify-reload workflow, that automatically hooks sighup to the service, and is newer. But RELOADING=1 -> READY=1 by itself should work with the older workflow where you manually specify an ExecReload=kill -HUP $MAINPID in the unit. Let me get your packages and test this.
Actually, I noticed a slight race here. You're sending the readiness notification from platform_pre_listen; but, as the name implies, this is called _before_ the server has started listening. The point of the readiness protocol is that the notification is only sent once the server is ready to accept connections. The notification should be moved to after the listen sockets are bound.
(In reply to Luca Boccassi from comment #19) > Mmmh hang on I don't think that should be the case. The > MONOTONIC_USEC is for the Type=notify-reload workflow, that > automatically hooks sighup to the service, and is newer. But > RELOADING=1 -> READY=1 by itself should work with the older workflow > where you manually specify an ExecReload=kill -HUP $MAINPID in the > unit. Ah, you may be right. I was just going by looking at the code and hadn't actually tested removing RELOADING=1. Probably best to leave it in then.
(In reply to Colin Watson from comment #21) > (In reply to Luca Boccassi from comment #19) > > Mmmh hang on I don't think that should be the case. The > > MONOTONIC_USEC is for the Type=notify-reload workflow, that > > automatically hooks sighup to the service, and is newer. But > > RELOADING=1 -> READY=1 by itself should work with the older workflow > > where you manually specify an ExecReload=kill -HUP $MAINPID in the > > unit. > > Ah, you may be right. I was just going by looking at the code and > hadn't actually tested removing RELOADING=1. Probably best to leave > it in then. I have tested the packages you published and the reloading notification is working: Mar 31 14:34:28 localhost systemd[1]: ssh.service: Trying to enqueue job ssh.service/reload/replace Mar 31 14:34:28 localhost systemd[1]: ssh.service: Installed new job ssh.service/reload as 1333 Mar 31 14:34:28 localhost systemd[1]: ssh.service: Enqueued job ssh.service/reload as 1333 Mar 31 14:34:28 localhost systemd[1]: ssh.service: Will spawn child (service_enter_reload): /usr/sbin/sshd Mar 31 14:34:28 localhost systemd[1]: ssh.service: About to execute: /usr/sbin/sshd -t Mar 31 14:34:28 localhost (sshd)[3824]: Found cgroup2 on /sys/fs/cgroup/, full unified hierarchy Mar 31 14:34:28 localhost systemd[1]: ssh.service: Forked /usr/sbin/sshd as 3824 (without CLONE_INTO_CGROUP) Mar 31 14:34:28 localhost systemd[1]: ssh.service: Changed running -> reload Mar 31 14:34:28 localhost systemd[1]: Reloading ssh.service - OpenBSD Secure Shell server... Mar 31 14:34:28 localhost (sshd)[3824]: Found cgroup2 on /sys/fs/cgroup/, full unified hierarchy Mar 31 14:34:28 localhost systemd[1]: ssh.service: Child 3824 belongs to ssh.service. Mar 31 14:34:28 localhost systemd[1]: ssh.service: Control process exited, code=exited, status=0/SUCCESS (success) Mar 31 14:34:28 localhost systemd[1]: ssh.service: Running next control command for state reload. Mar 31 14:34:28 localhost systemd[1]: ssh.service: Will spawn child (service_run_next_control): /bin/kill Mar 31 14:34:28 localhost systemd[1]: ssh.service: About to execute: /bin/kill -HUP "\$MAINPID" Mar 31 14:34:28 localhost (kill)[3826]: Found cgroup2 on /sys/fs/cgroup/, full unified hierarchy Mar 31 14:34:28 localhost systemd[1]: ssh.service: Forked /bin/kill as 3826 (without CLONE_INTO_CGROUP) Mar 31 14:34:28 localhost (kill)[3826]: Found cgroup2 on /sys/fs/cgroup/, full unified hierarchy Mar 31 14:34:28 localhost sshd[3812]: Received SIGHUP; restarting. Mar 31 14:34:28 localhost systemd[1]: ssh.service: Got notification message from PID 3812 (RELOADING=1) Mar 31 14:34:28 localhost systemd[1]: ssh.service: Child 3826 belongs to ssh.service. Mar 31 14:34:28 localhost systemd[1]: ssh.service: Control process exited, code=exited, status=0/SUCCESS (success) Mar 31 14:34:28 localhost systemd[1]: ssh.service: Got final SIGCHLD for state reload. Mar 31 14:34:28 localhost systemd[1]: ssh.service: Changed reload -> reload-notify Mar 31 14:34:28 localhost systemd[1]: ssh.service: Got notification message from PID 3812 (READY=1) Mar 31 14:34:28 localhost systemd[1]: ssh.service: Changed reload-notify -> running Mar 31 14:34:28 localhost systemd[1]: ssh.service: Job 1333 ssh.service/reload finished, result=done Mar 31 14:34:28 localhost systemd[1]: Reloaded ssh.service - OpenBSD Secure Shell server. Mar 31 14:34:28 localhost sshd[3812]: Server listening on 0.0.0.0 port 22. Mar 31 14:34:28 localhost sshd[3812]: Server listening on :: port 22.
(In reply to Colin Watson from comment #20) > Actually, I noticed a slight race here. You're sending the > readiness notification from platform_pre_listen; but, as the name > implies, this is called _before_ the server has started listening. > The point of the readiness protocol is that the notification is only > sent once the server is ready to accept connections. > > The notification should be moved to after the listen sockets are > bound. Yes, good catch, this should be fixed as it's important to avoid races that the notification is delivered after everything is up and running and ready to process requests.
Created attachment 3801 [details] standalone notify patch The attached patch fixes the issue by creating a platform_post_listen() hook, as suggested by Colin. Tested in a Debian testing VM, seems to do the right thing, including on reloading.
Created attachment 3802 [details] standalone notify patch Thinking about it, given there's no external dependency and the runtime behaviour is a no-op unless the NOTIFY_SOCKET env var is set (which is only set by systemd or systemd-compatible managers), I don't think the new autoconf option is needed? There's no downside to always including the implementation when building on Linux, like it's done with the OOM adjustments. New revision of the patch attached does just that.
Either version of Luca's patch looks fine to me.
Created attachment 3804 [details] standalone notify and timestamp patch > That's more tricky as the reload is called from signal handler context and we can't use snprint() there to format the usec part of the message. We'd have to refactor how sshd manages SIGHUP restarts. > > That would make some other things easier, but it's still a bigger change. I went back and had a look at this, and unless I am missing something the reloading message is not being sent from the signal handler? The handler is sighup_handler which just sets a boolean and returns, following the usual pattern: https://anongit.mindrot.org/openssh.git/tree/sshd.c#n298 but the notification message is sent from the platform_pre_restart() hook, which is called from the main context from the main loop via sighup_restart(): https://anongit.mindrot.org/openssh.git/tree/sshd.c#n304 This already does some logging, which uses format strings. Also platform_pre_restart() already calls oom_adjust_restore() which also uses format strings. So I went ahead and did the necessary modifications in the latest version, which also simplified the message handling as it can log unconditionally now, and added the timestamp too. I've tested this and seems to work just fine on Debian testing, I can change ssh.service to Type=notify-reload and reloading works just fine, including the state transitions.
Created attachment 3805 [details] simplified further Good catch about the sighup restart no longer running in a signal handler. We can simplify further if we make ssh_systemd_notify() accept a format string. We also have code to get the CLOCK_MONOTONIC timer that we can reuse.
(In reply to Damien Miller from comment #28) > Created attachment 3805 [details] > simplified further > > Good catch about the sighup restart no longer running in a signal > handler. > > We can simplify further if we make ssh_systemd_notify() accept a > format string. We also have code to get the CLOCK_MONOTONIC timer > that we can reuse. Looks good to me, tested on Debian testing as before, works as expected.
(In reply to Damien Miller from comment #28) > Good catch about the sighup restart no longer running in a signal > handler. (In reply to Damien Miller from comment #13) > ... > It it also signal-handler safe, which is not the case for the originally proposed diffs. The original diff (comment 10) already put the notification in sighup_restart() not in sighup_handler(), i.e. still the same place where platform_pre_restart() is called now, not a signal handler context AFAICS. platform_* hooks look like the appropriate places for these calls.
Created attachment 3809 [details] standalone notify and timestamp patch One more change, to support abstract namespace sockets (for containers) as per protocol defined at https://www.freedesktop.org/software/systemd/man/devel/sd_notify.html#Notes
Comment on attachment 3809 [details] standalone notify and timestamp patch This looks fine to me. I'll commit it. Thanks for you help!
Committed as 08f579231cd38 and will be in OpenSSH-9.8, due around June/July.
(In reply to Damien Miller from comment #33) > Committed as 08f579231cd38 and will be in OpenSSH-9.8, due around > June/July. Thank you!
*** Bug 3616 has been marked as a duplicate of this bug. ***