[systemd-devel] [PATCH] udev: warn instead of killing kmod loading

[Luis R. Rodriguez]

On Mon, Aug 11, 2014 at 03:50:47PM +0200, Lennart Poettering wrote:
> On Fri, 08.08.14 19:16, Luis R. Rodriguez (mcgrof at do-not-panic.com) wrote:
> 
> This looks really wrong. We shouldn't permit worker processes to be
> blocked indefinitely without any timeout applied. Designing a worker
> process system like that is simply wrong. It's one thing to allow
> changing the specific timeout applied, it's a very different thing to
> allow broken drivers to completely stall the worker process logic.

OK what if we enable customizations then on the timeout by the built-in
cmd type and we use a high multiplier for now for kmod ? A multiplier
for kmod of 10 would set the kmod timeout to 5 minutes then, as we
sweep up and clean drivers we can reduce this over time. This would also
enable us to keep the default timeout for the other type of workers.

diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c
index 00cd6d4..b45b542 100644
--- a/src/udev/udev-event.c
+++ b/src/udev/udev-event.c
@@ -880,6 +880,29 @@ void udev_event_execute_rules(struct udev_event *event,
         }
 }
 
+#ifdef HAVE_KMOD
+static void udev_set_timeout_pref_kmod(struct udev_event *event)
+{
+        event->timeout_multiplier = 10;
+}
+#else
+static void udev_set_timeout_pref_kmod(struct udev_event *event)
+{
+        return;
+}
+#endif
+
+static void udev_builtin_set_timeout_pref(enum udev_builtin_cmd builtin_cmd,
+                                          struct udev_event *event)
+{
+        switch (builtin_cmd) {
+        case UDEV_BUILTIN_KMOD:
+                udev_set_timeout_pref_kmod(event);
+        default:
+                event->timeout_multiplier = 1;
+        }
+}
+
 void udev_event_execute_run(struct udev_event *event, usec_t timeout_usec, const sigset_t *sigmask) {
         struct udev_list_entry *list_entry;
 
@@ -890,6 +913,7 @@ void udev_event_execute_run(struct udev_event *event, usec_t timeout_usec, const
                 if (builtin_cmd < UDEV_BUILTIN_MAX) {
                         char command[UTIL_PATH_SIZE];
 
+                        udev_builtin_set_timeout_pref(builtin_cmd, event);
                         udev_event_apply_format(event, cmd, command, sizeof(command));
                         udev_builtin_run(event->dev, builtin_cmd, command, false);
                 } else {
diff --git a/src/udev/udev.h b/src/udev/udev.h
index 4aca70b..950acf3 100644
--- a/src/udev/udev.h
+++ b/src/udev/udev.h
@@ -58,6 +58,7 @@ struct udev_event {
         bool name_final;
         bool devlink_final;
         bool run_final;
+        unsigned int timeout_multiplier;
 };
 
 struct udev_watch {
diff --git a/src/udev/udevd.c b/src/udev/udevd.c
index f882cfb..75aca6f 100644
--- a/src/udev/udevd.c
+++ b/src/udev/udevd.c
@@ -100,6 +100,7 @@ struct event {
         dev_t devnum;
         int ifindex;
         bool is_block;
+        usec_t timeout;
 #ifdef HAVE_FIRMWARE
         bool nodelay;
 #endif
@@ -304,10 +305,12 @@ static void worker_new(struct event *event) {
                                 }
                         }
 
+                        event->timeout = event_timeout_usec * udev_event->timeout_multiplier;
+
                         /* apply rules, create node, symlinks */
-                        udev_event_execute_rules(udev_event, event_timeout_usec, rules, &sigmask_orig);
+                        udev_event_execute_rules(udev_event, event->timeout, rules, &sigmask_orig);
 
-                        udev_event_execute_run(udev_event, event_timeout_usec, &sigmask_orig);
+                        udev_event_execute_run(udev_event, event->timeout, &sigmask_orig);
 
                         /* apply/restore inotify watch */
                         if (udev_event->inotify_watch) {
@@ -1312,7 +1315,7 @@ int main(int argc, char *argv[]) {
                 static usec_t last_usec;
                 struct epoll_event ev[8];
                 int fdcount;
-                int timeout;
+                unsigned int timeout;
                 bool is_worker, is_signal, is_inotify, is_netlink, is_ctrl;
                 int i;
 
@@ -1392,7 +1395,7 @@ int main(int argc, char *argv[]) {
                                 if (worker->state != WORKER_RUNNING)
                                         continue;
 
-                                if ((now(CLOCK_MONOTONIC) - worker->event_start_usec) > event_timeout_usec) {
+                                if ((now(CLOCK_MONOTONIC) - worker->event_start_usec) > worker->event->timeout) {
                                         log_error("worker [%u] %s timeout; kill it", worker->pid, worker->event->devpath);
                                         kill(worker->pid, SIGKILL);
                                         worker->state = WORKER_KILLED;