Thread (7 messages) 7 messages, 2 authors, 2018-12-13
STALE2786d
Revisions (218)
  1. v1 [diff vs current]
  2. v1 [diff vs current]
  3. v1 [diff vs current]
  4. v1 [diff vs current]
  5. v1 [diff vs current]
  6. v1 [diff vs current]
  7. v1 [diff vs current]
  8. v1 [diff vs current]
  9. v1 [diff vs current]
  10. v1 [diff vs current]
  11. v1 [diff vs current]
  12. v1 [diff vs current]
  13. v1 [diff vs current]
  14. v1 [diff vs current]
  15. v1 [diff vs current]
  16. v1 [diff vs current]
  17. v1 [diff vs current]
  18. v1 [diff vs current]
  19. v1 [diff vs current]
  20. v1 [diff vs current]
  21. v1 [diff vs current]
  22. v1 [diff vs current]
  23. v1 [diff vs current]
  24. v1 [diff vs current]
  25. v1 [diff vs current]
  26. v1 [diff vs current]
  27. v1 [diff vs current]
  28. v1 [diff vs current]
  29. v1 [diff vs current]
  30. v1 [diff vs current]
  31. v1 [diff vs current]
  32. v1 [diff vs current]
  33. v1 [diff vs current]
  34. v1 [diff vs current]
  35. v1 [diff vs current]
  36. v1 [diff vs current]
  37. v1 [diff vs current]
  38. v1 [diff vs current]
  39. v1 [diff vs current]
  40. v1 [diff vs current]
  41. v1 [diff vs current]
  42. v1 [diff vs current]
  43. v1 [diff vs current]
  44. v1 [diff vs current]
  45. v1 [diff vs current]
  46. v1 [diff vs current]
  47. v1 [diff vs current]
  48. v1 [diff vs current]
  49. v1 [diff vs current]
  50. v1 [diff vs current]
  51. v1 [diff vs current]
  52. v1 [diff vs current]
  53. v1 [diff vs current]
  54. v1 [diff vs current]
  55. v1 [diff vs current]
  56. v1 [diff vs current]
  57. v1 [diff vs current]
  58. v1 current
  59. v1 [diff vs current]
  60. v1 [diff vs current]
  61. v1 [diff vs current]
  62. v1 [diff vs current]
  63. v1 [diff vs current]
  64. v1 [diff vs current]
  65. v1 [diff vs current]
  66. v1 [diff vs current]
  67. v1 [diff vs current]
  68. v1 [diff vs current]
  69. v1 [diff vs current]
  70. v1 [diff vs current]
  71. v1 [diff vs current]
  72. v1 [diff vs current]
  73. v1 [diff vs current]
  74. v1 [diff vs current]
  75. v1 [diff vs current]
  76. v1 [diff vs current]
  77. v1 [diff vs current]
  78. v1 [diff vs current]
  79. v1 [diff vs current]
  80. v1 [diff vs current]
  81. v1 [diff vs current]
  82. v1 [diff vs current]
  83. v1 [diff vs current]
  84. v1 [diff vs current]
  85. v1 [diff vs current]
  86. v1 [diff vs current]
  87. v1 [diff vs current]
  88. v1 [diff vs current]
  89. v1 [diff vs current]
  90. v1 [diff vs current]
  91. v1 [diff vs current]
  92. v1 [diff vs current]
  93. v1 [diff vs current]
  94. v1 [diff vs current]
  95. v1 [diff vs current]
  96. v1 [diff vs current]
  97. v1 [diff vs current]
  98. v1 [diff vs current]
  99. v1 [diff vs current]
  100. v1 [diff vs current]
  101. v1 [diff vs current]
  102. v1 [diff vs current]
  103. v1 [diff vs current]
  104. v1 [diff vs current]
  105. v1 [diff vs current]
  106. v1 [diff vs current]
  107. v1 [diff vs current]
  108. v1 [diff vs current]
  109. v1 [diff vs current]
  110. v1 [diff vs current]
  111. v1 [diff vs current]
  112. v2 [diff vs current]
  113. v1 [diff vs current]
  114. v1 [diff vs current]
  115. v1 [diff vs current]
  116. v1 [diff vs current]
  117. v1 [diff vs current]
  118. v1 [diff vs current]
  119. v1 [diff vs current]
  120. v1 [diff vs current]
  121. v1 [diff vs current]
  122. v1 [diff vs current]
  123. v2 [diff vs current]
  124. v1 [diff vs current]
  125. v1 [diff vs current]
  126. v1 [diff vs current]
  127. v1 [diff vs current]
  128. v1 [diff vs current]
  129. v1 [diff vs current]
  130. v1 [diff vs current]
  131. v1 [diff vs current]
  132. v1 [diff vs current]
  133. v1 [diff vs current]
  134. v1 [diff vs current]
  135. v1 [diff vs current]
  136. v1 [diff vs current]
  137. v1 [diff vs current]
  138. v1 [diff vs current]
  139. v1 [diff vs current]
  140. v1 [diff vs current]
  141. v1 [diff vs current]
  142. v1 [diff vs current]
  143. v1 [diff vs current]
  144. v1 [diff vs current]
  145. v1 [diff vs current]
  146. v1 [diff vs current]
  147. v1 [diff vs current]
  148. v1 [diff vs current]
  149. v1 [diff vs current]
  150. v1 [diff vs current]
  151. v1 [diff vs current]
  152. v1 [diff vs current]
  153. v1 [diff vs current]
  154. v1 [diff vs current]
  155. v1 [diff vs current]
  156. v1 [diff vs current]
  157. v1 [diff vs current]
  158. v1 [diff vs current]
  159. v1 [diff vs current]
  160. v1 [diff vs current]
  161. v1 [diff vs current]
  162. v1 [diff vs current]
  163. v1 [diff vs current]
  164. v1 [diff vs current]
  165. v1 [diff vs current]
  166. v1 [diff vs current]
  167. v1 [diff vs current]
  168. v1 [diff vs current]
  169. v2 [diff vs current]
  170. v1 [diff vs current]
  171. v1 [diff vs current]
  172. v2 [diff vs current]
  173. v1 [diff vs current]
  174. v1 [diff vs current]
  175. v1 [diff vs current]
  176. v1 [diff vs current]
  177. v1 [diff vs current]
  178. v2 [diff vs current]
  179. v1 [diff vs current]
  180. v1 [diff vs current]
  181. v1 [diff vs current]
  182. v2 [diff vs current]
  183. v3 [diff vs current]
  184. v1 [diff vs current]
  185. v1 [diff vs current]
  186. v1 [diff vs current]
  187. v1 [diff vs current]
  188. v1 [diff vs current]
  189. v1 [diff vs current]
  190. v1 [diff vs current]
  191. v1 [diff vs current]
  192. v2 [diff vs current]
  193. v1 [diff vs current]
  194. v1 [diff vs current]
  195. v2 [diff vs current]
  196. v1 [diff vs current]
  197. v1 [diff vs current]
  198. v1 [diff vs current]
  199. v1 [diff vs current]
  200. v1 [diff vs current]
  201. v2 [diff vs current]
  202. v1 [diff vs current]
  203. v1 [diff vs current]
  204. v1 [diff vs current]
  205. v1 [diff vs current]
  206. v1 [diff vs current]
  207. v1 [diff vs current]
  208. v1 [diff vs current]
  209. v1 [diff vs current]
  210. v1 [diff vs current]
  211. v2 [diff vs current]
  212. v1 [diff vs current]
  213. v2 [diff vs current]
  214. v1 [diff vs current]
  215. v1 [diff vs current]
  216. v1 [diff vs current]
  217. v2 [diff vs current]
  218. v1 [diff vs current]

[PATCH 1/5] netfilter: nf_tables: fix suspicious RCU usage in nft_chain_stats_replace()

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: 2018-12-13 01:06:41
Also in: netfilter-devel
Subsystem: netfilter, networking [general], the rest · Maintainers: Pablo Neira Ayuso, Florian Westphal, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds

From: Taehee Yoo <ap420073@gmail.com>

basechain->stats is rcu protected data which is updated from
nft_chain_stats_replace(). This function is executed from the commit
phase which holds the pernet nf_tables commit mutex - not the global
nfnetlink subsystem mutex.

Test commands to reproduce the problem are:
   %iptables-nft -I INPUT
   %iptables-nft -Z
   %iptables-nft -Z

This patch uses RCU calls to handle basechain->stats updates to fix a
splat that looks like:

[89279.358755] =============================
[89279.363656] WARNING: suspicious RCU usage
[89279.368458] 4.20.0-rc2+ #44 Tainted: G        W    L
[89279.374661] -----------------------------
[89279.379542] net/netfilter/nf_tables_api.c:1404 suspicious rcu_dereference_protected() usage!
[...]
[89279.406556] 1 lock held by iptables-nft/5225:
[89279.411728]  #0: 00000000bf45a000 (&net->nft.commit_mutex){+.+.}, at: nf_tables_valid_genid+0x1f/0x70 [nf_tables]
[89279.424022] stack backtrace:
[89279.429236] CPU: 0 PID: 5225 Comm: iptables-nft Tainted: G        W    L    4.20.0-rc2+ #44
[89279.430135] Call Trace:
[89279.430135]  dump_stack+0xc9/0x16b
[89279.430135]  ? show_regs_print_info+0x5/0x5
[89279.430135]  ? lockdep_rcu_suspicious+0x117/0x160
[89279.430135]  nft_chain_commit_update+0x4ea/0x640 [nf_tables]
[89279.430135]  ? sched_clock_local+0xd4/0x140
[89279.430135]  ? check_flags.part.35+0x440/0x440
[89279.430135]  ? __rhashtable_remove_fast.constprop.67+0xec0/0xec0 [nf_tables]
[89279.430135]  ? sched_clock_cpu+0x126/0x170
[89279.430135]  ? find_held_lock+0x39/0x1c0
[89279.430135]  ? hlock_class+0x140/0x140
[89279.430135]  ? is_bpf_text_address+0x5/0xf0
[89279.430135]  ? check_flags.part.35+0x440/0x440
[89279.430135]  ? __lock_is_held+0xb4/0x140
[89279.430135]  nf_tables_commit+0x2555/0x39c0 [nf_tables]

Fixes: f102d66b335a4 ("netfilter: nf_tables: use dedicated mutex to guard transactions")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netfilter/nfnetlink.h | 12 ------------
 net/netfilter/nf_tables_api.c       | 21 +++++++++++++--------
 net/netfilter/nf_tables_core.c      |  2 +-
 3 files changed, 14 insertions(+), 21 deletions(-)
diff --git a/include/linux/netfilter/nfnetlink.h b/include/linux/netfilter/nfnetlink.h
index 4a520d3304a2..cf09ab37b45b 100644
--- a/include/linux/netfilter/nfnetlink.h
+++ b/include/linux/netfilter/nfnetlink.h
@@ -62,18 +62,6 @@ static inline bool lockdep_nfnl_is_held(__u8 subsys_id)
 }
 #endif /* CONFIG_PROVE_LOCKING */
 
-/*
- * nfnl_dereference - fetch RCU pointer when updates are prevented by subsys mutex
- *
- * @p: The pointer to read, prior to dereferencing
- * @ss: The nfnetlink subsystem ID
- *
- * Return the value of the specified RCU-protected pointer, but omit
- * the READ_ONCE(), because caller holds the NFNL subsystem mutex.
- */
-#define nfnl_dereference(p, ss)					\
-	rcu_dereference_protected(p, lockdep_nfnl_is_held(ss))
-
 #define MODULE_ALIAS_NFNL_SUBSYS(subsys) \
 	MODULE_ALIAS("nfnetlink-subsys-" __stringify(subsys))
 
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 2e61aab6ed73..6e548d7c9f67 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1216,7 +1216,8 @@ static int nf_tables_fill_chain_info(struct sk_buff *skb, struct net *net,
 		if (nla_put_string(skb, NFTA_CHAIN_TYPE, basechain->type->name))
 			goto nla_put_failure;
 
-		if (basechain->stats && nft_dump_stats(skb, basechain->stats))
+		if (rcu_access_pointer(basechain->stats) &&
+		    nft_dump_stats(skb, rcu_dereference(basechain->stats)))
 			goto nla_put_failure;
 	}
 
@@ -1392,7 +1393,8 @@ static struct nft_stats __percpu *nft_stats_alloc(const struct nlattr *attr)
 	return newstats;
 }
 
-static void nft_chain_stats_replace(struct nft_base_chain *chain,
+static void nft_chain_stats_replace(struct net *net,
+				    struct nft_base_chain *chain,
 				    struct nft_stats __percpu *newstats)
 {
 	struct nft_stats __percpu *oldstats;
@@ -1400,8 +1402,9 @@ static void nft_chain_stats_replace(struct nft_base_chain *chain,
 	if (newstats == NULL)
 		return;
 
-	if (chain->stats) {
-		oldstats = nfnl_dereference(chain->stats, NFNL_SUBSYS_NFTABLES);
+	if (rcu_access_pointer(chain->stats)) {
+		oldstats = rcu_dereference_protected(chain->stats,
+					lockdep_commit_lock_is_held(net));
 		rcu_assign_pointer(chain->stats, newstats);
 		synchronize_rcu();
 		free_percpu(oldstats);
@@ -1439,9 +1442,10 @@ static void nf_tables_chain_destroy(struct nft_ctx *ctx)
 		struct nft_base_chain *basechain = nft_base_chain(chain);
 
 		module_put(basechain->type->owner);
-		free_percpu(basechain->stats);
-		if (basechain->stats)
+		if (rcu_access_pointer(basechain->stats)) {
 			static_branch_dec(&nft_counters_enabled);
+			free_percpu(rcu_dereference_raw(basechain->stats));
+		}
 		kfree(chain->name);
 		kfree(basechain);
 	} else {
@@ -1590,7 +1594,7 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
 				kfree(basechain);
 				return PTR_ERR(stats);
 			}
-			basechain->stats = stats;
+			rcu_assign_pointer(basechain->stats, stats);
 			static_branch_inc(&nft_counters_enabled);
 		}
 
@@ -6180,7 +6184,8 @@ static void nft_chain_commit_update(struct nft_trans *trans)
 		return;
 
 	basechain = nft_base_chain(trans->ctx.chain);
-	nft_chain_stats_replace(basechain, nft_trans_chain_stats(trans));
+	nft_chain_stats_replace(trans->ctx.net, basechain,
+				nft_trans_chain_stats(trans));
 
 	switch (nft_trans_chain_policy(trans)) {
 	case NF_DROP:
diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index 3fbce3b9c5ec..a50500232b0a 100644
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -101,7 +101,7 @@ static noinline void nft_update_chain_stats(const struct nft_chain *chain,
 	struct nft_stats *stats;
 
 	base_chain = nft_base_chain(chain);
-	if (!base_chain->stats)
+	if (!rcu_access_pointer(base_chain->stats))
 		return;
 
 	local_bh_disable();
-- 
2.11.0
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help