[PATCH v4 nf-next 0/2] netfilter: nf_tables: fix reset request deadlock

[PATCH v4 nf-next 0/2] netfilter: nf_tables: fix reset request deadlock

[Brian Witte]

syzbot reported a possible circular locking dependency between
commit_mutex, nfnl_subsys_ipset and nlk_cb_mutex-NETFILTER:

  WARNING: possible circular locking dependency detected
  syz.3.970/9330 is trying to acquire lock:
  ffff888012d4ccd8 (&nft_net->commit_mutex){+.+.}-{4:4}, at: nf_tables_dumpreset_obj+0x6f/0xa0

  but task is already holding lock:
  ffff88802bce36f0 (nlk_cb_mutex-NETFILTER){+.+.}-{4:4}, at: __netlink_dump_start+0x150/0x990

  Chain exists of:
    &nft_net->commit_mutex --> nfnl_subsys_ipset --> nlk_cb_mutex-NETFILTER

   Possible unsafe locking scenario:

         CPU0                    CPU1
         ----                    ----
    lock(nlk_cb_mutex-NETFILTER);
                                 lock(nfnl_subsys_ipset);
                                 lock(nlk_cb_mutex-NETFILTER);
    lock(&nft_net->commit_mutex);

Link: https://syzkaller.appspot.com/bug?extid=ff16b505ec9152e5f448

The bug was introduced by commits that added commit_mutex locking to
serialize reset requests.

v4:
  - Push spinlock down into nft_counter_reset() instead of holding it
    across entire dump iteration, per Florian's review
  - Store struct net in counter priv to access the per-net spinlock
    during reset, avoiding skb->sk dereference which is NULL in
    single-element GET paths such as nft_get_set_elem
  - Use atomic64_xchg() for quota reset instead of spinlock, which is
    simpler per Pablo's suggestion

v3:
  - Restructured as 2-patch series per Florian's suggestion:
    1. Revert the 3 commits that added commit_mutex locking
    2. Add spinlock-based serialization for reset requests
  Link: https://lore.kernel.org/netfilter-devel/20260201195255.532559-1-brianwitte@mailfence.com/

v2:
  - Switched to a spinlock in nft_pernet instead of mutex
  - Spinlock doesn't sleep, so we stay in RCU read-side critical section
  - Removes the try_module_get/module_put and rcu_read_unlock/lock dance
  Link: https://lore.kernel.org/netfilter-devel/20260201062517.263087-1-brianwitte@mailfence.com/

v1:
  - Proposed using a dedicated reset_mutex instead of commit_mutex
  Link: https://lore.kernel.org/netfilter-devel/20260127030604.39982-1-brianwitte@mailfence.com/

Brian Witte (2):
  Revert nf_tables commit_mutex in reset path
  netfilter: nf_tables: serialize reset with spinlock and atomic

 include/net/netfilter/nf_tables.h |   1 +
 net/netfilter/nf_tables_api.c     | 249 ++++++------------------------
 net/netfilter/nft_counter.c       |  17 +-
 net/netfilter/nft_quota.c         |  12 +-
 4 files changed, 63 insertions(+), 216 deletions(-)

--
2.47.3

[PATCH v4 nf-next 1/2] Revert nf_tables commit_mutex in reset path

[Brian Witte]

Revert mutex-based locking for reset requests. It caused a circular
lock dependency between commit_mutex, nfnl_subsys_ipset, and
nlk_cb_mutex when nft reset, ipset list, and iptables-nft with set
match ran concurrently.

This reverts bd662c4218f9, 3d483faa6663, 3cb03edb4de3.

Reported-by: syzbot+ff16b505ec9152e5f448@syzkaller.appspotmail.com
Fixes: bd662c4218f9 ("netfilter: nf_tables: Add locking for NFT_MSG_GETOBJ_RESET requests")
Fixes: 3d483faa6663 ("netfilter: nf_tables: Add locking for NFT_MSG_GETSETELEM_RESET requests")
Fixes: 3cb03edb4de3 ("netfilter: nf_tables: Add locking for NFT_MSG_GETRULE_RESET requests")
Signed-off-by: Brian Witte <brianwitte@mailfence.com>
---
 net/netfilter/nf_tables_api.c | 248 ++++++----------------------------
 1 file changed, 43 insertions(+), 205 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 9923387907e3..9969d8488de4 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -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;
-
-	ctx->reset = true;
+	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)
@@ -4008,44 +3985,18 @@ nf_tables_getrule_single(u32 portid, const struct nfnl_info *info,
 
 static int nf_tables_getrule(struct sk_buff *skb, const struct nfnl_info *info,
 			     const struct nlattr * const nla[])
-{
-	u32 portid = NETLINK_CB(skb).portid;
-	struct net *net = info->net;
-	struct sk_buff *skb2;
-
-	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {
-		struct netlink_dump_control c = {
-			.start= nf_tables_dump_rules_start,
-			.dump = nf_tables_dump_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);
-	}
-
-	skb2 = nf_tables_getrule_single(portid, info, nla, false);
-	if (IS_ERR(skb2))
-		return PTR_ERR(skb2);
-
-	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;
+	bool reset = false;
 	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,
+			.start= nf_tables_dump_rules_start,
+			.dump = nf_tables_dump_rules,
 			.done = nf_tables_dump_rules_done,
 			.module = THIS_MODULE,
 			.data = (void *)nla,
@@ -4054,18 +4005,16 @@ static int nf_tables_getrule_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_getrule_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_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);
+
 	buf = kasprintf(GFP_ATOMIC, "%.*s:%u",
 			nla_len(nla[NFTA_RULE_TABLE]),
 			(char *)nla_data(nla[NFTA_RULE_TABLE]),
@@ -6324,6 +6273,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 +6292,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 +6535,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 +6551,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 +6562,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 +8448,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 +8464,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;
-
-	ctx->reset = true;
+	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)
@@ -8663,43 +8528,18 @@ 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 +8548,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 +9875,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 +9929,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 +9975,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,
-- 
2.47.3

[PATCH v4 nf-next 2/2] netfilter: nf_tables: serialize reset with spinlock and atomic

[Brian Witte]

Add a dedicated spinlock to serialize counter reset operations,
preventing concurrent dump-and-reset from underrunning values.

Store struct net in counter priv to access the per-net spinlock during
reset. This avoids dereferencing skb->sk which is NULL in single-element
GET paths such as nft_get_set_elem.

For quota, use atomic64_xchg() to atomically read and zero the consumed
value, which is simpler and doesn't require spinlock protection.

Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Suggested-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Brian Witte <brianwitte@mailfence.com>
---
 include/net/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c     |  3 +--
 net/netfilter/nft_counter.c       | 17 ++++++++++++-----
 net/netfilter/nft_quota.c         | 12 +++++++-----
 4 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 426534a711b0..c4b6b8cadf09 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1935,6 +1935,7 @@ struct nftables_pernet {
 	struct list_head	module_list;
 	struct list_head	notify_list;
 	struct mutex		commit_mutex;
+	spinlock_t		reset_lock;
 	u64			table_handle;
 	u64			tstamp;
 	unsigned int		gc_seq;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 9969d8488de4..146f29be834a 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3986,7 +3986,6 @@ nf_tables_getrule_single(u32 portid, const struct nfnl_info *info,
 static int nf_tables_getrule(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;
@@ -8529,7 +8528,6 @@ 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[])
 {
-	struct nftables_pernet *nft_net = nft_pernet(info->net);
 	u32 portid = NETLINK_CB(skb).portid;
 	struct net *net = info->net;
 	struct sk_buff *skb2;
@@ -12050,6 +12048,7 @@ static int __net_init nf_tables_init_net(struct net *net)
 	INIT_LIST_HEAD(&nft_net->module_list);
 	INIT_LIST_HEAD(&nft_net->notify_list);
 	mutex_init(&nft_net->commit_mutex);
+	spin_lock_init(&nft_net->reset_lock);
 	net->nft.base_seq = 1;
 	nft_net->gc_seq = 0;
 	nft_net->validate_state = NFT_VALIDATE_SKIP;
diff --git a/net/netfilter/nft_counter.c b/net/netfilter/nft_counter.c
index cc7325329496..54bcbf33e2b9 100644
--- a/net/netfilter/nft_counter.c
+++ b/net/netfilter/nft_counter.c
@@ -28,6 +28,7 @@ struct nft_counter_tot {
 
 struct nft_counter_percpu_priv {
 	struct nft_counter __percpu *counter;
+	struct net *net;
 };
 
 static DEFINE_PER_CPU(struct u64_stats_sync, nft_counter_sync);
@@ -61,7 +62,8 @@ static inline void nft_counter_obj_eval(struct nft_object *obj,
 }
 
 static int nft_counter_do_init(const struct nlattr * const tb[],
-			       struct nft_counter_percpu_priv *priv)
+			       struct nft_counter_percpu_priv *priv,
+			       struct net *net)
 {
 	struct nft_counter __percpu *cpu_stats;
 	struct nft_counter *this_cpu;
@@ -81,6 +83,7 @@ static int nft_counter_do_init(const struct nlattr * const tb[],
 	}
 
 	priv->counter = cpu_stats;
+	priv->net = net;
 	return 0;
 }
 
@@ -90,7 +93,7 @@ static int nft_counter_obj_init(const struct nft_ctx *ctx,
 {
 	struct nft_counter_percpu_priv *priv = nft_obj_data(obj);
 
-	return nft_counter_do_init(tb, priv);
+	return nft_counter_do_init(tb, priv, ctx->net);
 }
 
 static void nft_counter_do_destroy(struct nft_counter_percpu_priv *priv)
@@ -106,13 +109,15 @@ static void nft_counter_obj_destroy(const struct nft_ctx *ctx,
 	nft_counter_do_destroy(priv);
 }
 
-static void nft_counter_reset(struct nft_counter_percpu_priv *priv,
+static void nft_counter_reset(struct nftables_pernet *nft_net,
+			      struct nft_counter_percpu_priv *priv,
 			      struct nft_counter_tot *total)
 {
 	struct u64_stats_sync *nft_sync;
 	struct nft_counter *this_cpu;
 
 	local_bh_disable();
+	spin_lock(&nft_net->reset_lock);
 	this_cpu = this_cpu_ptr(priv->counter);
 	nft_sync = this_cpu_ptr(&nft_counter_sync);
 
@@ -121,6 +126,7 @@ static void nft_counter_reset(struct nft_counter_percpu_priv *priv,
 	u64_stats_add(&this_cpu->bytes, -total->bytes);
 	u64_stats_update_end(nft_sync);
 
+	spin_unlock(&nft_net->reset_lock);
 	local_bh_enable();
 }
 
@@ -163,7 +169,7 @@ static int nft_counter_do_dump(struct sk_buff *skb,
 		goto nla_put_failure;
 
 	if (reset)
-		nft_counter_reset(priv, &total);
+		nft_counter_reset(nft_pernet(priv->net), priv, &total);
 
 	return 0;
 
@@ -224,7 +230,7 @@ static int nft_counter_init(const struct nft_ctx *ctx,
 {
 	struct nft_counter_percpu_priv *priv = nft_expr_priv(expr);
 
-	return nft_counter_do_init(tb, priv);
+	return nft_counter_do_init(tb, priv, ctx->net);
 }
 
 static void nft_counter_destroy(const struct nft_ctx *ctx,
@@ -254,6 +260,7 @@ static int nft_counter_clone(struct nft_expr *dst, const struct nft_expr *src, g
 	u64_stats_set(&this_cpu->bytes, total.bytes);
 
 	priv_clone->counter = cpu_stats;
+	priv_clone->net = priv->net;
 	return 0;
 }
 
diff --git a/net/netfilter/nft_quota.c b/net/netfilter/nft_quota.c
index df0798da2329..34c77c872f79 100644
--- a/net/netfilter/nft_quota.c
+++ b/net/netfilter/nft_quota.c
@@ -140,11 +140,14 @@ static int nft_quota_do_dump(struct sk_buff *skb, struct nft_quota *priv,
 	u64 consumed, consumed_cap, quota;
 	u32 flags = priv->flags;
 
-	/* Since we inconditionally increment consumed quota for each packet
+	/* Since we unconditionally increment consumed quota for each packet
 	 * that we see, don't go over the quota boundary in what we send to
 	 * userspace.
 	 */
-	consumed = atomic64_read(priv->consumed);
+	if (reset)
+		consumed = atomic64_xchg(priv->consumed, 0);
+	else
+		consumed = atomic64_read(priv->consumed);
 	quota = atomic64_read(&priv->quota);
 	if (consumed >= quota) {
 		consumed_cap = quota;
@@ -160,10 +163,9 @@ static int nft_quota_do_dump(struct sk_buff *skb, struct nft_quota *priv,
 	    nla_put_be32(skb, NFTA_QUOTA_FLAGS, htonl(flags)))
 		goto nla_put_failure;
 
-	if (reset) {
-		atomic64_sub(consumed, priv->consumed);
+	if (reset)
 		clear_bit(NFT_QUOTA_DEPLETED_BIT, &priv->flags);
-	}
+
 	return 0;
 
 nla_put_failure:
-- 
2.47.3

Re: [PATCH v4 nf-next 2/2] netfilter: nf_tables: serialize reset with spinlock and atomic

[Florian Westphal]

Brian Witte <brianwitte@mailfence.com> wrote:
> Add a dedicated spinlock to serialize counter reset operations,
> preventing concurrent dump-and-reset from underrunning values.
> 
> Store struct net in counter priv to access the per-net spinlock during
> reset. This avoids dereferencing skb->sk which is NULL in single-element
> GET paths such as nft_get_set_elem.

Ouch, sorry about making a wrong suggestion.  I did not consider that
this reset infra also works via plain netlink requests rather than just
for netlink dumps.

> For quota, use atomic64_xchg() to atomically read and zero the consumed
> value, which is simpler and doesn't require spinlock protection.

I think you should split this, one patch for quota and one nft_counter.

> Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Suggested-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Brian Witte <brianwitte@mailfence.com>
> ---
>  include/net/netfilter/nf_tables.h |  1 +
>  net/netfilter/nf_tables_api.c     |  3 +--
>  net/netfilter/nft_counter.c       | 17 ++++++++++++-----
>  net/netfilter/nft_quota.c         | 12 +++++++-----
>  4 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index 426534a711b0..c4b6b8cadf09 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -1935,6 +1935,7 @@ struct nftables_pernet {
>  	struct list_head	module_list;
>  	struct list_head	notify_list;
>  	struct mutex		commit_mutex;
> +	spinlock_t		reset_lock;
>  	u64			table_handle;
>  	u64			tstamp;
>  	unsigned int		gc_seq;
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 9969d8488de4..146f29be834a 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -3986,7 +3986,6 @@ nf_tables_getrule_single(u32 portid, const struct nfnl_info *info,
>  static int nf_tables_getrule(struct sk_buff *skb, const struct nfnl_info *info,
>  			     const struct nlattr * const nla[])
>  {
> -	struct nftables_pernet *nft_net = nft_pernet(info->net);

This is odd, shouldn't that be in patch 1?
I would expect compiler to warn here, when just compiling with
patch 1 applied.

>  	u32 portid = NETLINK_CB(skb).portid;
>  	struct net *net = info->net;
>  	struct sk_buff *skb2;
> @@ -8529,7 +8528,6 @@ 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[])
>  {
> -	struct nftables_pernet *nft_net = nft_pernet(info->net);

Same.

> @@ -12050,6 +12048,7 @@ static int __net_init nf_tables_init_net(struct net *net)
>  	INIT_LIST_HEAD(&nft_net->module_list);
>  	INIT_LIST_HEAD(&nft_net->notify_list);
>  	mutex_init(&nft_net->commit_mutex);
> +	spin_lock_init(&nft_net->reset_lock);
>  	net->nft.base_seq = 1;
>  	nft_net->gc_seq = 0;
>  	nft_net->validate_state = NFT_VALIDATE_SKIP;
> diff --git a/net/netfilter/nft_counter.c b/net/netfilter/nft_counter.c
> index cc7325329496..54bcbf33e2b9 100644
> --- a/net/netfilter/nft_counter.c
> +++ b/net/netfilter/nft_counter.c
> @@ -28,6 +28,7 @@ struct nft_counter_tot {
>  
>  struct nft_counter_percpu_priv {
>  	struct nft_counter __percpu *counter;
> +	struct net *net;
>  };

Hmm, I think thats too high a price.

> -static void nft_counter_reset(struct nft_counter_percpu_priv *priv,
> +static void nft_counter_reset(struct nftables_pernet *nft_net,
> +			      struct nft_counter_percpu_priv *priv,
>  			      struct nft_counter_tot *total)
>  {
>  	struct u64_stats_sync *nft_sync;
>  	struct nft_counter *this_cpu;
>  
>  	local_bh_disable();
> +	spin_lock(&nft_net->reset_lock);

This lock is too late, it has to be taken before fetching 'total',
else you get following in parallel reset case:

cpu0						cpu1
 fetch total counter, get 1000
 						fetch total counter, get 1000

 subtract 1000
 						subtract 1000 again

Maybe something like this?  I think the single-spinlock is fine, I
don't expect frequent resets, much less from concurrent netns.

If we get a complaint, we can always take the code-churn route to pass
'struct net' down to the dumper callback, or resort to array-of-locks
or similar at a later time.

+/* control plane only: sync fetch+reset */
+static DEFINE_SPINLOCK(nft_counter_lock);
+
 static inline void nft_counter_do_eval(struct nft_counter_percpu_priv *priv,
                                       struct nft_regs *regs,
                                       const struct nft_pktinfo *pkt)
@@ -147,6 +151,14 @@ static void nft_counter_fetch(struct nft_counter_percpu_priv *priv,
                total->packets  += packets;
        }
 }
+static void nft_counter_fetch_and_reset(struct nft_counter_percpu_priv *priv,
+                                       struct nft_counter_tot *total)
+{
+       spin_lock(&nft_lock);
+       nft_counter_fetch(priv, total);
+       nft_counter_reset(priv, total);
+       spin_unlock(&nft_lock);
+}

 static int nft_counter_do_dump(struct sk_buff *skb,
                               struct nft_counter_percpu_priv *priv,
@@ -154,7 +166,10 @@ static int nft_counter_do_dump(struct sk_buff *skb,
 {
        struct nft_counter_tot total;

-       nft_counter_fetch(priv, &total);
+       if (unlikely(reset))
+               nft_counter_fetch_and_reset(priv, &total);
+       else
+               nft_counter_fetch(priv, &total);

        if (nla_put_be64(skb, NFTA_COUNTER_BYTES, cpu_to_be64(total.bytes),
                         NFTA_COUNTER_PAD) ||
@@ -162,9 +177,6 @@ static int nft_counter_do_dump(struct sk_buff *skb,
                         NFTA_COUNTER_PAD))
                goto nla_put_failure;

-       if (reset)
-               nft_counter_reset(priv, &total);
-
        return 0;

Re: [PATCH v4 nf-next 2/2] netfilter: nf_tables: serialize reset with spinlock and atomic

[Pablo Neira Ayuso]

On Tue, Feb 03, 2026 at 01:51:46PM +0100, Florian Westphal wrote:
> Brian Witte <brianwitte@mailfence.com> wrote:
> > Add a dedicated spinlock to serialize counter reset operations,
> > preventing concurrent dump-and-reset from underrunning values.
> > 
> > Store struct net in counter priv to access the per-net spinlock during
> > reset. This avoids dereferencing skb->sk which is NULL in single-element
> > GET paths such as nft_get_set_elem.
> 
> Ouch, sorry about making a wrong suggestion.  I did not consider that
> this reset infra also works via plain netlink requests rather than just
> for netlink dumps.

Maybe this so it covers for get and dump path?

static struct nftables_pernet *nft_pernet_from_nlskb(const struct sk_buff *skb)
{
        struct sock *sk = skb->sk ? : NETLINK_CB(skb).sk;

        return nft_pernet(sock_net(sk));
}

in case it is worth to skip the unique nft_counter_lock below.

[...]
> +/* control plane only: sync fetch+reset */
> +static DEFINE_SPINLOCK(nft_counter_lock);
> +
>  static inline void nft_counter_do_eval(struct nft_counter_percpu_priv *priv,
>                                        struct nft_regs *regs,
>                                        const struct nft_pktinfo *pkt)

Re: [PATCH v4 nf-next 2/2] netfilter: nf_tables: serialize reset with spinlock and atomic

[Brian Witte]

On Mon, Feb 03, 2026 at 11:19:46PM +0100, Pablo Neira Ayuso wrote:
> Maybe this so it covers for get and dump path?
>
> static struct nftables_pernet *nft_pernet_from_nlskb(const struct sk_buff *skb)
> {
>         struct sock *sk = skb->sk ? : NETLINK_CB(skb).sk;
>
>         return nft_pernet(sock_net(sk));
> }
>
> in case it is worth to skip the unique nft_counter_lock below.

I have v5 ready with Florian's global DEFINE_SPINLOCK approach:
split into 3 patches (revert, counter spinlock, quota atomic64_xchg),
with nft_counter_fetch_and_reset() wrapping fetch+reset under the
lock so parallel resets can't both read the same values. Tested and
working.

Before I send: should I go with the global spinlock, or would you
prefer the per-net lock via nft_pernet_from_nlskb()? Happy to do
either.

Thanks,
Brian

Re: [PATCH v4 nf-next 2/2] netfilter: nf_tables: serialize reset with spinlock and atomic

[Florian Westphal]

Brian Witte <brianwitte@mailfence.com> wrote:
> On Mon, Feb 03, 2026 at 11:19:46PM +0100, Pablo Neira Ayuso wrote:
> > Maybe this so it covers for get and dump path?
> >
> > static struct nftables_pernet *nft_pernet_from_nlskb(const struct sk_buff *skb)
> > {
> >         struct sock *sk = skb->sk ? : NETLINK_CB(skb).sk;
> >
> >         return nft_pernet(sock_net(sk));
> > }
> >
> > in case it is worth to skip the unique nft_counter_lock below.
> 
> I have v5 ready with Florian's global DEFINE_SPINLOCK approach:
> split into 3 patches (revert, counter spinlock, quota atomic64_xchg),
> with nft_counter_fetch_and_reset() wrapping fetch+reset under the
> lock so parallel resets can't both read the same values. Tested and
> working.

Thanks.

> Before I send: should I go with the global spinlock, or would you
> prefer the per-net lock via nft_pernet_from_nlskb()? Happy to do
> either.

I don't think the nft_pernet_from_nlskb() will work as-is, for the
get requests the target skb is allocated via alloc_skb() and I don't
think the control block is initialised to hold the origin netlink query
sk.

Re: [PATCH v4 nf-next 2/2] netfilter: nf_tables: serialize reset with spinlock and atomic

[Pablo Neira Ayuso]

On Wed, Feb 04, 2026 at 07:08:05PM +0100, Florian Westphal wrote:
> Brian Witte <brianwitte@mailfence.com> wrote:
> > On Mon, Feb 03, 2026 at 11:19:46PM +0100, Pablo Neira Ayuso wrote:
> > > Maybe this so it covers for get and dump path?
> > >
> > > static struct nftables_pernet *nft_pernet_from_nlskb(const struct sk_buff *skb)
> > > {
> > >         struct sock *sk = skb->sk ? : NETLINK_CB(skb).sk;
> > >
> > >         return nft_pernet(sock_net(sk));
> > > }
> > >
> > > in case it is worth to skip the unique nft_counter_lock below.
> > 
> > I have v5 ready with Florian's global DEFINE_SPINLOCK approach:
> > split into 3 patches (revert, counter spinlock, quota atomic64_xchg),
> > with nft_counter_fetch_and_reset() wrapping fetch+reset under the
> > lock so parallel resets can't both read the same values. Tested and
> > working.
> 
> Thanks.
> 
> > Before I send: should I go with the global spinlock, or would you
> > prefer the per-net lock via nft_pernet_from_nlskb()? Happy to do
> > either.
> 
> I don't think the nft_pernet_from_nlskb() will work as-is, for the
> get requests the target skb is allocated via alloc_skb() and I don't
> think the control block is initialised to hold the origin netlink query
> sk.

This is GET path:

static int netlink_unicast_kernel(struct sock *sk, struct sk_buff *skb,
                                  struct sock *ssk)
{
        int ret;
        struct netlink_sock *nlk = nlk_sk(sk);
 
        ret = -ECONNREFUSED;
        if (nlk->netlink_rcv != NULL) {
                ret = skb->len;
                atomic_add(skb->truesize, &sk->sk_rmem_alloc);
                netlink_skb_set_owner_r(skb, sk);
                NETLINK_CB(skb).sk = ssk; <---------------------
                netlink_deliver_tap_kernel(sk, ssk, skb);
                nlk->netlink_rcv(skb);
                consume_skb(skb);