Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 4c05ec47 authored by Taehee Yoo's avatar Taehee Yoo Committed by Pablo Neira Ayuso
Browse files

netfilter: nf_tables: fix suspicious RCU usage in nft_chain_stats_replace() 



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: f102d66b ("netfilter: nf_tables: use dedicated mutex to guard transactions")
Signed-off-by: default avatarTaehee Yoo <ap420073@gmail.com>
Signed-off-by: default avatarPablo Neira Ayuso <pablo@netfilter.org>
parent 986103e7

include/linux/netfilter/nfnetlink.h

+0 −12
Original line number Diff line number Diff line
@@ -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))

net/netfilter/nf_tables_api.c

+13 −8
Original line number Diff line number Diff line
@@ -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:

net/netfilter/nf_tables_core.c

+1 −1
Original line number Diff line number Diff line
@@ -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();