Skip to content

Commit 307201a

Browse files
davem330davem330
committed
Merge branch 'cls_u32-fix-refcount-leak'
Davide Caratti says: ==================== net/sched: cls_u32: fix refcount leak a refcount leak in the error path of u32_change() has been recently introduced. It can be observed with the following commands: [root@f31 ~]# tc filter replace dev eth0 ingress protocol ip prio 97 \ > u32 match ip src 127.0.0.1/32 indev notexist20 flowid 1:1 action drop RTNETLINK answers: Invalid argument We have an error talking to the kernel [root@f31 ~]# tc filter replace dev eth0 ingress protocol ip prio 98 \ > handle 42:42 u32 divisor 256 Error: cls_u32: Divisor can only be used on a hash table. We have an error talking to the kernel [root@f31 ~]# tc filter replace dev eth0 ingress protocol ip prio 99 \ > u32 ht 47:47 Error: cls_u32: Specified hash table not found. We have an error talking to the kernel they all legitimately return -EINVAL; however, they leave semi-configured filters at eth0 tc ingress: [root@f31 ~]# tc filter show dev eth0 ingress filter protocol ip pref 97 u32 chain 0 filter protocol ip pref 97 u32 chain 0 fh 800: ht divisor 1 filter protocol ip pref 98 u32 chain 0 filter protocol ip pref 98 u32 chain 0 fh 801: ht divisor 1 filter protocol ip pref 99 u32 chain 0 filter protocol ip pref 99 u32 chain 0 fh 802: ht divisor 1 With older kernels, filters were unconditionally considered empty (and thus de-refcounted) on the error path of ->change(). After commit 8b64678 ("net: sched: refactor tp insert/delete for concurrent execution"), filters were considered empty when the walk() function didn't set 'walker.stop' to 1. Finally, with commit 6676d5e ("net: sched: set dedicated tcf_walker flag when tp is empty"), tc filters are considered empty unless the walker function is called with a non-NULL handle. This last change doesn't fit cls_u32 design, because at least the "root hnode" is (almost) always non-NULL, as it's allocated in u32_init(). - patch 1/2 is a proposal to restore the original kernel behavior, where no filter was installed in the error path of u32_change(). - patch 2/2 adds tdc selftests that can be ued to verify the correct behavior of u32 in the error path of ->change(). ==================== Signed-off-by: David S. Miller <davem@davemloft.net>
2 parents 615f22f + 6649a3f commit 307201a

File tree

3 files changed

+230
-22
lines changed

3 files changed

+230
-22
lines changed

‎net/sched/cls_u32.c‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1108,10 +1108,33 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
11081108
return err;
11091109
}
11101110

1111+
static bool u32_hnode_empty(struct tc_u_hnode *ht, bool *non_root_ht)
1112+
{
1113+
int i;
1114+
1115+
if (!ht)
1116+
return true;
1117+
if (!ht->is_root) {
1118+
*non_root_ht = true;
1119+
return false;
1120+
}
1121+
if (*non_root_ht)
1122+
return false;
1123+
if (ht->refcnt < 2)
1124+
return true;
1125+
1126+
for (i = 0; i <= ht->divisor; i++) {
1127+
if (rtnl_dereference(ht->ht[i]))
1128+
return false;
1129+
}
1130+
return true;
1131+
}
1132+
11111133
static void u32_walk(struct tcf_proto *tp, struct tcf_walker *arg,
11121134
bool rtnl_held)
11131135
{
11141136
struct tc_u_common *tp_c = tp->data;
1137+
bool non_root_ht = false;
11151138
struct tc_u_hnode *ht;
11161139
struct tc_u_knode *n;
11171140
unsigned int h;
@@ -1124,6 +1147,8 @@ static void u32_walk(struct tcf_proto *tp, struct tcf_walker *arg,
11241147
ht = rtnl_dereference(ht->next)) {
11251148
if (ht->prio != tp->prio)
11261149
continue;
1150+
if (u32_hnode_empty(ht, &non_root_ht))
1151+
return;
11271152
if (arg->count >= arg->skip) {
11281153
if (arg->fn(tp, ht, arg) < 0) {
11291154
arg->stop = 1;

‎tools/testing/selftests/tc-testing/tc-tests/filters/tests.json‎

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,4 @@
11
[
2-
{
3-
"id": "e9a3",
4-
"name": "Add u32 with source match",
5-
"category": [
6-
"filter",
7-
"u32"
8-
],
9-
"plugins": {
10-
"requires": "nsPlugin"
11-
},
12-
"setup": [
13-
"$TC qdisc add dev $DEV1 ingress"
14-
],
15-
"cmdUnderTest": "$TC filter add dev $DEV1 parent ffff: protocol ip prio 1 u32 match ip src 127.0.0.1/32 flowid 1:1 action ok",
16-
"expExitCode": "0",
17-
"verifyCmd": "$TC filter show dev $DEV1 parent ffff:",
18-
"matchPattern": "match 7f000001/ffffffff at 12",
19-
"matchCount": "1",
20-
"teardown": [
21-
"$TC qdisc del dev $DEV1 ingress"
22-
]
23-
},
242
{
253
"id": "2638",
264
"name": "Add matchall and try to get it",

‎tools/testing/selftests/tc-testing/tc-tests/filters/u32.json‎

Lines changed: 205 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,205 @@
1+
[
2+
{
3+
"id": "afa9",
4+
"name": "Add u32 with source match",
5+
"category": [
6+
"filter",
7+
"u32"
8+
],
9+
"plugins": {
10+
"requires": "nsPlugin"
11+
},
12+
"setup": [
13+
"$TC qdisc add dev $DEV1 ingress"
14+
],
15+
"cmdUnderTest": "$TC filter add dev $DEV1 ingress protocol ip prio 1 u32 match ip src 127.0.0.1/32 flowid 1:1 action ok",
16+
"expExitCode": "0",
17+
"verifyCmd": "$TC filter show dev $DEV1 ingress",
18+
"matchPattern": "filter protocol ip pref 1 u32 chain (0[ ]+$|0 fh 800: ht divisor 1|0 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:1.*match 7f000001/ffffffff at 12)",
19+
"matchCount": "3",
20+
"teardown": [
21+
"$TC qdisc del dev $DEV1 ingress"
22+
]
23+
},
24+
{
25+
"id": "6aa7",
26+
"name": "Add/Replace u32 with source match and invalid indev",
27+
"category": [
28+
"filter",
29+
"u32"
30+
],
31+
"plugins": {
32+
"requires": "nsPlugin"
33+
},
34+
"setup": [
35+
"$TC qdisc add dev $DEV1 ingress"
36+
],
37+
"cmdUnderTest": "$TC filter replace dev $DEV1 ingress protocol ip prio 1 u32 match ip src 127.0.0.1/32 indev notexist20 flowid 1:1 action ok",
38+
"expExitCode": "2",
39+
"verifyCmd": "$TC filter show dev $DEV1 ingress",
40+
"matchPattern": "filter protocol ip pref 1 u32 chain 0",
41+
"matchCount": "0",
42+
"teardown": [
43+
"$TC qdisc del dev $DEV1 ingress"
44+
]
45+
},
46+
{
47+
"id": "bc4d",
48+
"name": "Replace valid u32 with source match and invalid indev",
49+
"category": [
50+
"filter",
51+
"u32"
52+
],
53+
"plugins": {
54+
"requires": "nsPlugin"
55+
},
56+
"setup": [
57+
"$TC qdisc add dev $DEV1 ingress",
58+
"$TC filter add dev $DEV1 ingress protocol ip prio 1 u32 match ip src 127.0.0.3/32 flowid 1:3 action ok"
59+
],
60+
"cmdUnderTest": "$TC filter replace dev $DEV1 ingress protocol ip prio 1 u32 match ip src 127.0.0.2/32 indev notexist20 flowid 1:2 action ok",
61+
"expExitCode": "2",
62+
"verifyCmd": "$TC filter show dev $DEV1 ingress",
63+
"matchPattern": "filter protocol ip pref 1 u32 chain (0[ ]+$|0 fh 800: ht divisor 1|0 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:3.*match 7f000003/ffffffff at 12)",
64+
"matchCount": "3",
65+
"teardown": [
66+
"$TC qdisc del dev $DEV1 ingress"
67+
]
68+
},
69+
{
70+
"id": "648b",
71+
"name": "Add u32 with custom hash table",
72+
"category": [
73+
"filter",
74+
"u32"
75+
],
76+
"plugins": {
77+
"requires": "nsPlugin"
78+
},
79+
"setup": [
80+
"$TC qdisc add dev $DEV1 ingress"
81+
],
82+
"cmdUnderTest": "$TC filter add dev $DEV1 ingress prio 99 handle 42: u32 divisor 256",
83+
"expExitCode": "0",
84+
"verifyCmd": "$TC filter show dev $DEV1 ingress",
85+
"matchPattern": "pref 99 u32 chain (0[ ]+$|0 fh 42: ht divisor 256|0 fh 800: ht divisor 1)",
86+
"matchCount": "3",
87+
"teardown": [
88+
"$TC qdisc del dev $DEV1 ingress"
89+
]
90+
},
91+
{
92+
"id": "6658",
93+
"name": "Add/Replace u32 with custom hash table and invalid handle",
94+
"category": [
95+
"filter",
96+
"u32"
97+
],
98+
"plugins": {
99+
"requires": "nsPlugin"
100+
},
101+
"setup": [
102+
"$TC qdisc add dev $DEV1 ingress"
103+
],
104+
"cmdUnderTest": "$TC filter replace dev $DEV1 ingress prio 99 handle 42:42 u32 divisor 256",
105+
"expExitCode": "2",
106+
"verifyCmd": "$TC filter show dev $DEV1 ingress",
107+
"matchPattern": "pref 99 u32 chain 0",
108+
"matchCount": "0",
109+
"teardown": [
110+
"$TC qdisc del dev $DEV1 ingress"
111+
]
112+
},
113+
{
114+
"id": "9d0a",
115+
"name": "Replace valid u32 with custom hash table and invalid handle",
116+
"category": [
117+
"filter",
118+
"u32"
119+
],
120+
"plugins": {
121+
"requires": "nsPlugin"
122+
},
123+
"setup": [
124+
"$TC qdisc add dev $DEV1 ingress",
125+
"$TC filter add dev $DEV1 ingress prio 99 handle 42: u32 divisor 256"
126+
],
127+
"cmdUnderTest": "$TC filter replace dev $DEV1 ingress prio 99 handle 42:42 u32 divisor 128",
128+
"expExitCode": "2",
129+
"verifyCmd": "$TC filter show dev $DEV1 ingress",
130+
"matchPattern": "pref 99 u32 chain (0[ ]+$|0 fh 42: ht divisor 256|0 fh 800: ht divisor 1)",
131+
"matchCount": "3",
132+
"teardown": [
133+
"$TC qdisc del dev $DEV1 ingress"
134+
]
135+
},
136+
{
137+
"id": "1644",
138+
"name": "Add u32 filter that links to a custom hash table",
139+
"category": [
140+
"filter",
141+
"u32"
142+
],
143+
"plugins": {
144+
"requires": "nsPlugin"
145+
},
146+
"setup": [
147+
"$TC qdisc add dev $DEV1 ingress",
148+
"$TC filter add dev $DEV1 ingress prio 99 handle 43: u32 divisor 256"
149+
],
150+
"cmdUnderTest": "$TC filter add dev $DEV1 ingress protocol ip prio 98 u32 link 43: hashkey mask 0x0000ff00 at 12 match ip src 192.168.0.0/16",
151+
"expExitCode": "0",
152+
"verifyCmd": "$TC filter show dev $DEV1 ingress",
153+
"matchPattern": "filter protocol ip pref 98 u32 chain (0[ ]+$|0 fh 801: ht divisor 1|0 fh 801::800 order 2048 key ht 801 bkt 0 link 43:.*match c0a80000/ffff0000 at 12.*hash mask 0000ff00 at 12)",
154+
"matchCount": "3",
155+
"teardown": [
156+
"$TC qdisc del dev $DEV1 ingress"
157+
]
158+
},
159+
{
160+
"id": "74c2",
161+
"name": "Add/Replace u32 filter with invalid hash table id",
162+
"category": [
163+
"filter",
164+
"u32"
165+
],
166+
"plugins": {
167+
"requires": "nsPlugin"
168+
},
169+
"setup": [
170+
"$TC qdisc add dev $DEV1 ingress"
171+
],
172+
"cmdUnderTest": "$TC filter replace dev $DEV1 ingress protocol ip prio 20 u32 ht 47:47 action drop",
173+
"expExitCode": "2",
174+
"verifyCmd": "$TC filter show dev $DEV1 ingress",
175+
"matchPattern": "filter protocol ip pref 20 u32 chain 0",
176+
"matchCount": "0",
177+
"teardown": [
178+
"$TC qdisc del dev $DEV1 ingress"
179+
]
180+
},
181+
{
182+
"id": "1fe6",
183+
"name": "Replace valid u32 filter with invalid hash table id",
184+
"category": [
185+
"filter",
186+
"u32"
187+
],
188+
"plugins": {
189+
"requires": "nsPlugin"
190+
},
191+
"setup": [
192+
"$TC qdisc add dev $DEV1 ingress",
193+
"$TC filter add dev $DEV1 ingress protocol ip prio 99 handle 43: u32 divisor 1",
194+
"$TC filter add dev $DEV1 ingress protocol ip prio 98 u32 ht 43: match tcp src 22 FFFF classid 1:3"
195+
],
196+
"cmdUnderTest": "$TC filter replace dev $DEV1 ingress protocol ip prio 98 u32 ht 43:1 match tcp src 23 FFFF classid 1:4",
197+
"expExitCode": "2",
198+
"verifyCmd": "$TC filter show dev $DEV1 ingress",
199+
"matchPattern": "filter protocol ip pref 99 u32 chain (0[ ]+$|0 fh (43|800): ht divisor 1|0 fh 43::800 order 2048 key ht 43 bkt 0 flowid 1:3.*match 00160000/ffff0000 at nexthdr\\+0)",
200+
"matchCount": "4",
201+
"teardown": [
202+
"$TC qdisc del dev $DEV1 ingress"
203+
]
204+
}
205+
]

0 commit comments

Comments
 (0)