[PATCH net 00/10] Netfilter/IPVS fixes for net

[PATCH net 00/10] Netfilter/IPVS fixes for net

[Pablo Neira Ayuso]

Hi,

The following patchset contains Netfilter/IPVS fixes net, this
includes fixes for ebtables nflog target, ipset hash type,
IPVS kthread estimator 

1) Prevent IPVS kthread estimator from draining the est_temp_list
   when netns is being dismantled. From Zhiling Zou.

2) Missing module nflog refcount bump from ebtables nflog target from
   .checkentry path. Similar dependency exists already in xt_NFLOG and
   nft_log. From Chengfeng Ye.

3) Use RCU to fix ipset bookkeeping of cidr values on weakly-ordered
   architectures. From Jozsef Kadlecsik.

4) Use atomic64_t for set->ext_size in ipset to fix parallel inserts
   and deletes racing on updating it. From Jozsef Kadlecsik.

5) Add small wrappers for hash and bucket size to prepare the update
   of ipset hash set types to rhashtable, from Florian Westphal.

6) Add mtype_del_cidr_all() and use it to prepare the migration of
   ipset hash types to rhashtable. From Florian Westphal.

7) Replace existing ipset call_rcu() based destruction with rcu_work
   api also to ease the transition to rhashtable. Also from Florian.

8) Avoid reading the IPv4 ihl field multiple times to prevent local
   attacker to cause out-of-bounds write in ip_vs_nat_icmp(), from
   Julian Anastasov.
 
9) Restore the checksum validations that could be needed by the IPVS
   FORWARD hook. Also from Julian.

10) Move custom ct expectation support to a helper. This is to address
    a report of possible reallocation of the ct extension while the
    expectations list contains entries, leading to stale .pprev.

Sashiko reports that more follow ups are needed for the ipset patches as
well as the ipset checksum fixes this is already known, but we consider
that this is improving the situation and those can be addressed
incrementally. Regarding patch #10, sashiko points to pre-existing
issues in this custom ct expectation feature that I plan to address in
net-next.

When merging this PR into net.git, there will be a conflict between
net.git and net-next.git related to patch #10 and d4beefc90a66
("netfilter: nft_ct: support expectation creation for natted flows")
in net-next, that can be address with the following patch:

diff --cc net/netfilter/nft_ct.c
index 30c9358dbf48,358b9287e12e..339e0c98fd70
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@@ -1228,73 -1226,6 +1228,91 @@@ static int nft_ct_expect_timeout_get(co
  	return 0;
  }
  
++#if IS_ENABLED(CONFIG_NF_NAT)
++static void nft_ct_nat_follow_master(struct nf_conn *ct, struct nf_conntrack_expect *this)
++{
++	const struct nf_ct_helper_expectfn *expfn;
++
++	expfn = nf_ct_helper_expectfn_find_by_name("nat-follow-master");
++	if (expfn)
++		expfn->expectfn(ct, this);
++}
++#endif
++
 +struct nft_ct_expect_data {
 +	struct nft_ct_expect_obj	obj;
 +	enum ip_conntrack_dir		dir;
 +	atomic_t			num_expects;
 +};
 +
 +static int ct_expect_help(struct sk_buff *skb, unsigned int protoff,
 +			  struct nf_conn *ct, enum ip_conntrack_info ctinfo)
 +{
 +	enum ip_conntrack_dir dir = CTINFO2DIR(ctinfo);
 +	struct nft_ct_expect_data *expect_data;
 +	struct nf_conntrack_expect *exp;
 +	int ret = NF_ACCEPT;
 +
 +	expect_data = nfct_help_data(ct);
 +	if (!expect_data)
 +		return NF_ACCEPT;
 +
 +	if (expect_data->dir != dir)
 +		return NF_ACCEPT;
 +
 +	if (!atomic_add_unless(&expect_data->num_expects, 1, expect_data->obj.size))
 +		return NF_ACCEPT;
 +
 +	exp = nf_ct_expect_alloc(ct);
 +	if (!exp) {
 +		atomic_dec(&expect_data->num_expects);
 +		return NF_DROP;
 +	}
 +
 +	nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT, nf_ct_l3num(ct),
 +			  &ct->tuplehash[!dir].tuple.src.u3,
 +			  &ct->tuplehash[!dir].tuple.dst.u3,
 +			  expect_data->obj.l4proto, NULL, &expect_data->obj.dport);
 +	exp->timeout += expect_data->obj.timeout;
 +
++#if IS_ENABLED(CONFIG_NF_NAT)
++	if (ct->status & IPS_NAT_MASK) {
++		exp->saved_proto.tcp.port = expect_data->obj.dport;
++		exp->dir = !dir;
++		exp->expectfn = nft_ct_nat_follow_master;
++	}
++#endif
 +	if (nf_ct_expect_related(exp, 0) != 0) {
 +		atomic_dec(&expect_data->num_expects);
 +		ret = NF_DROP;
 +	}
 +
 +	nf_ct_expect_put(exp);
 +
 +	return ret;
 +}
 +
 +static int nft_ct_expect_helper_alloc(struct nft_ct_expect_obj *priv)
 +{
 +	struct nf_conntrack_helper *ct_expect_helper;
 +
 +	ct_expect_helper = kzalloc_obj(struct nf_conntrack_helper);
 +	if (!ct_expect_helper)
 +		return -ENOMEM;
 +
 +	snprintf(ct_expect_helper->name, sizeof(ct_expect_helper->name), "%s",
 +		 "nft_ct_expect");
 +	ct_expect_helper->me = THIS_MODULE;
 +	ct_expect_helper->expect_policy[NF_CT_EXPECT_CLASS_DEFAULT].max_expected = priv->size;
 +	rcu_assign_pointer(ct_expect_helper->help, ct_expect_help);
 +	refcount_set(&ct_expect_helper->ct_refcnt, 1);
 +
 +	/* No need to register this helper, this is internal. */
 +	priv->helper = ct_expect_helper;
 +
 +	return 0;
 +}
 +
  static int nft_ct_expect_obj_init(const struct nft_ctx *ctx,
  				  const struct nlattr * const tb[],
  				  struct nft_object *obj)


Apologies for the extra work that the conflict resolution brings,
I was planning to merge this fix to net-next but I just saw a second
reporter finding exactly the same bug in net.git, so I am inclined
towards expediting inclusion upstream of patch #10 in this series.

Please, pull these changes from:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git nf-26-07-31

Thanks.

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

The following changes since commit 2195424c3da2ef1829a63b807e3a900a90e57d85:

  net/x25: fix use-after-free of the socket by its timers (2026-07-30 18:46:45 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git tags/nf-26-07-31

for you to fetch changes up to 0b541313c8a8ee49ab8bb1f620204720a83eb7fd:

  netfilter: nft_ct: move custom expectation support to helper (2026-07-31 16:46:55 +0200)

----------------------------------------------------------------
netfilter pull request 26-07-31

----------------------------------------------------------------
Chengfeng Ye (1):
      netfilter: ebt_nflog: pin the NFLOG backend

Florian Westphal (3):
      netfilter: ipset: add small wrappers for hash and bucket sizes
      netfilter: ipset: add and use mtype_del_cidr_all helper
      netfilter: ipset: switch to rcu work

Jozsef Kadlecsik (2):
      netfilter: ipset: rework cidr bookkeeping
      netfilter: ipset: switch ext_size to atomic64_t

Julian Anastasov (2):
      ipvs: avoid out-of-bounds write in ip_vs_nat_icmp
      ipvs: return the csum validation for forward hook

Pablo Neira Ayuso (1):
      netfilter: nft_ct: move custom expectation support to helper

Zhiling Zou (1):
      ipvs: stop estimator after disabled calc phase

 include/linux/netfilter/ipset/ip_set.h       |   6 +-
 include/net/ip_vs.h                          |  21 +-
 include/net/netfilter/nf_conntrack_helper.h  |   2 +
 net/bridge/netfilter/ebt_nflog.c             |  17 +-
 net/netfilter/ipset/ip_set_bitmap_gen.h      |   4 +-
 net/netfilter/ipset/ip_set_core.c            |  52 +++--
 net/netfilter/ipset/ip_set_hash_gen.h        | 304 ++++++++++++++++++---------
 net/netfilter/ipset/ip_set_hash_ipportnet.c  |   4 +-
 net/netfilter/ipset/ip_set_hash_net.c        |   4 +-
 net/netfilter/ipset/ip_set_hash_netiface.c   |   4 +-
 net/netfilter/ipset/ip_set_hash_netnet.c     |  12 +-
 net/netfilter/ipset/ip_set_hash_netport.c    |   4 +-
 net/netfilter/ipset/ip_set_hash_netportnet.c |  12 +-
 net/netfilter/ipset/ip_set_list_set.c        |   4 +-
 net/netfilter/ipvs/ip_vs_core.c              |  67 +++---
 net/netfilter/ipvs/ip_vs_est.c               |  10 +-
 net/netfilter/ipvs/ip_vs_proto_sctp.c        |   2 +-
 net/netfilter/ipvs/ip_vs_xmit.c              |   2 +-
 net/netfilter/nf_conntrack_helper.c          |  18 +-
 net/netfilter/nft_ct.c                       | 127 ++++++++---
 20 files changed, 445 insertions(+), 231 deletions(-)
d4beefc90a66 ("netfilter: nft_ct: support expectation creation for natted flows")

[PATCH net 01/10] ipvs: stop estimator after disabled calc phase

[Pablo Neira Ayuso]

From: Zhiling Zou <zhilinz@nebusec.ai>

IPVS estimator kthread 0 starts with zeroed chain and tick limits until
its initial calculation phase completes. If network namespace teardown
clears ipvs->enable during that phase, ip_vs_est_calc_phase() can return
without installing positive limits.

The kthread can then continue into its main loop and drain
est_temp_list with zero chain_max, tick_max and est_max_count values.
Each enqueue consumes one available tick row, but est_count never
reaches the zero est_max_count value. After all rows are consumed, the
row lookup returns IPVS_EST_NTICKS and ip_vs_enqueue_estimator() writes
past the ticks and tick_len arrays.

Exit kthread 0 after the calculation phase if the kthread is stopping or
IPVS has been disabled. That keeps temporary estimators from being
drained after the limits failed to initialize.

Estimator kthreads can now self-exit before teardown or reload stops
kd->task. Keep an extra task reference after creation and release it
with kthread_stop_put(), so kd->task remains valid until the stop paths
consume that reference.

Fixes: 705dd3444081 ("ipvs: use kthreads for stats estimation")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Signed-off-by: Zhiling Zou <zhilinz@nebusec.ai>
Acked-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/ipvs/ip_vs_est.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
index ab09f5182951..05a216a47b45 100644
--- a/net/netfilter/ipvs/ip_vs_est.c
+++ b/net/netfilter/ipvs/ip_vs_est.c
@@ -191,8 +191,11 @@ static int ip_vs_estimation_kthread(void *data)
 		}
 
 		/* kthread 0 will handle the calc phase */
-		if (ipvs->est_calc_phase)
+		if (ipvs->est_calc_phase) {
 			ip_vs_est_calc_phase(ipvs);
+			if (kthread_should_stop() || !READ_ONCE(ipvs->enable))
+				return 0;
+		}
 	}
 
 	while (1) {
@@ -270,6 +273,7 @@ int ip_vs_est_kthread_start(struct netns_ipvs *ipvs,
 		kd->task = NULL;
 		goto out;
 	}
+	get_task_struct(kd->task);
 
 	set_user_nice(kd->task, sysctl_est_nice(ipvs));
 	if (sysctl_est_preferred_cpulist(ipvs))
@@ -286,7 +290,7 @@ void ip_vs_est_kthread_stop(struct ip_vs_est_kt_data *kd)
 {
 	if (kd->task) {
 		pr_info("stopping estimator thread %d...\n", kd->id);
-		kthread_stop(kd->task);
+		kthread_stop_put(kd->task);
 		kd->task = NULL;
 	}
 }
@@ -526,7 +530,7 @@ static void ip_vs_est_kthread_destroy(struct ip_vs_est_kt_data *kd)
 	if (kd) {
 		if (kd->task) {
 			pr_info("stop unused estimator thread %d...\n", kd->id);
-			kthread_stop(kd->task);
+			kthread_stop_put(kd->task);
 		}
 		ip_vs_stats_free(kd->calc_stats);
 		kfree(kd);
-- 
2.47.3

[PATCH net 02/10] netfilter: ebt_nflog: pin the NFLOG backend

[Pablo Neira Ayuso]

From: Chengfeng Ye <nicoyip.dev@gmail.com>

nf_log_unregister() runs after the per-net teardown so its final RCU
grace period also drains readers that obtained the logger from a per-net
binding.  However, ebt_nflog passes an explicit ULOG log type to
nf_log_packet() without holding a reference on the selected logger module,
unlike the xt_NFLOG and nft_log frontends.

An ebtables nflog rule can therefore remain callable while nfnetlink_log
is unloaded.  The resulting interleaving is:

  CPU 0                               CPU 1
  nfnetlink_log_fini()
    unregister_pernet_subsys()
      kfree(nfnl_log_pernet(net))
                                      ebt_nflog_tg()
                                        nf_log_packet()
                                          nfulnl_log_packet()
                                            instance_lookup_get_rcu()

The global ULOG logger is still registered at this point, so CPU 1
dereferences the per-net state after CPU 0 has freed it.  KASAN reported:

  BUG: KASAN: slab-use-after-free in instance_lookup_get_rcu
  Read of size 8 at addr ff110001052e6210 by task poc/92
  Call Trace:
   instance_lookup_get_rcu+0x1ce/0x1f0 [nfnetlink_log]
   nfulnl_log_packet+0x248/0x2fb0 [nfnetlink_log]
   nf_log_packet+0x204/0x300
   ebt_nflog_tg+0x351/0x550
   ebt_do_table+0xedf/0x22b0
  Allocated by task 90:
   __kmalloc_noprof+0x186/0x470
   ops_init+0x6d/0x420
   register_pernet_operations+0x2f6/0x670
   register_pernet_subsys+0x23/0x40
  Freed by task 93:
   kfree+0x131/0x3c0
   ops_undo_list+0x3e3/0x700
   unregister_pernet_operations+0x232/0x490
   unregister_pernet_subsys+0x1c/0x30
   nfnetlink_log_fini+0x34/0x450 [nfnetlink_log]

Acquire the ULOG logger module reference when an ebt_nflog rule is
validated and release it when the rule is destroyed.  Request the NFLOG
backend for legacy callers when needed, matching xt_NFLOG.  This prevents
module teardown until all ebt_nflog rules have stopped using the logger.

Fixes: c83fa19603bd ("netfilter: nf_log: don't call synchronize_rcu in nf_log_unset")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/bridge/netfilter/ebt_nflog.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/net/bridge/netfilter/ebt_nflog.c b/net/bridge/netfilter/ebt_nflog.c
index 61bf8f4465ab..426f8adc912c 100644
--- a/net/bridge/netfilter/ebt_nflog.c
+++ b/net/bridge/netfilter/ebt_nflog.c
@@ -41,11 +41,25 @@ ebt_nflog_tg(struct sk_buff *skb, const struct xt_action_param *par)
 static int ebt_nflog_tg_check(const struct xt_tgchk_param *par)
 {
 	struct ebt_nflog_info *info = par->targinfo;
+	int ret;
 
 	if (info->flags & ~EBT_NFLOG_MASK)
 		return -EINVAL;
 	info->prefix[EBT_NFLOG_PREFIX_SIZE - 1] = '\0';
-	return 0;
+
+	ret = nf_logger_find_get(par->family, NF_LOG_TYPE_ULOG);
+	if (ret != 0 && !par->nft_compat) {
+		request_module("%s", "nfnetlink_log");
+
+		ret = nf_logger_find_get(par->family, NF_LOG_TYPE_ULOG);
+	}
+
+	return ret;
+}
+
+static void ebt_nflog_tg_destroy(const struct xt_tgdtor_param *par)
+{
+	nf_logger_put(par->family, NF_LOG_TYPE_ULOG);
 }
 
 static struct xt_target ebt_nflog_tg_reg __read_mostly = {
@@ -54,6 +68,7 @@ static struct xt_target ebt_nflog_tg_reg __read_mostly = {
 	.family     = NFPROTO_BRIDGE,
 	.target     = ebt_nflog_tg,
 	.checkentry = ebt_nflog_tg_check,
+	.destroy    = ebt_nflog_tg_destroy,
 	.targetsize = sizeof(struct ebt_nflog_info),
 	.me         = THIS_MODULE,
 };
-- 
2.47.3

[PATCH net 03/10] netfilter: ipset: rework cidr bookkeeping

[Pablo Neira Ayuso]

From: Jozsef Kadlecsik <kadlec@netfilter.org>

According to sashiko, the current bookkeeping of cidr values are unsafe
on weakly-ordered architectures. Replace the in-place updating with an
RCU based method: create the new bookeeping structure, update and replace
the old one with the new. Downside that we need to allocate memory when
deleting a cidr entry - in case of memory pressure fall back to leave holes
which possibility is taken into account at evaluation time.

Thanks to Pablo (Pablo Neira Ayuso <pablo@netfilter.org>) and Cyntia
(Cynthia <cynthia@kosmx.dev>) for helping me in debugging which resulted
the patch "netfilter: ipset: allocate the proper memory for the generic
hash structure" on which this very patch depends.

Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/ipset/ip_set_hash_gen.h        | 235 +++++++++++++------
 net/netfilter/ipset/ip_set_hash_ipportnet.c  |   4 +-
 net/netfilter/ipset/ip_set_hash_net.c        |   4 +-
 net/netfilter/ipset/ip_set_hash_netiface.c   |   4 +-
 net/netfilter/ipset/ip_set_hash_netnet.c     |  12 +-
 net/netfilter/ipset/ip_set_hash_netport.c    |   4 +-
 net/netfilter/ipset/ip_set_hash_netportnet.c |  12 +-
 7 files changed, 182 insertions(+), 93 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index b2d77973272d..dd31992c915c 100644
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -99,9 +99,15 @@ struct htable {
 #endif
 
 /* Book-keeping of the prefixes added to the set */
+struct net_prefix {
+	u8 cidr;			/* the cidr value */
+	u32 count;			/* number of elements of this cidr */
+};
+
 struct net_prefixes {
-	u32 nets[IPSET_NET_COUNT]; /* number of elements for this cidr */
-	u8 cidr[IPSET_NET_COUNT];  /* the cidr value */
+	struct rcu_head rcu;
+	u8 len;
+	struct net_prefix nets[] __counted_by(len);
 };
 
 /* Compute the hash table size */
@@ -127,11 +133,6 @@ htable_size(u8 hbits)
 #else
 #define __CIDR(cidr, i)		(cidr)
 #endif
-
-/* cidr + 1 is stored in net_prefixes to support /0 */
-#define NCIDR_PUT(cidr)		((cidr) + 1)
-#define NCIDR_GET(cidr)		((cidr) - 1)
-
 #ifdef IP_SET_HASH_WITH_NETS_PACKED
 /* When cidr is packed with nomatch, cidr - 1 is stored in the data entry */
 #define DCIDR_PUT(cidr)		((cidr) - 1)
@@ -141,21 +142,11 @@ htable_size(u8 hbits)
 #define DCIDR_GET(cidr, i)	__CIDR(cidr, i)
 #endif
 
-#define INIT_CIDR(cidr, host_mask)	\
-	DCIDR_PUT(((cidr) ? NCIDR_GET(cidr) : host_mask))
-
-#ifdef IP_SET_HASH_WITH_NET0
-/* cidr from 0 to HOST_MASK value and c = cidr + 1 */
-#define NLEN			(HOST_MASK + 1)
-#define CIDR_POS(c)		((c) - 1)
-#else
-/* cidr from 1 to HOST_MASK value and c = cidr + 1 */
-#define NLEN			HOST_MASK
-#define CIDR_POS(c)		((c) - 2)
-#endif
+#define INIT_CIDR(n, host_mask) ({				\
+	const struct net_prefixes *__n = rcu_dereference(n);		\
+	DCIDR_PUT((__n)->len ? (__n)->nets[0].cidr : host_mask);\
+})
 
-#else
-#define NLEN			0
 #endif /* IP_SET_HASH_WITH_NETS */
 
 #define SET_ELEM_EXPIRED(set, d)	\
@@ -292,6 +283,7 @@ static const union nf_inet_addr zeromask = {};
 /* The generic hash structure */
 struct htype {
 	struct htable __rcu *table; /* the hash table */
+	struct net_prefixes __rcu *rnets[IPSET_NET_COUNT]; /* cidr prefixes */
 	struct htable_gc gc;	/* gc workqueue */
 	u32 maxelem;		/* max elements in the hash */
 	u32 initval;		/* random jhash init value */
@@ -302,9 +294,6 @@ struct htype {
 #if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
 	u8 netmask;		/* netmask value for subnets to store */
 	union nf_inet_addr bitmask;	/* stores bitmask */
-#endif
-#ifdef IP_SET_HASH_WITH_NETS
-	struct net_prefixes nets[NLEN]; /* book-keeping of prefixes */
 #endif
 	/* Because 'next' is IPv4/IPv6 dependent, no elements of this
 	 * structure and referred in create() may come after 'next'.
@@ -326,50 +315,92 @@ struct mtype_resize_ad {
 /* Network cidr size book keeping when the hash stores different
  * sized networks. cidr == real cidr + 1 to support /0.
  */
-static void
+static int
 mtype_add_cidr(struct ip_set *set, struct htype *h, u8 cidr, u8 n)
 {
-	int i, j;
+	struct net_prefixes *nets, *tmp;
+	int i, j, found, len = 0, ret = 0;
 
 	spin_lock_bh(&set->lock);
+	nets = __ipset_dereference(h->rnets[n]);
 	/* Add in increasing prefix order, so larger cidr first */
-	for (i = 0, j = -1; i < NLEN && h->nets[i].cidr[n]; i++) {
-		if (j != -1) {
+	for (i = 0, found = -1; i < nets->len; i++) {
+		if (nets->nets[i].count)
+			len++;
+		if (found != -1) {
 			continue;
-		} else if (h->nets[i].cidr[n] < cidr) {
-			j = i;
-		} else if (h->nets[i].cidr[n] == cidr) {
-			h->nets[CIDR_POS(cidr)].nets[n]++;
+		} else if (nets->nets[i].cidr < cidr) {
+			found = i;
+		} else if (nets->nets[i].cidr == cidr) {
+			nets->nets[i].count++;
 			goto unlock;
 		}
 	}
-	if (j != -1) {
-		for (; i > j; i--)
-			h->nets[i].cidr[n] = h->nets[i - 1].cidr[n];
+	len++;
+	tmp = kzalloc_flex(*tmp, nets, len, GFP_ATOMIC);
+	if (!tmp) {
+		ret = -ENOMEM;
+		goto unlock;
 	}
-	h->nets[i].cidr[n] = cidr;
-	h->nets[CIDR_POS(cidr)].nets[n] = 1;
+
+	tmp->len = len;
+	for (i = 0, j = 0; i < nets->len; i++) {
+		if (i == found) {
+			tmp->nets[j].cidr = cidr;
+			tmp->nets[j++].count = 1;
+		}
+		if (!nets->nets[i].count)
+			continue;
+		tmp->nets[j].cidr = nets->nets[i].cidr;
+		tmp->nets[j++].count = nets->nets[i].count;
+	}
+	if (found == -1) {
+		tmp->nets[j].cidr = cidr;
+		tmp->nets[j].count = 1;
+	}
+	rcu_assign_pointer(h->rnets[n], tmp);
+	kfree_rcu(nets, rcu);
 unlock:
 	spin_unlock_bh(&set->lock);
+	return ret;
 }
 
 static void
 mtype_del_cidr(struct ip_set *set, struct htype *h, u8 cidr, u8 n)
 {
-	u8 i, j, net_end = NLEN - 1;
+	struct net_prefixes *nets, *tmp;
+	u8 i, j, len = 0;
+	int found;
 
 	spin_lock_bh(&set->lock);
-	for (i = 0; i < NLEN; i++) {
-		if (h->nets[i].cidr[n] != cidr)
-			continue;
-		h->nets[CIDR_POS(cidr)].nets[n]--;
-		if (h->nets[CIDR_POS(cidr)].nets[n] > 0)
-			goto unlock;
-		for (j = i; j < net_end && h->nets[j].cidr[n]; j++)
-			h->nets[j].cidr[n] = h->nets[j + 1].cidr[n];
-		h->nets[j].cidr[n] = 0;
+	nets = __ipset_dereference(h->rnets[n]);
+	for (i = 0, found = -1; i < nets->len; i++) {
+		if (nets->nets[i].count)
+			len++;
+		if (nets->nets[i].cidr == cidr)
+			found = i;
+	}
+	if (unlikely(found == -1))
+		goto unlock;
+
+	nets->nets[found].count--;
+	if (nets->nets[found].count)
 		goto unlock;
+	len--;
+	tmp = kzalloc_flex(*tmp, nets, len, GFP_ATOMIC);
+	if (!tmp)
+		/* Leave a hole */
+		goto unlock;
+
+	tmp->len = len;
+	for (i = 0, j = 0; i < nets->len; i++) {
+		if (!nets->nets[i].count || i == found)
+			continue;
+		tmp->nets[j].cidr = nets->nets[i].cidr;
+		tmp->nets[j++].count = nets->nets[i].count;
 	}
+	rcu_assign_pointer(h->rnets[n], tmp);
+	kfree_rcu(nets, rcu);
 unlock:
 	spin_unlock_bh(&set->lock);
 }
@@ -402,6 +433,9 @@ static void
 mtype_flush(struct ip_set *set)
 {
 	struct htype *h = set->data;
+#ifdef IP_SET_HASH_WITH_NETS
+	struct net_prefixes *nets, *tmp;
+#endif
 	struct htable *t;
 	struct hbucket *n;
 	u32 r, i;
@@ -425,7 +459,19 @@ mtype_flush(struct ip_set *set)
 		spin_unlock_bh(&t->hregion[r].lock);
 	}
 #ifdef IP_SET_HASH_WITH_NETS
-	memset(h->nets, 0, sizeof(h->nets));
+	for (i = 0; i < IPSET_NET_COUNT; i++) {
+		nets = ipset_dereference_nfnl(h->rnets[i]);
+		tmp = kzalloc_obj(*tmp, GFP_ATOMIC);
+		if (!tmp) {
+			u8 j;
+
+			for (j = 0; j < nets->len; j++)
+				nets->nets[j].count = 0;
+		} else {
+			rcu_assign_pointer(h->rnets[i], tmp);
+			kfree_rcu(nets, rcu);
+		}
+	}
 #endif
 }
 
@@ -433,6 +479,9 @@ mtype_flush(struct ip_set *set)
 static void
 mtype_ahash_destroy(struct ip_set *set, struct htable *t, bool ext_destroy)
 {
+#ifdef IP_SET_HASH_WITH_NETS
+	struct htype *h = set->data;
+#endif
 	struct hbucket *n;
 	u32 i;
 
@@ -446,6 +495,11 @@ mtype_ahash_destroy(struct ip_set *set, struct htable *t, bool ext_destroy)
 		kfree(n);
 	}
 
+#ifdef IP_SET_HASH_WITH_NETS
+	if (ext_destroy)
+		for (i = 0; i < IPSET_NET_COUNT; i++)
+			kfree(rcu_dereference_raw(h->rnets[i]));
+#endif
 	ip_set_free(t->hregion);
 	ip_set_free(t);
 }
@@ -519,8 +573,7 @@ mtype_gc_do(struct ip_set *set, struct htype *h, struct htable *t, u32 r)
 #ifdef IP_SET_HASH_WITH_NETS
 			for (k = 0; k < IPSET_NET_COUNT; k++)
 				mtype_del_cidr(set, h,
-					NCIDR_PUT(DCIDR_GET(data->cidr, k)),
-					k);
+					DCIDR_GET(data->cidr, k), k);
 #endif
 			t->hregion[r].elements--;
 			ip_set_ext_destroy(set, data);
@@ -950,8 +1003,7 @@ mtype_add(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 #ifdef IP_SET_HASH_WITH_NETS
 			for (i = 0; i < IPSET_NET_COUNT; i++)
 				mtype_del_cidr(set, h,
-					NCIDR_PUT(DCIDR_GET(data->cidr, i)),
-					i);
+					DCIDR_GET(data->cidr, i), i);
 #endif
 			ip_set_ext_destroy(set, data);
 			t->hregion[r].elements--;
@@ -996,7 +1048,7 @@ mtype_add(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 	t->hregion[r].elements++;
 #ifdef IP_SET_HASH_WITH_NETS
 	for (i = 0; i < IPSET_NET_COUNT; i++)
-		mtype_add_cidr(set, h, NCIDR_PUT(DCIDR_GET(d->cidr, i)), i);
+		mtype_add_cidr(set, h, DCIDR_GET(d->cidr, i), i);
 #endif
 	memcpy(data, d, sizeof(struct mtype_elem));
 overwrite_extensions:
@@ -1110,7 +1162,7 @@ mtype_del(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 #ifdef IP_SET_HASH_WITH_NETS
 		for (j = 0; j < IPSET_NET_COUNT; j++)
 			mtype_del_cidr(set, h,
-				       NCIDR_PUT(DCIDR_GET(d->cidr, j)), j);
+				DCIDR_GET(d->cidr, j), j);
 #endif
 		ip_set_ext_destroy(set, data);
 
@@ -1193,28 +1245,37 @@ mtype_test_cidrs(struct ip_set *set, struct mtype_elem *d,
 {
 	struct htype *h = set->data;
 	struct htable *t = rcu_dereference_bh(h->table);
+	struct net_prefixes *nets0;
 	struct hbucket *n;
 	struct mtype_elem *data;
 #if IPSET_NET_COUNT == 2
+	struct net_prefixes *nets1;
 	struct mtype_elem orig = *d;
-	int ret, i, j = 0, k;
+	int ret, i, j, k;
 #else
-	int ret, i, j = 0;
+	int ret, i, j;
 #endif
 	u32 key, multi = 0;
 	u8 pos;
 
 	pr_debug("test by nets\n");
-	for (; j < NLEN && h->nets[j].cidr[0] && !multi; j++) {
+	rcu_read_lock_bh();
+	nets0 = rcu_dereference_bh(h->rnets[0]);
+#if IPSET_NET_COUNT == 2
+	nets1 = rcu_dereference_bh(h->rnets[1]);
+#endif
+	for (j = 0; j < nets0->len && !multi; j++) {
+		if (!nets0->nets[j].count)
+			continue;
 #if IPSET_NET_COUNT == 2
 		mtype_data_reset_elem(d, &orig);
-		mtype_data_netmask(d, NCIDR_GET(h->nets[j].cidr[0]), false);
-		for (k = 0; k < NLEN && h->nets[k].cidr[1] && !multi;
-		     k++) {
-			mtype_data_netmask(d, NCIDR_GET(h->nets[k].cidr[1]),
-					   true);
+		mtype_data_netmask(d, nets0->nets[j].cidr, false);
+		for (k = 0; k < nets1->len && !multi; k++) {
+			if (!nets1->nets[k].count)
+				continue;
+			mtype_data_netmask(d, nets1->nets[k].cidr, true);
 #else
-		mtype_data_netmask(d, NCIDR_GET(h->nets[j].cidr[0]));
+		mtype_data_netmask(d, nets0->nets[j].cidr);
 #endif
 		key = HKEY(d, h->initval, t->htable_bits);
 		n = rcu_dereference_bh(hbucket(t, key));
@@ -1229,7 +1290,7 @@ mtype_test_cidrs(struct ip_set *set, struct mtype_elem *d,
 				continue;
 			ret = mtype_data_match(data, ext, mext, set, flags);
 			if (ret != 0)
-				return ret;
+				goto unlock;
 #ifdef IP_SET_HASH_WITH_MULTI
 			/* No match, reset multiple match flag */
 			multi = 0;
@@ -1239,7 +1300,10 @@ mtype_test_cidrs(struct ip_set *set, struct mtype_elem *d,
 		}
 #endif
 	}
-	return 0;
+	ret = 0;
+unlock:
+	rcu_read_unlock_bh();
+	return ret;
 }
 #endif
 
@@ -1504,6 +1568,9 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
 	int ret __attribute__((unused)) = 0;
 	u8 netmask = set->family == NFPROTO_IPV4 ? 32 : 128;
 	union nf_inet_addr bitmask = onesmask;
+#endif
+#ifdef IP_SET_HASH_WITH_NETS
+	struct net_prefixes *nets;
 #endif
 	size_t hsize;
 	struct htype *h;
@@ -1604,21 +1671,25 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
 	 */
 	hbits = fls(hashsize - 1);
 	hsize = htable_size(hbits);
-	if (hsize == 0) {
-		kfree(h);
-		return -ENOMEM;
-	}
+	if (hsize == 0)
+		goto free_h;
 	t = ip_set_alloc(hsize);
-	if (!t) {
-		kfree(h);
-		return -ENOMEM;
-	}
+	if (!t)
+		goto free_h;
 	t->hregion = ip_set_alloc(ahash_sizeof_regions(hbits));
-	if (!t->hregion) {
-		ip_set_free(t);
-		kfree(h);
-		return -ENOMEM;
+	if (!t->hregion)
+		goto free_t;
+#ifdef IP_SET_HASH_WITH_NETS
+	for (i = 0; i < IPSET_NET_COUNT; i++) {
+		nets = kzalloc_obj(*nets);
+		if (!nets) {
+			while (i > 0)
+				kfree(rcu_dereference_raw(h->rnets[--i]));
+			goto free_hregion;
+		}
+		RCU_INIT_POINTER(h->rnets[i], nets);
 	}
+#endif
 	h->gc.set = set;
 	spin_lock_init(&h->gc.lock);
 	for (i = 0; i < ahash_numof_locks(hbits); i++)
@@ -1682,6 +1753,16 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
 		 t->htable_bits, h->maxelem, set->data, t);
 
 	return 0;
+
+#ifdef IP_SET_HASH_WITH_NETS
+free_hregion:
+	ip_set_free(t->hregion);
+#endif
+free_t:
+	ip_set_free(t);
+free_h:
+	kfree(h);
+	return -ENOMEM;
 }
 #endif /* IP_SET_EMIT_CREATE */
 
diff --git a/net/netfilter/ipset/ip_set_hash_ipportnet.c b/net/netfilter/ipset/ip_set_hash_ipportnet.c
index 2d6652d43199..195853a25b06 100644
--- a/net/netfilter/ipset/ip_set_hash_ipportnet.c
+++ b/net/netfilter/ipset/ip_set_hash_ipportnet.c
@@ -138,7 +138,7 @@ hash_ipportnet4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	const struct hash_ipportnet4 *h = set->data;
 	ipset_adtfn adtfn = set->variant->adt[adt];
 	struct hash_ipportnet4_elem e = {
-		.cidr = INIT_CIDR(h->nets[0].cidr[0], HOST_MASK),
+		.cidr = INIT_CIDR(h->rnets[0], HOST_MASK),
 	};
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
@@ -398,7 +398,7 @@ hash_ipportnet6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	const struct hash_ipportnet6 *h = set->data;
 	ipset_adtfn adtfn = set->variant->adt[adt];
 	struct hash_ipportnet6_elem e = {
-		.cidr = INIT_CIDR(h->nets[0].cidr[0], HOST_MASK),
+		.cidr = INIT_CIDR(h->rnets[0], HOST_MASK),
 	};
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
diff --git a/net/netfilter/ipset/ip_set_hash_net.c b/net/netfilter/ipset/ip_set_hash_net.c
index ce0a9ce5a91f..092f3c9281b8 100644
--- a/net/netfilter/ipset/ip_set_hash_net.c
+++ b/net/netfilter/ipset/ip_set_hash_net.c
@@ -117,7 +117,7 @@ hash_net4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	const struct hash_net4 *h = set->data;
 	ipset_adtfn adtfn = set->variant->adt[adt];
 	struct hash_net4_elem e = {
-		.cidr = INIT_CIDR(h->nets[0].cidr[0], HOST_MASK),
+		.cidr = INIT_CIDR(h->rnets[0], HOST_MASK),
 	};
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
@@ -291,7 +291,7 @@ hash_net6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	const struct hash_net6 *h = set->data;
 	ipset_adtfn adtfn = set->variant->adt[adt];
 	struct hash_net6_elem e = {
-		.cidr = INIT_CIDR(h->nets[0].cidr[0], HOST_MASK),
+		.cidr = INIT_CIDR(h->rnets[0], HOST_MASK),
 	};
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
diff --git a/net/netfilter/ipset/ip_set_hash_netiface.c b/net/netfilter/ipset/ip_set_hash_netiface.c
index 30a655e5c4fd..b44b95f766b7 100644
--- a/net/netfilter/ipset/ip_set_hash_netiface.c
+++ b/net/netfilter/ipset/ip_set_hash_netiface.c
@@ -161,7 +161,7 @@ hash_netiface4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_netiface4 *h = set->data;
 	ipset_adtfn adtfn = set->variant->adt[adt];
 	struct hash_netiface4_elem e = {
-		.cidr = INIT_CIDR(h->nets[0].cidr[0], HOST_MASK),
+		.cidr = INIT_CIDR(h->rnets[0], HOST_MASK),
 		.elem = 1,
 	};
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
@@ -382,7 +382,7 @@ hash_netiface6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_netiface6 *h = set->data;
 	ipset_adtfn adtfn = set->variant->adt[adt];
 	struct hash_netiface6_elem e = {
-		.cidr = INIT_CIDR(h->nets[0].cidr[0], HOST_MASK),
+		.cidr = INIT_CIDR(h->rnets[0], HOST_MASK),
 		.elem = 1,
 	};
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
diff --git a/net/netfilter/ipset/ip_set_hash_netnet.c b/net/netfilter/ipset/ip_set_hash_netnet.c
index 8fbe649c9dd3..f7c8a1cc30fc 100644
--- a/net/netfilter/ipset/ip_set_hash_netnet.c
+++ b/net/netfilter/ipset/ip_set_hash_netnet.c
@@ -149,8 +149,10 @@ hash_netnet4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_netnet4_elem e = { };
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
-	e.cidr[0] = INIT_CIDR(h->nets[0].cidr[0], HOST_MASK);
-	e.cidr[1] = INIT_CIDR(h->nets[0].cidr[1], HOST_MASK);
+	rcu_read_lock_bh();
+	e.cidr[0] = INIT_CIDR(h->rnets[0], HOST_MASK);
+	e.cidr[1] = INIT_CIDR(h->rnets[1], HOST_MASK);
+	rcu_read_unlock_bh();
 	if (adt == IPSET_TEST)
 		e.ccmp = (HOST_MASK << (sizeof(e.cidr[0]) * 8)) | HOST_MASK;
 
@@ -388,8 +390,10 @@ hash_netnet6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_netnet6_elem e = { };
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
-	e.cidr[0] = INIT_CIDR(h->nets[0].cidr[0], HOST_MASK);
-	e.cidr[1] = INIT_CIDR(h->nets[0].cidr[1], HOST_MASK);
+	rcu_read_lock_bh();
+	e.cidr[0] = INIT_CIDR(h->rnets[0], HOST_MASK);
+	e.cidr[1] = INIT_CIDR(h->rnets[1], HOST_MASK);
+	rcu_read_unlock_bh();
 	if (adt == IPSET_TEST)
 		e.ccmp = (HOST_MASK << (sizeof(u8) * 8)) | HOST_MASK;
 
diff --git a/net/netfilter/ipset/ip_set_hash_netport.c b/net/netfilter/ipset/ip_set_hash_netport.c
index d1a0628df4ef..5de4b511de76 100644
--- a/net/netfilter/ipset/ip_set_hash_netport.c
+++ b/net/netfilter/ipset/ip_set_hash_netport.c
@@ -133,7 +133,7 @@ hash_netport4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	const struct hash_netport4 *h = set->data;
 	ipset_adtfn adtfn = set->variant->adt[adt];
 	struct hash_netport4_elem e = {
-		.cidr = INIT_CIDR(h->nets[0].cidr[0], HOST_MASK),
+		.cidr = INIT_CIDR(h->rnets[0], HOST_MASK),
 	};
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
@@ -353,7 +353,7 @@ hash_netport6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	const struct hash_netport6 *h = set->data;
 	ipset_adtfn adtfn = set->variant->adt[adt];
 	struct hash_netport6_elem e = {
-		.cidr = INIT_CIDR(h->nets[0].cidr[0], HOST_MASK),
+		.cidr = INIT_CIDR(h->rnets[0], HOST_MASK),
 	};
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
diff --git a/net/netfilter/ipset/ip_set_hash_netportnet.c b/net/netfilter/ipset/ip_set_hash_netportnet.c
index bf4f91b78e1d..6291532be7a5 100644
--- a/net/netfilter/ipset/ip_set_hash_netportnet.c
+++ b/net/netfilter/ipset/ip_set_hash_netportnet.c
@@ -157,8 +157,10 @@ hash_netportnet4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_netportnet4_elem e = { };
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
-	e.cidr[0] = INIT_CIDR(h->nets[0].cidr[0], HOST_MASK);
-	e.cidr[1] = INIT_CIDR(h->nets[0].cidr[1], HOST_MASK);
+	rcu_read_lock_bh();
+	e.cidr[0] = INIT_CIDR(h->rnets[0], HOST_MASK);
+	e.cidr[1] = INIT_CIDR(h->rnets[1], HOST_MASK);
+	rcu_read_unlock_bh();
 	if (adt == IPSET_TEST)
 		e.ccmp = (HOST_MASK << (sizeof(e.cidr[0]) * 8)) | HOST_MASK;
 
@@ -452,8 +454,10 @@ hash_netportnet6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_netportnet6_elem e = { };
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
-	e.cidr[0] = INIT_CIDR(h->nets[0].cidr[0], HOST_MASK);
-	e.cidr[1] = INIT_CIDR(h->nets[0].cidr[1], HOST_MASK);
+	rcu_read_lock_bh();
+	e.cidr[0] = INIT_CIDR(h->rnets[0], HOST_MASK);
+	e.cidr[1] = INIT_CIDR(h->rnets[1], HOST_MASK);
+	rcu_read_unlock_bh();
 	if (adt == IPSET_TEST)
 		e.ccmp = (HOST_MASK << (sizeof(u8) * 8)) | HOST_MASK;
 
-- 
2.47.3

[PATCH net 04/10] netfilter: ipset: switch ext_size to atomic64_t

[Pablo Neira Ayuso]

From: Jozsef Kadlecsik <kadlec@netfilter.org>

The hash types do not acquire set->lock, they use 'region locking' where
only part of the hash table is locked. Parallel inserts and deletes are
possible and CPUs can race on ->ext_size update.  Switch to atomic64_t.

This leaves another bug unresolved: there still can be a race on
comment extension re-init.  This will be handled in a later commit
when converting to rhashtable backend.

Fixes: f66ee0410b1c ("netfilter: ipset: Fix "INFO: rcu detected stall in hash_xxx" reports")
Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netfilter/ipset/ip_set.h  | 2 +-
 net/netfilter/ipset/ip_set_bitmap_gen.h | 4 ++--
 net/netfilter/ipset/ip_set_core.c       | 6 +++---
 net/netfilter/ipset/ip_set_hash_gen.h   | 2 +-
 net/netfilter/ipset/ip_set_list_set.c   | 4 ++--
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
index b98331572ad2..cadae9b2578f 100644
--- a/include/linux/netfilter/ipset/ip_set.h
+++ b/include/linux/netfilter/ipset/ip_set.h
@@ -273,7 +273,7 @@ struct ip_set {
 	/* Number of elements (vs timeout) */
 	u32 elements;
 	/* Size of the dynamic extensions (vs timeout) */
-	size_t ext_size;
+	atomic64_t ext_size;
 	/* Element data size */
 	size_t dsize;
 	/* Offsets to extensions in elements */
diff --git a/net/netfilter/ipset/ip_set_bitmap_gen.h b/net/netfilter/ipset/ip_set_bitmap_gen.h
index bb9b5bed10e1..226fdf17b683 100644
--- a/net/netfilter/ipset/ip_set_bitmap_gen.h
+++ b/net/netfilter/ipset/ip_set_bitmap_gen.h
@@ -77,7 +77,7 @@ mtype_flush(struct ip_set *set)
 		mtype_ext_cleanup(set);
 	bitmap_zero(map->members, map->elements);
 	set->elements = 0;
-	set->ext_size = 0;
+	atomic64_set(&set->ext_size, 0);
 }
 
 /* Calculate the actual memory size of the set data */
@@ -93,7 +93,7 @@ mtype_head(struct ip_set *set, struct sk_buff *skb)
 {
 	const struct mtype *map = set->data;
 	struct nlattr *nested;
-	size_t memsize = mtype_memsize(map, set->dsize) + set->ext_size;
+	size_t memsize = mtype_memsize(map, set->dsize) + atomic64_read(&set->ext_size);
 
 	nested = nla_nest_start(skb, IPSET_ATTR_DATA);
 	if (!nested)
diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index 6cfad152d7d1..822a53a7f502 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -350,7 +350,7 @@ ip_set_init_comment(struct ip_set *set, struct ip_set_comment *comment,
 	size_t len = ext->comment ? strlen(ext->comment) : 0;
 
 	if (unlikely(c)) {
-		set->ext_size -= sizeof(*c) + strlen(c->str) + 1;
+		atomic64_sub(sizeof(*c) + strlen(c->str) + 1, &set->ext_size);
 		rcu_assign_pointer(comment->c, NULL);
 		kfree_rcu(c, rcu);
 	}
@@ -362,7 +362,7 @@ ip_set_init_comment(struct ip_set *set, struct ip_set_comment *comment,
 	if (unlikely(!c))
 		return;
 	strscpy(c->str, ext->comment, len + 1);
-	set->ext_size += sizeof(*c) + strlen(c->str) + 1;
+	atomic64_add(sizeof(*c) + strlen(c->str) + 1, &set->ext_size);
 	rcu_assign_pointer(comment->c, c);
 }
 EXPORT_SYMBOL_GPL(ip_set_init_comment);
@@ -392,7 +392,7 @@ ip_set_comment_free(struct ip_set *set, void *ptr)
 	c = rcu_dereference_protected(comment->c, 1);
 	if (unlikely(!c))
 		return;
-	set->ext_size -= sizeof(*c) + strlen(c->str) + 1;
+	atomic64_sub(sizeof(*c) + strlen(c->str) + 1, &set->ext_size);
 	rcu_assign_pointer(comment->c, NULL);
 	kfree_rcu(c, rcu);
 }
diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index dd31992c915c..8841daf28f01 100644
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -1373,7 +1373,7 @@ mtype_head(struct ip_set *set, struct sk_buff *skb)
 	rcu_read_lock_bh();
 	t = rcu_dereference_bh(h->table);
 	mtype_ext_size(set, &elements, &ext_size);
-	memsize = mtype_ahash_memsize(h, t) + ext_size + set->ext_size;
+	memsize = mtype_ahash_memsize(h, t) + ext_size + atomic64_read(&set->ext_size);
 	htable_bits = t->htable_bits;
 	rcu_read_unlock_bh();
 
diff --git a/net/netfilter/ipset/ip_set_list_set.c b/net/netfilter/ipset/ip_set_list_set.c
index 1cef84f15e8c..ca3ef9479e83 100644
--- a/net/netfilter/ipset/ip_set_list_set.c
+++ b/net/netfilter/ipset/ip_set_list_set.c
@@ -421,7 +421,7 @@ list_set_flush(struct ip_set *set)
 	list_for_each_entry_safe(e, n, &map->members, list)
 		list_set_del(set, e);
 	set->elements = 0;
-	set->ext_size = 0;
+	atomic64_set(&set->ext_size, 0);
 }
 
 static void
@@ -455,7 +455,7 @@ list_set_head(struct ip_set *set, struct sk_buff *skb)
 {
 	const struct list_set *map = set->data;
 	struct nlattr *nested;
-	size_t memsize = list_set_memsize(map, set->dsize) + set->ext_size;
+	size_t memsize = list_set_memsize(map, set->dsize) + atomic64_read(&set->ext_size);
 
 	nested = nla_nest_start(skb, IPSET_ATTR_DATA);
 	if (!nested)
-- 
2.47.3

[PATCH net 05/10] netfilter: ipset: add small wrappers for hash and bucket sizes

[Pablo Neira Ayuso]

From: Florian Westphal <fw@strlen.de>

Preparation patch.  Once the ipset hash table is replaced with rhashtable
these functions are needed. Add them in extra commit to have reviewable
chunks.

Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/ipset/ip_set_hash_gen.h | 39 +++++++++++++++++++++------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index 8841daf28f01..ef586b486f51 100644
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -201,6 +201,8 @@ static const union nf_inet_addr zeromask = {};
 #undef mtype_same_set
 #undef mtype_kadt
 #undef mtype_uadt
+#undef mtype_bucket_size
+#undef mtype_hash_size
 
 #undef mtype_add
 #undef mtype_del
@@ -246,6 +248,8 @@ static const union nf_inet_addr zeromask = {};
 #define mtype_same_set		IPSET_TOKEN(MTYPE, _same_set)
 #define mtype_kadt		IPSET_TOKEN(MTYPE, _kadt)
 #define mtype_uadt		IPSET_TOKEN(MTYPE, _uadt)
+#define mtype_bucket_size	IPSET_TOKEN(MTYPE, _bucket_size)
+#define mtype_hash_size		IPSET_TOKEN(MTYPE, _hash_size)
 
 #define mtype_add		IPSET_TOKEN(MTYPE, _add)
 #define mtype_del		IPSET_TOKEN(MTYPE, _del)
@@ -1358,6 +1362,24 @@ mtype_test(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 	return ret;
 }
 
+static u32 mtype_hash_size(const struct htype *h)
+{
+	const struct htable *t;
+	u8 htable_bits;
+
+	rcu_read_lock();
+	t = rcu_dereference(h->table);
+	htable_bits = t->htable_bits;
+	rcu_read_unlock();
+
+	return jhash_size(htable_bits);
+}
+
+static u32 mtype_bucket_size(const struct htype *h)
+{
+	return h->bucketsize;
+}
+
 /* Reply a HEADER request: fill out the header part of the set */
 static int
 mtype_head(struct ip_set *set, struct sk_buff *skb)
@@ -1368,21 +1390,20 @@ mtype_head(struct ip_set *set, struct sk_buff *skb)
 	size_t memsize;
 	u32 elements = 0;
 	size_t ext_size = 0;
-	u8 htable_bits;
 
 	rcu_read_lock_bh();
 	t = rcu_dereference_bh(h->table);
 	mtype_ext_size(set, &elements, &ext_size);
 	memsize = mtype_ahash_memsize(h, t) + ext_size + atomic64_read(&set->ext_size);
-	htable_bits = t->htable_bits;
 	rcu_read_unlock_bh();
 
 	nested = nla_nest_start(skb, IPSET_ATTR_DATA);
 	if (!nested)
 		goto nla_put_failure;
-	if (nla_put_net32(skb, IPSET_ATTR_HASHSIZE,
-			  htonl(jhash_size(htable_bits))) ||
-	    nla_put_net32(skb, IPSET_ATTR_MAXELEM, htonl(h->maxelem)))
+
+	if (nla_put_net32(skb, IPSET_ATTR_HASHSIZE, htonl(mtype_hash_size(h))))
+		goto nla_put_failure;
+	if (nla_put_net32(skb, IPSET_ATTR_MAXELEM, htonl(h->maxelem)))
 		goto nla_put_failure;
 #ifdef IP_SET_HASH_WITH_BITMASK
 	/* if netmask is set to anything other than HOST_MASK we know that the user supplied netmask
@@ -1406,8 +1427,9 @@ mtype_head(struct ip_set *set, struct sk_buff *skb)
 		goto nla_put_failure;
 #endif
 	if (set->flags & IPSET_CREATE_FLAG_BUCKETSIZE) {
-		if (nla_put_u8(skb, IPSET_ATTR_BUCKETSIZE, h->bucketsize) ||
-		    nla_put_net32(skb, IPSET_ATTR_INITVAL, htonl(h->initval)))
+		if (nla_put_u8(skb, IPSET_ATTR_BUCKETSIZE, mtype_bucket_size(h)))
+			goto nla_put_failure;
+		if (nla_put_net32(skb, IPSET_ATTR_INITVAL, htonl(h->initval)))
 			goto nla_put_failure;
 	}
 	if (nla_put_net32(skb, IPSET_ATTR_REFERENCES, htonl(set->ref)) ||
@@ -1721,6 +1743,7 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
 	INIT_LIST_HEAD(&t->ad);
 	RCU_INIT_POINTER(h->table, t);
 	set->data = h;
+
 #ifndef IP_SET_PROTO_UNDEF
 	if (set->family == NFPROTO_IPV4) {
 #endif
@@ -1749,7 +1772,7 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
 #endif
 	}
 	pr_debug("create %s hashsize %u (%u) maxelem %u: %p(%p)\n",
-		 set->name, jhash_size(t->htable_bits),
+		 set->name, mtype_hash_size(h),
 		 t->htable_bits, h->maxelem, set->data, t);
 
 	return 0;
-- 
2.47.3

[PATCH net 06/10] netfilter: ipset: add and use mtype_del_cidr_all helper

[Pablo Neira Ayuso]

From: Florian Westphal <fw@strlen.de>

Reduces size of upcoming rhashtable conversion.

Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/ipset/ip_set_hash_gen.h | 34 +++++++++++++--------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index ef586b486f51..f00c82acd7f0 100644
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -195,6 +195,7 @@ static const union nf_inet_addr zeromask = {};
 #undef mtype_ext_cleanup
 #undef mtype_add_cidr
 #undef mtype_del_cidr
+#undef mtype_del_cidr_all
 #undef mtype_ahash_memsize
 #undef mtype_flush
 #undef mtype_destroy
@@ -242,6 +243,7 @@ static const union nf_inet_addr zeromask = {};
 #define mtype_ext_cleanup	IPSET_TOKEN(MTYPE, _ext_cleanup)
 #define mtype_add_cidr		IPSET_TOKEN(MTYPE, _add_cidr)
 #define mtype_del_cidr		IPSET_TOKEN(MTYPE, _del_cidr)
+#define mtype_del_cidr_all	IPSET_TOKEN(MTYPE, _del_cidr_all)
 #define mtype_ahash_memsize	IPSET_TOKEN(MTYPE, _ahash_memsize)
 #define mtype_flush		IPSET_TOKEN(MTYPE, _flush)
 #define mtype_destroy		IPSET_TOKEN(MTYPE, _destroy)
@@ -410,6 +412,17 @@ mtype_del_cidr(struct ip_set *set, struct htype *h, u8 cidr, u8 n)
 }
 #endif
 
+static void
+mtype_del_cidr_all(struct ip_set *set, struct htype *h, const struct mtype_elem *data)
+{
+#ifdef IP_SET_HASH_WITH_NETS
+	int k;
+
+	for (k = 0; k < IPSET_NET_COUNT; k++)
+		mtype_del_cidr(set, h, DCIDR_GET(data->cidr, k), k);
+#endif
+}
+
 /* Calculate the actual memory size of the set data */
 static size_t
 mtype_ahash_memsize(const struct htype *h, const struct htable *t)
@@ -551,9 +564,6 @@ mtype_gc_do(struct ip_set *set, struct htype *h, struct htable *t, u32 r)
 	struct mtype_elem *data;
 	u32 i, j, d;
 	size_t dsize = set->dsize;
-#ifdef IP_SET_HASH_WITH_NETS
-	u8 k;
-#endif
 	u8 pos, htable_bits = t->htable_bits;
 
 	spin_lock_bh(&t->hregion[r].lock);
@@ -574,11 +584,7 @@ mtype_gc_do(struct ip_set *set, struct htype *h, struct htable *t, u32 r)
 			pr_debug("expired %u/%u\n", i, j);
 			clear_bit(j, n->used);
 			smp_mb__after_atomic();
-#ifdef IP_SET_HASH_WITH_NETS
-			for (k = 0; k < IPSET_NET_COUNT; k++)
-				mtype_del_cidr(set, h,
-					DCIDR_GET(data->cidr, k), k);
-#endif
+			mtype_del_cidr_all(set, h, data);
 			t->hregion[r].elements--;
 			ip_set_ext_destroy(set, data);
 			d++;
@@ -1004,11 +1010,7 @@ mtype_add(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 			j = 0;
 		data = ahash_data(n, j, set->dsize);
 		if (!deleted) {
-#ifdef IP_SET_HASH_WITH_NETS
-			for (i = 0; i < IPSET_NET_COUNT; i++)
-				mtype_del_cidr(set, h,
-					DCIDR_GET(data->cidr, i), i);
-#endif
+			mtype_del_cidr_all(set, h, data);
 			ip_set_ext_destroy(set, data);
 			t->hregion[r].elements--;
 		}
@@ -1163,11 +1165,7 @@ mtype_del(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 		if (i + 1 == pos)
 			smp_store_release(&n->pos, --pos);
 		t->hregion[r].elements--;
-#ifdef IP_SET_HASH_WITH_NETS
-		for (j = 0; j < IPSET_NET_COUNT; j++)
-			mtype_del_cidr(set, h,
-				DCIDR_GET(d->cidr, j), j);
-#endif
+		mtype_del_cidr_all(set, h, d);
 		ip_set_ext_destroy(set, data);
 
 		if (t->resizing && ext && ext->target) {
-- 
2.47.3

[PATCH net 07/10] netfilter: ipset: switch to rcu work

[Pablo Neira Ayuso]

From: Florian Westphal <fw@strlen.de>

In the initial ipset rhashtable conversion RFC series syzbot reported
following splat:

BUG: sleeping function [..] at kernel/irq_work.c:289
in_atomic(): 1, [..]
 irq_work_sync.. kernel/irq_work.c:289
 rhashtable_free_and_destroy.. lib/rhashtable.c:1295
 hash_netport4_destroy.. net/netfilter/ipset/ip_set_hash_gen.h:420
 ip_set_destroy_set_rcu.. net/netfilter/ipset/ip_set_core.c:1169
 rcu_core.. kernel/rcu/tree.c:2897

This is because post-rhashtable-conversion hash implementation needs
to schedule in the destroy callback.  At this time this isn't allowed.

Replace existing call_rcu() based destruction with rcu_work api.

Also allows to undo split of set destruction and gc work cancelling in
a future patch.

Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netfilter/ipset/ip_set.h |  4 +--
 net/netfilter/ipset/ip_set_core.c      | 46 ++++++++++++++++----------
 2 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
index cadae9b2578f..c46864cc6623 100644
--- a/include/linux/netfilter/ipset/ip_set.h
+++ b/include/linux/netfilter/ipset/ip_set.h
@@ -244,8 +244,8 @@ extern void ip_set_type_unregister(struct ip_set_type *set_type);
 
 /* A generic IP set */
 struct ip_set {
-	/* For call_cru in destroy */
-	struct rcu_head rcu;
+	/* for set destruction */
+	struct rcu_work rwork;
 	/* The name of the set */
 	char name[IPSET_MAXNAMELEN];
 	/* Lock protecting the set data */
diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index 822a53a7f502..543851a923d0 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -25,6 +25,7 @@
 static LIST_HEAD(ip_set_type_list);		/* all registered set types */
 static DEFINE_MUTEX(ip_set_type_mutex);		/* protects ip_set_type_list */
 static DEFINE_RWLOCK(ip_set_ref_lock);		/* protects the set refs */
+static struct workqueue_struct *ipset_destroy_wq;
 
 struct ip_set_net {
 	struct ip_set * __rcu *ip_set_list;	/* all individual sets */
@@ -1178,22 +1179,26 @@ ip_set_setname_policy[IPSET_ATTR_CMD_MAX + 1] = {
 				    .len = IPSET_MAXNAMELEN - 1 },
 };
 
-/* In order to return quickly when destroying a single set, it is split
- * into two stages:
- * - Cancel garbage collector
- * - Destroy the set itself via call_rcu()
- */
-
 static void
-ip_set_destroy_set_rcu(struct rcu_head *head)
+destroy_and_free_set(struct ip_set *set)
 {
-	struct ip_set *set = container_of(head, struct ip_set, rcu);
-
 	set->variant->destroy(set);
 	module_put(set->type->me);
 	kfree(set);
 }
 
+/* In order to return quickly when destroying a single set,
+ * destruction is done asynchronously via work queues.
+ */
+static void
+ip_set_destroy_set_work(struct work_struct *work)
+{
+	struct ip_set *set = container_of(to_rcu_work(work),
+					  struct ip_set, rwork);
+
+	destroy_and_free_set(set);
+}
+
 static void
 _destroy_all_sets(struct ip_set_net *inst)
 {
@@ -1283,7 +1288,8 @@ static int ip_set_destroy(struct sk_buff *skb, const struct nfnl_info *info,
 			/* Must wait for flush to be really finished  */
 			rcu_barrier();
 		}
-		call_rcu(&s->rcu, ip_set_destroy_set_rcu);
+		INIT_RCU_WORK(&s->rwork, ip_set_destroy_set_work);
+		queue_rcu_work(ipset_destroy_wq, &s->rwork);
 	}
 	return 0;
 out:
@@ -2421,18 +2427,23 @@ static struct pernet_operations ip_set_net_ops = {
 static int __init
 ip_set_init(void)
 {
-	int ret = register_pernet_subsys(&ip_set_net_ops);
+	int ret;
+
+	ipset_destroy_wq = alloc_ordered_workqueue("ipset_destroy_wq", 0);
+	if (!ipset_destroy_wq)
+		return -ENOMEM;
 
+	ret = register_pernet_subsys(&ip_set_net_ops);
 	if (ret) {
 		pr_err("ip_set: cannot register pernet_subsys.\n");
-		return ret;
+		goto out_wq;
 	}
 
 	ret = nfnetlink_subsys_register(&ip_set_netlink_subsys);
 	if (ret != 0) {
 		pr_err("ip_set: cannot register with nfnetlink.\n");
 		unregister_pernet_subsys(&ip_set_net_ops);
-		return ret;
+		goto out_wq;
 	}
 
 	ret = nf_register_sockopt(&so_set);
@@ -2440,10 +2451,13 @@ ip_set_init(void)
 		pr_err("SO_SET registry failed: %d\n", ret);
 		nfnetlink_subsys_unregister(&ip_set_netlink_subsys);
 		unregister_pernet_subsys(&ip_set_net_ops);
-		return ret;
+		goto out_wq;
 	}
 
 	return 0;
+out_wq:
+	destroy_workqueue(ipset_destroy_wq);
+	return ret;
 }
 
 static void __exit
@@ -2453,9 +2467,7 @@ ip_set_fini(void)
 	nfnetlink_subsys_unregister(&ip_set_netlink_subsys);
 	unregister_pernet_subsys(&ip_set_net_ops);
 
-	/* Wait for call_rcu() in destroy */
-	rcu_barrier();
-
+	destroy_workqueue(ipset_destroy_wq);
 	pr_debug("these are the famous last words\n");
 }
 
-- 
2.47.3

[PATCH net 08/10] ipvs: avoid out-of-bounds write in ip_vs_nat_icmp

[Pablo Neira Ayuso]

From: Julian Anastasov <ja@ssi.bg>

Sashiko warns that local attacker can modify the packet
while it is processed by IPVS. Some places read the
IP ihl field multiple times which can cause out-of-bounds
access. One such place is ip_vs_nat_icmp where we
can write after the validated area.

Fix it by providing ciph argument just like it is done for
IPv6 and use ciph->len as offset to the embedded transport
header.

Modify some IPv4 header checks by reading the ihl field
only once.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Link: https://sashiko.dev/#/patchset/20260722101517.36313-1-ja%40ssi.bg
Signed-off-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/ip_vs.h             |  2 +-
 net/netfilter/ipvs/ip_vs_core.c | 67 +++++++++++++++++----------------
 net/netfilter/ipvs/ip_vs_xmit.c |  2 +-
 3 files changed, 36 insertions(+), 35 deletions(-)

diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index e6ca930a3507..1235f1934e94 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -2062,7 +2062,7 @@ static inline bool ip_vs_conn_use_hash2(struct ip_vs_conn *cp)
 
 void ip_vs_nat_icmp(struct sk_buff *skb, struct ip_vs_protocol *pp,
 		    struct ip_vs_conn *cp, int dir, unsigned int toff,
-		    bool has_ports);
+		    bool has_ports, struct ip_vs_iphdr *ciph);
 
 #ifdef CONFIG_IP_VS_IPV6
 void ip_vs_nat_icmp_v6(struct sk_buff *skb, struct ip_vs_protocol *pp,
diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index 6b79e0c4d9e2..0bdaeb4ed61e 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -925,28 +925,27 @@ static int ip_vs_route_me_harder(struct netns_ipvs *ipvs, int af,
  */
 void ip_vs_nat_icmp(struct sk_buff *skb, struct ip_vs_protocol *pp,
 		    struct ip_vs_conn *cp, int inout, unsigned int toff,
-		    bool has_ports)
+		    bool has_ports, struct ip_vs_iphdr *ciph)
 {
 	struct iphdr *iph	 = ip_hdr(skb);
 	struct icmphdr *icmph	 = (struct icmphdr *)(skb->data + toff);
-	struct iphdr *ciph	 = (struct iphdr *)(icmph + 1);
-	unsigned int coff __maybe_unused = toff + sizeof(struct icmphdr);
+	struct iphdr *cih	 = (struct iphdr *)(icmph + 1);
 
 	if (inout) {
 		iph->saddr = cp->vaddr.ip;
 		ip_send_check(iph);
-		ciph->daddr = cp->vaddr.ip;
-		ip_send_check(ciph);
+		cih->daddr = cp->vaddr.ip;
+		ip_send_check(cih);
 	} else {
 		iph->daddr = cp->daddr.ip;
 		ip_send_check(iph);
-		ciph->saddr = cp->daddr.ip;
-		ip_send_check(ciph);
+		cih->saddr = cp->daddr.ip;
+		ip_send_check(cih);
 	}
 
 	/* the TCP/UDP/SCTP port */
 	if (has_ports) {
-		__be16 *ports = (void *)ciph + ciph->ihl*4;
+		__be16 *ports = (void *)(skb->data + ciph->len);
 
 		if (inout)
 			ports[1] = cp->vport;
@@ -960,10 +959,10 @@ void ip_vs_nat_icmp(struct sk_buff *skb, struct ip_vs_protocol *pp,
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 	if (inout)
-		IP_VS_DBG_PKT(11, AF_INET, pp, skb, coff,
+		IP_VS_DBG_PKT(11, AF_INET, pp, skb, ciph->off,
 			      "Forwarding altered outgoing ICMP");
 	else
-		IP_VS_DBG_PKT(11, AF_INET, pp, skb, coff,
+		IP_VS_DBG_PKT(11, AF_INET, pp, skb, ciph->off,
 			      "Forwarding altered incoming ICMP");
 }
 
@@ -1056,7 +1055,7 @@ static int handle_response_icmp(int af, struct sk_buff *skb,
 		ip_vs_nat_icmp_v6(skb, pp, cp, 1, toff, has_ports, ciph);
 	else
 #endif
-		ip_vs_nat_icmp(skb, pp, cp, 1, toff, has_ports);
+		ip_vs_nat_icmp(skb, pp, cp, 1, toff, has_ports, ciph);
 
 	if (ip_vs_route_me_harder(cp->ipvs, af, skb, hooknum))
 		goto out;
@@ -1092,7 +1091,7 @@ static int ip_vs_out_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb,
 	struct ip_vs_iphdr ciph;
 	struct ip_vs_conn *cp;
 	struct ip_vs_protocol *pp;
-	unsigned int offset, ihl;
+	unsigned int offset;
 	union nf_inet_addr snet;
 
 	*related = 1;
@@ -1105,7 +1104,6 @@ static int ip_vs_out_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb,
 			return NF_ACCEPT;
 	}
 
-	ihl = ipvsh->len;
 	offset = ipvsh->len;
 	ic = skb_header_pointer(skb, offset, sizeof(_icmph), &_icmph);
 	if (ic == NULL)
@@ -1131,11 +1129,15 @@ static int ip_vs_out_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb,
 
 	/* Now find the contained IP header */
 	offset += sizeof(_icmph);
+	if (!ip_vs_fill_iph_skb_icmp(AF_INET, skb, offset, true, &ciph))
+		return NF_ACCEPT; /* The packet looks wrong, ignore */
+
 	cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
-	if (!(cih && cih->version == 4 && cih->ihl >= 5))
+	if (!(cih && cih->version == 4 &&
+	      ciph.len - ciph.off >= sizeof(struct iphdr)))
 		return NF_ACCEPT; /* The packet looks wrong, ignore */
 
-	pp = ip_vs_proto_get(cih->protocol);
+	pp = ip_vs_proto_get(ciph.protocol);
 	if (!pp)
 		return NF_ACCEPT;
 
@@ -1146,8 +1148,6 @@ static int ip_vs_out_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb,
 	IP_VS_DBG_PKT(11, AF_INET, pp, skb, offset,
 		      "Checking outgoing ICMP for");
 
-	ip_vs_fill_iph_skb_icmp(AF_INET, skb, offset, true, &ciph);
-
 	/* The embedded headers contain source and dest in reverse order */
 	cp = INDIRECT_CALL_1(pp->conn_out_get, ip_vs_conn_out_get_proto,
 			     ipvs, AF_INET, skb, &ciph);
@@ -1155,8 +1155,8 @@ static int ip_vs_out_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb,
 		return NF_ACCEPT;
 
 	snet.ip = ipvsh->saddr.ip;
-	return handle_response_icmp(AF_INET, skb, &snet, cp, pp, &ciph, ihl,
-				    hooknum);
+	return handle_response_icmp(AF_INET, skb, &snet, cp, pp, &ciph,
+				    ipvsh->len, hooknum);
 }
 
 #ifdef CONFIG_IP_VS_IPV6
@@ -1803,10 +1803,12 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
 	/* Now find the contained IP header */
 	offset += sizeof(_icmph);
 	cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
-	if (!(cih && cih->version == 4 && cih->ihl >= 5))
+	if (!cih)
 		return NF_ACCEPT; /* The packet looks wrong, ignore */
-	raddr = (union nf_inet_addr *)&cih->daddr;
 	hlen_ipip = cih->ihl * 4;
+	if (!(cih->version == 4 && hlen_ipip >= sizeof(struct iphdr)))
+		return NF_ACCEPT; /* The packet looks wrong, ignore */
+	raddr = (union nf_inet_addr *)&cih->daddr;
 
 	/* Special case for errors for IPIP/UDP/GRE tunnel packets */
 	tunnel = false;
@@ -1823,9 +1825,6 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
 		if (!dest || dest->tun_type != IP_VS_CONN_F_TUNNEL_TYPE_IPIP)
 			return NF_ACCEPT;
 		offset += hlen_ipip;
-		cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
-		if (!(cih && cih->version == 4 && cih->ihl >= 5))
-			return NF_ACCEPT; /* The packet looks wrong, ignore */
 		tunnel = true;
 	} else if ((cih->protocol == IPPROTO_UDP ||	/* Can be UDP encap */
 		    cih->protocol == IPPROTO_GRE) &&	/* Can be GRE encap */
@@ -1850,21 +1849,25 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
 			/* Skip IP and UDP/GRE tunnel headers */
 			offset = offset2 + ulen;
 			/* Now we should be at the original IP header */
-			cih = skb_header_pointer(skb, offset, sizeof(_ciph),
-						 &_ciph);
-			if (cih && cih->version == 4 && cih->ihl >= 5 &&
-			    iproto == IPPROTO_IPIP)
+			if (iproto == IPPROTO_IPIP)
 				tunnel = true;
 			else
 				return NF_ACCEPT;
 		}
 	}
 
-	pd = ip_vs_proto_data_get(ipvs, cih->protocol);
+	if (!ip_vs_fill_iph_skb_icmp(AF_INET, skb, offset, !tunnel, &ciph))
+		return NF_ACCEPT;
+	pd = ip_vs_proto_data_get(ipvs, ciph.protocol);
 	if (!pd)
 		return NF_ACCEPT;
 	pp = pd->pp;
 
+	cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
+	if (!(cih && cih->version == 4 &&
+	      ciph.len - ciph.off >= sizeof(struct iphdr)))
+		return NF_ACCEPT; /* The packet looks wrong, ignore */
+
 	/* Is the embedded protocol header present? */
 	if (unlikely(cih->frag_off & htons(IP_OFFSET) && !pp->dont_defrag))
 		return NF_ACCEPT;
@@ -1872,9 +1875,6 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
 	IP_VS_DBG_PKT(11, AF_INET, pp, skb, offset,
 		      "Checking incoming ICMP for");
 
-	offset2 = offset;
-	ip_vs_fill_iph_skb_icmp(AF_INET, skb, offset, !tunnel, &ciph);
-
 	/* The embedded headers contain source and dest in reverse order.
 	 * For IPIP/UDP/GRE tunnel this is error for request, not for reply.
 	 */
@@ -1904,11 +1904,12 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
 	}
 
 	if (tunnel) {
-		unsigned int hlen_orig = cih->ihl * 4;
+		unsigned int hlen_orig = ciph.len - ciph.off;
 		__be32 info = ic->un.gateway;
 		__u8 type = ic->type;
 		__u8 code = ic->code;
 
+		offset2 = offset;
 		/* Update the MTU */
 		if (ic->type == ICMP_DEST_UNREACH &&
 		    ic->code == ICMP_FRAG_NEEDED) {
diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index 0b0c5304993a..c4508f3f43dd 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -1580,7 +1580,7 @@ ip_vs_icmp_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
 	if (skb_cow(skb, rt->dst.dev->hard_header_len))
 		goto tx_error;
 
-	ip_vs_nat_icmp(skb, pp, cp, 0, toff, has_ports);
+	ip_vs_nat_icmp(skb, pp, cp, 0, toff, has_ports, ciph);
 
 	/* Another hack: avoid icmp_send in ip_fragment */
 	skb->ignore_df = 1;
-- 
2.47.3

[PATCH net 09/10] ipvs: return the csum validation for forward hook

[Pablo Neira Ayuso]

From: Julian Anastasov <ja@ssi.bg>

Sashiko notes that playing games with the skb dst and rt
flags instead of providing hooknum is not a good idea
when validating the checksums.

Also, skipping checksum validation for FORWARD packets
risk silent data corruption, even if the only user is
the FTP-CMD packets coming from the real server.

Sashiko also noticed that by using common checksum
helper in the previous commit we actually fixed old bug
where the TCP/UDP checksum for IPv6 on CHECKSUM_COMPLETE
was not validated correctly.

Fixes: e876b75b9020 ("ipvs: fix the checksum validations")
Link: https://sashiko.dev/#/patchset/20260722211420.153933-1-pablo%40netfilter.org
Link: https://sashiko.dev/#/patchset/20260727185024.67534-1-ja%40ssi.bg
Link: https://sashiko.dev/#/patchset/20260728202520.59179-1-ja%40ssi.bg
Signed-off-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/ip_vs.h                   | 19 +++++--------------
 net/netfilter/ipvs/ip_vs_proto_sctp.c |  2 +-
 2 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index 1235f1934e94..d2813eb795be 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -25,9 +25,7 @@
 #include <linux/netfilter.h>		/* for union nf_inet_addr */
 #include <linux/ip.h>
 #include <linux/ipv6.h>			/* for struct ipv6hdr */
-#include <net/route.h>
 #include <net/ipv6.h>
-#include <net/ip6_fib.h>
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
 #include <net/netfilter/nf_conntrack.h>
 #endif
@@ -2095,30 +2093,23 @@ static inline __wsum ip_vs_check_diff2(__be16 old, __be16 new, __wsum oldsum)
 	return csum_partial(diff, sizeof(diff), oldsum);
 }
 
-static inline bool ip_vs_checksum_needed(struct sk_buff *skb, int af)
+static inline bool ip_vs_checksum_needed(struct sk_buff *skb)
 {
 	/* Checksum unnecessary or already validated? */
 	if (skb_csum_unnecessary(skb))
 		return false;
-	/* LOCAL_OUT ? */
-	if (!skb->dev || skb->dev->flags & IFF_LOOPBACK)
+	/* Locally generated ? */
+	if (!skb->dev)
 		return false;
-	/* !LOCAL_IN (FORWARD) ? */
-	if (af == AF_INET6) {
-		if (!(dst_rt6_info(skb_dst(skb))->rt6i_flags & RTF_LOCAL))
-			return false;
-	} else {
-		if (!(skb_rtable(skb)->rt_flags & RTCF_LOCAL))
-			return false;
-	}
 	return true;
 }
 
 static inline bool ip_vs_checksum_common_check(struct sk_buff *skb,
 					       int offset, int proto, int af)
 {
-	if (!ip_vs_checksum_needed(skb, af))
+	if (!ip_vs_checksum_needed(skb))
 		return true;
+	/* Validate csum even for FORWARD */
 	return !nf_checksum(skb, NF_INET_LOCAL_IN, offset, proto, af);
 }
 
diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
index 3dbd3096e163..c80567c73469 100644
--- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
@@ -193,7 +193,7 @@ sctp_csum_check(int af, struct sk_buff *skb, struct ip_vs_protocol *pp,
 	struct sctphdr *sh;
 	__le32 cmp, val;
 
-	if (!ip_vs_checksum_needed(skb, af))
+	if (!ip_vs_checksum_needed(skb))
 		return 1;
 	sh = (struct sctphdr *)(skb->data + sctphoff);
 	cmp = sh->checksum;
-- 
2.47.3

[PATCH net 10/10] netfilter: nft_ct: move custom expectation support to helper

[Pablo Neira Ayuso]

Originally, the ct expectation support called nf_ct_helper_ext_add() for
confirmed conntracks, which is invalid, triggering a splat. This was
fixed by commit 1710eb913bdc ("netfilter: nft_ct: skip expectations for
confirmed conntrack") which restricted it to confirmed conntracks.

However, early insertion of expectations into the expectations list when
the conntrack is unconfirmed leads to stale entries pointing to the
wrong hlist_head through .pprev due to ct extension reallocation.

Commit 7c9664351980 ("netfilter: move nat hlist_head to nf_conn") moved
the nat hlist_head to nf_conn for this reason:

     1. ...
     2. When reallocation of extension area occurs we need to fixup the
        bysource hash head via hlist_replace_rcu.

But I'd rather not increase the size of the struct nf_conn for this
feature, it only supports for creating expectations in the other
direction and it was broken with DNAT too.

The existing feature has very limited scope because of a pre-existing
issue: two different connections can create the same expectation leading
to expect_clash(), resulting in packet drops.

To address this issue, add an internal ct helper and attach it to the
conntrack entry to streamline the custom ct expectation support with
existing ct helpers.

Expose a new nf_conntrack_helper_free() function to safely release the
internal helper that is allocated and attached to the conntrack entry to
create the custom expectations.

Fixes: 857b46027d6f ("netfilter: nft_ct: add ct expectations support")
Reported-by: Jaeyeong Lee <iostreampy@proton.me>
Link: https://patch.msgid.link/20260715144755.00ea7dfcd9f@proton.me
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_conntrack_helper.h |   2 +
 net/netfilter/nf_conntrack_helper.c         |  18 ++-
 net/netfilter/nft_ct.c                      | 127 +++++++++++++++-----
 3 files changed, 114 insertions(+), 33 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index c761cd8158b2..4420846e41f2 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -114,6 +114,8 @@ int nf_conntrack_helpers_register(struct nf_conntrack_helper *, unsigned int,
 void nf_conntrack_helpers_unregister(struct nf_conntrack_helper **,
 				     unsigned int);
 
+void nf_conntrack_helper_free(struct nf_conntrack_helper *me);
+
 #define nf_conntrack_helper_deprecated(name) \
 	pr_warn("The %s conntrack helper is scheduled for removal.\n"	\
 		"Please contact the netfilter-devel mailing list if you still need this.\n", name)
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 500509b17663..1197e8793494 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -456,13 +456,8 @@ static bool expect_iter_me(struct nf_conntrack_expect *exp, void *data)
 	return this == me;
 }
 
-void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
+void nf_conntrack_helper_free(struct nf_conntrack_helper *me)
 {
-	mutex_lock(&nf_ct_helper_mutex);
-	hlist_del_rcu(&me->hnode);
-	nf_ct_helper_count--;
-	mutex_unlock(&nf_ct_helper_mutex);
-
 	/* This helper is going away, disable it. */
 	rcu_assign_pointer(me->help, NULL);
 
@@ -476,6 +471,17 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
 	if (refcount_dec_and_test(&me->ct_refcnt))
 		kfree_rcu(me, rcu);
 }
+EXPORT_SYMBOL_GPL(nf_conntrack_helper_free);
+
+void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
+{
+	mutex_lock(&nf_ct_helper_mutex);
+	hlist_del_rcu(&me->hnode);
+	nf_ct_helper_count--;
+	mutex_unlock(&nf_ct_helper_mutex);
+
+	nf_conntrack_helper_free(me);
+}
 EXPORT_SYMBOL_GPL(nf_conntrack_helper_unregister);
 
 void nf_ct_helper_init(struct nf_conntrack_helper *helper,
diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index 03a88c77e0f0..30c9358dbf48 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -1213,6 +1213,8 @@ struct nft_ct_expect_obj {
 	u8		l4proto;
 	u8		size;
 	u32		timeout;
+
+	struct nf_conntrack_helper *helper;
 };
 
 static int nft_ct_expect_timeout_get(const struct nlattr *attr, u32 *val)
@@ -1226,6 +1228,73 @@ static int nft_ct_expect_timeout_get(const struct nlattr *attr, u32 *val)
 	return 0;
 }
 
+struct nft_ct_expect_data {
+	struct nft_ct_expect_obj	obj;
+	enum ip_conntrack_dir		dir;
+	atomic_t			num_expects;
+};
+
+static int ct_expect_help(struct sk_buff *skb, unsigned int protoff,
+			  struct nf_conn *ct, enum ip_conntrack_info ctinfo)
+{
+	enum ip_conntrack_dir dir = CTINFO2DIR(ctinfo);
+	struct nft_ct_expect_data *expect_data;
+	struct nf_conntrack_expect *exp;
+	int ret = NF_ACCEPT;
+
+	expect_data = nfct_help_data(ct);
+	if (!expect_data)
+		return NF_ACCEPT;
+
+	if (expect_data->dir != dir)
+		return NF_ACCEPT;
+
+	if (!atomic_add_unless(&expect_data->num_expects, 1, expect_data->obj.size))
+		return NF_ACCEPT;
+
+	exp = nf_ct_expect_alloc(ct);
+	if (!exp) {
+		atomic_dec(&expect_data->num_expects);
+		return NF_DROP;
+	}
+
+	nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT, nf_ct_l3num(ct),
+			  &ct->tuplehash[!dir].tuple.src.u3,
+			  &ct->tuplehash[!dir].tuple.dst.u3,
+			  expect_data->obj.l4proto, NULL, &expect_data->obj.dport);
+	exp->timeout += expect_data->obj.timeout;
+
+	if (nf_ct_expect_related(exp, 0) != 0) {
+		atomic_dec(&expect_data->num_expects);
+		ret = NF_DROP;
+	}
+
+	nf_ct_expect_put(exp);
+
+	return ret;
+}
+
+static int nft_ct_expect_helper_alloc(struct nft_ct_expect_obj *priv)
+{
+	struct nf_conntrack_helper *ct_expect_helper;
+
+	ct_expect_helper = kzalloc_obj(struct nf_conntrack_helper);
+	if (!ct_expect_helper)
+		return -ENOMEM;
+
+	snprintf(ct_expect_helper->name, sizeof(ct_expect_helper->name), "%s",
+		 "nft_ct_expect");
+	ct_expect_helper->me = THIS_MODULE;
+	ct_expect_helper->expect_policy[NF_CT_EXPECT_CLASS_DEFAULT].max_expected = priv->size;
+	rcu_assign_pointer(ct_expect_helper->help, ct_expect_help);
+	refcount_set(&ct_expect_helper->ct_refcnt, 1);
+
+	/* No need to register this helper, this is internal. */
+	priv->helper = ct_expect_helper;
+
+	return 0;
+}
+
 static int nft_ct_expect_obj_init(const struct nft_ctx *ctx,
 				  const struct nlattr * const tb[],
 				  struct nft_object *obj)
@@ -1233,6 +1302,8 @@ static int nft_ct_expect_obj_init(const struct nft_ctx *ctx,
 	struct nft_ct_expect_obj *priv = nft_obj_data(obj);
 	int err;
 
+	NF_CT_HELPER_BUILD_BUG_ON(sizeof(struct nft_ct_expect_data));
+
 	if (!tb[NFTA_CT_EXPECT_L4PROTO] ||
 	    !tb[NFTA_CT_EXPECT_DPORT] ||
 	    !tb[NFTA_CT_EXPECT_TIMEOUT] ||
@@ -1273,13 +1344,26 @@ static int nft_ct_expect_obj_init(const struct nft_ctx *ctx,
 	priv->dport = nla_get_be16(tb[NFTA_CT_EXPECT_DPORT]);
 	priv->size = nla_get_u8(tb[NFTA_CT_EXPECT_SIZE]);
 
-	return nf_ct_netns_get(ctx->net, ctx->family);
+	err = nf_ct_netns_get(ctx->net, ctx->family);
+	if (err < 0)
+		return err;
+
+	err = nft_ct_expect_helper_alloc(priv);
+	if (err < 0) {
+		nf_ct_netns_put(ctx->net, ctx->family);
+		return err;
+	}
+
+	return err;
 }
 
 static void nft_ct_expect_obj_destroy(const struct nft_ctx *ctx,
-				       struct nft_object *obj)
+				      struct nft_object *obj)
 {
+	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
+
 	nf_ct_netns_put(ctx->net, ctx->family);
+	nf_conntrack_helper_free(priv->helper);
 }
 
 static int nft_ct_expect_obj_dump(struct sk_buff *skb,
@@ -1302,50 +1386,39 @@ static void nft_ct_expect_obj_eval(struct nft_object *obj,
 				   const struct nft_pktinfo *pkt)
 {
 	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
-	struct nf_conntrack_expect *exp;
+	struct nft_ct_expect_data *expect_data;
 	enum ip_conntrack_info ctinfo;
 	struct nf_conn_help *help;
-	enum ip_conntrack_dir dir;
-	u16 l3num = priv->l3num;
 	struct nf_conn *ct;
 
 	ct = nf_ct_get(pkt->skb, &ctinfo);
-	if (!ct || nf_ct_is_confirmed(ct) || nf_ct_is_template(ct)) {
+	if (!ct || nf_ct_is_template(ct) || nf_ct_is_confirmed(ct)) {
 		regs->verdict.code = NFT_BREAK;
 		return;
 	}
-	dir = CTINFO2DIR(ctinfo);
 
 	help = nfct_help(ct);
-	if (!help)
-		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
-	if (!help) {
-		regs->verdict.code = NF_DROP;
-		return;
-	}
-
-	if (help->expecting[NF_CT_EXPECT_CLASS_DEFAULT] >= priv->size) {
+	if (help) {
 		regs->verdict.code = NFT_BREAK;
 		return;
 	}
-	if (l3num == NFPROTO_INET)
-		l3num = nf_ct_l3num(ct);
 
-	exp = nf_ct_expect_alloc(ct);
-	if (exp == NULL) {
+	help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
+	if (!help) {
 		regs->verdict.code = NF_DROP;
 		return;
 	}
-	nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT, l3num,
-		          &ct->tuplehash[!dir].tuple.src.u3,
-		          &ct->tuplehash[!dir].tuple.dst.u3,
-		          priv->l4proto, NULL, &priv->dport);
-	exp->timeout += priv->timeout;
 
-	if (nf_ct_expect_related(exp, 0) != 0)
-		regs->verdict.code = NF_DROP;
+	expect_data = nfct_help_data(ct);
+	if (!expect_data) {
+		regs->verdict.code = NFT_BREAK;
+		return;
+	}
+	expect_data->obj = *priv;
+	expect_data->dir = CTINFO2DIR(ctinfo);
 
-	nf_ct_expect_put(exp);
+	if (help && refcount_inc_not_zero(&priv->helper->ct_refcnt))
+		rcu_assign_pointer(help->helper, priv->helper);
 }
 
 static const struct nla_policy nft_ct_expect_policy[NFTA_CT_EXPECT_MAX + 1] = {
-- 
2.47.3

Re: [PATCH net 02/10] netfilter: ebt_nflog: pin the NFLOG backend

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: ebt_nflog: pin the NFLOG backend

This adds a module reference on the ULOG logger when an ebtables nflog
rule is validated, and drops it from a new .destroy callback, so that
nfnetlink_log cannot be unloaded while such a rule is installed.  It also
requests the nfnetlink_log module for legacy (non nft_compat) callers.

A few questions about the change and the commit message.

> nf_log_unregister() runs after the per-net teardown so its final RCU
> grace period also drains readers that obtained the logger from a per-net
> binding.

[Severity: High]
Is this description accurate?  This isn't a bug introduced by this patch,
but the ordering seems to be the other way around.

nfnetlink_log_fini() frees the per-net state first:

net/netfilter/nfnetlink_log.c:nfnetlink_log_fini() {
	...
	unregister_pernet_subsys(&nfnl_log_net_ops);
	nf_log_unregister(&nfulnl_logger);
	...
}

unregister_pernet_subsys() reaches ops_undo_list(), where the only
synchronize_rcu() happens before the exit callbacks run, and the kfree
follows immediately after them:

net/core/net_namespace.c:ops_undo_list() {
	...
	list_for_each_entry_continue_reverse(ops, ops_list, list)
		ops_exit_list(ops, net_exit_list);

	ops = saved_ops;
	list_for_each_entry_continue_reverse(ops, ops_list, list)
		ops_free_list(ops, net_exit_list);
	...
}

So the grace period inside nf_log_unregister() runs after struct
nfnl_log_net has already been freed, which would mean it protects the
module text rather than the per-net data.

Does the per-net binding path stay exposed after this patch?  A logger
bound per-net (sysctl /proc/sys/net/netfilter/nf_log/<pf>, or
nf_log_bind_pf()) is only cleared in the pernet exit hook:

net/netfilter/nfnetlink_log.c:nfnl_log_net_exit() {
	nf_log_unset(net, &nfulnl_logger);
	...
}

and nf_log_unset() does not wait for readers, since the grace period was
removed by the commit named in the Fixes: tag:

net/netfilter/nf_log.c:nf_log_unset() {
	...
			RCU_INIT_POINTER(net->nf.nf_loggers[i], NULL);
	...
}

A reader that resolved the logger through the per-net branch of
nf_log_packet():

	if (loginfo != NULL)
		logger = rcu_dereference(loggers[pf][loginfo->type]);
	else
		logger = rcu_dereference(net->nf.nf_loggers[pf]);

can still be inside nfulnl_log_packet() -> nfnl_log_pernet(net) ->
instance_lookup_get_rcu() -> __instance_lookup() when ops_free_list()
frees the blob, which is the same KASAN signature quoted above with no
ebtables rule involved.  Reachable loginfo == NULL callers today include
nf_log_trace(), the conntrack helper log in net/netfilter/nf_conntrack_helper.c
and net/netfilter/nfnetlink_osf.c.

Would reordering nfnetlink_log_fini() so that nf_log_unregister() runs
before unregister_pernet_subsys() close all of the frontends at once?

> diff --git a/net/bridge/netfilter/ebt_nflog.c b/net/bridge/netfilter/ebt_nflog.c
> index 61bf8f4465ab7..426f8adc912c7 100644
> --- a/net/bridge/netfilter/ebt_nflog.c
> +++ b/net/bridge/netfilter/ebt_nflog.c
> @@ -41,11 +41,25 @@ ebt_nflog_tg(struct sk_buff *skb, const struct xt_action_param *par)
>  static int ebt_nflog_tg_check(const struct xt_tgchk_param *par)
>  {
>  	struct ebt_nflog_info *info = par->targinfo;
> +	int ret;
>  
>  	if (info->flags & ~EBT_NFLOG_MASK)
>  		return -EINVAL;
>  	info->prefix[EBT_NFLOG_PREFIX_SIZE - 1] = '\0';
> -	return 0;
> +
> +	ret = nf_logger_find_get(par->family, NF_LOG_TYPE_ULOG);

[Severity: Medium]
Should the commit message mention that this can reject rulesets that used
to load?  ebt_nflog_tg_check() previously returned 0 unconditionally, so an
ebtables nflog rule installed fine and simply logged nothing when
loggers[pf][NF_LOG_TYPE_ULOG] was NULL.  Now the raw -ENOENT from
nf_logger_find_get() is propagated out through xt_check_target(), so
ebtables -A ... --nflog and ebtables-restore fail.  This seems worth
spelling out given the Cc: stable.

Also, is the stated parity with xt_NFLOG complete?  NETFILTER_XT_TARGET_NFLOG
carries select NETFILTER_NETLINK_LOG in net/netfilter/Kconfig, while
BRIDGE_EBT_NFLOG has no select or depends:

net/bridge/netfilter/Kconfig:
config BRIDGE_EBT_NFLOG
	tristate "ebt: nflog support"
	help
	  This option enables the nflog watcher, which allows to LOG
	  messages through the netfilter logging API, which can use
	  either the old LOG target, the old ULOG target or nfnetlink_log
	  as backend.

On a kernel with CONFIG_BRIDGE_EBT_NFLOG set and
CONFIG_NETFILTER_NETLINK_LOG=n, the request_module() fallback below can
never succeed, so the target becomes unusable.  Should a
select NETFILTER_NETLINK_LOG be added here?  The help text claiming the
watcher can use the old LOG or ULOG targets as backend also no longer
matches the code.

> +	if (ret != 0 && !par->nft_compat) {
> +		request_module("%s", "nfnetlink_log");
> +
> +		ret = nf_logger_find_get(par->family, NF_LOG_TYPE_ULOG);
> +	}
> +
> +	return ret;

[Severity: Medium]
Who loads nfnetlink_log for the nft_compat case?  The !par->nft_compat
skip is copied from xt_NFLOG, but xt_NFLOG also declares
MODULE_SOFTDEP("pre: nfnetlink_log"), and ebt_nflog.c has no
MODULE_SOFTDEP at all, so demand-loading ebt_nflog through
xt_request_find_target(NFPROTO_BRIDGE, "nflog", rev) does not bring in the
backend.

The -ENOENT recovery in nft_compat only matches the uppercase names:

net/netfilter/nft_compat.c:nft_target_init() {
	...
		if (ret == -ENOENT) {
			const char *modname = NULL;

			if (strcmp(target->name, "LOG") == 0)
				modname = "nf_log_syslog";
			else if (strcmp(target->name, "NFLOG") == 0)
				modname = "nfnetlink_log";

			if (modname &&
			    nft_request_module(ctx->net, "%s", modname) == -EAGAIN)
				return -EAGAIN;
		}
	...
}

The target registered here is .name = "nflog", so modname stays NULL, no
module is requested and there is no -EAGAIN retry.  Does an nftables
bridge-family compat rule using nflog now fail with -ENOENT unless
nfnetlink_log is already resident, where it succeeded before?  Adding
MODULE_SOFTDEP("pre: nfnetlink_log") to ebt_nflog.c, or extending the name
mapping in nft_target_init(), would seem to cover it.

> +}
> +
> +static void ebt_nflog_tg_destroy(const struct xt_tgdtor_param *par)
> +{
> +	nf_logger_put(par->family, NF_LOG_TYPE_ULOG);
>  }
>  
>  static struct xt_target ebt_nflog_tg_reg __read_mostly = {
> @@ -54,6 +68,7 @@ static struct xt_target ebt_nflog_tg_reg __read_mostly = {
>  	.family     = NFPROTO_BRIDGE,
>  	.target     = ebt_nflog_tg,
>  	.checkentry = ebt_nflog_tg_check,
> +	.destroy    = ebt_nflog_tg_destroy,
>  	.targetsize = sizeof(struct ebt_nflog_info),
>  	.me         = THIS_MODULE,
>  };

Re: [PATCH net 03/10] netfilter: ipset: rework cidr bookkeeping

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: ipset: rework cidr bookkeeping

The prefix bookkeeping of the hash:net* set types is moved out of struct
htype into separately allocated, RCU published struct net_prefixes arrays.
The in-place shifting of the cidr array is replaced by allocate-copy-publish,
with a documented fallback to leaving holes when the allocation fails.

A few questions about the new bookkeeping below.

> diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
> index b2d77973272d2..dd31992c915ca 100644
> --- a/net/netfilter/ipset/ip_set_hash_gen.h
> +++ b/net/netfilter/ipset/ip_set_hash_gen.h
> @@ -99,9 +99,15 @@ struct htable {
>  #endif
>  
>  /* Book-keeping of the prefixes added to the set */
> +struct net_prefix {
> +	u8 cidr;			/* the cidr value */
> +	u32 count;			/* number of elements of this cidr */
> +};
> +
>  struct net_prefixes {
> -	u32 nets[IPSET_NET_COUNT]; /* number of elements for this cidr */
> -	u8 cidr[IPSET_NET_COUNT];  /* the cidr value */
> +	struct rcu_head rcu;
> +	u8 len;
> +	struct net_prefix nets[] __counted_by(len);
>  };
>  
>  /* Compute the hash table size */
> @@ -127,11 +133,6 @@ htable_size(u8 hbits)
>  #else
>  #define __CIDR(cidr, i)		(cidr)
>  #endif
> -
> -/* cidr + 1 is stored in net_prefixes to support /0 */
> -#define NCIDR_PUT(cidr)		((cidr) + 1)
> -#define NCIDR_GET(cidr)		((cidr) - 1)
> -
>  #ifdef IP_SET_HASH_WITH_NETS_PACKED
>  /* When cidr is packed with nomatch, cidr - 1 is stored in the data entry */
>  #define DCIDR_PUT(cidr)		((cidr) - 1)
> @@ -141,21 +142,11 @@ htable_size(u8 hbits)
>  #define DCIDR_GET(cidr, i)	__CIDR(cidr, i)
>  #endif
>  
> -#define INIT_CIDR(cidr, host_mask)	\
> -	DCIDR_PUT(((cidr) ? NCIDR_GET(cidr) : host_mask))
> -
> -#ifdef IP_SET_HASH_WITH_NET0
> -/* cidr from 0 to HOST_MASK value and c = cidr + 1 */
> -#define NLEN			(HOST_MASK + 1)
> -#define CIDR_POS(c)		((c) - 1)
> -#else
> -/* cidr from 1 to HOST_MASK value and c = cidr + 1 */
> -#define NLEN			HOST_MASK
> -#define CIDR_POS(c)		((c) - 2)
> -#endif

[Severity: Low]
This isn't a bug, but these were the only readers of
IP_SET_HASH_WITH_NET0, and the flag is still defined by
ip_set_hash_netiface.c and ip_set_hash_netportnet.c:

  #define IP_SET_HASH_WITH_NET0

After this change nothing in ip_set_hash_gen.h consumes it, so /0 support
is now implicit (cidr 0 with count != 0).  Should those two defines be
dropped as well?  Grepping net/netfilter/ipset/ at the end of the series
still shows the two define lines and no consumers.

> +#define INIT_CIDR(n, host_mask) ({				\
> +	const struct net_prefixes *__n = rcu_dereference(n);		\
> +	DCIDR_PUT((__n)->len ? (__n)->nets[0].cidr : host_mask);\
> +})

[Severity: Medium]
Should INIT_CIDR() also look at nets[0].count?

Every other user of the array treats count == 0 as a stale hole.
mtype_test_cidrs() skips them:

	for (j = 0; j < nets0->len && !multi; j++) {
		if (!nets0->nets[j].count)
			continue;

and mtype_add_cidr()/mtype_del_cidr() only count entries with
nets->nets[i].count when computing the new length.  INIT_CIDR() tests len
only, and len is the number of allocated slots rather than the number of
live prefixes.

Two states created by this patch leave len non-zero with count == 0 at
index 0:

  - mtype_del_cidr() on allocation failure:

	tmp = kzalloc_flex(*tmp, nets, len, GFP_ATOMIC);
	if (!tmp)
		/* Leave a hole */
		goto unlock;

  - mtype_flush() on allocation failure, which zeroes the counts but keeps
    len and the stored cidr values.

In both cases INIT_CIDR() returns a prefix that is no longer in the set,
where the previous memset(h->nets, 0, sizeof(h->nets)) guaranteed the
host_mask fallback.  The value is then used for real work on the kadt
path, for example in hash_net4_kadt():

	struct hash_net4_elem e = {
		.cidr = INIT_CIDR(h->rnets[0], HOST_MASK),
	};
	...
	if (e.cidr == 0)
		return -EINVAL;
	...
	e.ip &= ip_set_netmask(e.cidr);

so an add or del driven by the SET target can end up operating on a
broader network than intended.  The same pattern applies to the
hash_netiface*, hash_netport*, hash_ipportnet*, hash_netnet* and
hash_netportnet* kadt functions.

The commit message says the hole possibility "is taken into account at
evaluation time" - that holds for mtype_test_cidrs(), but does it hold for
INIT_CIDR()?  This is unchanged at the end of the series (3a0d9fd64b67).

>  
> -#else
> -#define NLEN			0
>  #endif /* IP_SET_HASH_WITH_NETS */
>  
>  #define SET_ELEM_EXPIRED(set, d)	\
> @@ -292,6 +283,7 @@ static const union nf_inet_addr zeromask = {};
>  /* The generic hash structure */
>  struct htype {
>  	struct htable __rcu *table; /* the hash table */
> +	struct net_prefixes __rcu *rnets[IPSET_NET_COUNT]; /* cidr prefixes */
>  	struct htable_gc gc;	/* gc workqueue */
>  	u32 maxelem;		/* max elements in the hash */
>  	u32 initval;		/* random jhash init value */
> @@ -302,9 +294,6 @@ struct htype {
>  #if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
>  	u8 netmask;		/* netmask value for subnets to store */
>  	union nf_inet_addr bitmask;	/* stores bitmask */
> -#endif
> -#ifdef IP_SET_HASH_WITH_NETS
> -	struct net_prefixes nets[NLEN]; /* book-keeping of prefixes */
>  #endif

[Severity: Low]
Does the memsize reported to userspace need updating too?  The bookkeeping
is no longer part of struct htype, but mtype_ahash_memsize() still only
accounts for sizeof(*h):

	return sizeof(*h) + sizeof(*t) + ahash_sizeof_regions(t->htable_bits);

So the h->rnets[] allocations (up to HOST_MASK + 1 entries times
IPSET_NET_COUNT, and note struct net_prefix is 8 bytes after padding) are
no longer visible in IPSET_ATTR_MEMSIZE, and the reported value drops for
existing set types.  The commit message does not mention this user visible
change.  Still the case at the end of the series (3a0d9fd64b67).

>  	/* Because 'next' is IPv4/IPv6 dependent, no elements of this
>  	 * structure and referred in create() may come after 'next'.
> @@ -326,50 +315,92 @@ struct mtype_resize_ad {
>  /* Network cidr size book keeping when the hash stores different
>   * sized networks. cidr == real cidr + 1 to support /0.
>   */

[Severity: Low]
This isn't a bug, but the comment still describes the encoding this patch
removes.  NCIDR_PUT()/NCIDR_GET() are gone and all call sites now pass the
raw value, for example:

	mtype_add_cidr(set, h, DCIDR_GET(d->cidr, i), i);

Could the comment be updated to describe the new invariants instead
(descending cidr order, count == 0 marks a hole, len is the number of
allocated slots, /0 distinguished by count)?  The stale text is still
present at the end of the series (3a0d9fd64b67).

> -static void
> +static int
>  mtype_add_cidr(struct ip_set *set, struct htype *h, u8 cidr, u8 n)
>  {
> -	int i, j;
> +	struct net_prefixes *nets, *tmp;
> +	int i, j, found, len = 0, ret = 0;
>  
>  	spin_lock_bh(&set->lock);
> +	nets = __ipset_dereference(h->rnets[n]);
>  	/* Add in increasing prefix order, so larger cidr first */
> -	for (i = 0, j = -1; i < NLEN && h->nets[i].cidr[n]; i++) {
> -		if (j != -1) {
> +	for (i = 0, found = -1; i < nets->len; i++) {
> +		if (nets->nets[i].count)
> +			len++;
> +		if (found != -1) {
>  			continue;
> -		} else if (h->nets[i].cidr[n] < cidr) {
> -			j = i;
> -		} else if (h->nets[i].cidr[n] == cidr) {
> -			h->nets[CIDR_POS(cidr)].nets[n]++;
> +		} else if (nets->nets[i].cidr < cidr) {
> +			found = i;
> +		} else if (nets->nets[i].cidr == cidr) {
> +			nets->nets[i].count++;

[Severity: Low]
This isn't a bug on any supported architecture, but this increment (and
the matching decrement in mtype_del_cidr(), plus the bulk zeroing in the
mtype_flush() fallback) still mutates the already published object in
place, with plain stores and no WRITE_ONCE().

The lockless readers use plain loads of the same field:

	if (!nets0->nets[j].count)
		continue;

Since count now decides whether an entry is a live prefix or a hole, is it
worth annotating these accesses?  The commit message says the in-place
updating is replaced by an RCU based method, but that only covers the array
shape and ordering; the count field is still updated in place on the
published structure without going through rcu_assign_pointer().
Unchanged at the end of the series (3a0d9fd64b67).

>  			goto unlock;
>  		}
>  	}
> -	if (j != -1) {
> -		for (; i > j; i--)
> -			h->nets[i].cidr[n] = h->nets[i - 1].cidr[n];
> +	len++;
> +	tmp = kzalloc_flex(*tmp, nets, len, GFP_ATOMIC);
> +	if (!tmp) {
> +		ret = -ENOMEM;
> +		goto unlock;
>  	}
> -	h->nets[i].cidr[n] = cidr;
> -	h->nets[CIDR_POS(cidr)].nets[n] = 1;
> +
> +	tmp->len = len;
> +	for (i = 0, j = 0; i < nets->len; i++) {
> +		if (i == found) {
> +			tmp->nets[j].cidr = cidr;
> +			tmp->nets[j++].count = 1;
> +		}
> +		if (!nets->nets[i].count)
> +			continue;
> +		tmp->nets[j].cidr = nets->nets[i].cidr;
> +		tmp->nets[j++].count = nets->nets[i].count;
> +	}
> +	if (found == -1) {
> +		tmp->nets[j].cidr = cidr;
> +		tmp->nets[j].count = 1;
> +	}
> +	rcu_assign_pointer(h->rnets[n], tmp);
> +	kfree_rcu(nets, rcu);
>  unlock:
>  	spin_unlock_bh(&set->lock);
> +	return ret;
>  }
>  
>  static void
>  mtype_del_cidr(struct ip_set *set, struct htype *h, u8 cidr, u8 n)
>  {
> -	u8 i, j, net_end = NLEN - 1;
> +	struct net_prefixes *nets, *tmp;
> +	u8 i, j, len = 0;
> +	int found;
>  
>  	spin_lock_bh(&set->lock);
> -	for (i = 0; i < NLEN; i++) {
> -		if (h->nets[i].cidr[n] != cidr)
> -			continue;
> -		h->nets[CIDR_POS(cidr)].nets[n]--;
> -		if (h->nets[CIDR_POS(cidr)].nets[n] > 0)
> -			goto unlock;
> -		for (j = i; j < net_end && h->nets[j].cidr[n]; j++)
> -			h->nets[j].cidr[n] = h->nets[j + 1].cidr[n];
> -		h->nets[j].cidr[n] = 0;
> +	nets = __ipset_dereference(h->rnets[n]);
> +	for (i = 0, found = -1; i < nets->len; i++) {
> +		if (nets->nets[i].count)
> +			len++;
> +		if (nets->nets[i].cidr == cidr)
> +			found = i;
> +	}
> +	if (unlikely(found == -1))
> +		goto unlock;
> +
> +	nets->nets[found].count--;
> +	if (nets->nets[found].count)

[Severity: Medium]
Can nets->nets[found].count be zero here, making this decrement underflow?

The match loop selects an entry by cidr only and ignores count, but the new
design deliberately keeps count == 0 entries around: the "Leave a hole"
fallback just below, and the mtype_flush() fallback which zeroes all counts
while keeping len and the stored cidr values.

If found points at such a hole, count is a u32 and becomes 0xffffffff, and
the following test:

	if (nets->nets[found].count)
		goto unlock;

then keeps the entry forever.  mtype_test_cidrs() probes it on every packet
and INIT_CIDR() can hand it out as the most specific prefix.

One way to reach the decrement of a zero count: mtype_flush() clears the
buckets first, a concurrent SET target add registers a cidr and inserts an
element, the flush fallback afterwards zeroes that count, and the later
delete of the still present element decrements zero.  The skew caused by the
ignored -ENOMEM of mtype_add_cidr() (see below) gets there as well.

Before this patch the flush path used memset(h->nets, 0, sizeof(h->nets)),
which also cleared the stored cidr values, so a stale cidr could not be
matched.  Would guarding the decrement with a count check, or matching on
count && cidr, be preferable?  Still present at the end of the series
(3a0d9fd64b67).

>  		goto unlock;
> +	len--;
> +	tmp = kzalloc_flex(*tmp, nets, len, GFP_ATOMIC);
> +	if (!tmp)
> +		/* Leave a hole */
> +		goto unlock;
> +
> +	tmp->len = len;
> +	for (i = 0, j = 0; i < nets->len; i++) {
> +		if (!nets->nets[i].count || i == found)
> +			continue;
> +		tmp->nets[j].cidr = nets->nets[i].cidr;
> +		tmp->nets[j++].count = nets->nets[i].count;
>  	}
> +	rcu_assign_pointer(h->rnets[n], tmp);
> +	kfree_rcu(nets, rcu);
>  unlock:
>  	spin_unlock_bh(&set->lock);
>  }
> @@ -402,6 +433,9 @@ static void
>  mtype_flush(struct ip_set *set)
>  {
>  	struct htype *h = set->data;
> +#ifdef IP_SET_HASH_WITH_NETS
> +	struct net_prefixes *nets, *tmp;
> +#endif
>  	struct htable *t;
>  	struct hbucket *n;
>  	u32 r, i;
> @@ -425,7 +459,19 @@ mtype_flush(struct ip_set *set)
>  		spin_unlock_bh(&t->hregion[r].lock);
>  	}
>  #ifdef IP_SET_HASH_WITH_NETS
> -	memset(h->nets, 0, sizeof(h->nets));
> +	for (i = 0; i < IPSET_NET_COUNT; i++) {
> +		nets = ipset_dereference_nfnl(h->rnets[i]);
> +		tmp = kzalloc_obj(*tmp, GFP_ATOMIC);
> +		if (!tmp) {
> +			u8 j;
> +
> +			for (j = 0; j < nets->len; j++)
> +				nets->nets[j].count = 0;
> +		} else {
> +			rcu_assign_pointer(h->rnets[i], tmp);
> +			kfree_rcu(nets, rcu);
> +		}
> +	}
>  #endif
>  }

[Severity: Critical]
Does this replacement need set->lock?

mtype_add_cidr() and mtype_del_cidr() do the same read, publish and free
sequence under spin_lock_bh(&set->lock), but here only the nfnetlink mutex
is held.  For the hash types set->variant->region_lock is true, and
ip_set_lock() is then a no-op:

net/netfilter/ipset/ip_set_core.c:
static inline void
ip_set_lock(struct ip_set *set)
{
	if (!set->variant->region_lock)
		spin_lock_bh(&set->lock);
}

so ip_set_flush_set() -> mtype_flush() gets no serialization against the
cidr writers.  Those writers do not take the nfnl mutex either:

  SET target -> ip_set_add()/ip_set_del() -> kadt -> mtype_add()/mtype_del()
    -> mtype_add_cidr()/mtype_del_cidr()
  gc worker -> mtype_gc_do() -> mtype_del_cidr_all() -> mtype_del_cidr()

Can both sides then observe the same old pointer and each queue it?

CPU0 (ipset flush)                  CPU1 (SET target, softirq)
nets = ipset_dereference_nfnl(...)
                                    spin_lock_bh(&set->lock);
                                    nets = __ipset_dereference(h->rnets[n]);
rcu_assign_pointer(h->rnets[i], tmp)
kfree_rcu(nets, rcu);
                                    rcu_assign_pointer(h->rnets[n], tmp);
                                    kfree_rcu(nets, rcu);   /* same object */

That queues the same rcu_head twice, and the concurrent update is lost as
well.  The !tmp fallback above also writes nets->nets[j].count = 0 into an
object the other side may already have unpublished and passed to
kfree_rcu().

The pre-patch code did only memset(h->nets, 0, sizeof(h->nets)) in this
same unsynchronized spot, with no allocation and no freeing, so the memory
safety aspect looks new here.  The ipset_dereference_nfnl() annotation
suggests the nfnl mutex is sufficient; should this be set->lock based
instead?  This code is unchanged at the end of the series (3a0d9fd64b67).

[ ... ]

> @@ -996,7 +1048,7 @@ mtype_add(struct ip_set *set, void *value, const struct ip_set_ext *ext,
>  	t->hregion[r].elements++;
>  #ifdef IP_SET_HASH_WITH_NETS
>  	for (i = 0; i < IPSET_NET_COUNT; i++)
> -		mtype_add_cidr(set, h, NCIDR_PUT(DCIDR_GET(d->cidr, i)), i);
> +		mtype_add_cidr(set, h, DCIDR_GET(d->cidr, i), i);
>  #endif
>  	memcpy(data, d, sizeof(struct mtype_elem));

[Severity: High]
mtype_add_cidr() changed from void to int and can now fail:

	tmp = kzalloc_flex(*tmp, nets, len, GFP_ATOMIC);
	if (!tmp) {
		ret = -ENOMEM;
		goto unlock;
	}

Should this caller check the return value?  The memcpy() commits the
element unconditionally, so on allocation failure the element sits in the
bucket with no prefix registered in h->rnets[].

mtype_test_cidrs() only probes cidrs that are present with count != 0:

	for (j = 0; j < nets0->len && !multi; j++) {
		if (!nets0->nets[j].count)
			continue;

so does such an element become permanently unmatchable, while still being
listable and deletable?

A later delete of that element runs mtype_del_cidr() for the unregistered
cidr, and if other live elements share the same cidr their count is
decremented without a matching increment.  Can that count reach zero while
elements using the prefix remain, dropping the prefix out of the array and
stopping those elements from matching too?  mtype_resize() copies elements
only and does not rebuild the bookkeeping, so the skew looks permanent.

The add path is reachable from the packet path via the SET target
(ip_set_add() in softirq, hence GFP_ATOMIC), and unlike the delete path
there is no "leave a hole" style fallback and no rollback of the
per-dimension registrations that already succeeded.  This is unchanged at
the end of the series (3a0d9fd64b67).

[ ... ]

Re: [PATCH net 06/10] netfilter: ipset: add and use mtype_del_cidr_all helper

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: ipset: add and use mtype_del_cidr_all helper

This adds a mtype_del_cidr_all() helper that wraps the per-net-dimension
loop calling mtype_del_cidr(), and converts the three open-coded loops in
mtype_gc_do(), mtype_add() and mtype_del() to use it.  No functional
change is intended.

> diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
> index ef586b486f519..f00c82acd7f05 100644
> --- a/net/netfilter/ipset/ip_set_hash_gen.h
> +++ b/net/netfilter/ipset/ip_set_hash_gen.h

[ ... ]

> @@ -410,6 +412,17 @@ mtype_del_cidr(struct ip_set *set, struct htype *h, u8 cidr, u8 n)
>  }
>  #endif
>  
> +static void
> +mtype_del_cidr_all(struct ip_set *set, struct htype *h, const struct mtype_elem *data)
> +{
> +#ifdef IP_SET_HASH_WITH_NETS
> +	int k;
> +
> +	for (k = 0; k < IPSET_NET_COUNT; k++)
> +		mtype_del_cidr(set, h, DCIDR_GET(data->cidr, k), k);
> +#endif
> +}

[Severity: Critical]

This helper makes mtype_del_cidr() the single delete-side writer of
h->rnets[], so it may be worth asking about the locking of that array.
The problem does not originate in this patch but in the earlier commit of
this series, "netfilter: ipset: rework cidr bookkeeping", which turned the
fixed-size h->nets[] into an RCU-published, reallocated struct
net_prefixes; this patch routes all three delete sites into one of the
racing writers.

mtype_add_cidr() and mtype_del_cidr() both serialize on set->lock:

	spin_lock_bh(&set->lock);
	nets = __ipset_dereference(h->rnets[n]);
	...
	rcu_assign_pointer(h->rnets[n], tmp);
	kfree_rcu(nets, rcu);
unlock:
	spin_unlock_bh(&set->lock);

mtype_flush() does the same read-allocate-publish-free sequence with no
set->lock held:

	for (i = 0; i < IPSET_NET_COUNT; i++) {
		nets = ipset_dereference_nfnl(h->rnets[i]);
		tmp = kzalloc_obj(*tmp, GFP_ATOMIC);
		if (!tmp) {
			u8 j;

			for (j = 0; j < nets->len; j++)
				nets->nets[j].count = 0;
		} else {
			rcu_assign_pointer(h->rnets[i], tmp);
			kfree_rcu(nets, rcu);
		}
	}

ip_set_flush_set() does call ip_set_lock(set), but for the hash variants
(.region_lock = true) that is a no-op:

net/netfilter/ipset/ip_set_core.c:
	static inline void
	ip_set_lock(struct ip_set *set)
	{
		if (!set->variant->region_lock)
			spin_lock_bh(&set->lock);
	}

The hregion locks flush does take are all released before the rnets loop
runs, and mtype_add_cidr()/mtype_del_cidr() do not take hregion locks.

Can this interleaving happen?

CPU0 (ipset flush, process context, nfnl mutex only)
	nets_old = ipset_dereference_nfnl(h->rnets[i]);

CPU1 (SET target kadt in softirq -> mtype_add() -> mtype_add_cidr(),
      or gc worker -> mtype_gc_do() -> mtype_del_cidr_all())
	spin_lock_bh(&set->lock);
	nets_old = __ipset_dereference(h->rnets[n]);
	rcu_assign_pointer(h->rnets[n], tmp1);
	kfree_rcu(nets_old, rcu);
	spin_unlock_bh(&set->lock);

CPU0
	rcu_assign_pointer(h->rnets[i], tmp0);	/* tmp1 leaked */
	kfree_rcu(nets_old, rcu);		/* rcu_head reused */

Does the second kfree_rcu() on an object whose rcu_head is already queued
corrupt the callback list and double free the net_prefixes?  And does the
tmp0 publish also drop the accounting update CPU1 just made in tmp1?

On the flush allocation-failure path, flush writes nets->nets[j].count = 0
in place while mtype_del_cidr() may be doing nets->nets[found].count-- and
mtype_add_cidr() nets->nets[i].count++ on the same object.  Can that tear
the counts and the __counted_by(len) contents?

[ ... ]

> @@ -1004,11 +1010,7 @@ mtype_add(struct ip_set *set, void *value, const struct ip_set_ext *ext,
>  			j = 0;
>  		data = ahash_data(n, j, set->dsize);
>  		if (!deleted) {
> -#ifdef IP_SET_HASH_WITH_NETS
> -			for (i = 0; i < IPSET_NET_COUNT; i++)
> -				mtype_del_cidr(set, h,
> -					DCIDR_GET(data->cidr, i), i);
> -#endif
> +			mtype_del_cidr_all(set, h, data);
>  			ip_set_ext_destroy(set, data);
>  			t->hregion[r].elements--;
>  		}

[Severity: High]

Related to the decrement side this patch factors out: the matching
increment in mtype_add() ignores the new error return of
mtype_add_cidr().  As with the previous comment, this comes from
"netfilter: ipset: rework cidr bookkeeping" earlier in the series, which
changed mtype_add_cidr() from a void in-place update into an int that
allocates a replacement net_prefixes with GFP_ATOMIC and can return
-ENOMEM.

The insert path in mtype_add() still reads:

	#ifdef IP_SET_HASH_WITH_NETS
		for (i = 0; i < IPSET_NET_COUNT; i++)
			mtype_add_cidr(set, h, DCIDR_GET(d->cidr, i), i);
	#endif
		memcpy(data, d, sizeof(struct mtype_elem));

while mtype_add_cidr() can fail:

	tmp = kzalloc_flex(*tmp, nets, len, GFP_ATOMIC);
	if (!tmp) {
		ret = -ENOMEM;
		goto unlock;
	}

ret in mtype_add() stays 0, so ipset add reports success and the element
shows up in ipset list.  But mtype_test_cidrs() only builds lookup keys
from prefixes recorded in h->rnets[]:

	for (j = 0; j < nets0->len && !multi; j++) {
		if (!nets0->nets[j].count)
			continue;
		...
		key = HKEY(d, h->initval, t->htable_bits);

Can this leave an element that never matches a packet, even though
userspace was told the add succeeded?

There also seems to be a follow-on effect on unrelated members.  Since
mtype_add_cidr() only allocates when the cidr is absent, a failure means
that cidr is not registered at all.  If another element later registers
the same cidr (count = 1) and the unregistered element is then deleted,
mtype_del_cidr_all() -> mtype_del_cidr() finds that shared entry and does:

	nets->nets[found].count--;

Does that drop the count to 0 and remove a prefix that live elements still
use, so those elements stop matching too?

And in the case where mtype_del_cidr()'s own kzalloc_flex() previously
failed and left a hole (a count == 0 entry that keeps its cidr), deleting
the unregistered element decrements a count of 0.  Does that underflow the
u32 to 0xFFFFFFFF and pin the prefix in the lookup list permanently?

Would propagating the mtype_add_cidr() return value (and undoing the
already-successful increments) be preferable here?

Re: [PATCH net 08/10] ipvs: avoid out-of-bounds write in ip_vs_nat_icmp

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ipvs: avoid out-of-bounds write in ip_vs_nat_icmp

This passes the already-parsed ip_vs_iphdr down into ip_vs_nat_icmp() so
the embedded transport header offset comes from ciph->len instead of a
second read of the inner IPv4 ihl field.  The IPv4 header sanity checks in
ip_vs_out_icmp() and ip_vs_in_icmp() are also reworked to read ihl only
once.

Link: https://sashiko.dev/#/patchset/20260722101517.36313-1-ja%40ssi.bg

> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 6b79e0c4d9e28..0bdaeb4ed61e4 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -925,28 +925,27 @@ static int ip_vs_route_me_harder(struct netns_ipvs *ipvs, int af,
>   */
>  void ip_vs_nat_icmp(struct sk_buff *skb, struct ip_vs_protocol *pp,
>  		    struct ip_vs_conn *cp, int inout, unsigned int toff,
> -		    bool has_ports)
> +		    bool has_ports, struct ip_vs_iphdr *ciph)
>  {
>  	struct iphdr *iph	 = ip_hdr(skb);
>  	struct icmphdr *icmph	 = (struct icmphdr *)(skb->data + toff);
> -	struct iphdr *ciph	 = (struct iphdr *)(icmph + 1);
> -	unsigned int coff __maybe_unused = toff + sizeof(struct icmphdr);
> +	struct iphdr *cih	 = (struct iphdr *)(icmph + 1);
>  
>  	if (inout) {
>  		iph->saddr = cp->vaddr.ip;
>  		ip_send_check(iph);
> -		ciph->daddr = cp->vaddr.ip;
> -		ip_send_check(ciph);
> +		cih->daddr = cp->vaddr.ip;
> +		ip_send_check(cih);
>  	} else {
>  		iph->daddr = cp->daddr.ip;
>  		ip_send_check(iph);
> -		ciph->saddr = cp->daddr.ip;
> -		ip_send_check(ciph);
> +		cih->saddr = cp->daddr.ip;
> +		ip_send_check(cih);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but the two ip_send_check(cih)
calls here still read the inner ihl field a second time, which looks like
the same multi-read pattern the commit message describes fixing in this
function.

ip_send_check() takes its own fresh read of ihl and checksums 4 * ihl
bytes:

net/ipv4/ip_output.c:ip_send_check() {
	iph->check = 0;
	iph->check = ip_fast_csum((unsigned char *)iph, iph->ihl);
}

The only length the callers guarantee is the snapshot value ciph->len (plus
4 when ports are mangled):

net/netfilter/ipvs/ip_vs_core.c:handle_response_icmp() {
	unsigned int ctoff = ciph->len;
	...
		ctoff += 2 * sizeof(__u16);
	if (skb_ensure_writable(skb, ctoff))
		goto out;
	...
		ip_vs_nat_icmp(skb, pp, cp, 1, toff, has_ports, ciph);
}

ciph->len was computed earlier in ip_vs_fill_iph_skb_off() as
offset + iph->ihl * 4 from a skb_header_pointer() snapshot, and
skb_ensure_writable() -> pskb_may_pull() re-copies the bytes out of the
fragments, so the ihl byte ip_send_check() then reads can differ from the
one that sized the validated window.

If the fresh ihl is larger than the snapshot value, can ip_fast_csum() read
up to 40 bytes past what skb_ensure_writable() guaranteed, and fold
adjacent skb memory into the inner header checksum that is emitted?

If the fresh ihl ends up below 5 (for example the version/ihl byte going
from 0x45 to 0x40, compensated in tot_len so both the inner IP and ICMP
checksums stay valid), some architectures do not tolerate it:

arch/mips/include/asm/checksum.h:ip_fast_csum() {
	word += 4;
	do {
		csum += *word;
		carry = (csum < *word);
		csum += carry;
		word++;
	} while (word != stop);
}

with stop = iph + ihl, so for ihl <= 4 the loop termination is never hit.

arch/s390/include/asm/checksum.h:ip_fast_csum() {
	ihl -= 4;
	while (ihl--)
		csum += *ptr++;
}

where ihl == 0 underflows into roughly four billion forward word reads.

Would it make sense to checksum over the validated length
(ciph->len - ciph->off), or to re-validate version/ihl after
skb_ensure_writable(), matching what was done for the port offset?

The same sequence exists on the other call site:

net/netfilter/ipvs/ip_vs_xmit.c:ip_vs_icmp_xmit() {
	wlen = ciph->len;
	...
		wlen += 2 * sizeof(__u16); /* Also mangle ports */
	if (skb_ensure_writable(skb, wlen))
		goto tx_error;
	...
	ip_vs_nat_icmp(skb, pp, cp, 0, toff, has_ports, ciph);
}

>  
>  	/* the TCP/UDP/SCTP port */
>  	if (has_ports) {
> -		__be16 *ports = (void *)ciph + ciph->ihl*4;
> +		__be16 *ports = (void *)(skb->data + ciph->len);
>  
>  		if (inout)
>  			ports[1] = cp->vport;

[ ... ]

> @@ -1131,11 +1129,15 @@ static int ip_vs_out_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb,
>  
>  	/* Now find the contained IP header */
>  	offset += sizeof(_icmph);
> +	if (!ip_vs_fill_iph_skb_icmp(AF_INET, skb, offset, true, &ciph))
> +		return NF_ACCEPT; /* The packet looks wrong, ignore */
> +
>  	cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
> -	if (!(cih && cih->version == 4 && cih->ihl >= 5))
> +	if (!(cih && cih->version == 4 &&
> +	      ciph.len - ciph.off >= sizeof(struct iphdr)))
>  		return NF_ACCEPT; /* The packet looks wrong, ignore */

This check validates the snapshot length only, so nothing revalidates
version or ihl after the data is pulled into the linear area and handed to
ip_send_check().  Was that intentional?

[ ... ]

Re: [PATCH net 10/10] netfilter: nft_ct: move custom expectation support to helper

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: nft_ct: move custom expectation support to helper

This reworks the nftables ct expectation object so that the expectation is
no longer created directly from the rule evaluation, but from an internal
conntrack helper that is allocated per object and attached to the conntrack
entry. nf_conntrack_helper_free() is split out of
nf_conntrack_helper_unregister() and exported so the internal helper can be
released when the object goes away.

Link: https://patch.msgid.link/20260715144755.00ea7dfcd9f@proton.me

A few questions about the commit message first.

> Originally, the ct expectation support called nf_ct_helper_ext_add() for
> confirmed conntracks, which is invalid, triggering a splat. This was
> fixed by commit 1710eb913bdc ("netfilter: nft_ct: skip expectations for
> confirmed conntrack") which restricted it to confirmed conntracks.

[Severity: Low]
Is this description inverted?  That commit skips confirmed conntracks, so
it restricted the feature to unconfirmed ones, which is also the guard this
patch keeps in nft_ct_expect_obj_eval():

	if (!ct || nf_ct_is_template(ct) || nf_ct_is_confirmed(ct)) {
		regs->verdict.code = NFT_BREAK;
		return;
	}

> Fixes: 857b46027d6f ("netfilter: nft_ct: add ct expectations support")

[Severity: Low]
The Fixes tag points at the 2019 feature commit, but the implementation
relies on struct nf_conntrack_helper::ct_refcnt, which git blame on
include/net/netfilter/nf_conntrack_helper.h attributes to commit
ac46f3f35b6e68 (2026-06-04), on the refcount_inc_not_zero() guarded
help->helper assignment in __nf_ct_try_assign_helper(), and on the
kzalloc_obj() slab macro.

Would a note about stable applicability be worth adding, since a tree
without the ct_refcnt rework would not build this and has no equivalent
lifetime scheme for an unregistered helper?

> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> index 500509b17663e..1197e8793494b 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c
> @@ -456,13 +456,8 @@ static bool expect_iter_me(struct nf_conntrack_expect *exp, void *data)
>  	return this == me;
>  }
>  
> -void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
> +void nf_conntrack_helper_free(struct nf_conntrack_helper *me)
>  {
> -	mutex_lock(&nf_ct_helper_mutex);
> -	hlist_del_rcu(&me->hnode);
> -	nf_ct_helper_count--;
> -	mutex_unlock(&nf_ct_helper_mutex);
> -
>  	/* This helper is going away, disable it. */
>  	rcu_assign_pointer(me->help, NULL);
>  
> @@ -476,6 +471,17 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
>  	if (refcount_dec_and_test(&me->ct_refcnt))
>  		kfree_rcu(me, rcu);
>  }
> +EXPORT_SYMBOL_GPL(nf_conntrack_helper_free);
> +
> +void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
> +{
> +	mutex_lock(&nf_ct_helper_mutex);
> +	hlist_del_rcu(&me->hnode);
> +	nf_ct_helper_count--;
> +	mutex_unlock(&nf_ct_helper_mutex);
> +
> +	nf_conntrack_helper_free(me);
> +}
>  EXPORT_SYMBOL_GPL(nf_conntrack_helper_unregister);
>  
>  void nf_ct_helper_init(struct nf_conntrack_helper *helper,

[ ... ]

> diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> index 03a88c77e0f02..30c9358dbf48a 100644
> --- a/net/netfilter/nft_ct.c
> +++ b/net/netfilter/nft_ct.c
> @@ -1226,6 +1228,73 @@ static int nft_ct_expect_timeout_get(const struct nlattr *attr, u32 *val)
>  	return 0;
>  }
>  
> +struct nft_ct_expect_data {
> +	struct nft_ct_expect_obj	obj;
> +	enum ip_conntrack_dir		dir;
> +	atomic_t			num_expects;
> +};
> +
> +static int ct_expect_help(struct sk_buff *skb, unsigned int protoff,
> +			  struct nf_conn *ct, enum ip_conntrack_info ctinfo)
> +{
> +	enum ip_conntrack_dir dir = CTINFO2DIR(ctinfo);
> +	struct nft_ct_expect_data *expect_data;
> +	struct nf_conntrack_expect *exp;
> +	int ret = NF_ACCEPT;
> +
> +	expect_data = nfct_help_data(ct);
> +	if (!expect_data)
> +		return NF_ACCEPT;
> +
> +	if (expect_data->dir != dir)
> +		return NF_ACCEPT;
> +
> +	if (!atomic_add_unless(&expect_data->num_expects, 1, expect_data->obj.size))
> +		return NF_ACCEPT;

[Severity: Medium]
Does this change the meaning of NFTA_CT_EXPECT_SIZE?  The old gate in
nft_ct_expect_obj_eval() was:

	if (help->expecting[NF_CT_EXPECT_CLASS_DEFAULT] >= priv->size) {

that is a limit on live expectations, and nf_ct_unlink_expect() decrements
expecting[] once an expectation is consumed or times out.

num_expects here is only ever decremented on failure, so it looks like a
cumulative lifetime cap instead: after size expectations have been created,
no further expectation is created for that conntrack even when the previous
ones are long gone.

The other difference is when the callback runs.  nf_confirm() calls
helper_cb() for every packet of the flow once nfct_help(ct) is set, so with
size >= 2 the first packets of an already established connection re-enter
ct_expect_help() and re-insert the same expectation through the refresh
path in __nf_ct_expect_check():

	if (master_matches(i, expect, flags) && expect_matches(i, expect)) {
		...
		nf_ct_unlink_expect(i);
		break;

Should the commit message describe these two behaviour changes, since it
presents the patch as only moving the feature into a helper?

> +
> +	exp = nf_ct_expect_alloc(ct);
> +	if (!exp) {
> +		atomic_dec(&expect_data->num_expects);
> +		return NF_DROP;
> +	}
> +
> +	nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT, nf_ct_l3num(ct),
> +			  &ct->tuplehash[!dir].tuple.src.u3,
> +			  &ct->tuplehash[!dir].tuple.dst.u3,
> +			  expect_data->obj.l4proto, NULL, &expect_data->obj.dport);

[Severity: Low]
Does NFTA_CT_EXPECT_L3PROTO still do anything after this?  The old eval
passed the configured value:

	u16 l3num = priv->l3num;
	...
	if (l3num == NFPROTO_INET)
		l3num = nf_ct_l3num(ct);

Here nf_ct_l3num(ct) is used unconditionally and priv->l3num is not read by
any datapath code, while nft_ct_expect_obj_init() still parses and
range-checks the attribute and nft_ct_expect_obj_dump() still echoes it
back.  In an inet table, which is the one case where priv->l3num is allowed
to differ from ctx->family, "l3proto ipv4" now has no effect on which
conntracks get expectations.

> +	exp->timeout += expect_data->obj.timeout;
> +
> +	if (nf_ct_expect_related(exp, 0) != 0) {
> +		atomic_dec(&expect_data->num_expects);
> +		ret = NF_DROP;
> +	}

[Severity: Medium]
Can this blackhole an established flow?  The budget is given back on
failure and nf_confirm() runs the callback for every packet:

	ret = helper_cb(skb, protoff, ct, ctinfo);
	if (ret != NF_ACCEPT)
		return ret;

So for a persistent failure the retry happens per packet and each retry
drops the packet.  A persistent failure looks reachable: once expectation E
has been consumed or expired, another conntrack creating a byte-identical
tuple takes the clash path in __nf_ct_expect_check() rather than the
replace path:

	} else if (expect_clash(i, expect)) {
		ret = -EBUSY;
		goto out;

A full expect table (-EMFILE) has the same shape.  Before this patch the
NF_DROP could only happen for the unconfirmed first packet; now every
subsequent packet of the confirmed connection in that direction is dropped
for as long as the clash lasts.

> +
> +	nf_ct_expect_put(exp);
> +
> +	return ret;
> +}
> +
> +static int nft_ct_expect_helper_alloc(struct nft_ct_expect_obj *priv)
> +{
> +	struct nf_conntrack_helper *ct_expect_helper;
> +
> +	ct_expect_helper = kzalloc_obj(struct nf_conntrack_helper);

[Severity: Low]
Should this be accounted to the memcg?  kzalloc_obj() with no gfp argument
is GFP_KERNEL, while the equivalent allocation of the same struct in
nf_conntrack_helper_register() uses:

	new_helper = kzalloc_obj(*new_helper, GFP_KERNEL_ACCOUNT);

as do the nfnetlink_cthelper allocations.  The object is created from an
NFT_MSG_NEWOBJ request and can outlive the ruleset, since every conntrack
that got the helper assigned holds an extra ct_refcnt.

> +	if (!ct_expect_helper)
> +		return -ENOMEM;
> +
> +	snprintf(ct_expect_helper->name, sizeof(ct_expect_helper->name), "%s",
> +		 "nft_ct_expect");
> +	ct_expect_helper->me = THIS_MODULE;
> +	ct_expect_helper->expect_policy[NF_CT_EXPECT_CLASS_DEFAULT].max_expected = priv->size;

[Severity: Low]
This assignment skips the normalization that every registered helper gets
in __nf_conntrack_helper_register():

	if (!me->expect_policy[i].max_expected)
		me->expect_policy[i].max_expected = NF_CT_EXPECT_MAX_CNT;

With "size 0", which nft_ct_expect_obj_init() accepts, the attached helper
ends up with max_expected == 0.  __nf_ct_expect_check() then evaluates:

	p = &helper->expect_policy[expect->class];
	if (master_help->expecting[expect->class] >= p->max_expected)
		evict_oldest_expect(master_help, expect, p);

as always true, so any other creator of an expectation for that master
(ctnetlink, for instance) takes the eviction path every time.  Is
max_expected == 0 intended here?

There is also a mismatch of meaning: max_expected is a refillable
concurrent limit against master_help->expecting[], whereas the num_expects
gate in ct_expect_help() treats the same configured value as a cumulative
cap.

> +	rcu_assign_pointer(ct_expect_helper->help, ct_expect_help);
> +	refcount_set(&ct_expect_helper->ct_refcnt, 1);
> +
> +	/* No need to register this helper, this is internal. */
> +	priv->helper = ct_expect_helper;

[Severity: Low]
The helper's tuple stays zeroed here, so tuple.dst.protonum == 0, unlike
every helper built by nf_ct_helper_init() which sets a real IPPROTO_*.
The two dispatchers of ->help() disagree about that value.  nf_confirm()
calls the callback unconditionally, while nf_ct_helper() used by act_ct and
openvswitch filters first:

	if (helper->tuple.dst.protonum != proto)
		return NF_ACCEPT;

which can never match protonum 0.  So on the act_ct/OVS path this helper is
silently skipped and no expectation is ever created.

I could not construct a concrete hook ordering where nf_ct_helper() runs
before nf_confirm() has already created the expectation, so this may be
harmless today, but what protonum is intended for this helper?
NFTA_CT_EXPECT_L4PROTO describes the expected child connection, not the
master, so no obvious value is available at object init time.

> +
> +	return 0;
> +}
> +
>  static int nft_ct_expect_obj_init(const struct nft_ctx *ctx,
>  				  const struct nlattr * const tb[],
>  				  struct nft_object *obj)

[ ... ]

> @@ -1273,13 +1344,26 @@ static int nft_ct_expect_obj_init(const struct nft_ctx *ctx,
>  	priv->dport = nla_get_be16(tb[NFTA_CT_EXPECT_DPORT]);
>  	priv->size = nla_get_u8(tb[NFTA_CT_EXPECT_SIZE]);
>  
> -	return nf_ct_netns_get(ctx->net, ctx->family);
> +	err = nf_ct_netns_get(ctx->net, ctx->family);
> +	if (err < 0)
> +		return err;
> +
> +	err = nft_ct_expect_helper_alloc(priv);
> +	if (err < 0) {
> +		nf_ct_netns_put(ctx->net, ctx->family);
> +		return err;
> +	}
> +
> +	return err;
>  }
>  
>  static void nft_ct_expect_obj_destroy(const struct nft_ctx *ctx,
> -				       struct nft_object *obj)
> +				      struct nft_object *obj)
>  {
> +	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
> +
>  	nf_ct_netns_put(ctx->net, ctx->family);
> +	nf_conntrack_helper_free(priv->helper);
>  }

[Severity: High]
Is it acceptable to run nf_conntrack_helper_free() once per destroyed
object here?  It does:

	rcu_assign_pointer(me->help, NULL);
	synchronize_rcu();
	nf_ct_expect_iterate_destroy(expect_iter_me, me);

so one full RCU grace period plus a walk of the entire global expectation
hash under spin_lock_bh(&nf_conntrack_expect_lock), work that previously
only happened on helper module unregistration.

In the abort path this runs from __nf_tables_abort():

	synchronize_rcu();

	list_for_each_entry_safe_reverse(trans, next, &nft_net->commit_list, list) {
		nft_trans_list_del(trans);
		nf_tables_abort_release(trans);
	}

which reaches nft_obj_destroy() -> nft_ct_expect_obj_destroy() while
nft_net->commit_mutex and nfnl_lock are still held.  A batch that creates N
ct expect objects and then fails therefore serializes N grace periods under
the per-netns nftables mutex, with N chosen by the caller.
__nft_release_table() on netns exit has the same property.

The commit-release path already does one synchronize_rcu() for the whole
batch, and nf_tables_commit_release() carries the comment "Memory reclaim
happens asynchronously from work queue to prevent expensive
synchronize_rcu() in commit phase".  Could the helper release be deferred
to that work queue instead?

>  
>  static int nft_ct_expect_obj_dump(struct sk_buff *skb,
> @@ -1302,50 +1386,39 @@ static void nft_ct_expect_obj_eval(struct nft_object *obj,
>  				   const struct nft_pktinfo *pkt)
>  {
>  	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
> -	struct nf_conntrack_expect *exp;
> +	struct nft_ct_expect_data *expect_data;
>  	enum ip_conntrack_info ctinfo;
>  	struct nf_conn_help *help;
> -	enum ip_conntrack_dir dir;
> -	u16 l3num = priv->l3num;
>  	struct nf_conn *ct;
>  
>  	ct = nf_ct_get(pkt->skb, &ctinfo);
> -	if (!ct || nf_ct_is_confirmed(ct) || nf_ct_is_template(ct)) {
> +	if (!ct || nf_ct_is_template(ct) || nf_ct_is_confirmed(ct)) {
>  		regs->verdict.code = NFT_BREAK;
>  		return;
>  	}
> -	dir = CTINFO2DIR(ctinfo);
>  
>  	help = nfct_help(ct);
> -	if (!help)
> -		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
> -	if (!help) {
> -		regs->verdict.code = NF_DROP;
> -		return;
> -	}
> -
> -	if (help->expecting[NF_CT_EXPECT_CLASS_DEFAULT] >= priv->size) {
> +	if (help) {
>  		regs->verdict.code = NFT_BREAK;
>  		return;
>  	}

[Severity: High]
Does this permanently disable the statement for conntracks that already
carry a helper extension?  The old code reused an existing extension:

	help = nfct_help(ct);
	if (!help)
		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);

and created the expectation either way.  Now any conntrack with
nfct_help(ct) != NULL takes NFT_BREAK, which also aborts the remainder of
the rule, so "ct expect set e counter accept" no longer reaches accept and
the chain policy applies instead.

Three in-tree ways to be in that state before nft rules run:

  init_conntrack() for a conntrack born from an expectation with
  assign_helper set:

	help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
	if (help && refcount_inc_not_zero(&assign_helper->ct_refcnt))
		rcu_assign_pointer(help->helper, assign_helper);

  __nf_ct_try_assign_helper() during nf_conntrack_in() when the template
  carries a helper:

	if (help == NULL) {
		help = nf_ct_helper_ext_add(ct, flags);

  and a ruleset that runs "ct helper set" (nft_ct_helper_obj_eval()) or a
  second ct expect object on the same connection.

Should this case fall back to attaching the expectation data anyway, or at
least be documented?

>  
> -	exp = nf_ct_expect_alloc(ct);
> -	if (exp == NULL) {
> +	help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
> +	if (!help) {
>  		regs->verdict.code = NF_DROP;
>  		return;
>  	}
>  
> +	expect_data = nfct_help_data(ct);
> +	if (!expect_data) {
> +		regs->verdict.code = NFT_BREAK;
> +		return;
> +	}
> +	expect_data->obj = *priv;

[Severity: Low]
This copies the whole struct nft_ct_expect_obj, which now includes the raw
struct nf_conntrack_helper *helper pointer, into the conntrack helper
extension private area, without taking a reference for that copy.  Only
help->helper, referenced on the next line, is a valid handle.

Since the conntrack extension can outlive the nft object,
expect_data->obj.helper becomes stale once nft_ct_expect_obj_destroy() ->
nf_conntrack_helper_free() drops the last reference and kfree_rcu() runs.
Nothing reads it today, so this is latent, but would it be clearer to copy
only the fields ct_expect_help() uses?

It also spends 8 of the 32 bytes available in nf_conn_help->data, and
struct nft_ct_expect_data is already at the NF_CT_HELPER_BUILD_BUG_ON
limit.

> +	expect_data->dir = CTINFO2DIR(ctinfo);
>  
> -	nf_ct_expect_put(exp);
> +	if (help && refcount_inc_not_zero(&priv->helper->ct_refcnt))
> +		rcu_assign_pointer(help->helper, priv->helper);
>  }

[ ... ]

Re: [PATCH net 08/10] ipvs: avoid out-of-bounds write in ip_vs_nat_icmp

[Julian Anastasov]

	Hello,

On Tue, 4 Aug 2026, Jakub Kicinski wrote:

> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ipvs: avoid out-of-bounds write in ip_vs_nat_icmp
> 
> This passes the already-parsed ip_vs_iphdr down into ip_vs_nat_icmp() so
> the embedded transport header offset comes from ciph->len instead of a
> second read of the inner IPv4 ihl field.  The IPv4 header sanity checks in
> ip_vs_out_icmp() and ip_vs_in_icmp() are also reworked to read ihl only
> once.
> 
> Link: https://sashiko.dev/#/patchset/20260722101517.36313-1-ja%40ssi.bg
> 
> > diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> > index 6b79e0c4d9e28..0bdaeb4ed61e4 100644
> > --- a/net/netfilter/ipvs/ip_vs_core.c
> > +++ b/net/netfilter/ipvs/ip_vs_core.c
> > @@ -925,28 +925,27 @@ static int ip_vs_route_me_harder(struct netns_ipvs *ipvs, int af,
> >   */
> >  void ip_vs_nat_icmp(struct sk_buff *skb, struct ip_vs_protocol *pp,
> >  		    struct ip_vs_conn *cp, int inout, unsigned int toff,
> > -		    bool has_ports)
> > +		    bool has_ports, struct ip_vs_iphdr *ciph)
> >  {
> >  	struct iphdr *iph	 = ip_hdr(skb);
> >  	struct icmphdr *icmph	 = (struct icmphdr *)(skb->data + toff);
> > -	struct iphdr *ciph	 = (struct iphdr *)(icmph + 1);
> > -	unsigned int coff __maybe_unused = toff + sizeof(struct icmphdr);
> > +	struct iphdr *cih	 = (struct iphdr *)(icmph + 1);
> >  
> >  	if (inout) {
> >  		iph->saddr = cp->vaddr.ip;
> >  		ip_send_check(iph);
> > -		ciph->daddr = cp->vaddr.ip;
> > -		ip_send_check(ciph);
> > +		cih->daddr = cp->vaddr.ip;
> > +		ip_send_check(cih);
> >  	} else {
> >  		iph->daddr = cp->daddr.ip;
> >  		ip_send_check(iph);
> > -		ciph->saddr = cp->daddr.ip;
> > -		ip_send_check(ciph);
> > +		cih->saddr = cp->daddr.ip;
> > +		ip_send_check(cih);
> >  	}
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but the two ip_send_check(cih)
> calls here still read the inner ihl field a second time, which looks like
> the same multi-read pattern the commit message describes fixing in this
> function.
> 
> ip_send_check() takes its own fresh read of ihl and checksums 4 * ihl
> bytes:
> 
> net/ipv4/ip_output.c:ip_send_check() {
> 	iph->check = 0;
> 	iph->check = ip_fast_csum((unsigned char *)iph, iph->ihl);
> }
> 
> The only length the callers guarantee is the snapshot value ciph->len (plus
> 4 when ports are mangled):
> 
> net/netfilter/ipvs/ip_vs_core.c:handle_response_icmp() {
> 	unsigned int ctoff = ciph->len;
> 	...
> 		ctoff += 2 * sizeof(__u16);
> 	if (skb_ensure_writable(skb, ctoff))
> 		goto out;
> 	...
> 		ip_vs_nat_icmp(skb, pp, cp, 1, toff, has_ports, ciph);
> }
> 
> ciph->len was computed earlier in ip_vs_fill_iph_skb_off() as
> offset + iph->ihl * 4 from a skb_header_pointer() snapshot, and
> skb_ensure_writable() -> pskb_may_pull() re-copies the bytes out of the
> fragments, so the ihl byte ip_send_check() then reads can differ from the
> one that sized the validated window.
> 
> If the fresh ihl is larger than the snapshot value, can ip_fast_csum() read
> up to 40 bytes past what skb_ensure_writable() guaranteed, and fold
> adjacent skb memory into the inner header checksum that is emitted?
> 
> If the fresh ihl ends up below 5 (for example the version/ihl byte going
> from 0x45 to 0x40, compensated in tot_len so both the inner IP and ICMP
> checksums stay valid), some architectures do not tolerate it:
> 
> arch/mips/include/asm/checksum.h:ip_fast_csum() {
> 	word += 4;
> 	do {
> 		csum += *word;
> 		carry = (csum < *word);
> 		csum += carry;
> 		word++;
> 	} while (word != stop);
> }
> 
> with stop = iph + ihl, so for ihl <= 4 the loop termination is never hit.
> 
> arch/s390/include/asm/checksum.h:ip_fast_csum() {
> 	ihl -= 4;
> 	while (ihl--)
> 		csum += *ptr++;
> }
> 
> where ihl == 0 underflows into roughly four billion forward word reads.
> 
> Would it make sense to checksum over the validated length
> (ciph->len - ciph->off), or to re-validate version/ihl after
> skb_ensure_writable(), matching what was done for the port offset?

	New version of this patch was delayed on friday with
some hours, so now I have a separate fix that revalidates ihl,
ready to be applied on top of this patch...

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH net 02/10] netfilter: ebt_nflog: pin the NFLOG backend

[Florian Westphal]

Jakub Kicinski <kuba@kernel.org> wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> netfilter: ebt_nflog: pin the NFLOG backend

We need a way to annotate patches that they have known followups.

Either that or we will have to start sending much larger diffs that
change multiple things at once or we have to send much larger batches.

Re: [PATCH net 02/10] netfilter: ebt_nflog: pin the NFLOG backend

[Paolo Abeni]

On 8/5/26 9:11 AM, Florian Westphal wrote:
> Jakub Kicinski <kuba@kernel.org> wrote:
>> This is an AI-generated review of your patch. The human sending this
>> email has considered the AI review valid, or at least plausible.
>> ---
>> netfilter: ebt_nflog: pin the NFLOG backend
> 
> We need a way to annotate patches that they have known followups.

Willem used direct command to sashiko after the '---' separator:

https://lore.kernel.org/netdev/20260726202902.760552-8-willemdebruijn.kernel@gmail.com/

and it looks like that would work. I'm not sure if such per patch
annotation fits a PR creation, possibly landing it in the cover letter
would work. At least would work for human reviews.

/P

Re: [PATCH net 03/10] netfilter: ipset: rework cidr bookkeeping

[Florian Westphal]

Jakub Kicinski <kuba@kernel.org> wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> netfilter: ipset: rework cidr bookkeeping
> 
> The prefix bookkeeping of the hash:net* set types is moved out of struct
> htype into separately allocated, RCU published struct net_prefixes arrays.
> The in-place shifting of the cidr array is replaced by allocate-copy-publish,
> with a documented fallback to leaving holes when the allocation fails.
> 
> A few questions about the new bookkeeping below.

Sigh.  I knew this would happen, but what am I supposed to do now?

I explicitly noted that this isn't perfect and that I'm doing a
large series to convert ip_set_hash_gen.h over to rhashtable.

This series ATM has 19 patches.

What is your advice?  Should i squashmerge unrelated patches into one
thing?  Send the entire thing directly to net?  Make a PR just with that
entire thing?

I have no idea how to make progress here.

> > -/* cidr + 1 is stored in net_prefixes to support /0 */
> > -#define NCIDR_PUT(cidr)		((cidr) + 1)
> > -#define NCIDR_GET(cidr)		((cidr) - 1)
> > -
> >  #ifdef IP_SET_HASH_WITH_NETS_PACKED
> >  /* When cidr is packed with nomatch, cidr - 1 is stored in the data entry */
> >  #define DCIDR_PUT(cidr)		((cidr) - 1)
> > @@ -141,21 +142,11 @@ htable_size(u8 hbits)
> >  #define DCIDR_GET(cidr, i)	__CIDR(cidr, i)
> >  #endif
> >  
> > -#define INIT_CIDR(cidr, host_mask)	\
> > -	DCIDR_PUT(((cidr) ? NCIDR_GET(cidr) : host_mask))
> > -
> > -#ifdef IP_SET_HASH_WITH_NET0
> > -/* cidr from 0 to HOST_MASK value and c = cidr + 1 */
> > -#define NLEN			(HOST_MASK + 1)
> > -#define CIDR_POS(c)		((c) - 1)
> > -#else
> > -/* cidr from 1 to HOST_MASK value and c = cidr + 1 */
> > -#define NLEN			HOST_MASK
> > -#define CIDR_POS(c)		((c) - 2)
> > -#endif
> 
> [Severity: Low]
> This isn't a bug, but these were the only readers of
> IP_SET_HASH_WITH_NET0, and the flag is still defined by
> ip_set_hash_netiface.c and ip_set_hash_netportnet.c:

This sucks.  What am I supposed to do with this?

Do you want me to munge this patch to also zap this in the same commit?

> [Severity: Medium]
> Should INIT_CIDR() also look at nets[0].count?

Yes, but its not *that* simple.  Let alone better for
readability to change this here as well.

tentative direction to address this is:
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20260804170214.27237-1-fw@strlen.de/

I think I can squashmerge this one but it will be more difficult
to explain all that is going on at the same time.

> Two states created by this patch leave len non-zero with count == 0 at
> index 0:
> 
>   - mtype_del_cidr() on allocation failure:
> 
> 	tmp = kzalloc_flex(*tmp, nets, len, GFP_ATOMIC);
> 	if (!tmp)
> 		/* Leave a hole */
> 		goto unlock;

This is remove in the patch linked above.

>   - mtype_flush() on allocation failure, which zeroes the counts but keeps
>     len and the stored cidr values.

Won't do this after rhashtable conversion.

> The commit message says the hole possibility "is taken into account at
> evaluation time" - that holds for mtype_test_cidrs(), but does it hold for
> INIT_CIDR()?  This is unchanged at the end of the series (3a0d9fd64b67).

The series is partial.  The ipset -> rhashtable conversion patch set is large:
20 files changed, 931 insertions(+), 1271 deletions(-)

> [Severity: Low]
> Does the memsize reported to userspace need updating too?  The bookkeeping
> is no longer part of struct htype, but mtype_ahash_memsize() still only
> accounts for sizeof(*h):
> 
> 	return sizeof(*h) + sizeof(*t) + ahash_sizeof_regions(t->htable_bits);

I can munge this, sure.

> [Severity: Low]
> This isn't a bug, but the comment still describes the encoding this patch
> removes.  NCIDR_PUT()/NCIDR_GET() are gone and all call sites now pass the
> raw value, for example:
> 
> 	mtype_add_cidr(set, h, DCIDR_GET(d->cidr, i), i);
> 
> Could the comment be updated to describe the new invariants instead
> (descending cidr order, count == 0 marks a hole, len is the number of
> allocated slots, /0 distinguished by count)?  The stale text is still
> present at the end of the series (3a0d9fd64b67).

I can munge this.

> [Severity: Low]
> This isn't a bug on any supported architecture, but this increment (and
> the matching decrement in mtype_del_cidr(), plus the bulk zeroing in the
> mtype_flush() fallback) still mutates the already published object in
> place, with plain stores and no WRITE_ONCE().

Sigh.  Will inflate diff size even more.

> Since count now decides whether an entry is a live prefix or a hole, is it
> worth annotating these accesses?

Well, is it?

> One way to reach the decrement of a zero count: mtype_flush() clears the
> buckets first, a concurrent SET target add registers a cidr and inserts an
> element, the flush fallback afterwards zeroes that count, and the later
> delete of the still present element decrements zero.  The skew caused by the
> ignored -ENOMEM of mtype_add_cidr() (see below) gets there as well.

All of this is rewritten with the rhashtable conversion.

After conversion, mtype_flush no longer performs point-blank-clobbering,
and only entries that are successfuly detached from the rhashtable have
their cidr count changed under lock.

> > @@ -402,6 +433,9 @@ static void
> >  mtype_flush(struct ip_set *set)
> >  {
> >  	struct htype *h = set->data;
> > +#ifdef IP_SET_HASH_WITH_NETS
> > +	struct net_prefixes *nets, *tmp;
> > +#endif
> >  	struct htable *t;
> >  	struct hbucket *n;
> >  	u32 r, i;
> > @@ -425,7 +459,19 @@ mtype_flush(struct ip_set *set)
> >  		spin_unlock_bh(&t->hregion[r].lock);
> >  	}
> >  #ifdef IP_SET_HASH_WITH_NETS
> > -	memset(h->nets, 0, sizeof(h->nets));
> > +	for (i = 0; i < IPSET_NET_COUNT; i++) {
> > +		nets = ipset_dereference_nfnl(h->rnets[i]);
> > +		tmp = kzalloc_obj(*tmp, GFP_ATOMIC);
> > +		if (!tmp) {
> > +			u8 j;
> > +
> > +			for (j = 0; j < nets->len; j++)
> > +				nets->nets[j].count = 0;
> > +		} else {
> > +			rcu_assign_pointer(h->rnets[i], tmp);
> > +			kfree_rcu(nets, rcu);
> > +		}
> > +	}
> >  #endif
> >  }
> 
> [Severity: Critical]
> Does this replacement need set->lock?
> 
> mtype_add_cidr() and mtype_del_cidr() do the same read, publish and free
> sequence under spin_lock_bh(&set->lock), but here only the nfnetlink mutex
> is held.  For the hash types set->variant->region_lock is true, and
> ip_set_lock() is then a no-op:

Yes and no.  After rhashtable conversion, this mtype_flush replacement
is gone and so are region locks.

I can munge this but I considered a waste of time.

> net/netfilter/ipset/ip_set_core.c:
> static inline void
> ip_set_lock(struct ip_set *set)
> {
> 	if (!set->variant->region_lock)
> 		spin_lock_bh(&set->lock);
> }
> 
> so ip_set_flush_set() -> mtype_flush() gets no serialization against the
> cidr writers.  Those writers do not take the nfnl mutex either:

Yes, thats why all of this is on the chopping block...

> CPU0 (ipset flush)                  CPU1 (SET target, softirq)
> nets = ipset_dereference_nfnl(...)
>                                     spin_lock_bh(&set->lock);
>                                     nets = __ipset_dereference(h->rnets[n]);
> rcu_assign_pointer(h->rnets[i], tmp)
> kfree_rcu(nets, rcu);
>                                     rcu_assign_pointer(h->rnets[n], tmp);
>                                     kfree_rcu(nets, rcu);   /* same object */
> 
> That queues the same rcu_head twice, and the concurrent update is lost as
> well.  The !tmp fallback above also writes nets->nets[j].count = 0 into an
> object the other side may already have unpublished and passed to
> kfree_rcu().

I don't see how this is possible AFTER rhashtable conversion.

> [Severity: High]
> mtype_add_cidr() changed from void to int and can now fail:
> 
> 	tmp = kzalloc_flex(*tmp, nets, len, GFP_ATOMIC);
> 	if (!tmp) {
> 		ret = -ENOMEM;
> 		goto unlock;
> 	}
> 
> Should this caller check the return value?  The memcpy() commits the
> element unconditionally, so on allocation failure the element sits in the
> bucket with no prefix registered in h->rnets[].

Yes, I added this later in the series.  I can munge this here but it
again delays progress.  I had hoped to get this in but it increasingly
looks like this will have to be delayed to nf-next/net-next or even
further.  I hate this, as good as AI reviews are for finding issues they
also start to effectively prevent bug fixes from making the tree because
they are not meeting AI perfectness standards, whatever that means.

> mtype_test_cidrs() only probes cidrs that are present with count != 0:
> 
> 	for (j = 0; j < nets0->len && !multi; j++) {
> 		if (!nets0->nets[j].count)
> 			continue;
> 
> so does such an element become permanently unmatchable, while still being
> listable and deletable?

No idea.  I tire of reading AI mumbo jumbo.

Re: [PATCH net 00/10] Netfilter/IPVS fixes for net

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
Pablo, I am sorry for taking a shit on your doorstep.

I can't help it; the patches passed tests here and did not raise
any alarms here, especially because its prep work for large conversion
to rhashtable.

I therefore have no advice -- I don't know what to do to make progress.

Or, netdev maintainers accept this PR and we can handle this in
followups.

This would be my preferred choice, many of the points raised are valid,
but its either inconventient to address everything in one
patch (1000 lines changed syndrome), or the series would easily go past
20 patches mark.

Re: [PATCH net 02/10] netfilter: ebt_nflog: pin the NFLOG backend

[Pablo Neira Ayuso]

Hi Jakub,

This refers to pre-existing issue, it seems the model is now not
indicating this for some reason, see below for details.

On Tue, Aug 04, 2026 at 05:15:29PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> netfilter: ebt_nflog: pin the NFLOG backend
> 
> This adds a module reference on the ULOG logger when an ebtables nflog
> rule is validated, and drops it from a new .destroy callback, so that
> nfnetlink_log cannot be unloaded while such a rule is installed.  It also
> requests the nfnetlink_log module for legacy (non nft_compat) callers.
> 
> A few questions about the change and the commit message.

This patch is doing exactly the same as xt_LOG and xt_NFLOG so it is
needed.

> > nf_log_unregister() runs after the per-net teardown so its final RCU
> > grace period also drains readers that obtained the logger from a per-net
> > binding.
> 
> [Severity: High]
> Is this description accurate?  This isn't a bug introduced by this patch,
> but the ordering seems to be the other way around.
> 
> nfnetlink_log_fini() frees the per-net state first:
> 
> net/netfilter/nfnetlink_log.c:nfnetlink_log_fini() {
> 	...
> 	unregister_pernet_subsys(&nfnl_log_net_ops);
> 	nf_log_unregister(&nfulnl_logger);
> 	...
> }
> 
> unregister_pernet_subsys() reaches ops_undo_list(), where the only
> synchronize_rcu() happens before the exit callbacks run, and the kfree
> follows immediately after them:
> 
> net/core/net_namespace.c:ops_undo_list() {
> 	...
> 	list_for_each_entry_continue_reverse(ops, ops_list, list)
> 		ops_exit_list(ops, net_exit_list);
> 
> 	ops = saved_ops;
> 	list_for_each_entry_continue_reverse(ops, ops_list, list)
> 		ops_free_list(ops, net_exit_list);
> 	...
> }
> 
> So the grace period inside nf_log_unregister() runs after struct
> nfnl_log_net has already been freed, which would mean it protects the
> module text rather than the per-net data.
> 
> Does the per-net binding path stay exposed after this patch?  A logger
> bound per-net (sysctl /proc/sys/net/netfilter/nf_log/<pf>, or
> nf_log_bind_pf()) is only cleared in the pernet exit hook:
> 
> net/netfilter/nfnetlink_log.c:nfnl_log_net_exit() {
> 	nf_log_unset(net, &nfulnl_logger);
> 	...
> }
> 
> and nf_log_unset() does not wait for readers, since the grace period was
> removed by the commit named in the Fixes: tag:
> 
> net/netfilter/nf_log.c:nf_log_unset() {
> 	...
> 			RCU_INIT_POINTER(net->nf.nf_loggers[i], NULL);
> 	...
> }
> 
> A reader that resolved the logger through the per-net branch of
> nf_log_packet():
> 
> 	if (loginfo != NULL)
> 		logger = rcu_dereference(loggers[pf][loginfo->type]);
> 	else
> 		logger = rcu_dereference(net->nf.nf_loggers[pf]);
> 
> can still be inside nfulnl_log_packet() -> nfnl_log_pernet(net) ->
> instance_lookup_get_rcu() -> __instance_lookup() when ops_free_list()
> frees the blob, which is the same KASAN signature quoted above with no
> ebtables rule involved.  Reachable loginfo == NULL callers today include
> nf_log_trace(), the conntrack helper log in net/netfilter/nf_conntrack_helper.c
> and net/netfilter/nfnetlink_osf.c.
> 
> Would reordering nfnetlink_log_fini() so that nf_log_unregister() runs
> before unregister_pernet_subsys() close all of the frontends at once?

This sounds like a pre-existing issue.

This needs a follow up which does not belong to this patch IMO.

> > diff --git a/net/bridge/netfilter/ebt_nflog.c b/net/bridge/netfilter/ebt_nflog.c
> > index 61bf8f4465ab7..426f8adc912c7 100644
> > --- a/net/bridge/netfilter/ebt_nflog.c
> > +++ b/net/bridge/netfilter/ebt_nflog.c
> > @@ -41,11 +41,25 @@ ebt_nflog_tg(struct sk_buff *skb, const struct xt_action_param *par)
> >  static int ebt_nflog_tg_check(const struct xt_tgchk_param *par)
> >  {
> >  	struct ebt_nflog_info *info = par->targinfo;
> > +	int ret;
> >  
> >  	if (info->flags & ~EBT_NFLOG_MASK)
> >  		return -EINVAL;
> >  	info->prefix[EBT_NFLOG_PREFIX_SIZE - 1] = '\0';
> > -	return 0;
> > +
> > +	ret = nf_logger_find_get(par->family, NF_LOG_TYPE_ULOG);
> 
> [Severity: Medium]
> Should the commit message mention that this can reject rulesets that used
> to load?  ebt_nflog_tg_check() previously returned 0 unconditionally, so an
> ebtables nflog rule installed fine and simply logged nothing when
> loggers[pf][NF_LOG_TYPE_ULOG] was NULL.  Now the raw -ENOENT from
> nf_logger_find_get() is propagated out through xt_check_target(), so
> ebtables -A ... --nflog and ebtables-restore fail.  This seems worth
> spelling out given the Cc: stable.
> 
> Also, is the stated parity with xt_NFLOG complete?  NETFILTER_XT_TARGET_NFLOG
> carries select NETFILTER_NETLINK_LOG in net/netfilter/Kconfig, while
> BRIDGE_EBT_NFLOG has no select or depends:
> 
> net/bridge/netfilter/Kconfig:
> config BRIDGE_EBT_NFLOG
> 	tristate "ebt: nflog support"
> 	help
> 	  This option enables the nflog watcher, which allows to LOG
> 	  messages through the netfilter logging API, which can use
> 	  either the old LOG target, the old ULOG target or nfnetlink_log
> 	  as backend.
> 
> On a kernel with CONFIG_BRIDGE_EBT_NFLOG set and
> CONFIG_NETFILTER_NETLINK_LOG=n, the request_module() fallback below can
> never succeed, so the target becomes unusable.  Should a
> select NETFILTER_NETLINK_LOG be added here?  The help text claiming the
> watcher can use the old LOG or ULOG targets as backend also no longer
> matches the code.

This sounds again like a pre-existing issue in ebt_nflog's Kconfig.

> 
> > +	if (ret != 0 && !par->nft_compat) {
> > +		request_module("%s", "nfnetlink_log");
> > +
> > +		ret = nf_logger_find_get(par->family, NF_LOG_TYPE_ULOG);
> > +	}
> > +
> > +	return ret;
> 
> [Severity: Medium]
> Who loads nfnetlink_log for the nft_compat case?

This is again a pre-existing issue that should be handled in a follow
up, it affects xt_LOG and xt_NFLOG too.

> The !par->nft_compat skip is copied from xt_NFLOG, but xt_NFLOG also
> declares MODULE_SOFTDEP("pre: nfnetlink_log"), and ebt_nflog.c has
> no MODULE_SOFTDEP at all, so demand-loading ebt_nflog through
> xt_request_find_target(NFPROTO_BRIDGE, "nflog", rev) does not bring
> in the backend.
> 
> The -ENOENT recovery in nft_compat only matches the uppercase names:
> 
> net/netfilter/nft_compat.c:nft_target_init() {
> 	...
> 		if (ret == -ENOENT) {
> 			const char *modname = NULL;
> 
> 			if (strcmp(target->name, "LOG") == 0)
> 				modname = "nf_log_syslog";
> 			else if (strcmp(target->name, "NFLOG") == 0)
> 				modname = "nfnetlink_log";
> 
> 			if (modname &&
> 			    nft_request_module(ctx->net, "%s", modname) == -EAGAIN)
> 				return -EAGAIN;
> 		}
> 	...
> }
> 
> The target registered here is .name = "nflog", so modname stays NULL, no
> module is requested and there is no -EAGAIN retry.  Does an nftables
> bridge-family compat rule using nflog now fail with -ENOENT unless
> nfnetlink_log is already resident, where it succeeded before?  Adding
> MODULE_SOFTDEP("pre: nfnetlink_log") to ebt_nflog.c, or extending the name
> mapping in nft_target_init(), would seem to cover it.
> 
> > +}
> > +
> > +static void ebt_nflog_tg_destroy(const struct xt_tgdtor_param *par)
> > +{
> > +	nf_logger_put(par->family, NF_LOG_TYPE_ULOG);
> >  }
> >  
> >  static struct xt_target ebt_nflog_tg_reg __read_mostly = {
> > @@ -54,6 +68,7 @@ static struct xt_target ebt_nflog_tg_reg __read_mostly = {
> >  	.family     = NFPROTO_BRIDGE,
> >  	.target     = ebt_nflog_tg,
> >  	.checkentry = ebt_nflog_tg_check,
> > +	.destroy    = ebt_nflog_tg_destroy,
> >  	.targetsize = sizeof(struct ebt_nflog_info),
> >  	.me         = THIS_MODULE,
> >  };

Re: [PATCH net 03/10] netfilter: ipset: rework cidr bookkeeping

[Pablo Neira Ayuso]

On Tue, Aug 04, 2026 at 05:15:31PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> netfilter: ipset: rework cidr bookkeeping
> 
> The prefix bookkeeping of the hash:net* set types is moved out of struct
> htype into separately allocated, RCU published struct net_prefixes arrays.
> The in-place shifting of the cidr array is replaced by allocate-copy-publish,
> with a documented fallback to leaving holes when the allocation fails.

Florian needs these starter ipset patches to follow with more fixes to
move ipset hash to use rhashtable.

For his workflow, he considers it better to do this incrementally.

While this is not perfect, we prefer this path to address the existing
issues that LLMs have reported in ipset.

Re: [PATCH net 08/10] ipvs: avoid out-of-bounds write in ip_vs_nat_icmp

[Pablo Neira Ayuso]

Hi Jakub,

On Tue, Aug 04, 2026 at 05:15:34PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ipvs: avoid out-of-bounds write in ip_vs_nat_icmp
> 
> This passes the already-parsed ip_vs_iphdr down into ip_vs_nat_icmp() so
> the embedded transport header offset comes from ciph->len instead of a
> second read of the inner IPv4 ihl field.  The IPv4 header sanity checks in
> ip_vs_out_icmp() and ip_vs_in_icmp() are also reworked to read ihl only
> once.
> 
> Link: https://sashiko.dev/#/patchset/20260722101517.36313-1-ja%40ssi.bg

Julian (he's on Cc) is ready to follow to address the remaining
comments mentioned here.

Re: [PATCH net 10/10] netfilter: nft_ct: move custom expectation support to helper

[Pablo Neira Ayuso]

Hi Jakub,

TLDR; Will you accept to take this nf.git PR excluding this patch #10?

Main issue here is that this will trigger a conflict with net-next,
which will be hairy including follow up to fix this. Maybe I can
repurpose this patch to net-next.

Longer read: The original author of this feature added a stub with
limitations that were not addressed over time so its usecase is very
limited. But several of the LLM reports are correct and I can post an
improved version of this patch.

On Tue, Aug 04, 2026 at 05:15:35PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> netfilter: nft_ct: move custom expectation support to helper
> 
> This reworks the nftables ct expectation object so that the expectation is
> no longer created directly from the rule evaluation, but from an internal
> conntrack helper that is allocated per object and attached to the conntrack
> entry. nf_conntrack_helper_free() is split out of
> nf_conntrack_helper_unregister() and exported so the internal helper can be
> released when the object goes away.
> 
> Link: https://patch.msgid.link/20260715144755.00ea7dfcd9f@proton.me
> 
> A few questions about the commit message first.
> 
> > Originally, the ct expectation support called nf_ct_helper_ext_add() for
> > confirmed conntracks, which is invalid, triggering a splat. This was
> > fixed by commit 1710eb913bdc ("netfilter: nft_ct: skip expectations for
> > confirmed conntrack") which restricted it to confirmed conntracks.
> 
> [Severity: Low]
> Is this description inverted?  That commit skips confirmed conntracks, so
> it restricted the feature to unconfirmed ones, which is also the guard this
> patch keeps in nft_ct_expect_obj_eval():
> 
> 	if (!ct || nf_ct_is_template(ct) || nf_ct_is_confirmed(ct)) {
> 		regs->verdict.code = NFT_BREAK;
> 		return;
> 	}
> 
> > Fixes: 857b46027d6f ("netfilter: nft_ct: add ct expectations support")
> 
> [Severity: Low]
> The Fixes tag points at the 2019 feature commit, but the implementation
> relies on struct nf_conntrack_helper::ct_refcnt, which git blame on
> include/net/netfilter/nf_conntrack_helper.h attributes to commit
> ac46f3f35b6e68 (2026-06-04), on the refcount_inc_not_zero() guarded
> help->helper assignment in __nf_ct_try_assign_helper(), and on the
> kzalloc_obj() slab macro.
> 
> Would a note about stable applicability be worth adding, since a tree
> without the ct_refcnt rework would not build this and has no equivalent
> lifetime scheme for an unregistered helper?

The ct_refcnt is a dependency.

But the issue that this patch address is really related to
1710eb913bdc where this custom ct expectation infrastructure was
introduced.

> > diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> > index 500509b17663e..1197e8793494b 100644
> > --- a/net/netfilter/nf_conntrack_helper.c
> > +++ b/net/netfilter/nf_conntrack_helper.c
> > @@ -456,13 +456,8 @@ static bool expect_iter_me(struct nf_conntrack_expect *exp, void *data)
> >  	return this == me;
> >  }
> >  
> > -void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
> > +void nf_conntrack_helper_free(struct nf_conntrack_helper *me)
> >  {
> > -	mutex_lock(&nf_ct_helper_mutex);
> > -	hlist_del_rcu(&me->hnode);
> > -	nf_ct_helper_count--;
> > -	mutex_unlock(&nf_ct_helper_mutex);
> > -
> >  	/* This helper is going away, disable it. */
> >  	rcu_assign_pointer(me->help, NULL);
> >  
> > @@ -476,6 +471,17 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
> >  	if (refcount_dec_and_test(&me->ct_refcnt))
> >  		kfree_rcu(me, rcu);
> >  }
> > +EXPORT_SYMBOL_GPL(nf_conntrack_helper_free);
> > +
> > +void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
> > +{
> > +	mutex_lock(&nf_ct_helper_mutex);
> > +	hlist_del_rcu(&me->hnode);
> > +	nf_ct_helper_count--;
> > +	mutex_unlock(&nf_ct_helper_mutex);
> > +
> > +	nf_conntrack_helper_free(me);
> > +}
> >  EXPORT_SYMBOL_GPL(nf_conntrack_helper_unregister);
> >  
> >  void nf_ct_helper_init(struct nf_conntrack_helper *helper,
> 
> [ ... ]
> 
> > diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> > index 03a88c77e0f02..30c9358dbf48a 100644
> > --- a/net/netfilter/nft_ct.c
> > +++ b/net/netfilter/nft_ct.c
> > @@ -1226,6 +1228,73 @@ static int nft_ct_expect_timeout_get(const struct nlattr *attr, u32 *val)
> >  	return 0;
> >  }
> >  
> > +struct nft_ct_expect_data {
> > +	struct nft_ct_expect_obj	obj;
> > +	enum ip_conntrack_dir		dir;
> > +	atomic_t			num_expects;
> > +};
> > +
> > +static int ct_expect_help(struct sk_buff *skb, unsigned int protoff,
> > +			  struct nf_conn *ct, enum ip_conntrack_info ctinfo)
> > +{
> > +	enum ip_conntrack_dir dir = CTINFO2DIR(ctinfo);
> > +	struct nft_ct_expect_data *expect_data;
> > +	struct nf_conntrack_expect *exp;
> > +	int ret = NF_ACCEPT;
> > +
> > +	expect_data = nfct_help_data(ct);
> > +	if (!expect_data)
> > +		return NF_ACCEPT;
> > +
> > +	if (expect_data->dir != dir)
> > +		return NF_ACCEPT;
> > +
> > +	if (!atomic_add_unless(&expect_data->num_expects, 1, expect_data->obj.size))
> > +		return NF_ACCEPT;
> 
> [Severity: Medium]
> Does this change the meaning of NFTA_CT_EXPECT_SIZE?

This is a pre-existing issue.

This has only worked with NFTA_CT_EXPECT_SIZE == 1

This is because this creates over and over again the same expectation,
which results in expect_clash() returning -EBUSY.

> The old gate in nft_ct_expect_obj_eval() was:
> 
> 	if (help->expecting[NF_CT_EXPECT_CLASS_DEFAULT] >= priv->size) {
> 
> that is a limit on live expectations, and nf_ct_unlink_expect() decrements
> expecting[] once an expectation is consumed or times out.
> 
> num_expects here is only ever decremented on failure, so it looks like a
> cumulative lifetime cap instead: after size expectations have been created,
> no further expectation is created for that conntrack even when the previous
> ones are long gone.
> 
> The other difference is when the callback runs.  nf_confirm() calls
> helper_cb() for every packet of the flow once nfct_help(ct) is set, so with
> size >= 2 the first packets of an already established connection re-enter
> ct_expect_help() and re-insert the same expectation through the refresh
> path in __nf_ct_expect_check():
> 
> 	if (master_matches(i, expect, flags) && expect_matches(i, expect)) {
> 		...
> 		nf_ct_unlink_expect(i);
> 		break;
> 
> Should the commit message describe these two behaviour changes, since it
> presents the patch as only moving the feature into a helper?

There is no change in the behaviour, the LLM here is just missing
previous context on the status of this custom ct expectactation
feature.

> > +
> > +	exp = nf_ct_expect_alloc(ct);
> > +	if (!exp) {
> > +		atomic_dec(&expect_data->num_expects);
> > +		return NF_DROP;
> > +	}
> > +
> > +	nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT, nf_ct_l3num(ct),
> > +			  &ct->tuplehash[!dir].tuple.src.u3,
> > +			  &ct->tuplehash[!dir].tuple.dst.u3,
> > +			  expect_data->obj.l4proto, NULL, &expect_data->obj.dport);
> 
> [Severity: Low]
> Does NFTA_CT_EXPECT_L3PROTO still do anything after this?  The old eval
> passed the configured value:
> 
> 	u16 l3num = priv->l3num;
> 	...
> 	if (l3num == NFPROTO_INET)
> 		l3num = nf_ct_l3num(ct);
> 
> Here nf_ct_l3num(ct) is used unconditionally and priv->l3num is not read by
> any datapath code, while nft_ct_expect_obj_init() still parses and
> range-checks the attribute and nft_ct_expect_obj_dump() still echoes it
> back.  In an inet table, which is the one case where priv->l3num is allowed
> to differ from ctx->family, "l3proto ipv4" now has no effect on which
> conntracks get expectations.

This can be addressed in a follow up, but it is not a big deal.

> > +	exp->timeout += expect_data->obj.timeout;
> > +
> > +	if (nf_ct_expect_related(exp, 0) != 0) {
> > +		atomic_dec(&expect_data->num_expects);
> > +		ret = NF_DROP;
> > +	}
> 
> [Severity: Medium]
> Can this blackhole an established flow?

This is a pre-existing issue, this drops packets when
nf_ct_expect_related() fails with -EBUSY with clashes which is likely
to happen as explained above.

> The budget is given back on failure and nf_confirm() runs the
> callback for every packet:
> 
> 	ret = helper_cb(skb, protoff, ct, ctinfo);
> 	if (ret != NF_ACCEPT)
> 		return ret;
> 
> So for a persistent failure the retry happens per packet and each retry
> drops the packet.  A persistent failure looks reachable: once expectation E
> has been consumed or expired, another conntrack creating a byte-identical
> tuple takes the clash path in __nf_ct_expect_check() rather than the
> replace path:
> 
> 	} else if (expect_clash(i, expect)) {
> 		ret = -EBUSY;
> 		goto out;
> 
> A full expect table (-EMFILE) has the same shape.  Before this patch the
> NF_DROP could only happen for the unconfirmed first packet; now every
> subsequent packet of the confirmed connection in that direction is dropped
> for as long as the clash lasts.
> 
> > +
> > +	nf_ct_expect_put(exp);
> > +
> > +	return ret;
> > +}
> > +
> > +static int nft_ct_expect_helper_alloc(struct nft_ct_expect_obj *priv)
> > +{
> > +	struct nf_conntrack_helper *ct_expect_helper;
> > +
> > +	ct_expect_helper = kzalloc_obj(struct nf_conntrack_helper);
> 
> [Severity: Low]
> Should this be accounted to the memcg?  kzalloc_obj() with no gfp argument
> is GFP_KERNEL, while the equivalent allocation of the same struct in
> nf_conntrack_helper_register() uses:
> 
> 	new_helper = kzalloc_obj(*new_helper, GFP_KERNEL_ACCOUNT);

Yes, that is better.

> as do the nfnetlink_cthelper allocations.  The object is created from an
> NFT_MSG_NEWOBJ request and can outlive the ruleset, since every conntrack
> that got the helper assigned holds an extra ct_refcnt.
> 
> > +	if (!ct_expect_helper)
> > +		return -ENOMEM;
> > +
> > +	snprintf(ct_expect_helper->name, sizeof(ct_expect_helper->name), "%s",
> > +		 "nft_ct_expect");
> > +	ct_expect_helper->me = THIS_MODULE;
> > +	ct_expect_helper->expect_policy[NF_CT_EXPECT_CLASS_DEFAULT].max_expected = priv->size;
> 
> [Severity: Low]
> This assignment skips the normalization that every registered helper gets
> in __nf_conntrack_helper_register():
> 
> 	if (!me->expect_policy[i].max_expected)
> 		me->expect_policy[i].max_expected = NF_CT_EXPECT_MAX_CNT;
> 
> With "size 0", which nft_ct_expect_obj_init() accepts, the attached helper
> ends up with max_expected == 0.  __nf_ct_expect_check() then evaluates:
> 
> 	p = &helper->expect_policy[expect->class];
> 	if (master_help->expecting[expect->class] >= p->max_expected)
> 		evict_oldest_expect(master_help, expect, p);
> 
> as always true, so any other creator of an expectation for that master
> (ctnetlink, for instance) takes the eviction path every time.  Is
> max_expected == 0 intended here?

Yes, this should use max_expected = size.

But since this only works with NFTA_CT_EXPECT_SIZE == 1, so it is not
a big deal to run the eviction logic to remove the previous
expectation.

> There is also a mismatch of meaning: max_expected is a refillable
> concurrent limit against master_help->expecting[], whereas the num_expects
> gate in ct_expect_help() treats the same configured value as a cumulative
> cap.
> 
> > +	rcu_assign_pointer(ct_expect_helper->help, ct_expect_help);
> > +	refcount_set(&ct_expect_helper->ct_refcnt, 1);
> > +
> > +	/* No need to register this helper, this is internal. */
> > +	priv->helper = ct_expect_helper;
> 
> [Severity: Low]
> The helper's tuple stays zeroed here, so tuple.dst.protonum == 0, unlike
> every helper built by nf_ct_helper_init() which sets a real IPPROTO_*.
> The two dispatchers of ->help() disagree about that value.  nf_confirm()
> calls the callback unconditionally, while nf_ct_helper() used by act_ct and
> openvswitch filters first:
> 
> 	if (helper->tuple.dst.protonum != proto)
> 		return NF_ACCEPT;
> 
> which can never match protonum 0.  So on the act_ct/OVS path this helper is
> silently skipped and no expectation is ever created.

This does not work with act_ct/OVS and this tuple.dst.protonum has
been removed in net-next.

> I could not construct a concrete hook ordering where nf_ct_helper() runs
> before nf_confirm() has already created the expectation, so this may be
> harmless today, but what protonum is intended for this helper?
> NFTA_CT_EXPECT_L4PROTO describes the expected child connection, not the
> master, so no obvious value is available at object init time.
> 
> > +
> > +	return 0;
> > +}
> > +
> >  static int nft_ct_expect_obj_init(const struct nft_ctx *ctx,
> >  				  const struct nlattr * const tb[],
> >  				  struct nft_object *obj)
> 
> [ ... ]
> 
> > @@ -1273,13 +1344,26 @@ static int nft_ct_expect_obj_init(const struct nft_ctx *ctx,
> >  	priv->dport = nla_get_be16(tb[NFTA_CT_EXPECT_DPORT]);
> >  	priv->size = nla_get_u8(tb[NFTA_CT_EXPECT_SIZE]);
> >  
> > -	return nf_ct_netns_get(ctx->net, ctx->family);
> > +	err = nf_ct_netns_get(ctx->net, ctx->family);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	err = nft_ct_expect_helper_alloc(priv);
> > +	if (err < 0) {
> > +		nf_ct_netns_put(ctx->net, ctx->family);
> > +		return err;
> > +	}
> > +
> > +	return err;
> >  }
> >  
> >  static void nft_ct_expect_obj_destroy(const struct nft_ctx *ctx,
> > -				       struct nft_object *obj)
> > +				      struct nft_object *obj)
> >  {
> > +	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
> > +
> >  	nf_ct_netns_put(ctx->net, ctx->family);
> > +	nf_conntrack_helper_free(priv->helper);
> >  }
> 
> [Severity: High]
> Is it acceptable to run nf_conntrack_helper_free() once per destroyed
> object here?  It does:
> 
> 	rcu_assign_pointer(me->help, NULL);
> 	synchronize_rcu();
> 	nf_ct_expect_iterate_destroy(expect_iter_me, me);
> 
> so one full RCU grace period plus a walk of the entire global expectation
> hash under spin_lock_bh(&nf_conntrack_expect_lock), work that previously
> only happened on helper module unregistration.

I can remove this synchronize_rcu(), it is not needed since the
ct_refcnt has been added.

Basically, removal of expectations is best effort to retain the
existing behaviour users know, but master conntrack is responsible to
remove it if it goes away.

If an packets races to create an expectation of this type, it should
not be an issue.

> In the abort path this runs from __nf_tables_abort():
> 
> 	synchronize_rcu();
> 
> 	list_for_each_entry_safe_reverse(trans, next, &nft_net->commit_list, list) {
> 		nft_trans_list_del(trans);
> 		nf_tables_abort_release(trans);
> 	}
> 
> which reaches nft_obj_destroy() -> nft_ct_expect_obj_destroy() while
> nft_net->commit_mutex and nfnl_lock are still held.  A batch that creates N
> ct expect objects and then fails therefore serializes N grace periods under
> the per-netns nftables mutex, with N chosen by the caller.
> __nft_release_table() on netns exit has the same property.
> 
> The commit-release path already does one synchronize_rcu() for the whole
> batch, and nf_tables_commit_release() carries the comment "Memory reclaim
> happens asynchronously from work queue to prevent expensive
> synchronize_rcu() in commit phase".  Could the helper release be deferred
> to that work queue instead?
> 
> >  
> >  static int nft_ct_expect_obj_dump(struct sk_buff *skb,
> > @@ -1302,50 +1386,39 @@ static void nft_ct_expect_obj_eval(struct nft_object *obj,
> >  				   const struct nft_pktinfo *pkt)
> >  {
> >  	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
> > -	struct nf_conntrack_expect *exp;
> > +	struct nft_ct_expect_data *expect_data;
> >  	enum ip_conntrack_info ctinfo;
> >  	struct nf_conn_help *help;
> > -	enum ip_conntrack_dir dir;
> > -	u16 l3num = priv->l3num;
> >  	struct nf_conn *ct;
> >  
> >  	ct = nf_ct_get(pkt->skb, &ctinfo);
> > -	if (!ct || nf_ct_is_confirmed(ct) || nf_ct_is_template(ct)) {
> > +	if (!ct || nf_ct_is_template(ct) || nf_ct_is_confirmed(ct)) {
> >  		regs->verdict.code = NFT_BREAK;
> >  		return;
> >  	}
> > -	dir = CTINFO2DIR(ctinfo);
> >  
> >  	help = nfct_help(ct);
> > -	if (!help)
> > -		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
> > -	if (!help) {
> > -		regs->verdict.code = NF_DROP;
> > -		return;
> > -	}
> > -
> > -	if (help->expecting[NF_CT_EXPECT_CLASS_DEFAULT] >= priv->size) {
> > +	if (help) {
> >  		regs->verdict.code = NFT_BREAK;
> >  		return;
> >  	}
> 
> [Severity: High]
> Does this permanently disable the statement for conntracks that already
> carry a helper extension?

Yes, and it is intentional.

> The old code reused an existing extension:
> 
> 	help = nfct_help(ct);
> 	if (!help)
> 		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
> 
> and created the expectation either way.  Now any conntrack with
> nfct_help(ct) != NULL takes NFT_BREAK, which also aborts the remainder of
> the rule, so "ct expect set e counter accept" no longer reaches accept and
> the chain policy applies instead.
> 
> Three in-tree ways to be in that state before nft rules run:
> 
>   init_conntrack() for a conntrack born from an expectation with
>   assign_helper set:
> 
> 	help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
> 	if (help && refcount_inc_not_zero(&assign_helper->ct_refcnt))
> 		rcu_assign_pointer(help->helper, assign_helper);
> 
>   __nf_ct_try_assign_helper() during nf_conntrack_in() when the template
>   carries a helper:
> 
> 	if (help == NULL) {
> 		help = nf_ct_helper_ext_add(ct, flags);
> 
>   and a ruleset that runs "ct helper set" (nft_ct_helper_obj_eval()) or a
>   second ct expect object on the same connection.
> 
> Should this case fall back to attaching the expectation data anyway, or at
> least be documented?
> 
> >  
> > -	exp = nf_ct_expect_alloc(ct);
> > -	if (exp == NULL) {
> > +	help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
> > +	if (!help) {
> >  		regs->verdict.code = NF_DROP;
> >  		return;
> >  	}
> >  
> > +	expect_data = nfct_help_data(ct);
> > +	if (!expect_data) {
> > +		regs->verdict.code = NFT_BREAK;
> > +		return;
> > +	}
> > +	expect_data->obj = *priv;
> 
> [Severity: Low]
> This copies the whole struct nft_ct_expect_obj, which now includes the raw
> struct nf_conntrack_helper *helper pointer, into the conntrack helper
> extension private area, without taking a reference for that copy.  Only
> help->helper, referenced on the next line, is a valid handle.

This pointer is not used, I can just copy what it is needed.

> Since the conntrack extension can outlive the nft object,
> expect_data->obj.helper becomes stale once nft_ct_expect_obj_destroy() ->
> nf_conntrack_helper_free() drops the last reference and kfree_rcu() runs.
> Nothing reads it today, so this is latent, but would it be clearer to copy
> only the fields ct_expect_help() uses?
> 
> It also spends 8 of the 32 bytes available in nf_conn_help->data, and
> struct nft_ct_expect_data is already at the NF_CT_HELPER_BUILD_BUG_ON
> limit.
> 
> > +	expect_data->dir = CTINFO2DIR(ctinfo);
> >  
> > -	nf_ct_expect_put(exp);
> > +	if (help && refcount_inc_not_zero(&priv->helper->ct_refcnt))
> > +		rcu_assign_pointer(help->helper, priv->helper);
> >  }
> 
> [ ... ]

Re: [PATCH net 00/10] Netfilter/IPVS fixes for net

[Pablo Neira Ayuso]

Hi,

After posting the review of sashiko's comment my proposal is to
integrate patch 1 to 9 of this PR upstream, just excluding patch 10.

Thanks.

Re: [PATCH net 02/10] netfilter: ebt_nflog: pin the NFLOG backend

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >  	info->prefix[EBT_NFLOG_PREFIX_SIZE - 1] = '\0';
> > > -	return 0;
> > > +
> > > +	ret = nf_logger_find_get(par->family, NF_LOG_TYPE_ULOG);
> > 
> > [Severity: Medium]
> > Should the commit message mention that this can reject rulesets that used
> > to load?  ebt_nflog_tg_check() previously returned 0 unconditionally, so an
> > ebtables nflog rule installed fine and simply logged nothing when
> > loggers[pf][NF_LOG_TYPE_ULOG] was NULL.  Now the raw -ENOENT from
> > nf_logger_find_get() is propagated out through xt_check_target(), so
> > ebtables -A ... --nflog and ebtables-restore fail.  This seems worth
> > spelling out given the Cc: stable.
> > 
> > Also, is the stated parity with xt_NFLOG complete?  NETFILTER_XT_TARGET_NFLOG
> > carries select NETFILTER_NETLINK_LOG in net/netfilter/Kconfig, while
> > BRIDGE_EBT_NFLOG has no select or depends:
> > 
> > net/bridge/netfilter/Kconfig:
> > config BRIDGE_EBT_NFLOG
> > 	tristate "ebt: nflog support"
> > 	help
> > 	  This option enables the nflog watcher, which allows to LOG
> > 	  messages through the netfilter logging API, which can use
> > 	  either the old LOG target, the old ULOG target or nfnetlink_log
> > 	  as backend.
> > 
> > On a kernel with CONFIG_BRIDGE_EBT_NFLOG set and
> > CONFIG_NETFILTER_NETLINK_LOG=n, the request_module() fallback below can
> > never succeed, so the target becomes unusable.  Should a
> > select NETFILTER_NETLINK_LOG be added here?  The help text claiming the
> > watcher can use the old LOG or ULOG targets as backend also no longer
> > matches the code.
> 
> This sounds again like a pre-existing issue in ebt_nflog's Kconfig.

Yes, this is ancient by kernel standards.

> > > +	if (ret != 0 && !par->nft_compat) {
> > > +		request_module("%s", "nfnetlink_log");
> > > +
> > > +		ret = nf_logger_find_get(par->family, NF_LOG_TYPE_ULOG);
> > > +	}
> > > +
> > > +	return ret;
> > 
> > [Severity: Medium]
> > Who loads nfnetlink_log for the nft_compat case?
> 
> This is again a pre-existing issue that should be handled in a follow
> up, it affects xt_LOG and xt_NFLOG too.

Its not, actually: in ebtables case, user has to do it manually:
rule did load but doesn't work, now rule load would fail.

I think the best fix is to just add
MODULE_SOFTDEP("pre: nfnetlink_log");

like in xt_NFLOG and let modprobe handle it.
I don't think we should bother extending the workarounds for
modprobe we carry in nft_compat for this.

And also add a kconfig dependency to be sure.

And I think it should be done in a different commit.

Re: [PATCH net 03/10] netfilter: ipset: rework cidr bookkeeping

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Florian needs these starter ipset patches to follow with more fixes to
> move ipset hash to use rhashtable.
>
> For his workflow, he considers it better to do this incrementally.

Some things I can squash and resend, but in other cases it would indeed
be better to keep it extra to avoid too large commits and better
document which patch is fixing what issue.

Many of these (new) LLM findings have root causes in older, different
problems.

> While this is not perfect, we prefer this path to address the existing
> issues that LLMs have reported in ipset.

Yes, we continue to get bug reports in code that is on the chopping
block, so the more I changes I have to squash and then send new version
the longer it will take to get the real conversion patches in.

Around half of the LLM issues I already knew and deliberately did not
fix in this first series to keep diff size smaller.

Paolo linked to an unrelated patch having a 'sashiko: ' comment
in the diffstat section to suppress known benign reports, I will
try to remember and make use of this for future submissions that
have known warts.

Re: [PATCH net 00/10] Netfilter/IPVS fixes for net

[Jakub Kicinski]

On Wed, 5 Aug 2026 19:39:24 +0200 Pablo Neira Ayuso wrote:
> Hi,
> 
> After posting the review of sashiko's comment my proposal is to
> integrate patch 1 to 9 of this PR upstream, just excluding patch 10.

SG! Whatever you guys prefer. Sorry for dumping the reviews on the list
without much discernment, the netdev queue never dips below 400 patches
these days so when we can depend on other maintainers we'll just do as
told at this point..

Will pull the first 9 (and the -next PR).
Thanks!

Re: [PATCH net 01/10] ipvs: stop estimator after disabled calc phase

[patchwork-bot+netdevbpf]

Hello:

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

On Fri, 31 Jul 2026 17:17:57 +0200 you wrote:
> From: Zhiling Zou <zhilinz@nebusec.ai>
> 
> IPVS estimator kthread 0 starts with zeroed chain and tick limits until
> its initial calculation phase completes. If network namespace teardown
> clears ipvs->enable during that phase, ip_vs_est_calc_phase() can return
> without installing positive limits.
> 
> [...]

Here is the summary with links:
  - [net,01/10] ipvs: stop estimator after disabled calc phase
    https://git.kernel.org/netdev/net/c/558f67f1340f
  - [net,02/10] netfilter: ebt_nflog: pin the NFLOG backend
    https://git.kernel.org/netdev/net/c/30825970339c
  - [net,03/10] netfilter: ipset: rework cidr bookkeeping
    https://git.kernel.org/netdev/net/c/8e5fd2a55e24
  - [net,04/10] netfilter: ipset: switch ext_size to atomic64_t
    https://git.kernel.org/netdev/net/c/712a6f545c35
  - [net,05/10] netfilter: ipset: add small wrappers for hash and bucket sizes
    https://git.kernel.org/netdev/net/c/c266769e9ede
  - [net,06/10] netfilter: ipset: add and use mtype_del_cidr_all helper
    https://git.kernel.org/netdev/net/c/cdd97fae0e96
  - [net,07/10] netfilter: ipset: switch to rcu work
    https://git.kernel.org/netdev/net/c/7defddefa95b
  - [net,08/10] ipvs: avoid out-of-bounds write in ip_vs_nat_icmp
    https://git.kernel.org/netdev/net/c/646922a03794
  - [net,09/10] ipvs: return the csum validation for forward hook
    https://git.kernel.org/netdev/net/c/99609cb0aa78
  - [net,10/10] netfilter: nft_ct: move custom expectation support to helper
    (no matching commit)

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