network/tc: Avoid concurrent set modification in tclass_drop()/qdisc_drop()

With the current algorithm, we can end up removing entries from the
qdisc/tclass sets while having multiple open iterators over the sets at
various positions which leads to assertion failures in the hashmap logic
as it's only safe to remove the "current" entry.

To avoid the problem, let's split up marking and dropping of tclasses
and qdiscs. First, we recursively iterate tclasses/qdiscs and mark all
that need to be removed. Next, we iterate once over tclasses and qdiscs
and remove all marked entries.

Fixes 632d321050f58fe1b5bed7cfe769d212377c0301

diff --git a/src/network/tc/qdisc.c b/src/network/tc/qdisc.c
index 55ce16600a73e9f261a34ac9aea8834cc7b2fe3d..38dee2c00157febd0c425399d10e6979ab75c2c7 100644 (file)
--- a/src/network/tc/qdisc.c
+++ b/src/network/tc/qdisc.c
@@ -285,37 +285,57 @@ int link_find_qdisc(Link *link, uint32_t handle, const char *kind, QDisc **ret)
         return -ENOENT;
 }
 
-QDisc* qdisc_drop(QDisc *qdisc) {
+void qdisc_mark_recursive(QDisc *qdisc) {
         TClass *tclass;
-        Link *link;
 
         assert(qdisc);
+        assert(qdisc->link);
 
-        link = ASSERT_PTR(qdisc->link);
+        if (qdisc_is_marked(qdisc))
+                return;
 
-        qdisc_mark(qdisc); /* To avoid stack overflow. */
+        qdisc_mark(qdisc);
 
-        /* also drop all child classes assigned to the qdisc. */
-        SET_FOREACH(tclass, link->tclasses) {
-                if (tclass_is_marked(tclass))
+        /* also mark all child classes assigned to the qdisc. */
+        SET_FOREACH(tclass, qdisc->link->tclasses) {
+                if (TC_H_MAJ(tclass->classid) != qdisc->handle)
                         continue;
 
-                if (TC_H_MAJ(tclass->classid) != qdisc->handle)
+                tclass_mark_recursive(tclass);
+        }
+}
+
+void link_qdisc_drop_marked(Link *link) {
+        QDisc *qdisc;
+
+        assert(link);
+
+        SET_FOREACH(qdisc, link->qdiscs) {
+                if (!qdisc_is_marked(qdisc))
                         continue;
 
-                tclass_drop(tclass);
+                qdisc_unmark(qdisc);
+                qdisc_enter_removed(qdisc);
+
+                if (qdisc->state == 0) {
+                        log_qdisc_debug(qdisc, link, "Forgetting");
+                        qdisc_free(qdisc);
+                } else
+                        log_qdisc_debug(qdisc, link, "Removed");
         }
+}
 
-        qdisc_unmark(qdisc);
-        qdisc_enter_removed(qdisc);
+QDisc* qdisc_drop(QDisc *qdisc) {
+        assert(qdisc);
+        assert(qdisc->link);
 
-        if (qdisc->state == 0) {
-                log_qdisc_debug(qdisc, link, "Forgetting");
-                qdisc = qdisc_free(qdisc);
-        } else
-                log_qdisc_debug(qdisc, link, "Removed");
+        qdisc_mark_recursive(qdisc);
+
+        /* link_qdisc_drop_marked() may invalidate qdisc, so run link_tclass_drop_marked() first. */
+        link_tclass_drop_marked(qdisc->link);
+        link_qdisc_drop_marked(qdisc->link);
 
-        return qdisc;
+        return NULL;
 }
 
 static int qdisc_handler(sd_netlink *rtnl, sd_netlink_message *m, Request *req, Link *link, QDisc *qdisc) {

diff --git a/src/network/tc/qdisc.h b/src/network/tc/qdisc.h
index a62b9413eca6e97d2f4a9b7f096f1bbd351c24e3..cbba1bef71199a9b59849bf1cf405bfb7c86d857 100644 (file)
--- a/src/network/tc/qdisc.h
+++ b/src/network/tc/qdisc.h
@@ -77,7 +77,9 @@ DEFINE_NETWORK_CONFIG_STATE_FUNCTIONS(QDisc, qdisc);
 QDisc* qdisc_free(QDisc *qdisc);
 int qdisc_new_static(QDiscKind kind, Network *network, const char *filename, unsigned section_line, QDisc **ret);
 
+void qdisc_mark_recursive(QDisc *qdisc);
 QDisc* qdisc_drop(QDisc *qdisc);
+void link_qdisc_drop_marked(Link *link);
 
 int link_find_qdisc(Link *link, uint32_t handle, const char *kind, QDisc **qdisc);
 

diff --git a/src/network/tc/tclass.c b/src/network/tc/tclass.c
index 63229ec6e8df1eec44ec5c99055c19e2df32128d..fcbe8cbcf46bda8749fe74b50bd572202d4c69bb 100644 (file)
--- a/src/network/tc/tclass.c
+++ b/src/network/tc/tclass.c
@@ -252,37 +252,56 @@ static void log_tclass_debug(TClass *tclass, Link *link, const char *str) {
                        strna(tclass_get_tca_kind(tclass)));
 }
 
-TClass* tclass_drop(TClass *tclass) {
+void tclass_mark_recursive(TClass *tclass) {
         QDisc *qdisc;
-        Link *link;
 
         assert(tclass);
+        assert(tclass->link);
 
-        link = ASSERT_PTR(tclass->link);
+        if (tclass_is_marked(tclass))
+                return;
 
-        tclass_mark(tclass); /* To avoid stack overflow. */
+        tclass_mark(tclass);
 
-        /* Also drop all child qdiscs assigned to the class. */
-        SET_FOREACH(qdisc, link->qdiscs) {
-                if (qdisc_is_marked(qdisc))
+        /* Also mark all child qdiscs assigned to the class. */
+        SET_FOREACH(qdisc, tclass->link->qdiscs) {
+                if (qdisc->parent != tclass->classid)
                         continue;
 
-                if (qdisc->parent != tclass->classid)
+                qdisc_mark_recursive(qdisc);
+        }
+}
+
+void link_tclass_drop_marked(Link *link) {
+        TClass *tclass;
+
+        assert(link);
+
+        SET_FOREACH(tclass, link->tclasses) {
+                if (!tclass_is_marked(tclass))
                         continue;
 
-                qdisc_drop(qdisc);
+                tclass_unmark(tclass);
+                tclass_enter_removed(tclass);
+
+                if (tclass->state == 0) {
+                        log_tclass_debug(tclass, link, "Forgetting");
+                        tclass_free(tclass);
+                } else
+                        log_tclass_debug(tclass, link, "Removed");
         }
+}
 
-        tclass_unmark(tclass);
-        tclass_enter_removed(tclass);
+TClass* tclass_drop(TClass *tclass) {
+        assert(tclass);
 
-        if (tclass->state == 0) {
-                log_tclass_debug(tclass, link, "Forgetting");
-                tclass = tclass_free(tclass);
-        } else
-                log_tclass_debug(tclass, link, "Removed");
+        tclass_mark_recursive(tclass);
+
+        /* link_tclass_drop_marked() may invalidate tclass, so run link_qdisc_drop_marked() first. */
+        link_qdisc_drop_marked(tclass->link);
+        link_tclass_drop_marked(tclass->link);
 
-        return tclass;
+        return NULL;
 }
 
 static int tclass_handler(sd_netlink *rtnl, sd_netlink_message *m, Request *req, Link *link, TClass *tclass) {

diff --git a/src/network/tc/tclass.h b/src/network/tc/tclass.h
index e73e23c97f6213aa7dbe4e54bfa0c46f181ede88..85df57d42c21eebd9ac8e56a2b656a8e8fbfd1e7 100644 (file)
--- a/src/network/tc/tclass.h
+++ b/src/network/tc/tclass.h
@@ -58,7 +58,9 @@ DEFINE_NETWORK_CONFIG_STATE_FUNCTIONS(TClass, tclass);
 TClass* tclass_free(TClass *tclass);
 int tclass_new_static(TClassKind kind, Network *network, const char *filename, unsigned section_line, TClass **ret);
 
+void tclass_mark_recursive(TClass *tclass);
 TClass* tclass_drop(TClass *tclass);
+void link_tclass_drop_marked(Link *link);
 
 int link_find_tclass(Link *link, uint32_t classid, TClass **ret);