Commit 7f261bb9 authored by Brian Witte's avatar Brian Witte Committed by Florian Westphal
Browse files

netfilter: nf_tables: revert commit_mutex usage in reset path 

It causes circular lock dependency between commit_mutex, nfnl_subsys_ipset
and nlk_cb_mutex when nft reset, ipset list, and iptables-nft with '-m set'
rule run at the same time.

Previous patches made it safe to run individual reset handlers concurrently
so commit_mutex is no longer required to prevent this.

Fixes: bd662c42 ("netfilter: nf_tables: Add locking for NFT_MSG_GETOBJ_RESET requests")
Fixes: 3d483faa ("netfilter: nf_tables: Add locking for NFT_MSG_GETSETELEM_RESET requests")
Fixes: 3cb03edb ("netfilter: nf_tables: Add locking for NFT_MSG_GETRULE_RESET requests")
Link: https://lore.kernel.org/all/aUh_3mVRV8OrGsVo@strlen.de/


Reported-by: default avatar <syzbot+ff16b505ec9152e5f448@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=ff16b505ec9152e5f448


Signed-off-by: default avatarBrian Witte <brianwitte@mailfence.com>
Signed-off-by: default avatarFlorian Westphal <fw@strlen.de>
parent 30c4d7fb

net/netfilter/nf_tables_api.c

+42 −206
Original line number Diff line number Diff line
@@ -3901,23 +3901,6 @@ static int nf_tables_dump_rules(struct sk_buff *skb, 
	return skb->len;

}



static int nf_tables_dumpreset_rules(struct sk_buff *skb,

				     struct netlink_callback *cb)

{

	struct nftables_pernet *nft_net = nft_pernet(sock_net(skb->sk));

	int ret;



	/* Mutex is held is to prevent that two concurrent dump-and-reset calls

	 * do not underrun counters and quotas. The commit_mutex is used for

	 * the lack a better lock, this is not transaction path.

	 */

	mutex_lock(&nft_net->commit_mutex);

	ret = nf_tables_dump_rules(skb, cb);

	mutex_unlock(&nft_net->commit_mutex);



	return ret;

}



static int nf_tables_dump_rules_start(struct netlink_callback *cb)

{

	struct nft_rule_dump_ctx *ctx = (void *)cb->ctx;
@@ -3937,16 +3920,10 @@ static int nf_tables_dump_rules_start(struct netlink_callback *cb) 
			return -ENOMEM;

		}

	}

	return 0;

}



static int nf_tables_dumpreset_rules_start(struct netlink_callback *cb)

{

	struct nft_rule_dump_ctx *ctx = (void *)cb->ctx;



	if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) == NFT_MSG_GETRULE_RESET)

		ctx->reset = true;



	return nf_tables_dump_rules_start(cb);

	return 0;

}



static int nf_tables_dump_rules_done(struct netlink_callback *cb)
@@ -4012,6 +3989,8 @@ static int nf_tables_getrule(struct sk_buff *skb, const struct nfnl_info *info, 
	u32 portid = NETLINK_CB(skb).portid;

	struct net *net = info->net;

	struct sk_buff *skb2;

	bool reset = false;

	char *buf;



	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {

		struct netlink_dump_control c = {
@@ -4025,46 +4004,15 @@ static int nf_tables_getrule(struct sk_buff *skb, const struct nfnl_info *info, 
		return nft_netlink_dump_start_rcu(info->sk, skb, info->nlh, &c);

	}



	skb2 = nf_tables_getrule_single(portid, info, nla, false);

	if (NFNL_MSG_TYPE(info->nlh->nlmsg_type) == NFT_MSG_GETRULE_RESET)

		reset = true;



	skb2 = nf_tables_getrule_single(portid, info, nla, reset);

	if (IS_ERR(skb2))

		return PTR_ERR(skb2);



	if (!reset)

		return nfnetlink_unicast(skb2, net, portid);

}



static int nf_tables_getrule_reset(struct sk_buff *skb,

				   const struct nfnl_info *info,

				   const struct nlattr * const nla[])

{

	struct nftables_pernet *nft_net = nft_pernet(info->net);

	u32 portid = NETLINK_CB(skb).portid;

	struct net *net = info->net;

	struct sk_buff *skb2;

	char *buf;



	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {

		struct netlink_dump_control c = {

			.start= nf_tables_dumpreset_rules_start,

			.dump = nf_tables_dumpreset_rules,

			.done = nf_tables_dump_rules_done,

			.module = THIS_MODULE,

			.data = (void *)nla,

		};



		return nft_netlink_dump_start_rcu(info->sk, skb, info->nlh, &c);

	}



	if (!try_module_get(THIS_MODULE))

		return -EINVAL;

	rcu_read_unlock();

	mutex_lock(&nft_net->commit_mutex);

	skb2 = nf_tables_getrule_single(portid, info, nla, true);

	mutex_unlock(&nft_net->commit_mutex);

	rcu_read_lock();

	module_put(THIS_MODULE);



	if (IS_ERR(skb2))

		return PTR_ERR(skb2);



	buf = kasprintf(GFP_ATOMIC, "%.*s:%u",

			nla_len(nla[NFTA_RULE_TABLE]),
@@ -6324,6 +6272,10 @@ static int nf_tables_dump_set(struct sk_buff *skb, struct netlink_callback *cb) 
	nla_nest_end(skb, nest);

	nlmsg_end(skb, nlh);



	if (dump_ctx->reset && args.iter.count > args.iter.skip)

		audit_log_nft_set_reset(table, cb->seq,

					args.iter.count - args.iter.skip);



	rcu_read_unlock();



	if (args.iter.err && args.iter.err != -EMSGSIZE)
@@ -6339,26 +6291,6 @@ static int nf_tables_dump_set(struct sk_buff *skb, struct netlink_callback *cb) 
	return -ENOSPC;

}



static int nf_tables_dumpreset_set(struct sk_buff *skb,

				   struct netlink_callback *cb)

{

	struct nftables_pernet *nft_net = nft_pernet(sock_net(skb->sk));

	struct nft_set_dump_ctx *dump_ctx = cb->data;

	int ret, skip = cb->args[0];



	mutex_lock(&nft_net->commit_mutex);



	ret = nf_tables_dump_set(skb, cb);



	if (cb->args[0] > skip)

		audit_log_nft_set_reset(dump_ctx->ctx.table, cb->seq,

					cb->args[0] - skip);



	mutex_unlock(&nft_net->commit_mutex);



	return ret;

}



static int nf_tables_dump_set_start(struct netlink_callback *cb)

{

	struct nft_set_dump_ctx *dump_ctx = cb->data;
@@ -6602,8 +6534,13 @@ static int nf_tables_getsetelem(struct sk_buff *skb, 
{

	struct netlink_ext_ack *extack = info->extack;

	struct nft_set_dump_ctx dump_ctx;

	int rem, err = 0, nelems = 0;

	struct net *net = info->net;

	struct nlattr *attr;

	int rem, err = 0;

	bool reset = false;



	if (NFNL_MSG_TYPE(info->nlh->nlmsg_type) == NFT_MSG_GETSETELEM_RESET)

		reset = true;



	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {

		struct netlink_dump_control c = {
@@ -6613,7 +6550,7 @@ static int nf_tables_getsetelem(struct sk_buff *skb, 
			.module = THIS_MODULE,

		};



		err = nft_set_dump_ctx_init(&dump_ctx, skb, info, nla, false);

		err = nft_set_dump_ctx_init(&dump_ctx, skb, info, nla, reset);

		if (err)

			return err;
@@ -6624,75 +6561,21 @@ static int nf_tables_getsetelem(struct sk_buff *skb, 
	if (!nla[NFTA_SET_ELEM_LIST_ELEMENTS])

		return -EINVAL;



	err = nft_set_dump_ctx_init(&dump_ctx, skb, info, nla, false);

	err = nft_set_dump_ctx_init(&dump_ctx, skb, info, nla, reset);

	if (err)

		return err;



	nla_for_each_nested(attr, nla[NFTA_SET_ELEM_LIST_ELEMENTS], rem) {

		err = nft_get_set_elem(&dump_ctx.ctx, dump_ctx.set, attr, false);

		if (err < 0) {

			NL_SET_BAD_ATTR(extack, attr);

			break;

		}

	}



	return err;

}



static int nf_tables_getsetelem_reset(struct sk_buff *skb,

				      const struct nfnl_info *info,

				      const struct nlattr * const nla[])

{

	struct nftables_pernet *nft_net = nft_pernet(info->net);

	struct netlink_ext_ack *extack = info->extack;

	struct nft_set_dump_ctx dump_ctx;

	int rem, err = 0, nelems = 0;

	struct nlattr *attr;



	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {

		struct netlink_dump_control c = {

			.start = nf_tables_dump_set_start,

			.dump = nf_tables_dumpreset_set,

			.done = nf_tables_dump_set_done,

			.module = THIS_MODULE,

		};



		err = nft_set_dump_ctx_init(&dump_ctx, skb, info, nla, true);

		if (err)

			return err;



		c.data = &dump_ctx;

		return nft_netlink_dump_start_rcu(info->sk, skb, info->nlh, &c);

	}



	if (!nla[NFTA_SET_ELEM_LIST_ELEMENTS])

		return -EINVAL;



	if (!try_module_get(THIS_MODULE))

		return -EINVAL;

	rcu_read_unlock();

	mutex_lock(&nft_net->commit_mutex);

	rcu_read_lock();



	err = nft_set_dump_ctx_init(&dump_ctx, skb, info, nla, true);

	if (err)

		goto out_unlock;



	nla_for_each_nested(attr, nla[NFTA_SET_ELEM_LIST_ELEMENTS], rem) {

		err = nft_get_set_elem(&dump_ctx.ctx, dump_ctx.set, attr, true);

		err = nft_get_set_elem(&dump_ctx.ctx, dump_ctx.set, attr, reset);

		if (err < 0) {

			NL_SET_BAD_ATTR(extack, attr);

			break;

		}

		nelems++;

	}

	audit_log_nft_set_reset(dump_ctx.ctx.table, nft_base_seq(info->net), nelems);



out_unlock:

	rcu_read_unlock();

	mutex_unlock(&nft_net->commit_mutex);

	rcu_read_lock();

	module_put(THIS_MODULE);

	if (reset)

		audit_log_nft_set_reset(dump_ctx.ctx.table, nft_base_seq(net),

					nelems);



	return err;

}
@@ -8564,19 +8447,6 @@ static int nf_tables_dump_obj(struct sk_buff *skb, struct netlink_callback *cb) 
	return skb->len;

}



static int nf_tables_dumpreset_obj(struct sk_buff *skb,

				   struct netlink_callback *cb)

{

	struct nftables_pernet *nft_net = nft_pernet(sock_net(skb->sk));

	int ret;



	mutex_lock(&nft_net->commit_mutex);

	ret = nf_tables_dump_obj(skb, cb);

	mutex_unlock(&nft_net->commit_mutex);



	return ret;

}



static int nf_tables_dump_obj_start(struct netlink_callback *cb)

{

	struct nft_obj_dump_ctx *ctx = (void *)cb->ctx;
@@ -8593,16 +8463,10 @@ static int nf_tables_dump_obj_start(struct netlink_callback *cb) 
	if (nla[NFTA_OBJ_TYPE])

		ctx->type = ntohl(nla_get_be32(nla[NFTA_OBJ_TYPE]));



	return 0;

}



static int nf_tables_dumpreset_obj_start(struct netlink_callback *cb)

{

	struct nft_obj_dump_ctx *ctx = (void *)cb->ctx;



	if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) == NFT_MSG_GETOBJ_RESET)

		ctx->reset = true;



	return nf_tables_dump_obj_start(cb);

	return 0;

}



static int nf_tables_dump_obj_done(struct netlink_callback *cb)
@@ -8664,42 +8528,16 @@ nf_tables_getobj_single(u32 portid, const struct nfnl_info *info, 
static int nf_tables_getobj(struct sk_buff *skb, const struct nfnl_info *info,

			    const struct nlattr * const nla[])

{

	u32 portid = NETLINK_CB(skb).portid;

	struct sk_buff *skb2;



	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {

		struct netlink_dump_control c = {

			.start = nf_tables_dump_obj_start,

			.dump = nf_tables_dump_obj,

			.done = nf_tables_dump_obj_done,

			.module = THIS_MODULE,

			.data = (void *)nla,

		};



		return nft_netlink_dump_start_rcu(info->sk, skb, info->nlh, &c);

	}



	skb2 = nf_tables_getobj_single(portid, info, nla, false);

	if (IS_ERR(skb2))

		return PTR_ERR(skb2);



	return nfnetlink_unicast(skb2, info->net, portid);

}



static int nf_tables_getobj_reset(struct sk_buff *skb,

				  const struct nfnl_info *info,

				  const struct nlattr * const nla[])

{

	struct nftables_pernet *nft_net = nft_pernet(info->net);

	u32 portid = NETLINK_CB(skb).portid;

	struct net *net = info->net;

	struct sk_buff *skb2;

	bool reset = false;

	char *buf;



	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {

		struct netlink_dump_control c = {

			.start = nf_tables_dumpreset_obj_start,

			.dump = nf_tables_dumpreset_obj,

			.start = nf_tables_dump_obj_start,

			.dump = nf_tables_dump_obj,

			.done = nf_tables_dump_obj_done,

			.module = THIS_MODULE,

			.data = (void *)nla,
@@ -8708,18 +8546,16 @@ static int nf_tables_getobj_reset(struct sk_buff *skb, 
		return nft_netlink_dump_start_rcu(info->sk, skb, info->nlh, &c);

	}



	if (!try_module_get(THIS_MODULE))

		return -EINVAL;

	rcu_read_unlock();

	mutex_lock(&nft_net->commit_mutex);

	skb2 = nf_tables_getobj_single(portid, info, nla, true);

	mutex_unlock(&nft_net->commit_mutex);

	rcu_read_lock();

	module_put(THIS_MODULE);

	if (NFNL_MSG_TYPE(info->nlh->nlmsg_type) == NFT_MSG_GETOBJ_RESET)

		reset = true;



	skb2 = nf_tables_getobj_single(portid, info, nla, reset);

	if (IS_ERR(skb2))

		return PTR_ERR(skb2);



	if (!reset)

		return nfnetlink_unicast(skb2, net, NETLINK_CB(skb).portid);



	buf = kasprintf(GFP_ATOMIC, "%.*s:%u",

			nla_len(nla[NFTA_OBJ_TABLE]),

			(char *)nla_data(nla[NFTA_OBJ_TABLE]),
@@ -10037,7 +9873,7 @@ static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = { 
		.policy		= nft_rule_policy,

	},

	[NFT_MSG_GETRULE_RESET] = {

		.call		= nf_tables_getrule_reset,

		.call		= nf_tables_getrule,

		.type		= NFNL_CB_RCU,

		.attr_count	= NFTA_RULE_MAX,

		.policy		= nft_rule_policy,
@@ -10091,7 +9927,7 @@ static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = { 
		.policy		= nft_set_elem_list_policy,

	},

	[NFT_MSG_GETSETELEM_RESET] = {

		.call		= nf_tables_getsetelem_reset,

		.call		= nf_tables_getsetelem,

		.type		= NFNL_CB_RCU,

		.attr_count	= NFTA_SET_ELEM_LIST_MAX,

		.policy		= nft_set_elem_list_policy,
@@ -10137,7 +9973,7 @@ static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = { 
		.policy		= nft_obj_policy,

	},

	[NFT_MSG_GETOBJ_RESET] = {

		.call		= nf_tables_getobj_reset,

		.call		= nf_tables_getobj,

		.type		= NFNL_CB_RCU,

		.attr_count	= NFTA_OBJ_MAX,

		.policy		= nft_obj_policy,