Commit c7de2cf053420d63bac85133469c965d4b1083e1
Committed by
David S. Miller
1 parent
cbd6890c59
Exists in
master
and in
7 other branches
pkt_sched: gen_kill_estimator() rcu fixes
gen_kill_estimator() API is incomplete or not well documented, since caller should make sure an RCU grace period is respected before freeing stats_lock. This was partially addressed in commit 5d944c640b4 (gen_estimator: deadlock fix), but same problem exist for all gen_kill_estimator() users, if lock they use is not already RCU protected. A code review shows xt_RATEEST.c, act_api.c, act_police.c have this problem. Other are ok because they use qdisc lock, already RCU protected. Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net>
Showing 6 changed files with 36 additions and 3 deletions Side-by-side Diff
include/net/act_api.h
... | ... | @@ -20,6 +20,7 @@ |
20 | 20 | struct gnet_stats_queue tcfc_qstats; |
21 | 21 | struct gnet_stats_rate_est tcfc_rate_est; |
22 | 22 | spinlock_t tcfc_lock; |
23 | + struct rcu_head tcfc_rcu; | |
23 | 24 | }; |
24 | 25 | #define tcf_next common.tcfc_next |
25 | 26 | #define tcf_index common.tcfc_index |
... | ... | @@ -32,6 +33,7 @@ |
32 | 33 | #define tcf_qstats common.tcfc_qstats |
33 | 34 | #define tcf_rate_est common.tcfc_rate_est |
34 | 35 | #define tcf_lock common.tcfc_lock |
36 | +#define tcf_rcu common.tcfc_rcu | |
35 | 37 | |
36 | 38 | struct tcf_police { |
37 | 39 | struct tcf_common common; |
include/net/netfilter/xt_rateest.h
net/core/gen_estimator.c
... | ... | @@ -263,6 +263,7 @@ |
263 | 263 | * |
264 | 264 | * Removes the rate estimator specified by &bstats and &rate_est. |
265 | 265 | * |
266 | + * Note : Caller should respect an RCU grace period before freeing stats_lock | |
266 | 267 | */ |
267 | 268 | void gen_kill_estimator(struct gnet_stats_basic_packed *bstats, |
268 | 269 | struct gnet_stats_rate_est *rate_est) |
net/netfilter/xt_RATEEST.c
... | ... | @@ -60,13 +60,22 @@ |
60 | 60 | } |
61 | 61 | EXPORT_SYMBOL_GPL(xt_rateest_lookup); |
62 | 62 | |
63 | +static void xt_rateest_free_rcu(struct rcu_head *head) | |
64 | +{ | |
65 | + kfree(container_of(head, struct xt_rateest, rcu)); | |
66 | +} | |
67 | + | |
63 | 68 | void xt_rateest_put(struct xt_rateest *est) |
64 | 69 | { |
65 | 70 | mutex_lock(&xt_rateest_mutex); |
66 | 71 | if (--est->refcnt == 0) { |
67 | 72 | hlist_del(&est->list); |
68 | 73 | gen_kill_estimator(&est->bstats, &est->rstats); |
69 | - kfree(est); | |
74 | + /* | |
75 | + * gen_estimator est_timer() might access est->lock or bstats, | |
76 | + * wait a RCU grace period before freeing 'est' | |
77 | + */ | |
78 | + call_rcu(&est->rcu, xt_rateest_free_rcu); | |
70 | 79 | } |
71 | 80 | mutex_unlock(&xt_rateest_mutex); |
72 | 81 | } |
... | ... | @@ -179,6 +188,7 @@ |
179 | 188 | static void __exit xt_rateest_tg_fini(void) |
180 | 189 | { |
181 | 190 | xt_unregister_target(&xt_rateest_tg_reg); |
191 | + rcu_barrier(); /* Wait for completion of call_rcu()'s (xt_rateest_free_rcu) */ | |
182 | 192 | } |
183 | 193 | |
184 | 194 |
net/sched/act_api.c
... | ... | @@ -26,6 +26,11 @@ |
26 | 26 | #include <net/act_api.h> |
27 | 27 | #include <net/netlink.h> |
28 | 28 | |
29 | +static void tcf_common_free_rcu(struct rcu_head *head) | |
30 | +{ | |
31 | + kfree(container_of(head, struct tcf_common, tcfc_rcu)); | |
32 | +} | |
33 | + | |
29 | 34 | void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo) |
30 | 35 | { |
31 | 36 | unsigned int h = tcf_hash(p->tcfc_index, hinfo->hmask); |
... | ... | @@ -38,7 +43,11 @@ |
38 | 43 | write_unlock_bh(hinfo->lock); |
39 | 44 | gen_kill_estimator(&p->tcfc_bstats, |
40 | 45 | &p->tcfc_rate_est); |
41 | - kfree(p); | |
46 | + /* | |
47 | + * gen_estimator est_timer() might access p->tcfc_lock | |
48 | + * or bstats, wait a RCU grace period before freeing p | |
49 | + */ | |
50 | + call_rcu(&p->tcfc_rcu, tcf_common_free_rcu); | |
42 | 51 | return; |
43 | 52 | } |
44 | 53 | } |
net/sched/act_police.c
... | ... | @@ -97,6 +97,11 @@ |
97 | 97 | goto done; |
98 | 98 | } |
99 | 99 | |
100 | +static void tcf_police_free_rcu(struct rcu_head *head) | |
101 | +{ | |
102 | + kfree(container_of(head, struct tcf_police, tcf_rcu)); | |
103 | +} | |
104 | + | |
100 | 105 | static void tcf_police_destroy(struct tcf_police *p) |
101 | 106 | { |
102 | 107 | unsigned int h = tcf_hash(p->tcf_index, POL_TAB_MASK); |
... | ... | @@ -113,7 +118,11 @@ |
113 | 118 | qdisc_put_rtab(p->tcfp_R_tab); |
114 | 119 | if (p->tcfp_P_tab) |
115 | 120 | qdisc_put_rtab(p->tcfp_P_tab); |
116 | - kfree(p); | |
121 | + /* | |
122 | + * gen_estimator est_timer() might access p->tcf_lock | |
123 | + * or bstats, wait a RCU grace period before freeing p | |
124 | + */ | |
125 | + call_rcu(&p->tcf_rcu, tcf_police_free_rcu); | |
117 | 126 | return; |
118 | 127 | } |
119 | 128 | } |
... | ... | @@ -397,6 +406,7 @@ |
397 | 406 | police_cleanup_module(void) |
398 | 407 | { |
399 | 408 | tcf_unregister_action(&act_police_ops); |
409 | + rcu_barrier(); /* Wait for completion of call_rcu()'s (tcf_police_free_rcu) */ | |
400 | 410 | } |
401 | 411 | |
402 | 412 | module_init(police_init_module); |