From 8314de1d815667b0289423d7e6bfbe8d83759077 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Tue, 2 Jun 2015 17:07:21 +0200 Subject: [PATCH 1/9] udevd: simplify signal mask handling We used to block all signals, and restore the original signal mask before exec'ing external processes. Now we just block the signals we care about and unconditionally unblock all signals before exec'ing. --- src/test/test-udev.c | 13 ++++--------- src/udev/udev-event.c | 24 ++++++++++-------------- src/udev/udev-rules.c | 11 +++++------ src/udev/udev.h | 11 ++++------- src/udev/udevadm-test.c | 3 +-- src/udev/udevd.c | 13 +++++-------- 6 files changed, 29 insertions(+), 46 deletions(-) diff --git a/src/test/test-udev.c b/src/test/test-udev.c index f3953fe26a7..d1fe9530711 100644 --- a/src/test/test-udev.c +++ b/src/test/test-udev.c @@ -28,6 +28,7 @@ #include "missing.h" #include "selinux-util.h" +#include "signal-util.h" #include "udev.h" #include "udev-util.h" @@ -79,7 +80,6 @@ int main(int argc, char *argv[]) { char syspath[UTIL_PATH_SIZE]; const char *devpath; const char *action; - sigset_t mask, sigmask_orig; int err; err = fake_filesystems(); @@ -93,8 +93,6 @@ int main(int argc, char *argv[]) { log_debug("version %s", VERSION); mac_selinux_init("/dev"); - sigprocmask(SIG_SETMASK, NULL, &sigmask_orig); - action = argv[1]; if (action == NULL) { log_error("action missing"); @@ -118,8 +116,7 @@ int main(int argc, char *argv[]) { event = udev_event_new(dev); - sigfillset(&mask); - sigprocmask(SIG_SETMASK, &mask, &sigmask_orig); + assert_se(sigprocmask_many(SIG_BLOCK, SIGTERM, SIGINT, SIGHUP, SIGCHLD, -1) == 0); /* do what devtmpfs usually provides us */ if (udev_device_get_devnode(dev) != NULL) { @@ -142,11 +139,9 @@ int main(int argc, char *argv[]) { udev_event_execute_rules(event, 3 * USEC_PER_SEC, USEC_PER_SEC, NULL, - rules, - &sigmask_orig); + rules); udev_event_execute_run(event, - 3 * USEC_PER_SEC, USEC_PER_SEC, - NULL); + 3 * USEC_PER_SEC, USEC_PER_SEC); out: mac_selinux_finish(); diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index 3488ff3370e..4dcf8f2e1ca 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -385,7 +385,7 @@ out: } static int spawn_exec(struct udev_event *event, - const char *cmd, char *const argv[], char **envp, const sigset_t *sigmask, + const char *cmd, char *const argv[], char **envp, int fd_stdout, int fd_stderr) { _cleanup_close_ int fd = -1; @@ -413,9 +413,8 @@ static int spawn_exec(struct udev_event *event, /* terminate child in case parent goes away */ prctl(PR_SET_PDEATHSIG, SIGTERM); - /* restore original udev sigmask before exec */ - if (sigmask) - sigprocmask(SIG_SETMASK, sigmask, NULL); + /* restore sigmask before exec */ + (void) reset_signal_mask(); execve(argv[0], argv, envp); @@ -699,7 +698,7 @@ out: int udev_event_spawn(struct udev_event *event, usec_t timeout_usec, usec_t timeout_warn_usec, - const char *cmd, char **envp, const sigset_t *sigmask, + const char *cmd, char **envp, char *result, size_t ressize) { int outpipe[2] = {-1, -1}; int errpipe[2] = {-1, -1}; @@ -749,7 +748,7 @@ int udev_event_spawn(struct udev_event *event, log_debug("starting '%s'", cmd); - spawn_exec(event, cmd, argv, envp, sigmask, + spawn_exec(event, cmd, argv, envp, outpipe[WRITE_END], errpipe[WRITE_END]); _exit(2 ); @@ -811,8 +810,7 @@ static int rename_netif(struct udev_event *event) { void udev_event_execute_rules(struct udev_event *event, usec_t timeout_usec, usec_t timeout_warn_usec, struct udev_list *properties_list, - struct udev_rules *rules, - const sigset_t *sigmask) { + struct udev_rules *rules) { struct udev_device *dev = event->dev; if (udev_device_get_subsystem(dev) == NULL) @@ -828,8 +826,7 @@ void udev_event_execute_rules(struct udev_event *event, udev_rules_apply_to_event(rules, event, timeout_usec, timeout_warn_usec, - properties_list, - sigmask); + properties_list); if (major(udev_device_get_devnum(dev)) != 0) udev_node_remove(dev); @@ -847,8 +844,7 @@ void udev_event_execute_rules(struct udev_event *event, udev_rules_apply_to_event(rules, event, timeout_usec, timeout_warn_usec, - properties_list, - sigmask); + properties_list); /* rename a new network interface, if needed */ if (udev_device_get_ifindex(dev) > 0 && streq(udev_device_get_action(dev), "add") && @@ -911,7 +907,7 @@ void udev_event_execute_rules(struct udev_event *event, } } -void udev_event_execute_run(struct udev_event *event, usec_t timeout_usec, usec_t timeout_warn_usec, const sigset_t *sigmask) { +void udev_event_execute_run(struct udev_event *event, usec_t timeout_usec, usec_t timeout_warn_usec) { struct udev_list_entry *list_entry; udev_list_entry_foreach(list_entry, udev_list_get_entry(&event->run_list)) { @@ -934,7 +930,7 @@ void udev_event_execute_run(struct udev_event *event, usec_t timeout_usec, usec_ udev_event_apply_format(event, cmd, program, sizeof(program)); envp = udev_device_get_properties_envp(event->dev); - udev_event_spawn(event, timeout_usec, timeout_warn_usec, program, envp, sigmask, NULL, 0); + udev_event_spawn(event, timeout_usec, timeout_warn_usec, program, envp, NULL, 0); } } } diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index c2b20cf547b..915371525f5 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -633,7 +633,7 @@ static int import_file_into_properties(struct udev_device *dev, const char *file static int import_program_into_properties(struct udev_event *event, usec_t timeout_usec, usec_t timeout_warn_usec, - const char *program, const sigset_t *sigmask) { + const char *program) { struct udev_device *dev = event->dev; char **envp; char result[UTIL_LINE_SIZE]; @@ -641,7 +641,7 @@ static int import_program_into_properties(struct udev_event *event, int err; envp = udev_device_get_properties_envp(dev); - err = udev_event_spawn(event, timeout_usec, timeout_warn_usec, program, envp, sigmask, result, sizeof(result)); + err = udev_event_spawn(event, timeout_usec, timeout_warn_usec, program, envp, result, sizeof(result)); if (err < 0) return err; @@ -1895,8 +1895,7 @@ int udev_rules_apply_to_event(struct udev_rules *rules, struct udev_event *event, usec_t timeout_usec, usec_t timeout_warn_usec, - struct udev_list *properties_list, - const sigset_t *sigmask) { + struct udev_list *properties_list) { struct token *cur; struct token *rule; enum escape_type esc = ESCAPE_UNSET; @@ -2132,7 +2131,7 @@ int udev_rules_apply_to_event(struct udev_rules *rules, rules_str(rules, rule->rule.filename_off), rule->rule.filename_line); - if (udev_event_spawn(event, timeout_usec, timeout_warn_usec, program, envp, sigmask, result, sizeof(result)) < 0) { + if (udev_event_spawn(event, timeout_usec, timeout_warn_usec, program, envp, result, sizeof(result)) < 0) { if (cur->key.op != OP_NOMATCH) goto nomatch; } else { @@ -2168,7 +2167,7 @@ int udev_rules_apply_to_event(struct udev_rules *rules, rules_str(rules, rule->rule.filename_off), rule->rule.filename_line); - if (import_program_into_properties(event, timeout_usec, timeout_warn_usec, import, sigmask) != 0) + if (import_program_into_properties(event, timeout_usec, timeout_warn_usec, import) != 0) if (cur->key.op != OP_NOMATCH) goto nomatch; break; diff --git a/src/udev/udev.h b/src/udev/udev.h index 1b17c615b89..fd8504c424d 100644 --- a/src/udev/udev.h +++ b/src/udev/udev.h @@ -20,7 +20,6 @@ #include #include -#include #include "macro.h" #include "sd-rtnl.h" @@ -73,8 +72,7 @@ struct udev_rules *udev_rules_unref(struct udev_rules *rules); bool udev_rules_check_timestamp(struct udev_rules *rules); int udev_rules_apply_to_event(struct udev_rules *rules, struct udev_event *event, usec_t timeout_usec, usec_t timeout_warn_usec, - struct udev_list *properties_list, - const sigset_t *sigmask); + struct udev_list *properties_list); int udev_rules_apply_static_dev_perms(struct udev_rules *rules); /* udev-event.c */ @@ -86,14 +84,13 @@ int udev_event_apply_subsys_kernel(struct udev_event *event, const char *string, int udev_event_spawn(struct udev_event *event, usec_t timeout_usec, usec_t timeout_warn_usec, - const char *cmd, char **envp, const sigset_t *sigmask, + const char *cmd, char **envp, char *result, size_t ressize); void udev_event_execute_rules(struct udev_event *event, usec_t timeout_usec, usec_t timeout_warn_usec, struct udev_list *properties_list, - struct udev_rules *rules, - const sigset_t *sigset); -void udev_event_execute_run(struct udev_event *event, usec_t timeout_usec, usec_t timeout_warn_usec, const sigset_t *sigset); + struct udev_rules *rules); +void udev_event_execute_run(struct udev_event *event, usec_t timeout_usec, usec_t timeout_warn_usec); int udev_build_argv(struct udev *udev, char *cmd, int *argc, char *argv[]); /* udev-watch.c */ diff --git a/src/udev/udevadm-test.c b/src/udev/udevadm-test.c index 46ec0e32251..d04e618d0d2 100644 --- a/src/udev/udevadm-test.c +++ b/src/udev/udevadm-test.c @@ -135,8 +135,7 @@ static int adm_test(struct udev *udev, int argc, char *argv[]) { udev_event_execute_rules(event, 60 * USEC_PER_SEC, 20 * USEC_PER_SEC, NULL, - rules, - &sigmask_orig); + rules); udev_list_entry_foreach(entry, udev_device_get_properties_list_entry(dev)) printf("%s=%s\n", udev_list_entry_get_name(entry), udev_list_entry_get_value(entry)); diff --git a/src/udev/udevd.c b/src/udev/udevd.c index 056cf8c8ee6..830bad95412 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -42,6 +42,8 @@ #include "sd-daemon.h" #include "sd-event.h" + +#include "signal-util.h" #include "event-util.h" #include "rtnl-util.h" #include "cgroup-util.h" @@ -69,7 +71,6 @@ typedef struct Manager { struct udev_list_node events; char *cgroup; pid_t pid; /* the process that originally allocated the manager object */ - sigset_t sigmask_orig; struct udev_rules *rules; struct udev_list properties; @@ -448,12 +449,10 @@ static void worker_spawn(Manager *manager, struct event *event) { udev_event_execute_rules(udev_event, arg_event_timeout_usec, arg_event_timeout_warn_usec, &manager->properties, - manager->rules, - &manager->sigmask_orig); + manager->rules); udev_event_execute_run(udev_event, - arg_event_timeout_usec, arg_event_timeout_warn_usec, - &manager->sigmask_orig); + arg_event_timeout_usec, arg_event_timeout_warn_usec); if (udev_event->rtnl) /* in case rtnl was initialized */ @@ -1513,7 +1512,6 @@ static int manager_new(Manager **ret) { } static int manager_listen(Manager *manager) { - sigset_t mask; int r, fd_worker, one = 1; assert(manager); @@ -1536,8 +1534,7 @@ static int manager_listen(Manager *manager) { udev_watch_restore(manager->udev); /* block and listen to all signals on signalfd */ - sigfillset(&mask); - sigprocmask(SIG_SETMASK, &mask, &manager->sigmask_orig); + assert_se(sigprocmask_many(SIG_BLOCK, SIGTERM, SIGINT, SIGHUP, SIGCHLD, -1) == 0); r = sd_event_default(&manager->event); if (r < 0) From fcff1e7241377b44f1d6e2a68ed55940b154ed4e Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Tue, 2 Jun 2015 19:04:38 +0200 Subject: [PATCH 2/9] udevd: rename systemd_fds to listen_fds --- src/udev/udevd.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/src/udev/udevd.c b/src/udev/udevd.c index 830bad95412..1540f5c8f9d 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -1262,38 +1262,43 @@ static int on_post(sd_event_source *s, void *userdata) { return 1; } -static int systemd_fds(int *rctrl, int *rnetlink) { - int ctrl = -1, netlink = -1; +static int listen_fds(int *rctrl, int *rnetlink) { + int ctrl_fd = -1, netlink_fd = -1; int fd, n; + assert(rctrl); + assert(rnetlink); + n = sd_listen_fds(true); - if (n <= 0) - return -1; + if (n < 0) + return n; for (fd = SD_LISTEN_FDS_START; fd < n + SD_LISTEN_FDS_START; fd++) { if (sd_is_socket(fd, AF_LOCAL, SOCK_SEQPACKET, -1)) { - if (ctrl >= 0) - return -1; - ctrl = fd; + if (ctrl_fd >= 0) + return -EINVAL; + ctrl_fd = fd; continue; } if (sd_is_socket(fd, AF_NETLINK, SOCK_RAW, -1)) { - if (netlink >= 0) - return -1; - netlink = fd; + if (netlink_fd >= 0) + return -EINVAL; + netlink_fd = fd; continue; } - return -1; + return -EINVAL; } - if (ctrl < 0 || netlink < 0) - return -1; + if (ctrl_fd < 0 || netlink_fd < 0) + return -EINVAL; + + log_debug("ctrl=%i netlink=%i", ctrl_fd, netlink_fd); + + *rctrl = ctrl_fd; + *rnetlink = netlink_fd; - log_debug("ctrl=%i netlink=%i", ctrl, netlink); - *rctrl = ctrl; - *rnetlink = netlink; return 0; } @@ -1465,7 +1470,7 @@ static int manager_new(Manager **ret) { udev_list_node_init(&manager->events); udev_list_init(manager->udev, &manager->properties, true); - r = systemd_fds(&fd_ctrl, &fd_uevent); + r = listen_fds(&fd_ctrl, &fd_uevent); if (r >= 0) { /* get control and netlink socket from systemd */ manager->ctrl = udev_ctrl_new_from_fd(manager->udev, fd_ctrl); From bf6871639e25119cf3f8b414890d51e965ca3f97 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Tue, 2 Jun 2015 19:18:53 +0200 Subject: [PATCH 3/9] udevd: only bind ctrl and netlink sockets when we open them If they are passed from PID1 this is not necessary. --- src/udev/udevd.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/udev/udevd.c b/src/udev/udevd.c index 1540f5c8f9d..ccdec3de439 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -1500,16 +1500,16 @@ static int manager_new(Manager **ret) { fd_uevent = udev_monitor_get_fd(manager->monitor); (void) udev_monitor_set_receive_buffer_size(manager->monitor, 128 * 1024 * 1024); + + r = udev_monitor_enable_receiving(manager->monitor); + if (r < 0) + return log_error_errno(EINVAL, "error binding netlink socket"); + + r = udev_ctrl_enable_receiving(manager->ctrl); + if (r < 0) + return log_error_errno(EINVAL, "error binding udev control socket"); } - r = udev_monitor_enable_receiving(manager->monitor); - if (r < 0) - return log_error_errno(EINVAL, "error binding netlink socket"); - - r = udev_ctrl_enable_receiving(manager->ctrl); - if (r < 0) - return log_error_errno(EINVAL, "error binding udev control socket"); - *ret = manager; manager = NULL; From c26d1879c72fbaa147c0a82df433f676df139a90 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Wed, 3 Jun 2015 01:53:20 +0200 Subject: [PATCH 4/9] udevd: make cgroup logic independent of socket passing This should have no behavioural change, but it is odd to tie the cgroup cleaning to whether or not we are passed sockets. The point really is if we are guaranteed to be in a dedicated cgroup, so instead check for our parent being PID1 (we already implicitly only do this on systemd systems). --- src/udev/udevd.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/udev/udevd.c b/src/udev/udevd.c index ccdec3de439..121491753ac 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -69,7 +69,7 @@ typedef struct Manager { sd_event *event; Hashmap *workers; struct udev_list_node events; - char *cgroup; + const char *cgroup; pid_t pid; /* the process that originally allocated the manager object */ struct udev_rules *rules; @@ -307,7 +307,6 @@ static void manager_free(Manager *manager) { udev_list_cleanup(&manager->properties); udev_rules_unref(manager->rules); - free(manager->cgroup); safe_close(manager->fd_inotify); safe_close_pair(manager->worker_watch); @@ -1443,7 +1442,7 @@ static int parse_argv(int argc, char *argv[]) { return 1; } -static int manager_new(Manager **ret) { +static int manager_new(Manager **ret, const char *cgroup) { _cleanup_(manager_freep) Manager *manager = NULL; int r, fd_ctrl, fd_uevent; @@ -1470,6 +1469,8 @@ static int manager_new(Manager **ret) { udev_list_node_init(&manager->events); udev_list_init(manager->udev, &manager->properties, true); + manager->cgroup = cgroup; + r = listen_fds(&fd_ctrl, &fd_uevent); if (r >= 0) { /* get control and netlink socket from systemd */ @@ -1480,11 +1481,6 @@ static int manager_new(Manager **ret) { manager->monitor = udev_monitor_new_from_netlink_fd(manager->udev, "kernel", fd_uevent); if (!manager->monitor) return log_error_errno(EINVAL, "error taking over netlink socket"); - - /* get our own cgroup, we regularly kill everything udev has left behind */ - r = cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, 0, &manager->cgroup); - if (r < 0) - log_warning_errno(r, "failed to get cgroup: %m"); } else { /* open control and netlink socket */ manager->ctrl = udev_ctrl_new(manager->udev); @@ -1598,6 +1594,7 @@ static int manager_listen(Manager *manager) { int main(int argc, char *argv[]) { _cleanup_(manager_freep) Manager *manager = NULL; + _cleanup_free_ char *cgroup = NULL; int r; log_set_target(LOG_TARGET_AUTO); @@ -1655,7 +1652,16 @@ int main(int argc, char *argv[]) { dev_setup(NULL, UID_INVALID, GID_INVALID); - r = manager_new(&manager); + if (getppid() == 1) { + /* get our own cgroup, we regularly kill everything udev has left behind + we only do this on systemd systems, and only if we are directly spawned + by PID1. otherwise we are not guaranteed to have a dedicated cgroup */ + r = cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, 0, &cgroup); + if (r < 0) + log_warning_errno(r, "failed to get cgroup: %m"); + } + + r = manager_new(&manager, cgroup); if (r < 0) goto exit; From 44daf75d985683190255e9cf7eb2eea0f370be02 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Tue, 2 Jun 2015 20:57:52 +0200 Subject: [PATCH 5/9] libudev: monitor - set nl_pid when reusing fd in udev_monitor_new_from_netlink_fd This allows a fd to be created and configured as part of one monitor, to be passed in to create a second monitor without having to redo any of the configuration. --- src/libudev/libudev-monitor.c | 36 +++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/libudev/libudev-monitor.c b/src/libudev/libudev-monitor.c index b13c5794609..282aa2b0d9b 100644 --- a/src/libudev/libudev-monitor.c +++ b/src/libudev/libudev-monitor.c @@ -144,6 +144,22 @@ static bool udev_has_devtmpfs(struct udev *udev) { return false; } +static void monitor_set_nl_address(struct udev_monitor *udev_monitor) { + union sockaddr_union snl; + socklen_t addrlen; + int r; + + assert(udev_monitor); + + /* get the address the kernel has assigned us + * it is usually, but not necessarily the pid + */ + addrlen = sizeof(struct sockaddr_nl); + r = getsockname(udev_monitor->sock, &snl.sa, &addrlen); + if (r >= 0) + udev_monitor->snl.nl.nl_pid = snl.nl.nl_pid; +} + struct udev_monitor *udev_monitor_new_from_netlink_fd(struct udev *udev, const char *name, int fd) { struct udev_monitor *udev_monitor; @@ -183,7 +199,7 @@ struct udev_monitor *udev_monitor_new_from_netlink_fd(struct udev *udev, const c if (fd < 0) { udev_monitor->sock = socket(PF_NETLINK, SOCK_RAW|SOCK_CLOEXEC|SOCK_NONBLOCK, NETLINK_KOBJECT_UEVENT); - if (udev_monitor->sock == -1) { + if (udev_monitor->sock < 0) { log_debug_errno(errno, "error getting socket: %m"); free(udev_monitor); return NULL; @@ -191,6 +207,7 @@ struct udev_monitor *udev_monitor_new_from_netlink_fd(struct udev *udev, const c } else { udev_monitor->bound = true; udev_monitor->sock = fd; + monitor_set_nl_address(udev_monitor); } udev_monitor->snl.nl.nl_family = AF_NETLINK; @@ -366,6 +383,7 @@ int udev_monitor_allow_unicast_sender(struct udev_monitor *udev_monitor, struct udev_monitor->snl_trusted_sender.nl.nl_pid = sender->snl.nl.nl_pid; return 0; } + /** * udev_monitor_enable_receiving: * @udev_monitor: the monitor which should receive events @@ -388,19 +406,9 @@ _public_ int udev_monitor_enable_receiving(struct udev_monitor *udev_monitor) udev_monitor->bound = true; } - if (err >= 0) { - union sockaddr_union snl; - socklen_t addrlen; - - /* - * get the address the kernel has assigned us - * it is usually, but not necessarily the pid - */ - addrlen = sizeof(struct sockaddr_nl); - err = getsockname(udev_monitor->sock, &snl.sa, &addrlen); - if (err == 0) - udev_monitor->snl.nl.nl_pid = snl.nl.nl_pid; - } else { + if (err >= 0) + monitor_set_nl_address(udev_monitor); + else { log_debug_errno(errno, "bind failed: %m"); return -errno; } From f59118ec7911730bb79b273bb4034f5eb4c80868 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Tue, 2 Jun 2015 21:03:36 +0200 Subject: [PATCH 6/9] udevd: unify fd handling in forking/notify modes Hide the differenec in listen_fds, by simply opening the fds here in case they are not passed in. --- src/udev/udevd.c | 95 ++++++++++++++++++++++++++++++------------------ 1 file changed, 60 insertions(+), 35 deletions(-) diff --git a/src/udev/udevd.c b/src/udev/udevd.c index 121491753ac..259767b3f62 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -1262,8 +1262,9 @@ static int on_post(sd_event_source *s, void *userdata) { } static int listen_fds(int *rctrl, int *rnetlink) { + _cleanup_udev_unref_ struct udev *udev = NULL; int ctrl_fd = -1, netlink_fd = -1; - int fd, n; + int fd, n, r; assert(rctrl); assert(rnetlink); @@ -1290,10 +1291,57 @@ static int listen_fds(int *rctrl, int *rnetlink) { return -EINVAL; } - if (ctrl_fd < 0 || netlink_fd < 0) - return -EINVAL; + if (ctrl_fd < 0) { + _cleanup_udev_ctrl_unref_ struct udev_ctrl *ctrl = NULL; - log_debug("ctrl=%i netlink=%i", ctrl_fd, netlink_fd); + udev = udev_new(); + if (!udev) + return -ENOMEM; + + ctrl = udev_ctrl_new(udev); + if (!ctrl) + return log_error_errno(EINVAL, "error initializing udev control socket"); + + r = udev_ctrl_enable_receiving(ctrl); + if (r < 0) + return log_error_errno(EINVAL, "error binding udev control socket"); + + fd = udev_ctrl_get_fd(ctrl); + if (fd < 0) + return log_error_errno(EIO, "could not get ctrl fd"); + + ctrl_fd = fcntl(fd, F_DUPFD_CLOEXEC, 3); + if (ctrl_fd < 0) + return log_error_errno(errno, "could not dup ctrl fd: %m"); + } + + if (netlink_fd < 0) { + _cleanup_udev_monitor_unref_ struct udev_monitor *monitor = NULL; + + if (!udev) { + udev = udev_new(); + if (!udev) + return -ENOMEM; + } + + monitor = udev_monitor_new_from_netlink(udev, "kernel"); + if (!monitor) + return log_error_errno(EINVAL, "error initializing netlink socket"); + + (void) udev_monitor_set_receive_buffer_size(monitor, 128 * 1024 * 1024); + + r = udev_monitor_enable_receiving(monitor); + if (r < 0) + return log_error_errno(EINVAL, "error binding netlink socket"); + + fd = udev_monitor_get_fd(monitor); + if (fd < 0) + return log_error_errno(netlink_fd, "could not get uevent fd: %m"); + + netlink_fd = fcntl(fd, F_DUPFD_CLOEXEC, 3); + if (ctrl_fd < 0) + return log_error_errno(errno, "could not dup netlink fd: %m"); + } *rctrl = ctrl_fd; *rnetlink = netlink_fd; @@ -1472,39 +1520,16 @@ static int manager_new(Manager **ret, const char *cgroup) { manager->cgroup = cgroup; r = listen_fds(&fd_ctrl, &fd_uevent); - if (r >= 0) { - /* get control and netlink socket from systemd */ - manager->ctrl = udev_ctrl_new_from_fd(manager->udev, fd_ctrl); - if (!manager->ctrl) - return log_error_errno(EINVAL, "error taking over udev control socket"); + if (r < 0) + return log_error_errno(r, "could not listen on fds: %m"); - manager->monitor = udev_monitor_new_from_netlink_fd(manager->udev, "kernel", fd_uevent); - if (!manager->monitor) - return log_error_errno(EINVAL, "error taking over netlink socket"); - } else { - /* open control and netlink socket */ - manager->ctrl = udev_ctrl_new(manager->udev); - if (!manager->ctrl) - return log_error_errno(EINVAL, "error initializing udev control socket"); + manager->ctrl = udev_ctrl_new_from_fd(manager->udev, fd_ctrl); + if (!manager->ctrl) + return log_error_errno(EINVAL, "error taking over udev control socket"); - fd_ctrl = udev_ctrl_get_fd(manager->ctrl); - - manager->monitor = udev_monitor_new_from_netlink(manager->udev, "kernel"); - if (!manager->monitor) - return log_error_errno(EINVAL, "error initializing netlink socket"); - - fd_uevent = udev_monitor_get_fd(manager->monitor); - - (void) udev_monitor_set_receive_buffer_size(manager->monitor, 128 * 1024 * 1024); - - r = udev_monitor_enable_receiving(manager->monitor); - if (r < 0) - return log_error_errno(EINVAL, "error binding netlink socket"); - - r = udev_ctrl_enable_receiving(manager->ctrl); - if (r < 0) - return log_error_errno(EINVAL, "error binding udev control socket"); - } + manager->monitor = udev_monitor_new_from_netlink_fd(manager->udev, "kernel", fd_uevent); + if (!manager->monitor) + return log_error_errno(EINVAL, "error taking over netlink socket"); *ret = manager; manager = NULL; From b7f74dd48f7f3166caf781487c1f5c51f6b70d48 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Tue, 2 Jun 2015 23:05:40 +0200 Subject: [PATCH 7/9] udevd: manager - split listen_fds() out of manager_new() This will allow us in a follow-up commit to listen to fds before forking and still allocate the manager only after the fork. --- src/udev/udevd.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/udev/udevd.c b/src/udev/udevd.c index 259767b3f62..a6cddf6e5a4 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -1490,9 +1490,8 @@ static int parse_argv(int argc, char *argv[]) { return 1; } -static int manager_new(Manager **ret, const char *cgroup) { +static int manager_new(Manager **ret, int fd_ctrl, int fd_uevent, const char *cgroup) { _cleanup_(manager_freep) Manager *manager = NULL; - int r, fd_ctrl, fd_uevent; assert(ret); @@ -1519,10 +1518,6 @@ static int manager_new(Manager **ret, const char *cgroup) { manager->cgroup = cgroup; - r = listen_fds(&fd_ctrl, &fd_uevent); - if (r < 0) - return log_error_errno(r, "could not listen on fds: %m"); - manager->ctrl = udev_ctrl_new_from_fd(manager->udev, fd_ctrl); if (!manager->ctrl) return log_error_errno(EINVAL, "error taking over udev control socket"); @@ -1620,7 +1615,7 @@ static int manager_listen(Manager *manager) { int main(int argc, char *argv[]) { _cleanup_(manager_freep) Manager *manager = NULL; _cleanup_free_ char *cgroup = NULL; - int r; + int r, fd_ctrl, fd_uevent; log_set_target(LOG_TARGET_AUTO); log_parse_environment(); @@ -1686,7 +1681,13 @@ int main(int argc, char *argv[]) { log_warning_errno(r, "failed to get cgroup: %m"); } - r = manager_new(&manager, cgroup); + r = listen_fds(&fd_ctrl, &fd_uevent); + if (r < 0) { + r = log_error_errno(r, "could not listen on fds: %m"); + goto exit; + } + + r = manager_new(&manager, fd_ctrl, fd_uevent, cgroup); if (r < 0) goto exit; From 7500cd5e96cff52a726d03194e3b927771ddb1d4 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Tue, 2 Jun 2015 23:08:11 +0200 Subject: [PATCH 8/9] udevd: make sd_notify independent of forknig/notify mode This will simply silently fail on non-systemd systems, so there is no reason to make it conditional. Also make it clear that we notify systemd about being ready as the last step before starting the event loop, whereas the forking might need to happen earlier. --- src/udev/udevd.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/udev/udevd.c b/src/udev/udevd.c index a6cddf6e5a4..5fee67d8df1 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -1716,15 +1716,16 @@ int main(int argc, char *argv[]) { setsid(); write_string_file("/proc/self/oom_score_adj", "-1000"); - } else - sd_notify(false, - "READY=1\n" - "STATUS=Processing..."); + } r = manager_listen(manager); if (r < 0) return log_error_errno(r, "failed to set up fds and listen for events: %m"); + (void) sd_notify(false, + "READY=1\n" + "STATUS=Processing..."); + r = sd_event_loop(manager->event); if (r < 0) { log_error_errno(r, "event loop failed: %m"); From 11b1dd8cecd486d5b1dd8fb27373ac79ea467ff8 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Tue, 2 Jun 2015 23:14:34 +0200 Subject: [PATCH 9/9] udevd: merge manager_new() and manager_listen() again Now that listen_fds() have been split out, we can safely move the allocation of the manager object after doing the forking (the fork is done to notify legcay init-systems that the fds are ready). Subsequently, we can merge manager_listen() back into managre_new(). This entails a minor behaviour change: the application of permissions to static device nodes now happens after the fork (but still before notifying systemd about being ready). --- src/udev/udevd.c | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/src/udev/udevd.c b/src/udev/udevd.c index 5fee67d8df1..eb430911901 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -1492,8 +1492,11 @@ static int parse_argv(int argc, char *argv[]) { static int manager_new(Manager **ret, int fd_ctrl, int fd_uevent, const char *cgroup) { _cleanup_(manager_freep) Manager *manager = NULL; + int r, fd_worker, one = 1; assert(ret); + assert(fd_ctrl >= 0); + assert(fd_uevent >= 0); manager = new0(Manager, 1); if (!manager) @@ -1526,17 +1529,6 @@ static int manager_new(Manager **ret, int fd_ctrl, int fd_uevent, const char *cg if (!manager->monitor) return log_error_errno(EINVAL, "error taking over netlink socket"); - *ret = manager; - manager = NULL; - - return 0; -} - -static int manager_listen(Manager *manager) { - int r, fd_worker, one = 1; - - assert(manager); - /* unnamed socket from workers to the main daemon */ r = socketpair(AF_LOCAL, SOCK_DGRAM|SOCK_CLOEXEC, 0, manager->worker_watch); if (r < 0) @@ -1581,7 +1573,7 @@ static int manager_listen(Manager *manager) { if (r < 0) return log_error_errno(r, "error creating watchdog event source: %m"); - r = sd_event_add_io(manager->event, &manager->ctrl_event, udev_ctrl_get_fd(manager->ctrl), EPOLLIN, on_ctrl_msg, manager); + r = sd_event_add_io(manager->event, &manager->ctrl_event, fd_ctrl, EPOLLIN, on_ctrl_msg, manager); if (r < 0) return log_error_errno(r, "error creating ctrl event source: %m"); @@ -1597,7 +1589,7 @@ static int manager_listen(Manager *manager) { if (r < 0) return log_error_errno(r, "error creating inotify event source: %m"); - r = sd_event_add_io(manager->event, &manager->uevent_event, udev_monitor_get_fd(manager->monitor), EPOLLIN, on_uevent, manager); + r = sd_event_add_io(manager->event, &manager->uevent_event, fd_uevent, EPOLLIN, on_uevent, manager); if (r < 0) return log_error_errno(r, "error creating uevent event source: %m"); @@ -1609,6 +1601,9 @@ static int manager_listen(Manager *manager) { if (r < 0) return log_error_errno(r, "error creating post event source: %m"); + *ret = manager; + manager = NULL; + return 0; } @@ -1687,14 +1682,6 @@ int main(int argc, char *argv[]) { goto exit; } - r = manager_new(&manager, fd_ctrl, fd_uevent, cgroup); - if (r < 0) - goto exit; - - r = udev_rules_apply_static_dev_perms(manager->rules); - if (r < 0) - log_error_errno(r, "failed to apply permissions on static device nodes: %m"); - if (arg_daemonize) { pid_t pid; @@ -1718,9 +1705,15 @@ int main(int argc, char *argv[]) { write_string_file("/proc/self/oom_score_adj", "-1000"); } - r = manager_listen(manager); + r = manager_new(&manager, fd_ctrl, fd_uevent, cgroup); + if (r < 0) { + r = log_error_errno(r, "failed to allocate manager object: %m"); + goto exit; + } + + r = udev_rules_apply_static_dev_perms(manager->rules); if (r < 0) - return log_error_errno(r, "failed to set up fds and listen for events: %m"); + log_error_errno(r, "failed to apply permissions on static device nodes: %m"); (void) sd_notify(false, "READY=1\n"