Merge pull request #32611 from DaanDeMeyer/qdisc

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

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. */
-
-        /* also drop all child classes assigned to the qdisc. */
-        SET_FOREACH(tclass, link->tclasses) {
-                if (tclass_is_marked(tclass))
-                        continue;
+        qdisc_mark(qdisc);
 
+        /* also mark all child classes assigned to the qdisc. */
+        SET_FOREACH(tclass, qdisc->link->tclasses) {
                 if (TC_H_MAJ(tclass->classid) != qdisc->handle)
                         continue;
 
-                tclass_drop(tclass);
+                tclass_mark_recursive(tclass);
         }
+}
 
-        qdisc_unmark(qdisc);
-        qdisc_enter_removed(qdisc);
+void link_qdisc_drop_marked(Link *link) {
+        QDisc *qdisc;
 
-        if (qdisc->state == 0) {
-                log_qdisc_debug(qdisc, link, "Forgetting");
-                qdisc = qdisc_free(qdisc);
-        } else
-                log_qdisc_debug(qdisc, link, "Removed");
+        assert(link);
 
-        return qdisc;
+        SET_FOREACH(qdisc, link->qdiscs) {
+                if (!qdisc_is_marked(qdisc))
+                        continue;
+
+                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* qdisc_drop(QDisc *qdisc) {
+        assert(qdisc);
+        assert(qdisc->link);
+
+        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 NULL;
 }
 
 static int qdisc_handler(sd_netlink *rtnl, sd_netlink_message *m, Request *req, Link *link, QDisc *qdisc) {

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);
 

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. */
-
-        /* Also drop all child qdiscs assigned to the class. */
-        SET_FOREACH(qdisc, link->qdiscs) {
-                if (qdisc_is_marked(qdisc))
-                        continue;
+        tclass_mark(tclass);
 
+        /* Also mark all child qdiscs assigned to the class. */
+        SET_FOREACH(qdisc, tclass->link->qdiscs) {
                 if (qdisc->parent != tclass->classid)
                         continue;
 
-                qdisc_drop(qdisc);
+                qdisc_mark_recursive(qdisc);
         }
+}
 
-        tclass_unmark(tclass);
-        tclass_enter_removed(tclass);
+void link_tclass_drop_marked(Link *link) {
+        TClass *tclass;
 
-        if (tclass->state == 0) {
-                log_tclass_debug(tclass, link, "Forgetting");
-                tclass = tclass_free(tclass);
-        } else
-                log_tclass_debug(tclass, link, "Removed");
+        assert(link);
 
-        return tclass;
+        SET_FOREACH(tclass, link->tclasses) {
+                if (!tclass_is_marked(tclass))
+                        continue;
+
+                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* tclass_drop(TClass *tclass) {
+        assert(tclass);
+
+        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 NULL;
 }
 
 static int tclass_handler(sd_netlink *rtnl, sd_netlink_message *m, Request *req, Link *link, TClass *tclass) {

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);
 

test/meson.build

@@ -29,7 +29,7 @@ if install_tests
                 # 'follow_symlinks' once we require Meson 1.3.0 or greater, see:
                 # https://github.com/mesonbuild/meson/commit/0af126fec798d6dbb0d1ad52168cc1f3f1758acd
                 if rsync.found()
-                        rsync_r = rsync.full_path() + ' -rlpt --exclude .gitattributes -- "@0@" "${DESTDIR:-}@1@"'
+                        rsync_r = rsync.full_path() + ' -rLpt --exclude .gitattributes -- "@0@" "${DESTDIR:-}@1@"'
                         meson.add_install_script(sh, '-c',
                                                  rsync_r.format(meson.current_source_dir() / subdir, testdata_dir))
                 else

test/test-network/systemd-networkd-tests.py

@@ -365,12 +365,15 @@ def clear_networkd_conf_dropins():
     rm_rf(networkd_conf_dropin_dir)
 
 def setup_systemd_udev_rules():
-    if not build_dir:
+    if not build_dir and not source_dir:
         return
 
     mkdir_p(udev_rules_dir)
 
     for path in [build_dir, source_dir]:
+        if not path:
+            continue
+
         path = os.path.join(path, "rules.d")
         print(f"Copying udev rules from {path} to {udev_rules_dir}")
 
@@ -450,7 +453,7 @@ def create_service_dropin(service, command, additional_settings=None):
     create_unit_dropin(f'{service}.service', drop_in)
 
 def setup_system_units():
-    if build_dir:
+    if build_dir or source_dir:
         mkdir_p('/run/systemd/system/')
 
         for unit in [
@@ -462,6 +465,9 @@ def setup_system_units():
                 'systemd-udevd.service',
         ]:
             for path in [build_dir, source_dir]:
+                if not path:
+                    continue
+
                 fullpath = os.path.join(os.path.join(path, "units"), unit)
                 if os.path.exists(fullpath):
                     print(f"Copying unit file from {fullpath} to /run/systemd/system/")
@@ -7753,9 +7759,11 @@ if __name__ == '__main__':
 
     if ns.source_dir:
         source_dir = ns.source_dir
-    else:
+        assert os.path.exists(os.path.join(source_dir, "meson_options.txt")), f"{source_dir} doesn't appear to be a systemd source tree."
+    elif os.path.exists(os.path.normpath(os.path.join(os.path.dirname(os.path.abspath(__file__)), "../../meson_options.txt"))):
         source_dir = os.path.normpath(os.path.join(os.path.dirname(os.path.abspath(__file__)), "../../"))
-    assert os.path.exists(os.path.join(source_dir, "meson_options.txt")), f"{source_dir} doesn't appear to be a systemd source tree."
+    else:
+        source_dir = None
 
     use_valgrind = ns.use_valgrind
     enable_debug = ns.enable_debug