From 1853f30cf5c84971f99788a76207c6f745380896 Mon Sep 17 00:00:00 2001 From: Victor Nogueira Date: Mon, 7 Sep 2026 16:21:30 -0300 Subject: [PATCH 1/4] net/sched: cls_route: free emptied bucket on filter move route4_change can move an existing filter to a different top-level bucket: route4_set_parms recomputes the handle from TCA_ROUTE4_TO/ FROM/IIF, and the handle-mismatch check is gated on the 'new' flag, so for an existing filter the new handle may differ from the old one and land in a different bucket. When this happens, the filter is unlinked from the old bucket, but the bucket itself is never freed once it goes empty. The stale empty bucket remains in head->table[], causing route4_delete to report *last=false even after the last live filter is gone. That pins the empty tcf_proto and causes a leak. Fix this by refcounting the filters linked to a bucket and freeing the bucket when the count drops to zero. The existing scan in route4_delete goes away with it. The count is updated at all sites that link or unlink a filter during add, change and delete, and the bucket is dropped from head->table[] as soon as it reaches zero. Conditions to recreate the bug: CONFIG_NET_CLS_ROUTE4=y, CONFIG_NET_SCH_INGRESS=y, CONFIG_NET_CLS_ACT=y. tc qdisc replace dev lo clsact tc filter add dev lo ingress protocol ip pref 100 route from 1 to 1 tc filter change dev lo ingress protocol ip pref 100 handle 0x10001 \ route from 1 to 2 tc filter del dev lo ingress protocol ip pref 100 handle 0x10002 \ route from 1 to 2 tc filter show dev lo ingress | grep -c 'pref 100 route chain 0 ' Fixes: 1e052be69d04 ("net_sched: destroy proto tp when all filters are gone") Reported-by: Vega Acked-by: Jamal Hadi Salim Signed-off-by: Victor Nogueira Link: https://patch.msgid.link/20260907192133.2639067-2-victor@mojatatu.com Signed-off-by: Paolo Abeni --- net/sched/cls_route.c | 45 +++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c index 0d1324c90583..17b0ebb76662 100644 --- a/net/sched/cls_route.c +++ b/net/sched/cls_route.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -41,6 +42,7 @@ struct route4_head { struct route4_bucket { /* 16 FROM buckets + 16 IIF buckets + 1 wildcard bucket */ struct route4_filter __rcu *ht[16 + 16 + 1]; + refcount_t filters_ref; struct rcu_head rcu; }; @@ -336,7 +338,7 @@ static int route4_delete(struct tcf_proto *tp, void *arg, bool *last, struct route4_filter *nf; struct route4_bucket *b; unsigned int h = 0; - int i, h1; + int h1; if (!head || !f) return -EINVAL; @@ -362,23 +364,14 @@ static int route4_delete(struct tcf_proto *tp, void *arg, bool *last, tcf_exts_get_net(&f->exts); tcf_queue_work(&f->rwork, route4_delete_filter_work); - /* Strip RTNL protected tree */ - for (i = 0; i <= 32; i++) { - struct route4_filter *rt; - - rt = rtnl_dereference(b->ht[i]); - if (rt) - goto out; + if (refcount_dec_and_test(&b->filters_ref)) { + RCU_INIT_POINTER(head->table[to_hash(h)], NULL); + kfree_rcu(b, rcu); } - - /* OK, session has no flows */ - RCU_INIT_POINTER(head->table[to_hash(h)], NULL); - kfree_rcu(b, rcu); break; } } -out: *last = true; for (h1 = 0; h1 <= 256; h1++) { if (rcu_access_pointer(head->table[h1])) { @@ -459,6 +452,7 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp, if (b == NULL) return -ENOBUFS; + refcount_set(&b->filters_ref, 1); rcu_assign_pointer(head->table[h1], b); } else { unsigned int h2 = from_hash(nhandle >> 16); @@ -468,6 +462,8 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp, fp = rtnl_dereference(fp->next)) if (fp->handle == f->handle) return -EEXIST; + + refcount_inc(&b->filters_ref); } if (tb[TCA_ROUTE4_TO]) @@ -500,7 +496,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb, struct route4_filter *fold, *f1, *pfp, *f = NULL; struct route4_bucket *b; struct nlattr *tb[TCA_ROUTE4_MAX + 1]; - unsigned int h, th; + unsigned int h; int err; bool new = true; @@ -560,17 +556,20 @@ static int route4_change(struct net *net, struct sk_buff *in_skb, rcu_assign_pointer(*fp, f); if (fold) { - th = to_hash(fold->handle); + b = fold->bkt; h = from_hash(fold->handle >> 16); - b = rtnl_dereference(head->table[th]); - if (b) { - fp = &b->ht[h]; - for (pfp = rtnl_dereference(*fp); pfp; - fp = &pfp->next, pfp = rtnl_dereference(*fp)) { - if (pfp == fold) { - rcu_assign_pointer(*fp, fold->next); - break; + fp = &b->ht[h]; + for (pfp = rtnl_dereference(*fp); pfp; + fp = &pfp->next, pfp = rtnl_dereference(*fp)) { + if (pfp == fold) { + rcu_assign_pointer(*fp, fold->next); + if (refcount_dec_and_test(&b->filters_ref)) { + unsigned int th = to_hash(fold->handle); + + RCU_INIT_POINTER(head->table[th], NULL); + kfree_rcu(b, rcu); } + break; } } } From b74a8455a2f271f54695b6a8ec1f113824a46c0e Mon Sep 17 00:00:00 2001 From: Victor Nogueira Date: Mon, 7 Sep 2026 16:21:31 -0300 Subject: [PATCH 2/4] net/sched: cls_route: Reject handle aliasing route4_set_parms() rejects a duplicate by scanning the destination chain for f->handle, but f->handle is the handle the filter has before the update, not the one it is about to be linked under. The comparison and the insertion therefore use different handles, which causes breakage. When a change moves the filter to a chain that already holds nhandle, the scan looks for the old handle instead, misses the collision and links a second filter with the same handle: tc filter add dev lo ingress protocol ip pref 100 \ route from 1 to 1 classid 1:1 action ok tc filter add dev lo ingress protocol ip pref 100 \ route from 2 to 2 classid 1:2 action drop tc filter change dev lo ingress protocol ip pref 100 handle 0x10001 \ route from 2 to 2 classid 1:1 action ok tc filter show dev lo ingress ... fh 0x00020002 flowid 1:2 to 2 from 2 ... fh 0x00020002 flowid 1:1 to 2 from 2 The newcomer is appended after the incumbent, and both end up with the same f->id. route4_get() returns the first match, so the second filter can no longer be addressed by handle, and route4_classify() stops at the first filter whose f->id matches. The second filter is dumped but is effectively dead. Fix this by comparing against nhandle. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: Sashiko Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260829205422.854785-1-victor%40mojatatu.com Acked-by: Jamal Hadi Salim Signed-off-by: Victor Nogueira Link: https://patch.msgid.link/20260907192133.2639067-3-victor@mojatatu.com Signed-off-by: Paolo Abeni --- net/sched/cls_route.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c index 17b0ebb76662..9710b77d379c 100644 --- a/net/sched/cls_route.c +++ b/net/sched/cls_route.c @@ -460,8 +460,12 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp, for (fp = rtnl_dereference(b->ht[h2]); fp; fp = rtnl_dereference(fp->next)) - if (fp->handle == f->handle) + if (fp->handle == nhandle) { + NL_SET_ERR_MSG_FMT(extack, + "Handle %x is already in use", + nhandle); return -EEXIST; + } refcount_inc(&b->filters_ref); } From 41e85e54e5649a1617698438b0ce64c6f9d83d69 Mon Sep 17 00:00:00 2001 From: Victor Nogueira Date: Mon, 7 Sep 2026 16:21:32 -0300 Subject: [PATCH 3/4] net/sched: cls_route: Fix in-place replace Building on the previous patch, route4_set_parms rejects a duplicate by scanning the destination chain for nhandle, but the scan doesn't exclude the older version it is replacing, so an in-place replace will match the older version's handle and fail. Fix this by passing the older filter as a parameter to route4_set_parms (replacing "new") and skipping it in the scan. Excluding the older version is not enough on its own. nhandle is built out of TCA_ROUTE4_TO, TCA_ROUTE4_FROM and TCA_ROUTE4_IIF alone, while the 0x7F00 bits, which only tell apart filters sharing one key, are folded in on the create path. Letting the replace through would therefore rename the filter it replaces: replacing handle 0x10101 stored it back as 0x10001, and a sibling at 0x10201 could then no longer be replaced at all, since its own nhandle collided with the renamed filter. tc filter add ... handle 0x10101 route from 1 to 1 classid 1:1 tc filter add ... handle 0x10201 route from 1 to 1 classid 1:2 tc filter replace ... handle 0x10101 route from 1 to 1 classid 1:9 ... fh 0x00010001 flowid 1:9 to 1 from 1 ... fh 0x00010201 flowid 1:2 to 1 from 1 tc filter replace ... handle 0x10201 route from 1 to 1 classid 1:8 Error: Handle 10001 is already in use. So carry those bits over when the key the request builds is the key the older filter already has. An in-place replace then keeps the handle userspace named the filter by, while a request that does change the key still renames it, as it did before. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: Sashiko Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260829205422.854785-1-victor%40mojatatu.com Acked-by: Jamal Hadi Salim Signed-off-by: Victor Nogueira Link: https://patch.msgid.link/20260907192133.2639067-4-victor@mojatatu.com Signed-off-by: Paolo Abeni --- net/sched/cls_route.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c index 9710b77d379c..0f211f030fd9 100644 --- a/net/sched/cls_route.c +++ b/net/sched/cls_route.c @@ -393,8 +393,9 @@ static const struct nla_policy route4_policy[TCA_ROUTE4_MAX + 1] = { static int route4_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base, struct route4_filter *f, u32 handle, struct route4_head *head, - struct nlattr **tb, struct nlattr *est, int new, - u32 flags, struct netlink_ext_ack *extack) + struct nlattr **tb, struct nlattr *est, + struct route4_filter *fold, u32 flags, + struct netlink_ext_ack *extack) { u32 id = 0, to = 0, nhandle = 0x8000; struct route4_filter *fp; @@ -407,7 +408,7 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp, return err; if (tb[TCA_ROUTE4_TO]) { - if (new && handle & 0x8000) { + if (!fold && handle & 0x8000) { NL_SET_ERR_MSG(extack, "Invalid handle"); return -EINVAL; } @@ -430,14 +431,14 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp, } else nhandle |= 0xFFFF << 16; - if (handle && new) { + if (handle && (!fold || nhandle == (handle & ~0x7F00))) nhandle |= handle & 0x7F00; - if (nhandle != handle) { - NL_SET_ERR_MSG_FMT(extack, - "Handle mismatch constructed: %x (expected: %x)", - handle, nhandle); - return -EINVAL; - } + + if (handle && !fold && nhandle != handle) { + NL_SET_ERR_MSG_FMT(extack, + "Handle mismatch constructed: %x (expected: %x)", + handle, nhandle); + return -EINVAL; } if (!nhandle) { @@ -460,7 +461,7 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp, for (fp = rtnl_dereference(b->ht[h2]); fp; fp = rtnl_dereference(fp->next)) - if (fp->handle == nhandle) { + if (fp != fold && fp->handle == nhandle) { NL_SET_ERR_MSG_FMT(extack, "Handle %x is already in use", nhandle); @@ -502,7 +503,6 @@ static int route4_change(struct net *net, struct sk_buff *in_skb, struct nlattr *tb[TCA_ROUTE4_MAX + 1]; unsigned int h; int err; - bool new = true; if (!handle) { NL_SET_ERR_MSG(extack, "Creating with handle of 0 is invalid"); @@ -539,11 +539,10 @@ static int route4_change(struct net *net, struct sk_buff *in_skb, f->tp = fold->tp; f->bkt = fold->bkt; - new = false; } err = route4_set_parms(net, tp, base, f, handle, head, tb, - tca[TCA_RATE], new, flags, extack); + tca[TCA_RATE], fold, flags, extack); if (err < 0) goto errout; From e190a7aabbea4fbfec0e74de134144cb4d040738 Mon Sep 17 00:00:00 2001 From: Victor Nogueira Date: Mon, 7 Sep 2026 16:21:33 -0300 Subject: [PATCH 4/4] selftests/tc-testing: Add cls_route bucket move and change tests Add 4 tdc tests for the cls_route bugs fixed earlier in this series: - Delete a route filter that was moved to another bucket (a7d2): Validates that deleting a filter, and making a bucket empty, does not leave a dangling empty bucket - Try to change a route filter onto an already used handle (c05a): Validates that attempting to change an existing filter's handle to an already taken one fails - Replace a route filter that shares its key with another filter (3f21): Validates that an in-place replace keeps the handle userspace named the filter by, rather than dropping the 0x7F00 bits from it - Replace both route filters sharing a key (9d0e): Validates that replacing one of the two does not make the other one unreplaceable Acked-by: Jamal Hadi Salim Signed-off-by: Victor Nogueira Link: https://patch.msgid.link/20260907192133.2639067-5-victor@mojatatu.com Signed-off-by: Paolo Abeni --- .../tc-testing/tc-tests/filters/route.json | 210 ++++++++++++++++++ 1 file changed, 210 insertions(+) diff --git a/tools/testing/selftests/tc-testing/tc-tests/filters/route.json b/tools/testing/selftests/tc-testing/tc-tests/filters/route.json index 05cedca67cca..2d5843aebd72 100644 --- a/tools/testing/selftests/tc-testing/tc-tests/filters/route.json +++ b/tools/testing/selftests/tc-testing/tc-tests/filters/route.json @@ -202,5 +202,215 @@ "teardown": [ "$TC qdisc del dev $DEV1 parent root drr" ] + }, + { + "id": "a7d2", + "name": "Delete a route filter that was moved to another bucket", + "category": [ + "filter", + "route" + ], + "plugins": { + "requires": "nsPlugin" + }, + "setup": [ + "$TC qdisc add dev $DEV1 ingress", + "$TC filter add dev $DEV1 parent ffff: protocol ip prio 100 route from 1 to 1 classid 1:1", + "$TC filter change dev $DEV1 parent ffff: protocol ip prio 100 handle 0x10001 route from 1 to 2 classid 1:1", + "$TC filter add dev $DEV1 parent ffff: protocol ip prio 200 route from 5 to 5 classid 1:5" + ], + "cmdUnderTest": "$TC filter del dev $DEV1 parent ffff: protocol ip prio 100 handle 0x10002 route from 1 to 2", + "expExitCode": "0", + "verifyCmd": "$TC -j filter show dev $DEV1 parent ffff:", + "matchJSON": [ + { + "protocol": "ip", + "pref": 200, + "kind": "route", + "chain": 0 + }, + { + "protocol": "ip", + "pref": 200, + "kind": "route", + "chain": 0, + "options": { + "fh": "0x50005", + "flowid": "1:5" + } + } + ], + "teardown": [ + "$TC qdisc del dev $DEV1 ingress" + ] + }, + { + "id": "c05a", + "name": "Try to change a route filter onto an already used handle", + "category": [ + "filter", + "route" + ], + "plugins": { + "requires": "nsPlugin" + }, + "setup": [ + "$TC qdisc add dev $DEV1 ingress", + "$TC filter add dev $DEV1 parent ffff: protocol ip prio 100 route from 1 to 1 classid 1:1 action ok", + "$TC filter add dev $DEV1 parent ffff: protocol ip prio 100 route from 2 to 2 classid 1:2 action drop" + ], + "cmdUnderTest": "$TC filter change dev $DEV1 parent ffff: protocol ip prio 100 handle 0x10001 route from 2 to 2 classid 1:1 action ok", + "expExitCode": "2", + "verifyCmd": "$TC -j filter show dev $DEV1 parent ffff:", + "matchJSON": [ + { + "protocol": "ip", + "pref": 100, + "kind": "route", + "chain": 0 + }, + { + "protocol": "ip", + "pref": 100, + "kind": "route", + "chain": 0, + "options": { + "fh": "0x10001", + "flowid": "1:1", + "actions": [ + { + "order": 1, + "kind": "gact", + "control_action": { + "type": "pass" + } + } + ] + } + }, + { + "protocol": "ip", + "pref": 100, + "kind": "route", + "chain": 0, + "options": { + "fh": "0x20002", + "flowid": "1:2", + "actions": [ + { + "order": 1, + "kind": "gact", + "control_action": { + "type": "drop" + } + } + ] + } + } + ], + "teardown": [ + "$TC qdisc del dev $DEV1 ingress" + ] + }, + { + "id": "3f21", + "name": "Replace a route filter that shares its key with another filter", + "category": [ + "filter", + "route" + ], + "plugins": { + "requires": "nsPlugin" + }, + "setup": [ + "$TC qdisc add dev $DEV1 ingress", + "$TC filter add dev $DEV1 parent ffff: protocol ip prio 100 handle 0x10101 route from 1 to 1 classid 1:1", + "$TC filter add dev $DEV1 parent ffff: protocol ip prio 100 handle 0x10201 route from 1 to 1 classid 1:2" + ], + "cmdUnderTest": "$TC filter replace dev $DEV1 parent ffff: protocol ip prio 100 handle 0x10101 route from 1 to 1 classid 1:9", + "expExitCode": "0", + "verifyCmd": "$TC -j filter show dev $DEV1 parent ffff:", + "matchJSON": [ + { + "protocol": "ip", + "pref": 100, + "kind": "route", + "chain": 0 + }, + { + "protocol": "ip", + "pref": 100, + "kind": "route", + "chain": 0, + "options": { + "fh": "0x10101", + "flowid": "1:9" + } + }, + { + "protocol": "ip", + "pref": 100, + "kind": "route", + "chain": 0, + "options": { + "fh": "0x10201", + "flowid": "1:2" + } + } + ], + "teardown": [ + "$TC qdisc del dev $DEV1 ingress" + ] + }, + { + "id": "9d0e", + "name": "Replace both route filters sharing a key", + "category": [ + "filter", + "route" + ], + "plugins": { + "requires": "nsPlugin" + }, + "setup": [ + "$TC qdisc add dev $DEV1 ingress", + "$TC filter add dev $DEV1 parent ffff: protocol ip prio 100 handle 0x10101 route from 1 to 1 classid 1:1", + "$TC filter add dev $DEV1 parent ffff: protocol ip prio 100 handle 0x10201 route from 1 to 1 classid 1:2", + "$TC filter replace dev $DEV1 parent ffff: protocol ip prio 100 handle 0x10101 route from 1 to 1 classid 1:9" + ], + "cmdUnderTest": "$TC filter replace dev $DEV1 parent ffff: protocol ip prio 100 handle 0x10201 route from 1 to 1 classid 1:8", + "expExitCode": "0", + "verifyCmd": "$TC -j filter show dev $DEV1 parent ffff:", + "matchJSON": [ + { + "protocol": "ip", + "pref": 100, + "kind": "route", + "chain": 0 + }, + { + "protocol": "ip", + "pref": 100, + "kind": "route", + "chain": 0, + "options": { + "fh": "0x10101", + "flowid": "1:9" + } + }, + { + "protocol": "ip", + "pref": 100, + "kind": "route", + "chain": 0, + "options": { + "fh": "0x10201", + "flowid": "1:8" + } + } + ], + "teardown": [ + "$TC qdisc del dev $DEV1 ingress" + ] } ]