netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 00/11] Netfilter updates for net-next
@ 2024-11-06 23:46 Pablo Neira Ayuso
  2024-11-06 23:46 ` [PATCH net-next 01/11] netfilter: Make legacy configs user selectable Pablo Neira Ayuso
                   ` (11 more replies)
  0 siblings, 12 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2024-11-06 23:46 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet, fw

Hi,

The following series contains Netfilter updates for net-next:

1) Make legacy xtables configs user selectable, from Breno Leitao.

2) Fix a few sparse warnings related to percpu, from Uros Bizjak.

3) Use strscpy_pad, from Justin Stitt.

4) Use nft_trans_elem_alloc() in catchall flush, from Florian Westphal.

5) A series of 7 patches to fix false positive with CONFIG_RCU_LIST=y.
   Florian also sees possible issue with 10 while module load/removal
   when requesting an expression that is available via module. As for
   patch 11, object is being updated so reference on the module already
   exists so I don't see any real issue.

   Florian says:

   "Unfortunately there are many more errors, and not all are false positives.

   First patches pass lockdep_commit_lock_is_held() to the rcu list traversal
   macro so that those splats are avoided.

   The last two patches are real code change as opposed to
   'pass the transaction mutex to relax rcu check':

   Those two lists are not protected by transaction mutex so could be altered
   in parallel.

   This targets nf-next because these are long-standing issues."

Please, pull these changes from:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf-next.git nf-next-24-11-07

Thanks.

----------------------------------------------------------------

The following changes since commit f66ebf37d69cc700ca884c6a18c2258caf8b151b:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net (2024-10-03 10:05:55 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf-next.git nf-next-24-11-07

for you to fetch changes up to cddc04275f95ca3b18da5c0fb111705ac173af89:

  netfilter: nf_tables: must hold rcu read lock while iterating object type list (2024-11-05 22:07:12 +0100)

----------------------------------------------------------------
netfilter pull request 24-11-07

----------------------------------------------------------------
Breno Leitao (1):
      netfilter: Make legacy configs user selectable

Florian Westphal (8):
      netfilter: nf_tables: prefer nft_trans_elem_alloc helper
      netfilter: nf_tables: avoid false-positive lockdep splat on rule deletion
      netfilter: nf_tables: avoid false-positive lockdep splats with sets
      netfilter: nf_tables: avoid false-positive lockdep splats with flowtables
      netfilter: nf_tables: avoid false-positive lockdep splats in set walker
      netfilter: nf_tables: avoid false-positive lockdep splats with basechain hook
      netfilter: nf_tables: must hold rcu read lock while iterating expression type list
      netfilter: nf_tables: must hold rcu read lock while iterating object type list

Justin Stitt (1):
      netfilter: nf_tables: replace deprecated strncpy with strscpy_pad

Uros Bizjak (1):
      netfilter: nf_tables: Fix percpu address space issues in nf_tables_api.c

 include/net/netfilter/nf_tables.h      |   3 +-
 net/bridge/netfilter/Kconfig           |   8 +-
 net/bridge/netfilter/nft_meta_bridge.c |   2 +-
 net/ipv4/netfilter/Kconfig             |  16 +++-
 net/ipv6/netfilter/Kconfig             |   9 ++-
 net/netfilter/nf_tables_api.c          | 132 +++++++++++++++++++--------------
 net/netfilter/nft_flow_offload.c       |   4 +-
 net/netfilter/nft_set_bitmap.c         |  10 ++-
 net/netfilter/nft_set_hash.c           |   3 +-
 9 files changed, 119 insertions(+), 68 deletions(-)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net-next 01/11] netfilter: Make legacy configs user selectable
  2024-11-06 23:46 [PATCH net-next 00/11] Netfilter updates for net-next Pablo Neira Ayuso
@ 2024-11-06 23:46 ` Pablo Neira Ayuso
  2024-11-07 12:00   ` patchwork-bot+netdevbpf
  2024-11-06 23:46 ` [PATCH net-next 02/11] netfilter: nf_tables: Fix percpu address space issues in nf_tables_api.c Pablo Neira Ayuso
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 18+ messages in thread
From: Pablo Neira Ayuso @ 2024-11-06 23:46 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet, fw

From: Breno Leitao <leitao@debian.org>

This option makes legacy Netfilter Kconfig user selectable, giving users
the option to configure iptables without enabling any other config.

Make the following KConfig entries user selectable:
 * BRIDGE_NF_EBTABLES_LEGACY
 * IP_NF_ARPTABLES
 * IP_NF_IPTABLES_LEGACY
 * IP6_NF_IPTABLES_LEGACY

Signed-off-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/bridge/netfilter/Kconfig |  8 +++++++-
 net/ipv4/netfilter/Kconfig   | 16 ++++++++++++++--
 net/ipv6/netfilter/Kconfig   |  9 ++++++++-
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/net/bridge/netfilter/Kconfig b/net/bridge/netfilter/Kconfig
index 104c0125e32e..f16bbbbb9481 100644
--- a/net/bridge/netfilter/Kconfig
+++ b/net/bridge/netfilter/Kconfig
@@ -41,7 +41,13 @@ config NF_CONNTRACK_BRIDGE
 
 # old sockopt interface and eval loop
 config BRIDGE_NF_EBTABLES_LEGACY
-	tristate
+	tristate "Legacy EBTABLES support"
+	depends on BRIDGE && NETFILTER_XTABLES
+	default n
+	help
+	 Legacy ebtables packet/frame classifier.
+	 This is not needed if you are using ebtables over nftables
+	 (iptables-nft).
 
 menuconfig BRIDGE_NF_EBTABLES
 	tristate "Ethernet Bridge tables (ebtables) support"
diff --git a/net/ipv4/netfilter/Kconfig b/net/ipv4/netfilter/Kconfig
index 1b991b889506..ef8009281da5 100644
--- a/net/ipv4/netfilter/Kconfig
+++ b/net/ipv4/netfilter/Kconfig
@@ -12,7 +12,13 @@ config NF_DEFRAG_IPV4
 
 # old sockopt interface and eval loop
 config IP_NF_IPTABLES_LEGACY
-	tristate
+	tristate "Legacy IP tables support"
+	default	n
+	select NETFILTER_XTABLES
+	help
+	  iptables is a legacy packet classifier.
+	  This is not needed if you are using iptables over nftables
+	  (iptables-nft).
 
 config NF_SOCKET_IPV4
 	tristate "IPv4 socket lookup support"
@@ -318,7 +324,13 @@ endif # IP_NF_IPTABLES
 
 # ARP tables
 config IP_NF_ARPTABLES
-	tristate
+	tristate "Legacy ARPTABLES support"
+	depends on NETFILTER_XTABLES
+	default n
+	help
+	  arptables is a legacy packet classifier.
+	  This is not needed if you are using arptables over nftables
+	  (iptables-nft).
 
 config NFT_COMPAT_ARP
 	tristate
diff --git a/net/ipv6/netfilter/Kconfig b/net/ipv6/netfilter/Kconfig
index f3c8e2d918e1..e087a8e97ba7 100644
--- a/net/ipv6/netfilter/Kconfig
+++ b/net/ipv6/netfilter/Kconfig
@@ -8,7 +8,14 @@ menu "IPv6: Netfilter Configuration"
 
 # old sockopt interface and eval loop
 config IP6_NF_IPTABLES_LEGACY
-	tristate
+	tristate "Legacy IP6 tables support"
+	depends on INET && IPV6
+	select NETFILTER_XTABLES
+	default n
+	help
+	  ip6tables is a legacy packet classifier.
+	  This is not needed if you are using iptables over nftables
+	  (iptables-nft).
 
 config NF_SOCKET_IPV6
 	tristate "IPv6 socket lookup support"
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 02/11] netfilter: nf_tables: Fix percpu address space issues in nf_tables_api.c
  2024-11-06 23:46 [PATCH net-next 00/11] Netfilter updates for net-next Pablo Neira Ayuso
  2024-11-06 23:46 ` [PATCH net-next 01/11] netfilter: Make legacy configs user selectable Pablo Neira Ayuso
@ 2024-11-06 23:46 ` Pablo Neira Ayuso
  2024-11-06 23:46 ` [PATCH net-next 03/11] netfilter: nf_tables: replace deprecated strncpy with strscpy_pad Pablo Neira Ayuso
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2024-11-06 23:46 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet, fw

From: Uros Bizjak <ubizjak@gmail.com>

Compiling nf_tables_api.c results in several sparse warnings:

nf_tables_api.c:2077:31: warning: incorrect type in return expression (different address spaces)
nf_tables_api.c:2080:31: warning: incorrect type in return expression (different address spaces)
nf_tables_api.c:2084:31: warning: incorrect type in return expression (different address spaces)

nf_tables_api.c:2740:23: warning: incorrect type in assignment (different address spaces)
nf_tables_api.c:2752:38: warning: incorrect type in assignment (different address spaces)
nf_tables_api.c:2798:21: warning: incorrect type in argument 1 (different address spaces)

Use {ERR_PTR,IS_ERR,PTR_ERR}_PCPU() macros when crossing between generic
and percpu address spaces and add __percpu annotation to *stats pointer
to fix these warnings.

Found by GCC's named address space checks.

There were no changes in the resulting object files.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index a24fe62650a7..6552ec616745 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2082,14 +2082,14 @@ static struct nft_stats __percpu *nft_stats_alloc(const struct nlattr *attr)
 	err = nla_parse_nested_deprecated(tb, NFTA_COUNTER_MAX, attr,
 					  nft_counter_policy, NULL);
 	if (err < 0)
-		return ERR_PTR(err);
+		return ERR_PTR_PCPU(err);
 
 	if (!tb[NFTA_COUNTER_BYTES] || !tb[NFTA_COUNTER_PACKETS])
-		return ERR_PTR(-EINVAL);
+		return ERR_PTR_PCPU(-EINVAL);
 
 	newstats = netdev_alloc_pcpu_stats(struct nft_stats);
 	if (newstats == NULL)
-		return ERR_PTR(-ENOMEM);
+		return ERR_PTR_PCPU(-ENOMEM);
 
 	/* Restore old counters on this cpu, no problem. Per-cpu statistics
 	 * are not exposed to userspace.
@@ -2533,10 +2533,10 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
 
 		if (nla[NFTA_CHAIN_COUNTERS]) {
 			stats = nft_stats_alloc(nla[NFTA_CHAIN_COUNTERS]);
-			if (IS_ERR(stats)) {
+			if (IS_ERR_PCPU(stats)) {
 				nft_chain_release_hook(&hook);
 				kfree(basechain);
-				return PTR_ERR(stats);
+				return PTR_ERR_PCPU(stats);
 			}
 			rcu_assign_pointer(basechain->stats, stats);
 		}
@@ -2650,7 +2650,7 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
 	struct nft_table *table = ctx->table;
 	struct nft_chain *chain = ctx->chain;
 	struct nft_chain_hook hook = {};
-	struct nft_stats *stats = NULL;
+	struct nft_stats __percpu *stats = NULL;
 	struct nft_hook *h, *next;
 	struct nf_hook_ops *ops;
 	struct nft_trans *trans;
@@ -2746,8 +2746,8 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
 		}
 
 		stats = nft_stats_alloc(nla[NFTA_CHAIN_COUNTERS]);
-		if (IS_ERR(stats)) {
-			err = PTR_ERR(stats);
+		if (IS_ERR_PCPU(stats)) {
+			err = PTR_ERR_PCPU(stats);
 			goto err_hooks;
 		}
 	}
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 03/11] netfilter: nf_tables: replace deprecated strncpy with strscpy_pad
  2024-11-06 23:46 [PATCH net-next 00/11] Netfilter updates for net-next Pablo Neira Ayuso
  2024-11-06 23:46 ` [PATCH net-next 01/11] netfilter: Make legacy configs user selectable Pablo Neira Ayuso
  2024-11-06 23:46 ` [PATCH net-next 02/11] netfilter: nf_tables: Fix percpu address space issues in nf_tables_api.c Pablo Neira Ayuso
@ 2024-11-06 23:46 ` Pablo Neira Ayuso
  2024-11-06 23:46 ` [PATCH net-next 04/11] netfilter: nf_tables: prefer nft_trans_elem_alloc helper Pablo Neira Ayuso
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2024-11-06 23:46 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet, fw

From: Justin Stitt <justinstitt@google.com>

strncpy() is deprecated for use on NUL-terminated destination strings [1] and
as such we should prefer more robust and less ambiguous string interfaces.

In this particular instance, the usage of strncpy() is fine and works as
expected. However, towards the goal of [2], we should consider replacing
it with an alternative as many instances of strncpy() are bug-prone. Its
removal from the kernel promotes better long term health for the
codebase.

The current usage of strncpy() likely just wants the NUL-padding
behavior offered by strncpy() and doesn't care about the
NUL-termination. Since the compiler doesn't know the size of @dest, we
can't use strtomem_pad(). Instead, use strscpy_pad() which behaves
functionally the same as strncpy() in this context -- as we expect
br_dev->name to be NUL-terminated itself.

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90 [2]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
Cc: Kees Cook <keescook@chromium.org>
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/bridge/netfilter/nft_meta_bridge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bridge/netfilter/nft_meta_bridge.c b/net/bridge/netfilter/nft_meta_bridge.c
index d12a221366d6..5adced1e7d0c 100644
--- a/net/bridge/netfilter/nft_meta_bridge.c
+++ b/net/bridge/netfilter/nft_meta_bridge.c
@@ -63,7 +63,7 @@ static void nft_meta_bridge_get_eval(const struct nft_expr *expr,
 		return nft_meta_get_eval(expr, regs, pkt);
 	}
 
-	strncpy((char *)dest, br_dev ? br_dev->name : "", IFNAMSIZ);
+	strscpy_pad((char *)dest, br_dev ? br_dev->name : "", IFNAMSIZ);
 	return;
 err:
 	regs->verdict.code = NFT_BREAK;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 04/11] netfilter: nf_tables: prefer nft_trans_elem_alloc helper
  2024-11-06 23:46 [PATCH net-next 00/11] Netfilter updates for net-next Pablo Neira Ayuso
                   ` (2 preceding siblings ...)
  2024-11-06 23:46 ` [PATCH net-next 03/11] netfilter: nf_tables: replace deprecated strncpy with strscpy_pad Pablo Neira Ayuso
@ 2024-11-06 23:46 ` Pablo Neira Ayuso
  2024-11-06 23:46 ` [PATCH net-next 05/11] netfilter: nf_tables: avoid false-positive lockdep splat on rule deletion Pablo Neira Ayuso
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2024-11-06 23:46 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet, fw

From: Florian Westphal <fw@strlen.de>

Reduce references to sizeof(struct nft_trans_elem).
Preparation patch to move this to a flexiable array to store
elem references.

Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 6552ec616745..30331688301e 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -6409,7 +6409,7 @@ static void nf_tables_setelem_notify(const struct nft_ctx *ctx,
 	nfnetlink_set_err(net, portid, NFNLGRP_NFTABLES, -ENOBUFS);
 }
 
-static struct nft_trans *nft_trans_elem_alloc(struct nft_ctx *ctx,
+static struct nft_trans *nft_trans_elem_alloc(const struct nft_ctx *ctx,
 					      int msg_type,
 					      struct nft_set *set)
 {
@@ -7471,13 +7471,11 @@ static int __nft_set_catchall_flush(const struct nft_ctx *ctx,
 {
 	struct nft_trans *trans;
 
-	trans = nft_trans_alloc_gfp(ctx, NFT_MSG_DELSETELEM,
-				    sizeof(struct nft_trans_elem), GFP_KERNEL);
+	trans = nft_trans_elem_alloc(ctx, NFT_MSG_DELSETELEM, set);
 	if (!trans)
 		return -ENOMEM;
 
 	nft_setelem_data_deactivate(ctx->net, set, elem_priv);
-	nft_trans_elem_set(trans) = set;
 	nft_trans_elem_priv(trans) = elem_priv;
 	nft_trans_commit_list_add_tail(ctx->net, trans);
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 05/11] netfilter: nf_tables: avoid false-positive lockdep splat on rule deletion
  2024-11-06 23:46 [PATCH net-next 00/11] Netfilter updates for net-next Pablo Neira Ayuso
                   ` (3 preceding siblings ...)
  2024-11-06 23:46 ` [PATCH net-next 04/11] netfilter: nf_tables: prefer nft_trans_elem_alloc helper Pablo Neira Ayuso
@ 2024-11-06 23:46 ` Pablo Neira Ayuso
  2024-11-06 23:46 ` [PATCH net-next 06/11] netfilter: nf_tables: avoid false-positive lockdep splats with sets Pablo Neira Ayuso
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2024-11-06 23:46 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet, fw

From: Florian Westphal <fw@strlen.de>

On rule delete we get:
 WARNING: suspicious RCU usage
 net/netfilter/nf_tables_api.c:3420 RCU-list traversed in non-reader section!!
 1 lock held by iptables/134:
   #0: ffff888008c4fcc8 (&nft_net->commit_mutex){+.+.}-{3:3}, at: nf_tables_valid_genid (include/linux/jiffies.h:101) nf_tables

Code is fine, no other CPU can change the list because we're holding
transaction mutex.

Pass the needed lockdep annotation to the iterator and fix
two comments for functions that are no longer restricted to rcu-only
context.

This is enough to resolve rule delete, but there are several other
missing annotations, added in followup-patches.

Fixes: 28875945ba98 ("rcu: Add support for consolidated-RCU reader checking")
Reported-by: Matthieu Baerts <matttbe@kernel.org>
Tested-by: Matthieu Baerts <matttbe@kernel.org>
Closes: https://lore.kernel.org/netfilter-devel/da27f17f-3145-47af-ad0f-7fd2a823623e@kernel.org/
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 30331688301e..80c285ac7e07 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3411,13 +3411,15 @@ void nft_expr_destroy(const struct nft_ctx *ctx, struct nft_expr *expr)
  * Rules
  */
 
-static struct nft_rule *__nft_rule_lookup(const struct nft_chain *chain,
+static struct nft_rule *__nft_rule_lookup(const struct net *net,
+					  const struct nft_chain *chain,
 					  u64 handle)
 {
 	struct nft_rule *rule;
 
 	// FIXME: this sucks
-	list_for_each_entry_rcu(rule, &chain->rules, list) {
+	list_for_each_entry_rcu(rule, &chain->rules, list,
+				lockdep_commit_lock_is_held(net)) {
 		if (handle == rule->handle)
 			return rule;
 	}
@@ -3425,13 +3427,14 @@ static struct nft_rule *__nft_rule_lookup(const struct nft_chain *chain,
 	return ERR_PTR(-ENOENT);
 }
 
-static struct nft_rule *nft_rule_lookup(const struct nft_chain *chain,
+static struct nft_rule *nft_rule_lookup(const struct net *net,
+					const struct nft_chain *chain,
 					const struct nlattr *nla)
 {
 	if (nla == NULL)
 		return ERR_PTR(-EINVAL);
 
-	return __nft_rule_lookup(chain, be64_to_cpu(nla_get_be64(nla)));
+	return __nft_rule_lookup(net, chain, be64_to_cpu(nla_get_be64(nla)));
 }
 
 static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
@@ -3732,7 +3735,7 @@ static int nf_tables_dump_rules_done(struct netlink_callback *cb)
 	return 0;
 }
 
-/* called with rcu_read_lock held */
+/* Caller must hold rcu read lock or transaction mutex */
 static struct sk_buff *
 nf_tables_getrule_single(u32 portid, const struct nfnl_info *info,
 			 const struct nlattr * const nla[], bool reset)
@@ -3759,7 +3762,7 @@ nf_tables_getrule_single(u32 portid, const struct nfnl_info *info,
 		return ERR_CAST(chain);
 	}
 
-	rule = nft_rule_lookup(chain, nla[NFTA_RULE_HANDLE]);
+	rule = nft_rule_lookup(net, chain, nla[NFTA_RULE_HANDLE]);
 	if (IS_ERR(rule)) {
 		NL_SET_BAD_ATTR(extack, nla[NFTA_RULE_HANDLE]);
 		return ERR_CAST(rule);
@@ -4057,7 +4060,7 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
 
 	if (nla[NFTA_RULE_HANDLE]) {
 		handle = be64_to_cpu(nla_get_be64(nla[NFTA_RULE_HANDLE]));
-		rule = __nft_rule_lookup(chain, handle);
+		rule = __nft_rule_lookup(net, chain, handle);
 		if (IS_ERR(rule)) {
 			NL_SET_BAD_ATTR(extack, nla[NFTA_RULE_HANDLE]);
 			return PTR_ERR(rule);
@@ -4079,7 +4082,7 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
 
 		if (nla[NFTA_RULE_POSITION]) {
 			pos_handle = be64_to_cpu(nla_get_be64(nla[NFTA_RULE_POSITION]));
-			old_rule = __nft_rule_lookup(chain, pos_handle);
+			old_rule = __nft_rule_lookup(net, chain, pos_handle);
 			if (IS_ERR(old_rule)) {
 				NL_SET_BAD_ATTR(extack, nla[NFTA_RULE_POSITION]);
 				return PTR_ERR(old_rule);
@@ -4296,7 +4299,7 @@ static int nf_tables_delrule(struct sk_buff *skb, const struct nfnl_info *info,
 
 	if (chain) {
 		if (nla[NFTA_RULE_HANDLE]) {
-			rule = nft_rule_lookup(chain, nla[NFTA_RULE_HANDLE]);
+			rule = nft_rule_lookup(info->net, chain, nla[NFTA_RULE_HANDLE]);
 			if (IS_ERR(rule)) {
 				if (PTR_ERR(rule) == -ENOENT &&
 				    NFNL_MSG_TYPE(info->nlh->nlmsg_type) == NFT_MSG_DESTROYRULE)
@@ -8101,7 +8104,7 @@ static int nf_tables_dump_obj_done(struct netlink_callback *cb)
 	return 0;
 }
 
-/* called with rcu_read_lock held */
+/* Caller must hold rcu read lock or transaction mutex */
 static struct sk_buff *
 nf_tables_getobj_single(u32 portid, const struct nfnl_info *info,
 			const struct nlattr * const nla[], bool reset)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 06/11] netfilter: nf_tables: avoid false-positive lockdep splats with sets
  2024-11-06 23:46 [PATCH net-next 00/11] Netfilter updates for net-next Pablo Neira Ayuso
                   ` (4 preceding siblings ...)
  2024-11-06 23:46 ` [PATCH net-next 05/11] netfilter: nf_tables: avoid false-positive lockdep splat on rule deletion Pablo Neira Ayuso
@ 2024-11-06 23:46 ` Pablo Neira Ayuso
  2024-11-06 23:46 ` [PATCH net-next 07/11] netfilter: nf_tables: avoid false-positive lockdep splats with flowtables Pablo Neira Ayuso
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2024-11-06 23:46 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet, fw

From: Florian Westphal <fw@strlen.de>

Same as previous patch.  All set handling functions here can be called
with transaction mutex held (but not the rcu read lock).

The transaction mutex prevents concurrent add/delete, so this is fine.

Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 80c285ac7e07..a51731d76401 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3986,7 +3986,8 @@ int nft_set_catchall_validate(const struct nft_ctx *ctx, struct nft_set *set)
 	struct nft_set_ext *ext;
 	int ret = 0;
 
-	list_for_each_entry_rcu(catchall, &set->catchall_list, list) {
+	list_for_each_entry_rcu(catchall, &set->catchall_list, list,
+				lockdep_commit_lock_is_held(ctx->net)) {
 		ext = nft_set_elem_ext(set, catchall->elem);
 		if (!nft_set_elem_active(ext, dummy_iter.genmask))
 			continue;
@@ -4459,7 +4460,8 @@ static const struct nla_policy nft_set_desc_policy[NFTA_SET_DESC_MAX + 1] = {
 	[NFTA_SET_DESC_CONCAT]		= NLA_POLICY_NESTED_ARRAY(nft_concat_policy),
 };
 
-static struct nft_set *nft_set_lookup(const struct nft_table *table,
+static struct nft_set *nft_set_lookup(const struct net *net,
+				      const struct nft_table *table,
 				      const struct nlattr *nla, u8 genmask)
 {
 	struct nft_set *set;
@@ -4467,7 +4469,8 @@ static struct nft_set *nft_set_lookup(const struct nft_table *table,
 	if (nla == NULL)
 		return ERR_PTR(-EINVAL);
 
-	list_for_each_entry_rcu(set, &table->sets, list) {
+	list_for_each_entry_rcu(set, &table->sets, list,
+				lockdep_commit_lock_is_held(net)) {
 		if (!nla_strcmp(nla, set->name) &&
 		    nft_active_genmask(set, genmask))
 			return set;
@@ -4517,7 +4520,7 @@ struct nft_set *nft_set_lookup_global(const struct net *net,
 {
 	struct nft_set *set;
 
-	set = nft_set_lookup(table, nla_set_name, genmask);
+	set = nft_set_lookup(net, table, nla_set_name, genmask);
 	if (IS_ERR(set)) {
 		if (!nla_set_id)
 			return set;
@@ -4893,7 +4896,7 @@ static int nf_tables_getset(struct sk_buff *skb, const struct nfnl_info *info,
 	if (!nla[NFTA_SET_TABLE])
 		return -EINVAL;
 
-	set = nft_set_lookup(table, nla[NFTA_SET_NAME], genmask);
+	set = nft_set_lookup(net, table, nla[NFTA_SET_NAME], genmask);
 	if (IS_ERR(set)) {
 		NL_SET_BAD_ATTR(extack, nla[NFTA_SET_NAME]);
 		return PTR_ERR(set);
@@ -5229,7 +5232,7 @@ static int nf_tables_newset(struct sk_buff *skb, const struct nfnl_info *info,
 
 	nft_ctx_init(&ctx, net, skb, info->nlh, family, table, NULL, nla);
 
-	set = nft_set_lookup(table, nla[NFTA_SET_NAME], genmask);
+	set = nft_set_lookup(net, table, nla[NFTA_SET_NAME], genmask);
 	if (IS_ERR(set)) {
 		if (PTR_ERR(set) != -ENOENT) {
 			NL_SET_BAD_ATTR(extack, nla[NFTA_SET_NAME]);
@@ -5431,7 +5434,7 @@ static int nf_tables_delset(struct sk_buff *skb, const struct nfnl_info *info,
 		set = nft_set_lookup_byhandle(table, attr, genmask);
 	} else {
 		attr = nla[NFTA_SET_NAME];
-		set = nft_set_lookup(table, attr, genmask);
+		set = nft_set_lookup(net, table, attr, genmask);
 	}
 
 	if (IS_ERR(set)) {
@@ -5495,7 +5498,8 @@ static int nft_set_catchall_bind_check(const struct nft_ctx *ctx,
 	struct nft_set_ext *ext;
 	int ret = 0;
 
-	list_for_each_entry_rcu(catchall, &set->catchall_list, list) {
+	list_for_each_entry_rcu(catchall, &set->catchall_list, list,
+				lockdep_commit_lock_is_held(ctx->net)) {
 		ext = nft_set_elem_ext(set, catchall->elem);
 		if (!nft_set_elem_active(ext, genmask))
 			continue;
@@ -6261,7 +6265,7 @@ static int nft_set_dump_ctx_init(struct nft_set_dump_ctx *dump_ctx,
 		return PTR_ERR(table);
 	}
 
-	set = nft_set_lookup(table, nla[NFTA_SET_ELEM_LIST_SET], genmask);
+	set = nft_set_lookup(net, table, nla[NFTA_SET_ELEM_LIST_SET], genmask);
 	if (IS_ERR(set)) {
 		NL_SET_BAD_ATTR(extack, nla[NFTA_SET_ELEM_LIST_SET]);
 		return PTR_ERR(set);
@@ -7493,7 +7497,8 @@ static int nft_set_catchall_flush(const struct nft_ctx *ctx,
 	struct nft_set_ext *ext;
 	int ret = 0;
 
-	list_for_each_entry_rcu(catchall, &set->catchall_list, list) {
+	list_for_each_entry_rcu(catchall, &set->catchall_list, list,
+				lockdep_commit_lock_is_held(ctx->net)) {
 		ext = nft_set_elem_ext(set, catchall->elem);
 		if (!nft_set_elem_active(ext, genmask))
 			continue;
@@ -7543,7 +7548,7 @@ static int nf_tables_delsetelem(struct sk_buff *skb,
 		return PTR_ERR(table);
 	}
 
-	set = nft_set_lookup(table, nla[NFTA_SET_ELEM_LIST_SET], genmask);
+	set = nft_set_lookup(net, table, nla[NFTA_SET_ELEM_LIST_SET], genmask);
 	if (IS_ERR(set)) {
 		NL_SET_BAD_ATTR(extack, nla[NFTA_SET_ELEM_LIST_SET]);
 		return PTR_ERR(set);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 07/11] netfilter: nf_tables: avoid false-positive lockdep splats with flowtables
  2024-11-06 23:46 [PATCH net-next 00/11] Netfilter updates for net-next Pablo Neira Ayuso
                   ` (5 preceding siblings ...)
  2024-11-06 23:46 ` [PATCH net-next 06/11] netfilter: nf_tables: avoid false-positive lockdep splats with sets Pablo Neira Ayuso
@ 2024-11-06 23:46 ` Pablo Neira Ayuso
  2024-11-06 23:46 ` [PATCH net-next 08/11] netfilter: nf_tables: avoid false-positive lockdep splats in set walker Pablo Neira Ayuso
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2024-11-06 23:46 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet, fw

From: Florian Westphal <fw@strlen.de>

The transaction mutex prevents concurrent add/delete, its ok to iterate
those lists outside of rcu read side critical sections.

Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_tables.h |  3 ++-
 net/netfilter/nf_tables_api.c     | 15 +++++++++------
 net/netfilter/nft_flow_offload.c  |  4 ++--
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 91ae20cb7648..c1513bd14568 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1463,7 +1463,8 @@ struct nft_flowtable {
 	struct nf_flowtable		data;
 };
 
-struct nft_flowtable *nft_flowtable_lookup(const struct nft_table *table,
+struct nft_flowtable *nft_flowtable_lookup(const struct net *net,
+					   const struct nft_table *table,
 					   const struct nlattr *nla,
 					   u8 genmask);
 
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index a51731d76401..9e367e134691 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -8378,12 +8378,14 @@ static const struct nla_policy nft_flowtable_policy[NFTA_FLOWTABLE_MAX + 1] = {
 	[NFTA_FLOWTABLE_FLAGS]		= { .type = NLA_U32 },
 };
 
-struct nft_flowtable *nft_flowtable_lookup(const struct nft_table *table,
+struct nft_flowtable *nft_flowtable_lookup(const struct net *net,
+					   const struct nft_table *table,
 					   const struct nlattr *nla, u8 genmask)
 {
 	struct nft_flowtable *flowtable;
 
-	list_for_each_entry_rcu(flowtable, &table->flowtables, list) {
+	list_for_each_entry_rcu(flowtable, &table->flowtables, list,
+				lockdep_commit_lock_is_held(net)) {
 		if (!nla_strcmp(nla, flowtable->name) &&
 		    nft_active_genmask(flowtable, genmask))
 			return flowtable;
@@ -8739,7 +8741,7 @@ static int nf_tables_newflowtable(struct sk_buff *skb,
 		return PTR_ERR(table);
 	}
 
-	flowtable = nft_flowtable_lookup(table, nla[NFTA_FLOWTABLE_NAME],
+	flowtable = nft_flowtable_lookup(net, table, nla[NFTA_FLOWTABLE_NAME],
 					 genmask);
 	if (IS_ERR(flowtable)) {
 		err = PTR_ERR(flowtable);
@@ -8933,7 +8935,7 @@ static int nf_tables_delflowtable(struct sk_buff *skb,
 		flowtable = nft_flowtable_lookup_byhandle(table, attr, genmask);
 	} else {
 		attr = nla[NFTA_FLOWTABLE_NAME];
-		flowtable = nft_flowtable_lookup(table, attr, genmask);
+		flowtable = nft_flowtable_lookup(net, table, attr, genmask);
 	}
 
 	if (IS_ERR(flowtable)) {
@@ -9003,7 +9005,8 @@ static int nf_tables_fill_flowtable_info(struct sk_buff *skb, struct net *net,
 	if (!hook_list)
 		hook_list = &flowtable->hook_list;
 
-	list_for_each_entry_rcu(hook, hook_list, list) {
+	list_for_each_entry_rcu(hook, hook_list, list,
+				lockdep_commit_lock_is_held(net)) {
 		if (nla_put_string(skb, NFTA_DEVICE_NAME, hook->ops.dev->name))
 			goto nla_put_failure;
 	}
@@ -9145,7 +9148,7 @@ static int nf_tables_getflowtable(struct sk_buff *skb,
 		return PTR_ERR(table);
 	}
 
-	flowtable = nft_flowtable_lookup(table, nla[NFTA_FLOWTABLE_NAME],
+	flowtable = nft_flowtable_lookup(net, table, nla[NFTA_FLOWTABLE_NAME],
 					 genmask);
 	if (IS_ERR(flowtable)) {
 		NL_SET_BAD_ATTR(extack, nla[NFTA_FLOWTABLE_NAME]);
diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
index 2f732fae5a83..65199c23c75c 100644
--- a/net/netfilter/nft_flow_offload.c
+++ b/net/netfilter/nft_flow_offload.c
@@ -409,8 +409,8 @@ static int nft_flow_offload_init(const struct nft_ctx *ctx,
 	if (!tb[NFTA_FLOW_TABLE_NAME])
 		return -EINVAL;
 
-	flowtable = nft_flowtable_lookup(ctx->table, tb[NFTA_FLOW_TABLE_NAME],
-					 genmask);
+	flowtable = nft_flowtable_lookup(ctx->net, ctx->table,
+					 tb[NFTA_FLOW_TABLE_NAME], genmask);
 	if (IS_ERR(flowtable))
 		return PTR_ERR(flowtable);
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 08/11] netfilter: nf_tables: avoid false-positive lockdep splats in set walker
  2024-11-06 23:46 [PATCH net-next 00/11] Netfilter updates for net-next Pablo Neira Ayuso
                   ` (6 preceding siblings ...)
  2024-11-06 23:46 ` [PATCH net-next 07/11] netfilter: nf_tables: avoid false-positive lockdep splats with flowtables Pablo Neira Ayuso
@ 2024-11-06 23:46 ` Pablo Neira Ayuso
  2024-11-06 23:46 ` [PATCH net-next 09/11] netfilter: nf_tables: avoid false-positive lockdep splats with basechain hook Pablo Neira Ayuso
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2024-11-06 23:46 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet, fw

From: Florian Westphal <fw@strlen.de>

Its not possible to add or delete elements from hash and bitmap sets,
as long as caller is holding the transaction mutex, so its ok to iterate
the list outside of rcu read side critical section.

Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_set_bitmap.c | 10 ++++++----
 net/netfilter/nft_set_hash.c   |  3 ++-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nft_set_bitmap.c b/net/netfilter/nft_set_bitmap.c
index 1caa04619dc6..12390d2e994f 100644
--- a/net/netfilter/nft_set_bitmap.c
+++ b/net/netfilter/nft_set_bitmap.c
@@ -88,13 +88,15 @@ bool nft_bitmap_lookup(const struct net *net, const struct nft_set *set,
 }
 
 static struct nft_bitmap_elem *
-nft_bitmap_elem_find(const struct nft_set *set, struct nft_bitmap_elem *this,
+nft_bitmap_elem_find(const struct net *net,
+		     const struct nft_set *set, struct nft_bitmap_elem *this,
 		     u8 genmask)
 {
 	const struct nft_bitmap *priv = nft_set_priv(set);
 	struct nft_bitmap_elem *be;
 
-	list_for_each_entry_rcu(be, &priv->list, head) {
+	list_for_each_entry_rcu(be, &priv->list, head,
+				lockdep_is_held(&nft_pernet(net)->commit_mutex)) {
 		if (memcmp(nft_set_ext_key(&be->ext),
 			   nft_set_ext_key(&this->ext), set->klen) ||
 		    !nft_set_elem_active(&be->ext, genmask))
@@ -132,7 +134,7 @@ static int nft_bitmap_insert(const struct net *net, const struct nft_set *set,
 	u8 genmask = nft_genmask_next(net);
 	u32 idx, off;
 
-	be = nft_bitmap_elem_find(set, new, genmask);
+	be = nft_bitmap_elem_find(net, set, new, genmask);
 	if (be) {
 		*elem_priv = &be->priv;
 		return -EEXIST;
@@ -201,7 +203,7 @@ nft_bitmap_deactivate(const struct net *net, const struct nft_set *set,
 
 	nft_bitmap_location(set, elem->key.val.data, &idx, &off);
 
-	be = nft_bitmap_elem_find(set, this, genmask);
+	be = nft_bitmap_elem_find(net, set, this, genmask);
 	if (!be)
 		return NULL;
 
diff --git a/net/netfilter/nft_set_hash.c b/net/netfilter/nft_set_hash.c
index daa56dda737a..65bd291318f2 100644
--- a/net/netfilter/nft_set_hash.c
+++ b/net/netfilter/nft_set_hash.c
@@ -647,7 +647,8 @@ static void nft_hash_walk(const struct nft_ctx *ctx, struct nft_set *set,
 	int i;
 
 	for (i = 0; i < priv->buckets; i++) {
-		hlist_for_each_entry_rcu(he, &priv->table[i], node) {
+		hlist_for_each_entry_rcu(he, &priv->table[i], node,
+					 lockdep_is_held(&nft_pernet(ctx->net)->commit_mutex)) {
 			if (iter->count < iter->skip)
 				goto cont;
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 09/11] netfilter: nf_tables: avoid false-positive lockdep splats with basechain hook
  2024-11-06 23:46 [PATCH net-next 00/11] Netfilter updates for net-next Pablo Neira Ayuso
                   ` (7 preceding siblings ...)
  2024-11-06 23:46 ` [PATCH net-next 08/11] netfilter: nf_tables: avoid false-positive lockdep splats in set walker Pablo Neira Ayuso
@ 2024-11-06 23:46 ` Pablo Neira Ayuso
  2024-11-06 23:46 ` [PATCH net-next 10/11] netfilter: nf_tables: must hold rcu read lock while iterating expression type list Pablo Neira Ayuso
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2024-11-06 23:46 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet, fw

From: Florian Westphal <fw@strlen.de>

Like previous patches: iteration is ok if the list cannot be altered in
parallel.

Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 9e367e134691..3b5154f2dd79 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1824,7 +1824,8 @@ static int nft_dump_stats(struct sk_buff *skb, struct nft_stats __percpu *stats)
 	return -ENOSPC;
 }
 
-static int nft_dump_basechain_hook(struct sk_buff *skb, int family,
+static int nft_dump_basechain_hook(struct sk_buff *skb,
+				   const struct net *net, int family,
 				   const struct nft_base_chain *basechain,
 				   const struct list_head *hook_list)
 {
@@ -1849,7 +1850,8 @@ static int nft_dump_basechain_hook(struct sk_buff *skb, int family,
 		if (!hook_list)
 			hook_list = &basechain->hook_list;
 
-		list_for_each_entry_rcu(hook, hook_list, list) {
+		list_for_each_entry_rcu(hook, hook_list, list,
+					lockdep_commit_lock_is_held(net)) {
 			if (!first)
 				first = hook;
 
@@ -1900,7 +1902,7 @@ static int nf_tables_fill_chain_info(struct sk_buff *skb, struct net *net,
 		const struct nft_base_chain *basechain = nft_base_chain(chain);
 		struct nft_stats __percpu *stats;
 
-		if (nft_dump_basechain_hook(skb, family, basechain, hook_list))
+		if (nft_dump_basechain_hook(skb, net, family, basechain, hook_list))
 			goto nla_put_failure;
 
 		if (nla_put_be32(skb, NFTA_CHAIN_POLICY,
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 10/11] netfilter: nf_tables: must hold rcu read lock while iterating expression type list
  2024-11-06 23:46 [PATCH net-next 00/11] Netfilter updates for net-next Pablo Neira Ayuso
                   ` (8 preceding siblings ...)
  2024-11-06 23:46 ` [PATCH net-next 09/11] netfilter: nf_tables: avoid false-positive lockdep splats with basechain hook Pablo Neira Ayuso
@ 2024-11-06 23:46 ` Pablo Neira Ayuso
  2024-11-06 23:46 ` [PATCH net-next 11/11] netfilter: nf_tables: must hold rcu read lock while iterating object " Pablo Neira Ayuso
  2024-11-07  0:19 ` [PATCH net-next 00/11] Netfilter updates for net-next Jakub Kicinski
  11 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2024-11-06 23:46 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet, fw

From: Florian Westphal <fw@strlen.de>

nft shell tests trigger:
 WARNING: suspicious RCU usage
 net/netfilter/nf_tables_api.c:3125 RCU-list traversed in non-reader section!!
 1 lock held by nft/2068:
  #0: ffff888106c6f8c8 (&nft_net->commit_mutex){+.+.}-{4:4}, at: nf_tables_valid_genid+0x3c/0xf0

But the transaction mutex doesn't protect this list, the nfnl subsystem
mutex would, but we can't acquire it here without risk of ABBA
deadlocks.

Acquire the rcu read lock to avoid this issue.

v3: add a comment that explains the ->inner_ops check implies
expression is builtin and lack of a module owner reference is ok.

Fixes: 3a07327d10a0 ("netfilter: nft_inner: support for inner tunnel header matching")
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 3b5154f2dd79..de8e48a5c62d 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3296,25 +3296,37 @@ int nft_expr_inner_parse(const struct nft_ctx *ctx, const struct nlattr *nla,
 	if (!tb[NFTA_EXPR_DATA] || !tb[NFTA_EXPR_NAME])
 		return -EINVAL;
 
+	rcu_read_lock();
+
 	type = __nft_expr_type_get(ctx->family, tb[NFTA_EXPR_NAME]);
-	if (!type)
-		return -ENOENT;
+	if (!type) {
+		err = -ENOENT;
+		goto out_unlock;
+	}
 
-	if (!type->inner_ops)
-		return -EOPNOTSUPP;
+	if (!type->inner_ops) {
+		err = -EOPNOTSUPP;
+		goto out_unlock;
+	}
 
 	err = nla_parse_nested_deprecated(info->tb, type->maxattr,
 					  tb[NFTA_EXPR_DATA],
 					  type->policy, NULL);
 	if (err < 0)
-		goto err_nla_parse;
+		goto out_unlock;
 
 	info->attr = nla;
 	info->ops = type->inner_ops;
 
+	/* No module reference will be taken on type->owner.
+	 * Presence of type->inner_ops implies that the expression
+	 * is builtin, so it cannot go away.
+	 */
+	rcu_read_unlock();
 	return 0;
 
-err_nla_parse:
+out_unlock:
+	rcu_read_unlock();
 	return err;
 }
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 11/11] netfilter: nf_tables: must hold rcu read lock while iterating object type list
  2024-11-06 23:46 [PATCH net-next 00/11] Netfilter updates for net-next Pablo Neira Ayuso
                   ` (9 preceding siblings ...)
  2024-11-06 23:46 ` [PATCH net-next 10/11] netfilter: nf_tables: must hold rcu read lock while iterating expression type list Pablo Neira Ayuso
@ 2024-11-06 23:46 ` Pablo Neira Ayuso
  2024-11-07  0:19 ` [PATCH net-next 00/11] Netfilter updates for net-next Jakub Kicinski
  11 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2024-11-06 23:46 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet, fw

From: Florian Westphal <fw@strlen.de>

Update of stateful object triggers:
WARNING: suspicious RCU usage
net/netfilter/nf_tables_api.c:7759 RCU-list traversed in non-reader section!!

other info that might help us debug this:
rcu_scheduler_active = 2, debug_locks = 1
1 lock held by nft/3060:
 #0: ffff88810f0578c8 (&nft_net->commit_mutex){+.+.}-{4:4}, [..]

... but this list is not protected by the transaction mutex but the
nfnl nftables subsystem mutex.

Switch to nft_obj_type_get which will acquire rcu read lock,
bump refcount, and returns the result.

v3: Dan Carpenter points out nft_obj_type_get returns error pointer, not
NULL, on error.

Fixes: dad3bdeef45f ("netfilter: nf_tables: fix memory leak during stateful obj update").
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index de8e48a5c62d..b7a817e483aa 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -7809,9 +7809,7 @@ static int nf_tables_updobj(const struct nft_ctx *ctx,
 	struct nft_trans *trans;
 	int err = -ENOMEM;
 
-	if (!try_module_get(type->owner))
-		return -ENOENT;
-
+	/* caller must have obtained type->owner reference. */
 	trans = nft_trans_alloc(ctx, NFT_MSG_NEWOBJ,
 				sizeof(struct nft_trans_obj));
 	if (!trans)
@@ -7879,15 +7877,16 @@ static int nf_tables_newobj(struct sk_buff *skb, const struct nfnl_info *info,
 		if (info->nlh->nlmsg_flags & NLM_F_REPLACE)
 			return -EOPNOTSUPP;
 
-		type = __nft_obj_type_get(objtype, family);
-		if (WARN_ON_ONCE(!type))
-			return -ENOENT;
-
 		if (!obj->ops->update)
 			return 0;
 
+		type = nft_obj_type_get(net, objtype, family);
+		if (WARN_ON_ONCE(IS_ERR(type)))
+			return PTR_ERR(type);
+
 		nft_ctx_init(&ctx, net, skb, info->nlh, family, table, NULL, nla);
 
+		/* type->owner reference is put when transaction object is released. */
 		return nf_tables_updobj(&ctx, type, nla[NFTA_OBJ_DATA], obj);
 	}
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 00/11] Netfilter updates for net-next
  2024-11-06 23:46 [PATCH net-next 00/11] Netfilter updates for net-next Pablo Neira Ayuso
                   ` (10 preceding siblings ...)
  2024-11-06 23:46 ` [PATCH net-next 11/11] netfilter: nf_tables: must hold rcu read lock while iterating object " Pablo Neira Ayuso
@ 2024-11-07  0:19 ` Jakub Kicinski
  2024-11-07  7:08   ` Florian Westphal
  11 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2024-11-07  0:19 UTC (permalink / raw)
  To: Pablo Neira Ayuso, fw; +Cc: netfilter-devel, davem, netdev, pabeni, edumazet

On Thu,  7 Nov 2024 00:46:14 +0100 Pablo Neira Ayuso wrote:
> "Unfortunately there are many more errors, and not all are false positives.

Thanks a lot for jumping on fixing the CONFIG_RCU_LIST=y splats!
To clarify should the selftests be splat-free now or there is more
work required to get there?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 00/11] Netfilter updates for net-next
  2024-11-07  0:19 ` [PATCH net-next 00/11] Netfilter updates for net-next Jakub Kicinski
@ 2024-11-07  7:08   ` Florian Westphal
  2024-11-07 20:48     ` Jakub Kicinski
  0 siblings, 1 reply; 18+ messages in thread
From: Florian Westphal @ 2024-11-07  7:08 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Pablo Neira Ayuso, fw, netfilter-devel, davem, netdev, pabeni,
	edumazet

Jakub Kicinski <kuba@kernel.org> wrote:
> On Thu,  7 Nov 2024 00:46:14 +0100 Pablo Neira Ayuso wrote:
> > "Unfortunately there are many more errors, and not all are false positives.
> 
> Thanks a lot for jumping on fixing the CONFIG_RCU_LIST=y splats!
> To clarify should the selftests be splat-free now or there is more
> work required to get there?

I tried to repro last week on net-next (not nf-next!) + v2 of these patches
and I did not see splats, but I'll re-run everything later today to make
sure they've been fixed up.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 01/11] netfilter: Make legacy configs user selectable
  2024-11-06 23:46 ` [PATCH net-next 01/11] netfilter: Make legacy configs user selectable Pablo Neira Ayuso
@ 2024-11-07 12:00   ` patchwork-bot+netdevbpf
  0 siblings, 0 replies; 18+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-11-07 12:00 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: netfilter-devel, davem, netdev, kuba, pabeni, edumazet, fw

Hello:

This series was applied to netdev/net-next.git (main)
by Pablo Neira Ayuso <pablo@netfilter.org>:

On Thu,  7 Nov 2024 00:46:15 +0100 you wrote:
> From: Breno Leitao <leitao@debian.org>
> 
> This option makes legacy Netfilter Kconfig user selectable, giving users
> the option to configure iptables without enabling any other config.
> 
> Make the following KConfig entries user selectable:
>  * BRIDGE_NF_EBTABLES_LEGACY
>  * IP_NF_ARPTABLES
>  * IP_NF_IPTABLES_LEGACY
>  * IP6_NF_IPTABLES_LEGACY
> 
> [...]

Here is the summary with links:
  - [net-next,01/11] netfilter: Make legacy configs user selectable
    https://git.kernel.org/netdev/net-next/c/6c959fd5e173
  - [net-next,02/11] netfilter: nf_tables: Fix percpu address space issues in nf_tables_api.c
    https://git.kernel.org/netdev/net-next/c/0741f5559354
  - [net-next,03/11] netfilter: nf_tables: replace deprecated strncpy with strscpy_pad
    https://git.kernel.org/netdev/net-next/c/544dded8cb63
  - [net-next,04/11] netfilter: nf_tables: prefer nft_trans_elem_alloc helper
    https://git.kernel.org/netdev/net-next/c/08e52cccae11
  - [net-next,05/11] netfilter: nf_tables: avoid false-positive lockdep splat on rule deletion
    https://git.kernel.org/netdev/net-next/c/9adbb4198bf6
  - [net-next,06/11] netfilter: nf_tables: avoid false-positive lockdep splats with sets
    https://git.kernel.org/netdev/net-next/c/8f5f3786dba7
  - [net-next,07/11] netfilter: nf_tables: avoid false-positive lockdep splats with flowtables
    https://git.kernel.org/netdev/net-next/c/b3e8f29d6b45
  - [net-next,08/11] netfilter: nf_tables: avoid false-positive lockdep splats in set walker
    https://git.kernel.org/netdev/net-next/c/28b7a6b84c0a
  - [net-next,09/11] netfilter: nf_tables: avoid false-positive lockdep splats with basechain hook
    https://git.kernel.org/netdev/net-next/c/3567146b94af
  - [net-next,10/11] netfilter: nf_tables: must hold rcu read lock while iterating expression type list
    https://git.kernel.org/netdev/net-next/c/ee666a541ed9
  - [net-next,11/11] netfilter: nf_tables: must hold rcu read lock while iterating object type list
    https://git.kernel.org/netdev/net-next/c/cddc04275f95

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 00/11] Netfilter updates for net-next
  2024-11-07  7:08   ` Florian Westphal
@ 2024-11-07 20:48     ` Jakub Kicinski
  2024-11-07 21:07       ` Florian Westphal
  2024-11-07 21:09       ` Eric Dumazet
  0 siblings, 2 replies; 18+ messages in thread
From: Jakub Kicinski @ 2024-11-07 20:48 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Pablo Neira Ayuso, netfilter-devel, davem, netdev, pabeni,
	edumazet

On Thu, 7 Nov 2024 08:08:34 +0100 Florian Westphal wrote:
> Jakub Kicinski <kuba@kernel.org> wrote:
> > On Thu,  7 Nov 2024 00:46:14 +0100 Pablo Neira Ayuso wrote:  
> > > "Unfortunately there are many more errors, and not all are false positives.  
> > 
> > Thanks a lot for jumping on fixing the CONFIG_RCU_LIST=y splats!
> > To clarify should the selftests be splat-free now or there is more
> > work required to get there?  
> 
> I tried to repro last week on net-next (not nf-next!) + v2 of these patches
> and I did not see splats, but I'll re-run everything later today to make
> sure they've been fixed up.

Great! I was double checking if you know of any selftest-triggered
problems before I re-enable that config in our CI.

I flipped it back on few hours ago and looks like it's only hitting
mcast routing and sctp bugs we already know about, so all good :)

Thanks again!

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 00/11] Netfilter updates for net-next
  2024-11-07 20:48     ` Jakub Kicinski
@ 2024-11-07 21:07       ` Florian Westphal
  2024-11-07 21:09       ` Eric Dumazet
  1 sibling, 0 replies; 18+ messages in thread
From: Florian Westphal @ 2024-11-07 21:07 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Florian Westphal, Pablo Neira Ayuso, netfilter-devel, davem,
	netdev, pabeni, edumazet

Jakub Kicinski <kuba@kernel.org> wrote:
> > I tried to repro last week on net-next (not nf-next!) + v2 of these patches
> > and I did not see splats, but I'll re-run everything later today to make
> > sure they've been fixed up.
> 
> Great! I was double checking if you know of any selftest-triggered
> problems before I re-enable that config in our CI.

The only splat I saw today on re-run is in kernel/events/core.c, but
Matthieu Baerts tells me there is a fix pending for it.

> I flipped it back on few hours ago and looks like it's only hitting
> mcast routing and sctp bugs we already know about, so all good :)

Great.  It finds real bugs so its good that it can be turned on again
to catch future issues.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 00/11] Netfilter updates for net-next
  2024-11-07 20:48     ` Jakub Kicinski
  2024-11-07 21:07       ` Florian Westphal
@ 2024-11-07 21:09       ` Eric Dumazet
  1 sibling, 0 replies; 18+ messages in thread
From: Eric Dumazet @ 2024-11-07 21:09 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Florian Westphal, Pablo Neira Ayuso, netfilter-devel, davem,
	netdev, pabeni

On Thu, Nov 7, 2024 at 9:48 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 7 Nov 2024 08:08:34 +0100 Florian Westphal wrote:
> > Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Thu,  7 Nov 2024 00:46:14 +0100 Pablo Neira Ayuso wrote:
> > > > "Unfortunately there are many more errors, and not all are false positives.
> > >
> > > Thanks a lot for jumping on fixing the CONFIG_RCU_LIST=y splats!
> > > To clarify should the selftests be splat-free now or there is more
> > > work required to get there?
> >
> > I tried to repro last week on net-next (not nf-next!) + v2 of these patches
> > and I did not see splats, but I'll re-run everything later today to make
> > sure they've been fixed up.
>
> Great! I was double checking if you know of any selftest-triggered
> problems before I re-enable that config in our CI.
>
> I flipped it back on few hours ago and looks like it's only hitting
> mcast routing and sctp bugs we already know about, so all good :)
>

sctp fix :

https://patchwork.kernel.org/project/netdevbpf/patch/20241107192021.2579789-1-edumazet@google.com/

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2024-11-07 21:09 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-06 23:46 [PATCH net-next 00/11] Netfilter updates for net-next Pablo Neira Ayuso
2024-11-06 23:46 ` [PATCH net-next 01/11] netfilter: Make legacy configs user selectable Pablo Neira Ayuso
2024-11-07 12:00   ` patchwork-bot+netdevbpf
2024-11-06 23:46 ` [PATCH net-next 02/11] netfilter: nf_tables: Fix percpu address space issues in nf_tables_api.c Pablo Neira Ayuso
2024-11-06 23:46 ` [PATCH net-next 03/11] netfilter: nf_tables: replace deprecated strncpy with strscpy_pad Pablo Neira Ayuso
2024-11-06 23:46 ` [PATCH net-next 04/11] netfilter: nf_tables: prefer nft_trans_elem_alloc helper Pablo Neira Ayuso
2024-11-06 23:46 ` [PATCH net-next 05/11] netfilter: nf_tables: avoid false-positive lockdep splat on rule deletion Pablo Neira Ayuso
2024-11-06 23:46 ` [PATCH net-next 06/11] netfilter: nf_tables: avoid false-positive lockdep splats with sets Pablo Neira Ayuso
2024-11-06 23:46 ` [PATCH net-next 07/11] netfilter: nf_tables: avoid false-positive lockdep splats with flowtables Pablo Neira Ayuso
2024-11-06 23:46 ` [PATCH net-next 08/11] netfilter: nf_tables: avoid false-positive lockdep splats in set walker Pablo Neira Ayuso
2024-11-06 23:46 ` [PATCH net-next 09/11] netfilter: nf_tables: avoid false-positive lockdep splats with basechain hook Pablo Neira Ayuso
2024-11-06 23:46 ` [PATCH net-next 10/11] netfilter: nf_tables: must hold rcu read lock while iterating expression type list Pablo Neira Ayuso
2024-11-06 23:46 ` [PATCH net-next 11/11] netfilter: nf_tables: must hold rcu read lock while iterating object " Pablo Neira Ayuso
2024-11-07  0:19 ` [PATCH net-next 00/11] Netfilter updates for net-next Jakub Kicinski
2024-11-07  7:08   ` Florian Westphal
2024-11-07 20:48     ` Jakub Kicinski
2024-11-07 21:07       ` Florian Westphal
2024-11-07 21:09       ` Eric Dumazet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).