github-mirror/linux
1
0
Fork 0
mirror of https://github.com/torvalds/linux.git synced 2026-09-12 20:53:03 +02:00
Code Issues Actions Packages Projects Releases Wiki Activity Graph

Merge branch 'net-sched-cls_route-fix-bucket-retention-and-handle-recomputation'

Browse Source 
Victor Nogueira says:

====================
net/sched: cls_route: fix bucket retention and handle recomputation

Patch 1 is the v1 patch, unchanged. route4_change() can move an existing
filter to a different top-level bucket, since route4_set_parms()
recomputes the handle from TCA_ROUTE4_TO/FROM/IIF. The filter is
unlinked from the old bucket, but the bucket itself is never freed once
it goes empty, so route4_delete() keeps reporting *last=false after the
last live filter is gone. That pins the empty tcf_proto and leaks it.
The filters linked to a bucket are refcounted now, and the bucket is
dropped from head->table[] as soon as the count reaches zero.

Reviewing v1, Sashiko pointed out that the duplicate scan in
route4_set_parms() compares against the wrong handle [1]. Patches 2 and
3 fix the two symptoms of that.

Patch 2 makes the scan compare against nhandle. f->handle is the handle
the filter has before the update, not the one it is about to be linked
under, so a change that moves a filter into a chain already holding
nhandle misses the collision and links a second filter under the same
handle. The newcomer is then unreachable: route4_get() returns the
incumbent, and route4_classify() stops at the first filter whose f->id
matches.

Patch 3 handles the mirror case. An in-place replace computes an nhandle
that the filter being replaced already carries, so the scan finds that
filter and rejects the request with -EEXIST. The older filter is passed
to route4_set_parms() and skipped in the scan. Skipping it alone would
rename the filter it replaces: the 0x7F00 order bits are carried in no
attribute and were folded into nhandle on the create path alone, so an
order 1 filter came back as order 0, and a sibling sharing its key could
then no longer be replaced at all. They are carried over now whenever
the request builds the key the filter already has, which leaves a
request that does change the key renaming the filter as before.

Patch 4 adds tdc coverage for all three, including the cross-bucket move
case Sashiko noted route.json had no test for.

[1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260829205422.854785-1-victor%40mojatatu.com
====================

Link: https://patch.msgid.link/20260907192133.2639067-1-victor@mojatatu.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
This commit is contained in:
Paolo Abeni 2026-09-10 11:27:53 +02:00
commit 4770e9f851
2 changed files with 249 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

76
net/sched/cls_route.c
View File

@ -11,6 +11,7 @@
#include <linux/kernel.h>
#include <linux/string.h>
#include <linux/errno.h>
#include <linux/refcount.h>
#include <linux/skbuff.h>
#include <net/dst.h>
#include <net/route.h>
@ -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])) {
@ -400,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;
@ -414,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;
}
@ -437,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) {
@ -459,6 +453,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);
@ -466,8 +461,14 @@ 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 != fold && fp->handle == nhandle) {
NL_SET_ERR_MSG_FMT(extack,
"Handle %x is already in use",
nhandle);
return -EEXIST;
}
refcount_inc(&b->filters_ref);
}
if (tb[TCA_ROUTE4_TO])
@ -500,9 +501,8 @@ 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;
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;
@ -560,17 +559,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;
}
}
}

210
tools/testing/selftests/tc-testing/tc-tests/filters/route.json
View File

@ -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"
]
}
]