Date: Thu, 19 Sep 2024 01:27:46 GMT From: Alan Somers <asomers@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 5f89aea7b74a - main - ctld: fix several process setup/teardown bugs Message-ID: <202409190127.48J1RkCu007695@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by asomers: URL: https://cgit.FreeBSD.org/src/commit/?id=5f89aea7b74aa4605b25af62e31303097a4a48cc commit 5f89aea7b74aa4605b25af62e31303097a4a48cc Author: Alan Somers <asomers@FreeBSD.org> AuthorDate: 2024-08-07 15:21:08 +0000 Commit: Alan Somers <asomers@FreeBSD.org> CommitDate: 2024-09-18 20:06:31 +0000 ctld: fix several process setup/teardown bugs All of the below bugs could result in a system where ctld is not running, but LUNs and targets still exist in the kernel; a difficult situation to recover from. * open the pidfile earlier. Open the pidfile before reading the kernel's current state, so two racing ctld processes won't step on each others' toes. * close the pidfile later. Close it after tearing down the configuration, for the same reason. * If the configured pidfile changes, then rename it on SIGHUP rather than remove and recreate it. * When running in debug mode, don't close the pidfile while handling a new connection. Only do that in non-debug mode, in the child of the fork. * Register signal handlers earlier. Otherwise a SIGTERM signal received during startup could kill ctld without tearing down the configuration. MFC after: 2 weeks PR: 271460 Sponsored by: Axcient Reviewed by: mav Pull Request: https://github.com/freebsd/freebsd-src/pull/1370 --- usr.sbin/ctld/ctld.c | 70 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 30 deletions(-) diff --git a/usr.sbin/ctld/ctld.c b/usr.sbin/ctld/ctld.c index 805648f0465f..346d1908aa6f 100644 --- a/usr.sbin/ctld/ctld.c +++ b/usr.sbin/ctld/ctld.c @@ -1915,7 +1915,6 @@ conf_apply(struct conf *oldconf, struct conf *newconf) struct portal *oldp, *newp; struct port *oldport, *newport, *tmpport; struct isns *oldns, *newns; - pid_t otherpid; int changed, cumulated_error = 0, error, sockbuf; int one = 1; @@ -1924,32 +1923,25 @@ conf_apply(struct conf *oldconf, struct conf *newconf) log_init(newconf->conf_debug); } - if (oldconf->conf_pidfh != NULL) { - assert(oldconf->conf_pidfile_path != NULL); - if (newconf->conf_pidfile_path != NULL && - strcmp(oldconf->conf_pidfile_path, - newconf->conf_pidfile_path) == 0) { - newconf->conf_pidfh = oldconf->conf_pidfh; - oldconf->conf_pidfh = NULL; - } else { - log_debugx("removing pidfile %s", - oldconf->conf_pidfile_path); - pidfile_remove(oldconf->conf_pidfh); - oldconf->conf_pidfh = NULL; - } - } - - if (newconf->conf_pidfh == NULL && newconf->conf_pidfile_path != NULL) { - log_debugx("opening pidfile %s", newconf->conf_pidfile_path); - newconf->conf_pidfh = - pidfile_open(newconf->conf_pidfile_path, 0600, &otherpid); - if (newconf->conf_pidfh == NULL) { - if (errno == EEXIST) - log_errx(1, "daemon already running, pid: %jd.", - (intmax_t)otherpid); - log_err(1, "cannot open or create pidfile \"%s\"", - newconf->conf_pidfile_path); + if (oldconf->conf_pidfile_path != NULL && + newconf->conf_pidfile_path != NULL) + { + if (strcmp(oldconf->conf_pidfile_path, + newconf->conf_pidfile_path) != 0) + { + /* pidfile has changed. rename it */ + log_debugx("moving pidfile to %s", + newconf->conf_pidfile_path); + if (rename(oldconf->conf_pidfile_path, + newconf->conf_pidfile_path)) + { + log_err(1, "renaming pidfile %s -> %s", + oldconf->conf_pidfile_path, + newconf->conf_pidfile_path); + } } + newconf->conf_pidfh = oldconf->conf_pidfh; + oldconf->conf_pidfh = NULL; } /* @@ -2471,8 +2463,8 @@ handle_connection(struct portal *portal, int fd, close(fd); return; } + pidfile_close(conf->conf_pidfh); } - pidfile_close(conf->conf_pidfh); error = getnameinfo(client_sa, client_sa->sa_len, host, sizeof(host), NULL, 0, NI_NUMERICHOST); @@ -2807,6 +2799,7 @@ main(int argc, char **argv) struct isns *newns; const char *config_path = DEFAULT_CONFIG_PATH; int debug = 0, ch, error; + pid_t otherpid; bool dont_daemonize = false; bool test_config = false; bool use_ucl = false; @@ -2846,7 +2839,6 @@ main(int argc, char **argv) kernel_init(); TAILQ_INIT(&kports.pports); - oldconf = conf_new_from_kernel(&kports); newconf = conf_new_from_file(config_path, use_ucl); if (newconf == NULL) @@ -2855,6 +2847,22 @@ main(int argc, char **argv) if (test_config) return (0); + assert(newconf->conf_pidfile_path != NULL); + log_debugx("opening pidfile %s", newconf->conf_pidfile_path); + newconf->conf_pidfh = pidfile_open(newconf->conf_pidfile_path, 0600, + &otherpid); + if (newconf->conf_pidfh == NULL) { + if (errno == EEXIST) + log_errx(1, "daemon already running, pid: %jd.", + (intmax_t)otherpid); + log_err(1, "cannot open or create pidfile \"%s\"", + newconf->conf_pidfile_path); + } + + register_signals(); + + oldconf = conf_new_from_kernel(&kports); + if (debug > 0) { oldconf->conf_debug = debug; newconf->conf_debug = debug; @@ -2870,8 +2878,6 @@ main(int argc, char **argv) conf_delete(oldconf); oldconf = NULL; - register_signals(); - if (dont_daemonize == false) { log_debugx("daemonizing"); if (daemon(0, 0) == -1) { @@ -2926,6 +2932,10 @@ main(int argc, char **argv) error = conf_apply(oldconf, newconf); if (error != 0) log_warnx("failed to apply configuration"); + if (oldconf->conf_pidfh) { + pidfile_remove(oldconf->conf_pidfh); + oldconf->conf_pidfh = NULL; + } conf_delete(newconf); conf_delete(oldconf); oldconf = NULL;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202409190127.48J1RkCu007695>