ipcpd: Fix shutdown of link-state routing

The link_state component tried to unregister from the notifier while
it had threads running.

The deadlock happens when notifier_event had a rdlock trying to delete
a neighbour, while notifier_unreg tries to take the write lock. And
the delete can't complete in the notifier.

https://tree.taiga.io/project/dstaesse-ouroboros/us/113

We need to make sure that all threads are stopped before unregistering
from the notifier.

Updated link_state to use the start/stop() paradigm and not
create/cancel threads within init/fini().

Signed-off-by: Dimitri Staessens <dimitri@ouroboros.rocks>
Signed-off-by: Sander Vrijders <sander@ouroboros.rocks>

6 files changed, 85 insertions, 34 deletions

diff --git a/src/ipcpd/unicast/dt.c b/src/ipcpd/unicast/dt.c
index 39b0025f..38cee75d 100644
--- a/src/ipcpd/unicast/dt.c
+++ b/src/ipcpd/unicast/dt.c
@@ -726,8 +726,16 @@ int dt_start(void)
                 goto fail_listener;
         }
 
+        if (routing_start() < 0) {
+                log_err("Failed to start routing.");
+                goto fail_routing;
+        }
+
         return 0;
 
+ fail_routing:
+        pthread_cancel(dt.listener);
+        pthread_join(dt.listener, NULL);
  fail_listener:
         notifier_unreg(&handle_event);
  fail_notifier_reg:
@@ -738,6 +746,8 @@ int dt_start(void)
 
 void dt_stop(void)
 {
+        routing_stop();
+
         pthread_cancel(dt.listener);
         pthread_join(dt.listener, NULL);
 
diff --git a/src/ipcpd/unicast/routing.c b/src/ipcpd/unicast/routing.c
index f5417c24..4efc3ea6 100644
--- a/src/ipcpd/unicast/routing.c
+++ b/src/ipcpd/unicast/routing.c
@@ -57,6 +57,11 @@ int routing_init(enum pol_routing pr)
         return pff_type;
 }
 
+int routing_start(void)
+{
+        return r_ops->start();
+}
+
 struct routing_i * routing_i_create(struct pff * pff)
 {
         return r_ops->routing_i_create(pff);
@@ -67,6 +72,11 @@ void routing_i_destroy(struct routing_i * instance)
         return r_ops->routing_i_destroy(instance);
 }
 
+void routing_stop(void)
+{
+        r_ops->stop();
+}
+
 void routing_fini(void)
 {
         r_ops->fini();
diff --git a/src/ipcpd/unicast/routing.h b/src/ipcpd/unicast/routing.h
index d5d833ae..9ac8a547 100644
--- a/src/ipcpd/unicast/routing.h
+++ b/src/ipcpd/unicast/routing.h
@@ -34,6 +34,10 @@ int                routing_init(enum pol_routing pr);
 
 void               routing_fini(void);
 
+int                routing_start(void);
+
+void               routing_stop(void);
+
 struct routing_i * routing_i_create(struct pff * pff);
 
 void               routing_i_destroy(struct routing_i * instance);
diff --git a/src/ipcpd/unicast/routing/link-state.c b/src/ipcpd/unicast/routing/link-state.c
index 7ad40df7..31b5a112 100644
--- a/src/ipcpd/unicast/routing/link-state.c
+++ b/src/ipcpd/unicast/routing/link-state.c
@@ -139,6 +139,8 @@ struct {
 struct routing_ops link_state_ops = {
         .init              = link_state_init,
         .fini              = link_state_fini,
+        .start             = link_state_start,
+        .stop              = link_state_stop,
         .routing_i_create  = link_state_routing_i_create,
         .routing_i_destroy = link_state_routing_i_destroy
 };
@@ -959,13 +961,63 @@ void link_state_routing_i_destroy(struct routing_i * instance)
         free(instance);
 }
 
+int link_state_start(void)
+{
+        if (notifier_reg(handle_event, NULL)) {
+                log_err("Failed to register link-state with notifier.");
+                goto fail_notifier_reg;
+        }
+
+        if (pthread_create(&ls.lsupdate, NULL, lsupdate, NULL)) {
+                log_err("Failed to create lsupdate thread.");
+                goto fail_pthread_create_lsupdate;
+        }
+
+        if (pthread_create(&ls.lsreader, NULL, lsreader, NULL)) {
+                log_err("Failed to create lsreader thread.");
+                goto fail_pthread_create_lsreader;
+        }
+
+        if (pthread_create(&ls.listener, NULL, ls_conn_handle, NULL)) {
+                log_err("Failed to create listener thread.");
+                goto fail_pthread_create_listener;
+        }
+
+        return 0;
+
+ fail_pthread_create_listener:
+        pthread_cancel(ls.lsreader);
+        pthread_join(ls.lsreader, NULL);
+ fail_pthread_create_lsreader:
+        pthread_cancel(ls.lsupdate);
+        pthread_join(ls.lsupdate, NULL);
+ fail_pthread_create_lsupdate:
+        notifier_unreg(handle_event);
+ fail_notifier_reg:
+        return -1;
+}
+
+void link_state_stop(void)
+{
+        pthread_cancel(ls.listener);
+        pthread_cancel(ls.lsreader);
+        pthread_cancel(ls.lsupdate);
+
+        pthread_join(ls.listener, NULL);
+        pthread_join(ls.lsreader, NULL);
+        pthread_join(ls.lsupdate, NULL);
+
+        notifier_unreg(handle_event);
+}
+
+
 int link_state_init(enum pol_routing pr)
 {
         struct conn_info info;
 
         memset(&info, 0, sizeof(info));
 
-        ls.addr    = addr_auth_address();
+        ls.addr = addr_auth_address();
 
         strcpy(info.comp_name, LS_COMP);
         strcpy(info.protocol, LS_PROTO);
@@ -994,9 +1046,6 @@ int link_state_init(enum pol_routing pr)
         if (ls.graph == NULL)
                 goto fail_graph;
 
-        if (notifier_reg(handle_event, NULL))
-                goto fail_notifier_reg;
-
         if (pthread_rwlock_init(&ls.db_lock, NULL))
                 goto fail_db_lock_init;
 
@@ -1014,15 +1063,6 @@ int link_state_init(enum pol_routing pr)
         list_head_init(&ls.nbs);
         list_head_init(&ls.routing_instances);
 
-        if (pthread_create(&ls.lsupdate, NULL, lsupdate, NULL))
-                goto fail_pthread_create_lsupdate;
-
-        if (pthread_create(&ls.lsreader, NULL, lsreader, NULL))
-                goto fail_pthread_create_lsreader;
-
-        if (pthread_create(&ls.listener, NULL, ls_conn_handle, NULL))
-                goto fail_pthread_create_listener;
-
         if (rib_reg(LSDB, &r_ops))
                 goto fail_rib_reg;
 
@@ -1032,15 +1072,6 @@ int link_state_init(enum pol_routing pr)
         return 0;
 
  fail_rib_reg:
-        pthread_cancel(ls.listener);
-        pthread_join(ls.listener, NULL);
- fail_pthread_create_listener:
-        pthread_cancel(ls.lsreader);
-        pthread_join(ls.lsreader, NULL);
- fail_pthread_create_lsreader:
-        pthread_cancel(ls.lsupdate);
-        pthread_join(ls.lsupdate, NULL);
- fail_pthread_create_lsupdate:
         fset_destroy(ls.mgmt_set);
  fail_fset_create:
         connmgr_comp_fini(COMPID_MGMT);
@@ -1049,8 +1080,6 @@ int link_state_init(enum pol_routing pr)
  fail_routing_i_lock_init:
         pthread_rwlock_destroy(&ls.db_lock);
  fail_db_lock_init:
-        notifier_unreg(handle_event);
- fail_notifier_reg:
         graph_destroy(ls.graph);
  fail_graph:
         return -1;
@@ -1063,16 +1092,6 @@ void link_state_fini(void)
 
         rib_unreg(LSDB);
 
-        notifier_unreg(handle_event);
-
-        pthread_cancel(ls.listener);
-        pthread_cancel(ls.lsreader);
-        pthread_cancel(ls.lsupdate);
-
-        pthread_join(ls.listener, NULL);
-        pthread_join(ls.lsreader, NULL);
-        pthread_join(ls.lsupdate, NULL);
-
         fset_destroy(ls.mgmt_set);
 
         connmgr_comp_fini(COMPID_MGMT);
diff --git a/src/ipcpd/unicast/routing/link-state.h b/src/ipcpd/unicast/routing/link-state.h
index d77d72df..7f78536b 100644
--- a/src/ipcpd/unicast/routing/link-state.h
+++ b/src/ipcpd/unicast/routing/link-state.h
@@ -32,6 +32,10 @@ int                link_state_init(enum pol_routing pr);
 
 void               link_state_fini(void);
 
+int                link_state_start(void);
+
+void               link_state_stop(void);
+
 struct routing_i * link_state_routing_i_create(struct pff * pff);
 
 void               link_state_routing_i_destroy(struct routing_i * instance);
diff --git a/src/ipcpd/unicast/routing/ops.h b/src/ipcpd/unicast/routing/ops.h
index 8a79b7ec..3bc445bd 100644
--- a/src/ipcpd/unicast/routing/ops.h
+++ b/src/ipcpd/unicast/routing/ops.h
@@ -30,6 +30,10 @@ struct routing_ops {
 
         void               (* fini)(void);
 
+        int                (* start)(void);
+
+        void               (* stop)(void);
+
         struct routing_i * (* routing_i_create)(struct pff * pff);
 
         void               (* routing_i_destroy)(struct routing_i * instance);