1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
|
From: Alexander Duyck <alexander.h.duyck@redhat.com>
Date: Thu, 22 Jan 2015 15:51:14 -0800
Subject: [PATCH] fib_trie: Fix RCU bug and merge similar bits of inflate/halve
This patch addresses two issues.
The first issue is the fact that I believe I had the RCU freeing sequence
slightly out of order. As a result we could get into an issue if a caller
went into a child of a child of the new node, then backtraced into the to be
freed parent, and then attempted to access a child of a child that may have
been consumed in a resize of one of the new nodes children. To resolve this I
have moved the resize after we have freed the oldtnode. The only side effect
of this is that we will now be calling resize on more nodes in the case of
inflate due to the fact that we don't have a good way to test to see if a
full_tnode on the new node was there before or after the allocation. This
should have minimal impact however since the node should already be
correctly size so it is just the cost of calling should_inflate that we
will be taking on the node which is only a couple of cycles.
The second issue is the fact that inflate and halve were essentially doing
the same thing after the new node was added to the trie replacing the old
one. As such it wasn't really necessary to keep the code in both functions
so I have split it out into two other functions, called replace and
update_children.
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -396,8 +396,30 @@ static void put_child(struct tnode *tn,
rcu_assign_pointer(tn->child[i], n);
}
-static void put_child_root(struct tnode *tp, struct trie *t,
- t_key key, struct tnode *n)
+static void update_children(struct tnode *tn)
+{
+ unsigned long i;
+
+ /* update all of the child parent pointers */
+ for (i = tnode_child_length(tn); i;) {
+ struct tnode *inode = tnode_get_child(tn, --i);
+
+ if (!inode)
+ continue;
+
+ /* Either update the children of a tnode that
+ * already belongs to us or update the child
+ * to point to ourselves.
+ */
+ if (node_parent(inode) == tn)
+ update_children(inode);
+ else
+ node_set_parent(inode, tn);
+ }
+}
+
+static inline void put_child_root(struct tnode *tp, struct trie *t,
+ t_key key, struct tnode *n)
{
if (tp)
put_child(tp, get_index(key, tp), n);
@@ -434,10 +456,35 @@ static void tnode_free(struct tnode *tn)
}
}
+static void replace(struct trie *t, struct tnode *oldtnode, struct tnode *tn)
+{
+ struct tnode *tp = node_parent(oldtnode);
+ unsigned long i;
+
+ /* setup the parent pointer out of and back into this node */
+ NODE_INIT_PARENT(tn, tp);
+ put_child_root(tp, t, tn->key, tn);
+
+ /* update all of the child parent pointers */
+ update_children(tn);
+
+ /* all pointers should be clean so we are done */
+ tnode_free(oldtnode);
+
+ /* resize children now that oldtnode is freed */
+ for (i = tnode_child_length(tn); i;) {
+ struct tnode *inode = tnode_get_child(tn, --i);
+
+ /* resize child node */
+ if (tnode_full(tn, inode))
+ resize(t, inode);
+ }
+}
+
static int inflate(struct trie *t, struct tnode *oldtnode)
{
- struct tnode *inode, *node0, *node1, *tn, *tp;
- unsigned long i, j, k;
+ struct tnode *tn;
+ unsigned long i;
t_key m;
pr_debug("In inflate\n");
@@ -446,13 +493,18 @@ static int inflate(struct trie *t, struc
if (!tn)
return -ENOMEM;
+ /* prepare oldtnode to be freed */
+ tnode_free_init(oldtnode);
+
/* Assemble all of the pointers in our cluster, in this case that
* represents all of the pointers out of our allocated nodes that
* point to existing tnodes and the links between our allocated
* nodes.
*/
for (i = tnode_child_length(oldtnode), m = 1u << tn->pos; i;) {
- inode = tnode_get_child(oldtnode, --i);
+ struct tnode *inode = tnode_get_child(oldtnode, --i);
+ struct tnode *node0, *node1;
+ unsigned long j, k;
/* An empty child */
if (inode == NULL)
@@ -464,6 +516,9 @@ static int inflate(struct trie *t, struc
continue;
}
+ /* drop the node in the old tnode free list */
+ tnode_free_append(oldtnode, inode);
+
/* An internal node with two children */
if (inode->bits == 1) {
put_child(tn, 2 * i + 1, tnode_get_child(inode, 1));
@@ -488,9 +543,9 @@ static int inflate(struct trie *t, struc
node1 = tnode_new(inode->key | m, inode->pos, inode->bits - 1);
if (!node1)
goto nomem;
- tnode_free_append(tn, node1);
+ node0 = tnode_new(inode->key, inode->pos, inode->bits - 1);
- node0 = tnode_new(inode->key & ~m, inode->pos, inode->bits - 1);
+ tnode_free_append(tn, node1);
if (!node0)
goto nomem;
tnode_free_append(tn, node0);
@@ -512,53 +567,9 @@ static int inflate(struct trie *t, struc
put_child(tn, 2 * i, node0);
}
- /* setup the parent pointer into and out of this node */
- tp = node_parent(oldtnode);
- NODE_INIT_PARENT(tn, tp);
- put_child_root(tp, t, tn->key, tn);
-
- /* prepare oldtnode to be freed */
- tnode_free_init(oldtnode);
-
- /* update all child nodes parent pointers to route to us */
- for (i = tnode_child_length(oldtnode); i;) {
- inode = tnode_get_child(oldtnode, --i);
-
- /* A leaf or an internal node with skipped bits */
- if (!tnode_full(oldtnode, inode)) {
- node_set_parent(inode, tn);
- continue;
- }
-
- /* drop the node in the old tnode free list */
- tnode_free_append(oldtnode, inode);
-
- /* fetch new nodes */
- node1 = tnode_get_child(tn, 2 * i + 1);
- node0 = tnode_get_child(tn, 2 * i);
+ /* setup the parent pointers into and out of this node */
+ replace(t, oldtnode, tn);
- /* bits == 1 then node0 and node1 represent inode's children */
- if (inode->bits == 1) {
- node_set_parent(node1, tn);
- node_set_parent(node0, tn);
- continue;
- }
-
- /* update parent pointers in child node's children */
- for (k = tnode_child_length(inode), j = k / 2; j;) {
- node_set_parent(tnode_get_child(inode, --k), node1);
- node_set_parent(tnode_get_child(inode, --j), node0);
- node_set_parent(tnode_get_child(inode, --k), node1);
- node_set_parent(tnode_get_child(inode, --j), node0);
- }
-
- /* resize child nodes */
- resize(t, node1);
- resize(t, node0);
- }
-
- /* we completed without error, prepare to free old node */
- tnode_free(oldtnode);
return 0;
nomem:
/* all pointers should be clean so we are done */
@@ -568,7 +579,7 @@ nomem:
static int halve(struct trie *t, struct tnode *oldtnode)
{
- struct tnode *tn, *tp, *inode, *node0, *node1;
+ struct tnode *tn;
unsigned long i;
pr_debug("In halve\n");
@@ -577,14 +588,18 @@ static int halve(struct trie *t, struct
if (!tn)
return -ENOMEM;
+ /* prepare oldtnode to be freed */
+ tnode_free_init(oldtnode);
+
/* Assemble all of the pointers in our cluster, in this case that
* represents all of the pointers out of our allocated nodes that
* point to existing tnodes and the links between our allocated
* nodes.
*/
for (i = tnode_child_length(oldtnode); i;) {
- node1 = tnode_get_child(oldtnode, --i);
- node0 = tnode_get_child(oldtnode, --i);
+ struct tnode *node1 = tnode_get_child(oldtnode, --i);
+ struct tnode *node0 = tnode_get_child(oldtnode, --i);
+ struct tnode *inode;
/* At least one of the children is empty */
if (!node1 || !node0) {
@@ -609,34 +624,8 @@ static int halve(struct trie *t, struct
put_child(tn, i / 2, inode);
}
- /* setup the parent pointer out of and back into this node */
- tp = node_parent(oldtnode);
- NODE_INIT_PARENT(tn, tp);
- put_child_root(tp, t, tn->key, tn);
-
- /* prepare oldtnode to be freed */
- tnode_free_init(oldtnode);
-
- /* update all of the child parent pointers */
- for (i = tnode_child_length(tn); i;) {
- inode = tnode_get_child(tn, --i);
-
- /* only new tnodes will be considered "full" nodes */
- if (!tnode_full(tn, inode)) {
- node_set_parent(inode, tn);
- continue;
- }
-
- /* Two nonempty children */
- node_set_parent(tnode_get_child(inode, 1), inode);
- node_set_parent(tnode_get_child(inode, 0), inode);
-
- /* resize child node */
- resize(t, inode);
- }
-
- /* all pointers should be clean so we are done */
- tnode_free(oldtnode);
+ /* setup the parent pointers into and out of this node */
+ replace(t, oldtnode, tn);
return 0;
}
|
