From ee8f605ded4fea6b93aae018415efae877c26ed2 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 1 May 2024 14:41:41 +0200 Subject: 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 --- src/network/tc/qdisc.c | 54 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 17 deletions(-) (limited to 'src/network/tc/qdisc.c') diff --git a/src/network/tc/qdisc.c b/src/network/tc/qdisc.c index 55ce16600a..38dee2c001 100644 --- 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) { -- cgit v1.2.3