[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 for net:

1) Set on HW_DEAD after HW_PENDING is cleared in the flowtable offload
   to ensure GC does not zap it, from Jérémy Jean.

2) Hold the nfnetlink_queue mutex while removing the queue instance
   from the netlink notifier that handles NETLINK_URELEASE to fix a
   possible race with the UNBIND command. From Florian Westphal.

3) Reject route with NULL rt6i_idev in ip6t_rpfilter. From Weiming Shi.

4) Reject rtinfo->addrnr set to zero from ip6t_rt .checkentry path.
   This also fortifies the datapath loop as per Florian's request.
   From Luxiao Xu.

5) Fix checksuming in nft_synproxy for IPv6, from Karl Mehltretter.

6) Revalidate ihl before calling icmp_send() in IPVS,
   from Julian Anastasov.

7) Sanitize flags in IPVS sync messages that are received in the
   backup, from Julian Anastasov.

8) Fix suspicious RCU usage splat in ctnetlink with expectations.

9) Remove WARN_ON_ONCE in dev_fill_forward_path(). Recent support
   from IPIP tunnels allow for loops. From Farhad Alemi.

10) Check for expired catchall elements in the insert and deactivate
    path. From Aohan Mei.

Please, pull these changes from:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git nf-26-09-17

Thanks.

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

The following changes since commit ceac0de741bfb47ca255eee075257b3bb31f0651:

  netlink: do not free nlk->groups while lockless readers can use it (2026-09-15 18:44:02 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git nf-26-09-17

for you to fetch changes up to d9be517d5aef7ebf132b0f33965c7a8232a02da9:

  netfilter: nf_tables: skip expired catchall elements on insert and delete (2026-09-17 01:00:01 +0200)

----------------------------------------------------------------
netfilter pull request 26-09-17

----------------------------------------------------------------
Aohan Mei (1):
      netfilter: nf_tables: skip expired catchall elements on insert and delete

Farhad Alemi (1):
      net: remove WARN_ON_ONCE() from the dev_fill_forward_path() loop check

Florian Westphal (1):
      netfilter: nfnetlink_queue: hold nfnl mutex in event notifier

Julian Anastasov (2):
      ipvs: revalidate ihl before icmp_send
      ipvs: filter some flags received in the backup server

Jérémy Jean (1):
      netfilter: flowtable: publish HW_DEAD after worker is done

Karl Mehltretter (1):
      netfilter: nft_synproxy: use the family-aware checksum helper

Luxiao Xu (1):
      netfilter: ip6t_rt: fix zero-address non-strict match out-of-bounds read

Naman Gulati (1):
      netfilter: ctnetlink: fix suspicious RCU usage in expect_iter_name

Weiming Shi (1):
      netfilter: ip6t_rpfilter: reject routes without inet6_dev

 net/core/dev.c                        |  2 +-
 net/ipv6/netfilter/ip6t_rpfilter.c    |  2 +-
 net/ipv6/netfilter/ip6t_rt.c          | 11 ++++++++---
 net/netfilter/ipvs/ip_vs_core.c       |  6 ++++++
 net/netfilter/ipvs/ip_vs_sync.c       | 33 ++++++++++++++++++++++++++++++---
 net/netfilter/nf_conntrack_netlink.c  |  3 ++-
 net/netfilter/nf_flow_table_offload.c |  7 ++++++-
 net/netfilter/nf_tables_api.c         |  8 ++++++--
 net/netfilter/nfnetlink_queue.c       |  8 +++++---
 net/netfilter/nft_synproxy.c          |  3 ++-
 10 files changed, 67 insertions(+), 16 deletions(-)

[PATCH net 01/10] netfilter: flowtable: publish HW_DEAD after worker is done

[Pablo Neira Ayuso]

From: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>

flow_offload_work_del() sets NF_FLOW_HW_DEAD before the work handler
clears NF_FLOW_HW_PENDING. Once a flow is both HW_DYING and HW_DEAD, a
concurrent garbage collection pass can remove it and schedule it for RCU
freeing.

The offload worker holds neither an RCU read lock nor a reference to the
flow. If it is preempted after publishing HW_DEAD, the RCU callback can
free the flow before the worker resumes and clears HW_PENDING, resulting
in a use-after-free.

Move HW_DEAD publication to the common worker epilogue after the pending
bit is cleared, making it the final flow access by destroy work. Order all
preceding flow accesses before publishing the bit that allows garbage
collection to free the object.

Fixes: 2c8897953f3b ("netfilter: flowtable: Add pending bit for offload work")
Assisted-by: Codex:gpt-5
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_offload.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index 801a3dd9ceea..6757fd89c1f1 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -995,7 +995,6 @@ static void flow_offload_work_del(struct flow_offload_work *offload)
 	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_ORIGINAL);
 	if (test_bit(NF_FLOW_HW_BIDIRECTIONAL, &offload->flow->flags))
 		flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_REPLY);
-	set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags);
 }
 
 static void flow_offload_tuple_stats(struct flow_offload_work *offload,
@@ -1059,6 +1058,12 @@ static void flow_offload_work_handler(struct work_struct *work)
 	}
 
 	clear_bit(NF_FLOW_HW_PENDING, &offload->flow->flags);
+	if (offload->cmd == FLOW_CLS_DESTROY) {
+		/* Publish after the worker's last flow access. */
+		smp_mb__before_atomic();
+		set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags);
+	}
+
 	kfree(offload);
 }
 
-- 
2.47.3

Re: [PATCH net 01/10] netfilter: flowtable: publish HW_DEAD after worker is done

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 17 Sep 2026 01:16:42 +0200 you wrote:
> From: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
> 
> flow_offload_work_del() sets NF_FLOW_HW_DEAD before the work handler
> clears NF_FLOW_HW_PENDING. Once a flow is both HW_DYING and HW_DEAD, a
> concurrent garbage collection pass can remove it and schedule it for RCU
> freeing.
> 
> [...]

Here is the summary with links:
  - [net,01/10] netfilter: flowtable: publish HW_DEAD after worker is done
    (no matching commit)
  - [net,02/10] netfilter: nfnetlink_queue: hold nfnl mutex in event notifier
    (no matching commit)
  - [net,03/10] netfilter: ip6t_rpfilter: reject routes without inet6_dev
    (no matching commit)
  - [net,04/10] netfilter: ip6t_rt: fix zero-address non-strict match out-of-bounds read
    (no matching commit)
  - [net,05/10] netfilter: nft_synproxy: use the family-aware checksum helper
    (no matching commit)
  - [net,06/10] ipvs: revalidate ihl before icmp_send
    (no matching commit)
  - [net,07/10] ipvs: filter some flags received in the backup server
    (no matching commit)
  - [net,08/10] netfilter: ctnetlink: fix suspicious RCU usage in expect_iter_name
    (no matching commit)
  - [net,09/10] net: remove WARN_ON_ONCE() from the dev_fill_forward_path() loop check
    https://git.kernel.org/netdev/net/c/150dba2c69e9
  - [net,10/10] netfilter: nf_tables: skip expired catchall elements on insert and delete
    (no matching commit)

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

Re: [PATCH net 01/10] netfilter: flowtable: publish HW_DEAD after worker is done

[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: flowtable: publish HW_DEAD after worker is done

The patch moves the NF_FLOW_HW_DEAD publication out of
flow_offload_work_del() into the common epilogue of
flow_offload_work_handler(), so that it happens after
clear_bit(NF_FLOW_HW_PENDING) rather than before it.  The stated goal is
to make setting HW_DEAD the last flow access performed by destroy work,
so garbage collection cannot free the flow while the worker is still
touching it.

> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> index 801a3dd9ceea3..6757fd89c1f1a 100644
> --- a/net/netfilter/nf_flow_table_offload.c
> +++ b/net/netfilter/nf_flow_table_offload.c
> @@ -995,7 +995,6 @@ static void flow_offload_work_del(struct flow_offload_work *offload)
>  	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_ORIGINAL);
>  	if (test_bit(NF_FLOW_HW_BIDIRECTIONAL, &offload->flow->flags))
>  		flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_REPLY);
> -	set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags);
>  }
>  
>  static void flow_offload_tuple_stats(struct flow_offload_work *offload,
> @@ -1059,6 +1058,12 @@ static void flow_offload_work_handler(struct work_struct *work)
>  	}
>  
>  	clear_bit(NF_FLOW_HW_PENDING, &offload->flow->flags);
> +	if (offload->cmd == FLOW_CLS_DESTROY) {
> +		/* Publish after the worker's last flow access. */
> +		smp_mb__before_atomic();
> +		set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags);
> +	}
> +

[Severity: High]
Does this reordering close the whole use-after-free, or only the
FLOW_CLS_DESTROY part of it?

The new invariant ("setting HW_DEAD is the last flow access") only holds
for destroy work, but nf_flow_offload_gc_step() in nf_flow_table_core.c
relies on the flow being idle for all commands.  It decides to unlink and
RCU-free based on the flag bits alone and never looks at
NF_FLOW_HW_PENDING:

net/netfilter/nf_flow_table_core.c:nf_flow_offload_gc_step() {
	if (teardown) {
		if (test_bit(NF_FLOW_HW, &flow->flags)) {
			if (!test_bit(NF_FLOW_HW_DYING, &flow->flags))
				nf_flow_offload_del(flow_table, flow);
			else if (test_bit(NF_FLOW_HW_DEAD, &flow->flags))
				flow_offload_del(flow_table, flow);
	...
	} else if (test_bit(NF_FLOW_CLOSING, &flow->flags) &&
		   test_bit(NF_FLOW_HW, &flow->flags) &&
		   !test_bit(NF_FLOW_HW_DYING, &flow->flags)) {
		nf_flow_offload_del(flow_table, flow);
	} else if (test_bit(NF_FLOW_HW, &flow->flags)) {
		nf_flow_offload_stats(flow_table, flow);
	}
}

Is the following sequence still possible with this patch applied?

A FIN or RST sets NF_FLOW_CLOSING without NF_FLOW_TEARDOWN in
nf_flow_table_ip.c:nf_flow_state_check():

	if ((tcph->fin || tcph->rst) &&
	    !test_bit(NF_FLOW_CLOSING, &flow->flags))
		set_bit(NF_FLOW_CLOSING, &flow->flags);

gc then takes the CLOSING branch above, nf_flow_offload_del() sets
HW_DYING plus HW_PENDING and queues FLOW_CLS_DESTROY, and the destroy
worker now publishes HW_DEAD while the flow is still linked and TEARDOWN
is still clear.

From then on the CLOSING branch is skipped (it requires !HW_DYING) and gc
falls through to nf_flow_offload_stats(), which only checks the timeout
delta before calling nf_flow_offload_work_alloc().  That helper gates
purely on the pending bit and stores a raw flow pointer:

net/netfilter/nf_flow_table_offload.c:nf_flow_offload_work_alloc() {
	if (test_and_set_bit(NF_FLOW_HW_PENDING, &flow->flags))
		return NULL;
	...
	offload->flow = flow;
}

so a FLOW_CLS_STATS item can be queued for a flow that already has
HW_DYING and HW_DEAD published.  When TEARDOWN is later set (expiry, a
dying ct, dst check failure, nf_flow_custom_gc, or a SYN on the CLOSING
flow), gc matches teardown + NF_FLOW_HW + HW_DYING + HW_DEAD and calls
flow_offload_del() -> flow_offload_free() -> call_rcu(), regardless of
the outstanding stats work.

Can the stats worker then write into freed memory?
flow_offload_work_stats() does:

	offload->flow->timeout = max_t(u64, offload->flow->timeout,
				       lastused + flow_offload_get_timeout(offload->flow));
	...
		nf_ct_acct_add(offload->flow->ct, ...);

and the epilogue in this hunk still unconditionally runs
clear_bit(NF_FLOW_HW_PENDING, &offload->flow->flags) for every command,
after flow_offload_free_rcu() has already done nf_ct_put(flow->ct) and
kfree(flow).

Is there a similar window for FLOW_CLS_REPLACE?  flow_offload_refresh()
filters only NF_FLOW_CLOSING, not TEARDOWN, HW_DYING or HW_DEAD:

net/netfilter/nf_flow_table_core.c:flow_offload_refresh() {
	if (likely(!nf_flowtable_hw_offload(flow_table)) ||
	    test_bit(NF_FLOW_CLOSING, &flow->flags))
		return;

	if (test_bit(NF_FLOW_HW, &flow->flags))
		nf_flow_offload_refresh(flow_table, flow);
}

so a datapath thread that looked the flow up before teardown can win the
freshly cleared HW_PENDING bit, including in the window opened between
the clear_bit() and the new set_bit() here, and queue a replace for a
flow gc is about to free.

gc runs on system_power_efficient_wq while the handlers run on the
unbound nf_ft_offload_{add,del,stats} queues, and the driver flow_block
callbacks can sleep, so there is no serialization between the two sides.

Would it make more sense for nf_flow_offload_gc_step() to refuse to free
while NF_FLOW_HW_PENDING is set, or for the handler to hold a reference
or RCU section covering the whole flow access, instead of reordering the
bits?  The commit message states that "the offload worker holds neither
an RCU read lock nor a reference to the flow", which remains true for
stats and replace work after this change.

Re: [PATCH net 01/10] netfilter: flowtable: publish HW_DEAD after worker is done

[Pablo Neira Ayuso]

On Thu, Sep 17, 2026 at 07:04:06PM -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: flowtable: publish HW_DEAD after worker is done
> 
> The patch moves the NF_FLOW_HW_DEAD publication out of
> flow_offload_work_del() into the common epilogue of
> flow_offload_work_handler(), so that it happens after
> clear_bit(NF_FLOW_HW_PENDING) rather than before it.  The stated goal is
> to make setting HW_DEAD the last flow access performed by destroy work,
> so garbage collection cannot free the flow while the worker is still
> touching it.
> 
> > diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> > index 801a3dd9ceea3..6757fd89c1f1a 100644
> > --- a/net/netfilter/nf_flow_table_offload.c
> > +++ b/net/netfilter/nf_flow_table_offload.c
> > @@ -995,7 +995,6 @@ static void flow_offload_work_del(struct flow_offload_work *offload)
> >  	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_ORIGINAL);
> >  	if (test_bit(NF_FLOW_HW_BIDIRECTIONAL, &offload->flow->flags))
> >  		flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_REPLY);
> > -	set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags);
> >  }
> >  
> >  static void flow_offload_tuple_stats(struct flow_offload_work *offload,
> > @@ -1059,6 +1058,12 @@ static void flow_offload_work_handler(struct work_struct *work)
> >  	}
> >  
> >  	clear_bit(NF_FLOW_HW_PENDING, &offload->flow->flags);
> > +	if (offload->cmd == FLOW_CLS_DESTROY) {
> > +		/* Publish after the worker's last flow access. */
> > +		smp_mb__before_atomic();
> > +		set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags);
> > +	}
> > +
> 
> [Severity: High]
> Does this reordering close the whole use-after-free, or only the
> FLOW_CLS_DESTROY part of it?

This describes a pre-existing issue. I posted patches for this I have
to pick up.

my summary of LLM judgement in this PR [was Re: [PATCH net 01/10] netfilter: flowtable: publish HW_DEAD after worker is done]

[Pablo Neira Ayuso]

Hi Jakub,

I will be posting a v2 of this PR in a few minutes.

LLM found one real issue in the last, it also refer to two
pre-existing issues that I can address.

One of the reported pre-existing issues in nfnetlink_queue, Florian
hesitates if it is theoretical.

Julian also wants to send a v2 for its IPVS patch to ... write a
better commit description to please the LLM, it seems.

In summary, one real issue in this PR I would say, which is my fault
because I misguided the patch submitter.

I'm sure these tools can help us get things better, I understand too
we have to try them all so we can evaluate. But I'm starting to wonder
if it is worth stalling the pipeline and retrigger a PR continuosly,
when we can follow up for those that we deem to be addressed.

Thanks.

Re: my summary of LLM judgement in this PR [was Re: [PATCH net 01/10] netfilter: flowtable: publish HW_DEAD after worker is done]

[Jakub Kicinski]

On Fri, 18 Sep 2026 10:56:15 +0200 Pablo Neira Ayuso wrote:
> I'm sure these tools can help us get things better, I understand too
> we have to try them all so we can evaluate. But I'm starting to wonder
> if it is worth stalling the pipeline and retrigger a PR continuosly,
> when we can follow up for those that we deem to be addressed.

Yes, always your call. FWIW I'm also trying to get an AI reviewer
for netdev subsystems, we'll see if I can get it done before rage
quitting

[PATCH net 02/10] netfilter: nfnetlink_queue: hold nfnl mutex in event notifier

[Pablo Neira Ayuso]

From: Florian Westphal <fw@strlen.de>

We must serialize the release notifier and the config netlink function.
A concurrent thread can issue close() which can call the release function
while unrelated socket processes UNBIND request for same portid:

Oops: general protection fault, [..]
RIP: 0010:__instance_destroy+0x60/0x210 [nfnetlink_queue]
Call Trace:
 nfqnl_recv_config+0x9b0/0xdc0 [nfnetlink_queue]
 nfnetlink_rcv_msg+0x7c2/0xeb0
 ? __pfx_nfnetlink_rcv_msg+0x10/0x10

After this, parallel UNBIND and URELEASE events are impossible.

This change isn't nice, but its the shortest fix given instances
are not refcounted and the nfnetlink config callback drops the
rcu read lock early due to need for sleeping allocations.

Fixes: 7af4cc3fa158 ("[NETFILTER]: Add "nfnetlink_queue" netfilter queue handler over nfnetlink")
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nfnetlink_queue.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index c727668b0c5b..a3bc00280051 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -1593,6 +1593,7 @@ nfqnl_rcv_nl_event(struct notifier_block *this,
 	if (event == NETLINK_URELEASE && n->protocol == NETLINK_NETFILTER) {
 		int i;
 
+		nfnl_lock(NFNL_SUBSYS_QUEUE);
 		/* destroy all instances for this portid */
 		spin_lock(&q->instances_lock);
 		for (i = 0; i < INSTANCE_BUCKETS; i++) {
@@ -1606,6 +1607,7 @@ nfqnl_rcv_nl_event(struct notifier_block *this,
 			}
 		}
 		spin_unlock(&q->instances_lock);
+		nfnl_unlock(NFNL_SUBSYS_QUEUE);
 	}
 	return NOTIFY_DONE;
 }
@@ -1925,9 +1927,9 @@ static int nfqnl_recv_config(struct sk_buff *skb, const struct nfnl_info *info,
 
 	/* Lookup queue under RCU. After peer_portid check (or for new queue
 	 * in BIND case), the queue is owned by the socket sending this message.
-	 * A socket cannot simultaneously send a message and close, so while
-	 * processing this CONFIG message, nfqnl_rcv_nl_event() (triggered by
-	 * socket close) cannot destroy this queue. Safe to use without RCU.
+	 * nfqnl_rcv_nl_event() will block on the nfnl subsys mutex that is
+	 * held by the caller, so the queue cannot be destroyed in parallel,
+	 * even after we drop the RCU read lock.
 	 */
 	rcu_read_lock();
 	queue = instance_lookup(q, queue_num);
-- 
2.47.3

Re: [PATCH net 02/10] netfilter: nfnetlink_queue: hold nfnl mutex in event notifier

[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: nfnetlink_queue: hold nfnl mutex in event notifier

This takes the NFNL_SUBSYS_QUEUE mutex around the instance destroy loop in
nfqnl_rcv_nl_event() so that the NETLINK_URELEASE notifier can no longer run
in parallel with nfqnl_recv_config().  The stale comment in nfqnl_recv_config()
is updated to describe the new serialization.

> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index c727668b0c5be..a3bc002800510 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -1593,6 +1593,7 @@ nfqnl_rcv_nl_event(struct notifier_block *this,
>  	if (event == NETLINK_URELEASE && n->protocol == NETLINK_NETFILTER) {
>  		int i;
>  
> +		nfnl_lock(NFNL_SUBSYS_QUEUE);
>  		/* destroy all instances for this portid */
>  		spin_lock(&q->instances_lock);
>  		for (i = 0; i < INSTANCE_BUCKETS; i++) {

[Severity: Medium]
This is a pre-existing issue, not something introduced here, but does waiting
on the nfnl queue mutex here make it easier to destroy an instance that belongs
to a different socket?

The destroy loop in nfqnl_rcv_nl_event() picks victims by portid only:

	hlist_for_each_entry_safe(inst, t2, head, hlist) {
		if (n->portid == inst->peer_portid)
			__instance_destroy(inst);
	}

and net/netlink/af_netlink.c:netlink_release() makes the portid reusable well
before the notifier runs:

netlink_release()
	netlink_remove(sk);		/* portid is free for reuse here */
	sock_orphan(sk);
	...
	blocking_notifier_call_chain(&netlink_chain,
				     NETLINK_URELEASE, &n);

With the new nfnl_lock(NFNL_SUBSYS_QUEUE), the notifier now blocks for the
whole duration of any NFQNL_MSG_CONFIG handling (including instance_create()
and its sleeping rhashtable_init()), in any netns, since
net/netfilter/nfnetlink.c:nfnl_lock() takes the single global
table[NFNL_SUBSYS_QUEUE].mutex.  So this sequence looks possible:

thread 1: close() of a NETLINK_NETFILTER socket with portid X
	  netlink_remove() releases X, notifier is pending

thread 2: binds portid X, sends NFQNL_CFG_CMD_BIND
	  nfnetlink_rcv_msg() -> nfqnl_recv_config() under the queue mutex
	  instance_create() links a fresh instance with peer_portid == X

thread 1: notifier finally acquires the mutex, matches
	  inst->peer_portid == X and calls __instance_destroy() on the
	  brand new instance

Can the ownership check that the new comment relies on detect this?

	queue = instance_lookup(q, queue_num);
	if (queue && queue->peer_portid != NETLINK_CB(skb).portid) {

peer_portid matches in the recycled-portid case, so the check passes and the
instance is still torn down by the older socket's notification.

There is no memory safety problem, the instance is unlinked once and freed via
queue_rcu_work().  The visible effect is that a live socket's queue silently
disappears, its queued packets are flushed, packets hitting the NFQUEUE rule
then fail with -ESRCH (dropped unless fail-open or bypass is set), and later
CONFIG messages for that queue number return -ENODEV.  It requires
CAP_NET_ADMIN in the netns and a concurrent close/bind, and no later commit in
this series touches net/netfilter/nfnetlink_queue.c.

>  			}
>  		}
>  		spin_unlock(&q->instances_lock);
> +		nfnl_unlock(NFNL_SUBSYS_QUEUE);
>  	}
>  	return NOTIFY_DONE;
>  }
> @@ -1925,9 +1927,9 @@ static int nfqnl_recv_config(struct sk_buff *skb, const struct nfnl_info *info,
>  
>  	/* Lookup queue under RCU. After peer_portid check (or for new queue
>  	 * in BIND case), the queue is owned by the socket sending this message.
> -	 * A socket cannot simultaneously send a message and close, so while
> -	 * processing this CONFIG message, nfqnl_rcv_nl_event() (triggered by
> -	 * socket close) cannot destroy this queue. Safe to use without RCU.
> +	 * nfqnl_rcv_nl_event() will block on the nfnl subsys mutex that is
> +	 * held by the caller, so the queue cannot be destroyed in parallel,
> +	 * even after we drop the RCU read lock.
>  	 */

[ ... ]

Re: [PATCH net 02/10] netfilter: nfnetlink_queue: hold nfnl mutex in event notifier

[Pablo Neira Ayuso]

On Thu, Sep 17, 2026 at 07:04:08PM -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: nfnetlink_queue: hold nfnl mutex in event notifier
> 
> This takes the NFNL_SUBSYS_QUEUE mutex around the instance destroy loop in
> nfqnl_rcv_nl_event() so that the NETLINK_URELEASE notifier can no longer run
> in parallel with nfqnl_recv_config().  The stale comment in nfqnl_recv_config()
> is updated to describe the new serialization.
> 
> > diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> > index c727668b0c5be..a3bc002800510 100644
> > --- a/net/netfilter/nfnetlink_queue.c
> > +++ b/net/netfilter/nfnetlink_queue.c
> > @@ -1593,6 +1593,7 @@ nfqnl_rcv_nl_event(struct notifier_block *this,
> >  	if (event == NETLINK_URELEASE && n->protocol == NETLINK_NETFILTER) {
> >  		int i;
> >  
> > +		nfnl_lock(NFNL_SUBSYS_QUEUE);
> >  		/* destroy all instances for this portid */
> >  		spin_lock(&q->instances_lock);
> >  		for (i = 0; i < INSTANCE_BUCKETS; i++) {
> 
> [Severity: Medium]
> This is a pre-existing issue, not something introduced here, but does waiting
> on the nfnl queue mutex here make it easier to destroy an instance that belongs
> to a different socket?

This patch is fine.

For the theoretical described issue, I posted a patch.

But this is a pre-existing issue. Patch is fine.

Re: [PATCH net 02/10] netfilter: nfnetlink_queue: hold nfnl mutex in event notifier

[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: nfnetlink_queue: hold nfnl mutex in event notifier
> 
> This takes the NFNL_SUBSYS_QUEUE mutex around the instance destroy loop in
> nfqnl_rcv_nl_event() so that the NETLINK_URELEASE notifier can no longer run
> in parallel with nfqnl_recv_config().  The stale comment in nfqnl_recv_config()
> is updated to describe the new serialization.

This fixes a unpriv netns root exploit.  At least I think so.  Its
simple enough to look at and should be small enough to backport.

The LLM report makes me wonder:

1. Does this patch not fix it?  If so, how does the reproducer need
to be changed to make it trigger KASAN/UaF again?  Doesn't say.

2. Does this add a regression?  Doesn't say.  Tests I have pass.

> > diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> > index c727668b0c5be..a3bc002800510 100644
> > --- a/net/netfilter/nfnetlink_queue.c
> > +++ b/net/netfilter/nfnetlink_queue.c
> > @@ -1593,6 +1593,7 @@ nfqnl_rcv_nl_event(struct notifier_block *this,
> >  	if (event == NETLINK_URELEASE && n->protocol == NETLINK_NETFILTER) {
> >  		int i;
> >  
> > +		nfnl_lock(NFNL_SUBSYS_QUEUE);
> >  		/* destroy all instances for this portid */
> >  		spin_lock(&q->instances_lock);
> >  		for (i = 0; i < INSTANCE_BUCKETS; i++) {
> 
> [Severity: Medium]
> This is a pre-existing issue, not something introduced here, but does waiting
> on the nfnl queue mutex here make it easier to destroy an instance that belongs
> to a different socket?

So what?

This is a 20 year old interface. NOONE, and I really mean, NO ONE, has
raised a bug report in all these years.

*WHAT* does a *SENSIBLE* userspace program need to do so that it ends up
in a bad state?

From where I'm standing, there is a crash bug that is fixed by
this patch without observable new regression.

Aside from not crashing, kernel should also not have to perform absurd
stunts to somehow make sense of whatever crap userspace is trying to do.

> The destroy loop in nfqnl_rcv_nl_event() picks victims by portid only:
> 
> 	hlist_for_each_entry_safe(inst, t2, head, hlist) {
> 		if (n->portid == inst->peer_portid)
> 			__instance_destroy(inst);
> 	}
> 
> and net/netlink/af_netlink.c:netlink_release() makes the portid reusable well
> before the notifier runs:
> 
> netlink_release()
> 	netlink_remove(sk);		/* portid is free for reuse here */
> 	sock_orphan(sk);
> 	...
> 	blocking_notifier_call_chain(&netlink_chain,
> 				     NETLINK_URELEASE, &n);
> 
> With the new nfnl_lock(NFNL_SUBSYS_QUEUE), the notifier now blocks for the
> whole duration of any NFQNL_MSG_CONFIG handling (including instance_create()
> and its sleeping rhashtable_init()), in any netns, since
> net/netfilter/nfnetlink.c:nfnl_lock() takes the single global
> table[NFNL_SUBSYS_QUEUE].mutex.  So this sequence looks possible:
> 
> thread 1: close() of a NETLINK_NETFILTER socket with portid X
> 	  netlink_remove() releases X, notifier is pending

This is normally done by some applications when they shut down as
part of SIGTERM for example. Others just exit().

> thread 2: binds portid X, sends NFQNL_CFG_CMD_BIND
> 	  nfnetlink_rcv_msg() -> nfqnl_recv_config() under the queue mutex
> 	  instance_create() links a fresh instance with peer_portid == X

Why would one try do that...?  Just... don't do that?
And I don't follow how this can work.  LLM clains thread 1 is now
blocked in nfnl_lock(NFNL_SUBSYS_QUEUE), which thread 2 holds.

Therefore, as thread 1 couldn't yet run the notifier, the old queue is
still registered... no?

> thread 1: notifier finally acquires the mutex, matches
> 	  inst->peer_portid == X and calls __instance_destroy() on the
> 	  brand new instance

> Can the ownership check that the new comment relies on detect this?
> 
> 	queue = instance_lookup(q, queue_num);
> 	if (queue && queue->peer_portid != NETLINK_CB(skb).portid) {
> 
> peer_portid matches in the recycled-portid case, so the check passes and the
> instance is still torn down by the older socket's notification.

Not following :-/

Even if that test passes, why does it not hit the 'return -EBUSY' in
NFQNL_CFG_CMD_BIND?

If it did not, then instance_lookup() did not return a result, i.e.
queue was already done?  Can't make sense of anything anymore.

> There is no memory safety problem, the instance is unlinked once and freed via
> queue_rcu_work().  The visible effect is that a live socket's queue silently
> disappears, its queued packets are flushed, packets hitting the NFQUEUE rule
> then fail with -ESRCH (dropped unless fail-open or bypass is set), and later
> CONFIG messages for that queue number return -ENODEV.  It requires
> CAP_NET_ADMIN in the netns and a concurrent close/bind, and no later commit in
> this series touches net/netfilter/nfnetlink_queue.c.

I have no idea how to sensibly change this patch to address this 'bug'.

And thats my main source of frustration with all the LLM walls we have
now in place.

A human telling me that I fucked up usually has the courtesy of providing
an alternative solution or a hint to get things move in the right
direction again.

Or even 'I don't like it because of problem XYZ but I don't have a better
idea either'.

LLM just says 'something is wrong, you figure out the details of fixing
this'.  I would like to make better patches, but I don't know how.

[PATCH net 03/10] netfilter: ip6t_rpfilter: reject routes without inet6_dev

[Pablo Neira Ayuso]

From: Weiming Shi <bestswngs@gmail.com>

ip6_route_lookup() can return an error-free route whose rt6i_idev is
NULL. Lowering an external nexthop device's MTU below IPV6_MIN_MTU tears
down its inet6_dev while fib6_ifdown() leaves routes using nexthop objects
in the FIB. An unprivileged user can construct this state with rtnetlink
in a private user and network namespace, then trigger a NULL dereference
through an IPv6 rpfilter lookup:

  Oops: general protection fault, probably for non-canonical address
  0xdffffc0000000000
  KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
  RIP: rpfilter_mt (net/ipv6/netfilter/ip6t_rpfilter.c:75)
  Call Trace:
  ip6t_do_table (net/ipv6/netfilter/ip6_tables.c:316)
  nf_hook_slow (net/netfilter/core.c:619)
  ipv6_rcv (net/ipv6/ip6_input.c:351)
  __netif_receive_skb_one_core (net/core/dev.c:6216)
  process_backlog (net/core/dev.c:6680)
  __napi_poll (net/core/dev.c:7739)
  net_rx_action (net/core/dev.c:7959)
  handle_softirqs (kernel/softirq.c:622)
  do_softirq.part.0 (kernel/softirq.c:523)
  __local_bh_enable_ip (kernel/softirq.c:450)
  __dev_queue_xmit (net/core/dev.c:4913)
  packet_sendmsg (net/packet/af_packet.c:3139)
  __sys_sendto (net/socket.c:2252)
  __x64_sys_sendto (net/socket.c:2259)
  do_syscall_64 (arch/x86/entry/syscall_64.c:94)
  entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:121)
  Kernel panic - not syncing: Fatal exception in interrupt

Reject routes without an inet6_dev immediately after lookup. Such routes
are not eligible for reverse-path filtering, and the check protects all
later rt6i_idev dereferences.

Fixes: e26f9a480fb6 ("netfilter: add ipv6 reverse path filter match")
Reported-by: co+459f67f4d8af8ce6@bugs.sh
Closes: https://lore.kernel.org/all/VtWUkE8QzJt5CroTj2V2v3ZQ0gwbXZ7nq7I3@bugs.sh/
Suggested-by: Florian Westphal <fw@strlen.de>
Assisted-by: Claude:gpt-5
Cc: stable@vger.kernel.org
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/ipv6/netfilter/ip6t_rpfilter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/netfilter/ip6t_rpfilter.c b/net/ipv6/netfilter/ip6t_rpfilter.c
index 67c87a88cde4..b5def30c3127 100644
--- a/net/ipv6/netfilter/ip6t_rpfilter.c
+++ b/net/ipv6/netfilter/ip6t_rpfilter.c
@@ -61,7 +61,7 @@ static bool rpfilter_lookup_reverse6(struct net *net, const struct sk_buff *skb,
 		fl6.flowi6_oif = dev->ifindex;
 
 	rt = (void *)ip6_route_lookup(net, &fl6, skb, lookup_flags);
-	if (rt->dst.error)
+	if (rt->dst.error || !rt->rt6i_idev)
 		goto out;
 
 	if (rt->rt6i_flags & (RTF_REJECT|RTF_ANYCAST))
-- 
2.47.3

Re: [PATCH net 03/10] netfilter: ip6t_rpfilter: reject routes without inet6_dev

[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: ip6t_rpfilter: reject routes without inet6_dev

This adds a NULL check on rt6i_idev right after the ip6_route_lookup()
call in rpfilter_lookup_reverse6(), so routes that survived in the FIB
after their nexthop device lost its inet6_dev are skipped. It protects
the later rt6i_idev->dev dereferences in the same function.

Closes: https://lore.kernel.org/all/VtWUkE8QzJt5CroTj2V2v3ZQ0gwbXZ7nq7I3@bugs.sh/

> diff --git a/net/ipv6/netfilter/ip6t_rpfilter.c b/net/ipv6/netfilter/ip6t_rpfilter.c
> index 67c87a88cde4f..b5def30c3127e 100644
> --- a/net/ipv6/netfilter/ip6t_rpfilter.c
> +++ b/net/ipv6/netfilter/ip6t_rpfilter.c
> @@ -61,7 +61,7 @@ static bool rpfilter_lookup_reverse6(struct net *net, const struct sk_buff *skb,
>  		fl6.flowi6_oif = dev->ifindex;
>  
>  	rt = (void *)ip6_route_lookup(net, &fl6, skb, lookup_flags);
> -	if (rt->dst.error)
> +	if (rt->dst.error || !rt->rt6i_idev)
>  		goto out;

[Severity: High]
This isn't a bug introduced by this patch, but if rt6i_idev can be NULL on
an error-free route, does the same dereference remain unguarded in
validate_ipv6_net_dev() in drivers/infiniband/core/cma.c?

drivers/infiniband/core/cma.c:validate_ipv6_net_dev() {
	struct rt6_info *rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,
					 &src_addr->sin6_addr, net_dev->ifindex,
					 NULL, strict);
	bool ret;

	if (!rt)
		return false;

	ret = rt->rt6i_idev->dev == net_dev;
	...
}

The !rt test there looks equivalent to the rt->dst.error test this patch
just found insufficient, since rt6_lookup() only returns NULL for the
error case:

net/ipv6/route.c:rt6_lookup() {
	dst = fib6_rule_lookup(net, &fl6, skb, flags, ip6_pol_route_lookup);
	if (dst->error == 0)
		return dst_rt6_info(dst);

	dst_release(dst);

	return NULL;
}

So the route class described in this commit message, an error-free cache
route whose rt6i_idev was left NULL by ip6_rt_copy_init():

net/ipv6/route.c:ip6_rt_copy_init() {
	...
	rt->rt6i_idev = dev ? in6_dev_get(dev) : NULL;
	...
}

would pass the !rt check and then be dereferenced when validating an
incoming RoCE CM request via validate_net_dev() -> validate_ipv6_net_dev().

The other in-tree readers of the field already treat NULL as legal:

net/ipv6/fib6_rules.c:fib6_rule_suppress() {
	...
	if (rt->rt6i_idev)
		dev = rt->rt6i_idev->dev;
	...
}

net/ipv6/icmp.c:icmp6_send() {
	...
		if (rt6 && rt6->rt6i_idev)
			dev = rt6->rt6i_idev->dev;
	...
}

Would a follow-up patch adding the same guard to cma.c make sense? Note
that reaching the cma.c site is not unprivileged end-to-end the way the
rpfilter path is: it needs the NULL-idev route to exist in the namespace
of the RoCE netdev, which normally means CAP_NET_ADMIN in init_net plus an
RDMA-capable device. Nothing later in this series touches
drivers/infiniband/core/cma.c or net/ipv6/route.c, so the call site is
still unguarded at the end of the series.

Re: [PATCH net 03/10] netfilter: ip6t_rpfilter: reject routes without inet6_dev

[Pablo Neira Ayuso]

On Thu, Sep 17, 2026 at 07:04:10PM -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: ip6t_rpfilter: reject routes without inet6_dev
> 
> This adds a NULL check on rt6i_idev right after the ip6_route_lookup()
> call in rpfilter_lookup_reverse6(), so routes that survived in the FIB
> after their nexthop device lost its inet6_dev are skipped. It protects
> the later rt6i_idev->dev dereferences in the same function.
> 
> Closes: https://lore.kernel.org/all/VtWUkE8QzJt5CroTj2V2v3ZQ0gwbXZ7nq7I3@bugs.sh/
> 
> > diff --git a/net/ipv6/netfilter/ip6t_rpfilter.c b/net/ipv6/netfilter/ip6t_rpfilter.c
> > index 67c87a88cde4f..b5def30c3127e 100644
> > --- a/net/ipv6/netfilter/ip6t_rpfilter.c
> > +++ b/net/ipv6/netfilter/ip6t_rpfilter.c
> > @@ -61,7 +61,7 @@ static bool rpfilter_lookup_reverse6(struct net *net, const struct sk_buff *skb,
> >  		fl6.flowi6_oif = dev->ifindex;
> >  
> >  	rt = (void *)ip6_route_lookup(net, &fl6, skb, lookup_flags);
> > -	if (rt->dst.error)
> > +	if (rt->dst.error || !rt->rt6i_idev)
> >  		goto out;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but if rt6i_idev can be NULL on
> an error-free route, does the same dereference remain unguarded in
> validate_ipv6_net_dev() in drivers/infiniband/core/cma.c?
> 
> drivers/infiniband/core/cma.c:validate_ipv6_net_dev() {

Not Netfilter related.

[PATCH net 04/10] netfilter: ip6t_rt: fix zero-address non-strict match out-of-bounds read

[Pablo Neira Ayuso]

From: Luxiao Xu <rakukuip@gmail.com>

rt_mt6_check() permits rules to be configured with rtinfo->addrnr == 0
even when address matching (IP6T_RT_FST_MASK) is requested.

In the IP6T_RT_FST_NSTRICT path, rt_mt6() evaluates packet routing
addresses against rtinfo->addrs[i] and terminates backwards at the bottom
of the loop:

    if (ipv6_addr_equal(ap, &rtinfo->addrs[i])) {
        i++;
    }
    if (i == rtinfo->addrnr)
        break;

When addrnr is 0, if the first packet address matches rtinfo->addrs[0],
i is incremented to 1. Because i is now strictly greater than addrnr (0),
the loop termination condition (i == rtinfo->addrnr) is bypassed and will
never be satisfied.

If a crafted IPv6 packet contains matching routing addresses, i will
advance past IP6T_RT_HOPS (16). The subsequent call to ipv6_addr_equal()
reads beyond struct ip6t_rt, triggering UBSAN/KASAN out-of-bounds warnings
or kernel panics.

Fix this by:
1. Rejecting rules in rt_mt6_check() where IP6T_RT_FST_MASK is set but
   rtinfo->addrnr is zero.
2. In rt_mt6(), moving the termination condition (i < rtinfo->addrnr)
   into the for-loop header condition and removing the backwards break
   at the end of the loop body.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Suggested-by: Florian Westphal <fw@strlen.de>
Assisted-by: LLM
Signed-off-by: Luxiao Xu <rakukuip@gmail.com>
Signed-off-by: Ren Wei <weir@nebusec.ai>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/ipv6/netfilter/ip6t_rt.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/netfilter/ip6t_rt.c b/net/ipv6/netfilter/ip6t_rt.c
index 8051425213dd..9880faf3cc7d 100644
--- a/net/ipv6/netfilter/ip6t_rt.c
+++ b/net/ipv6/netfilter/ip6t_rt.c
@@ -96,7 +96,8 @@ static bool rt_mt6(const struct sk_buff *skb, struct xt_action_param *par)
 			unsigned int i = 0;
 
 			for (temp = 0;
-			     temp < (unsigned int)((hdrlen - 8) / 16);
+			     temp < (unsigned int)((hdrlen - 8) / 16) &&
+			     i < rtinfo->addrnr;
 			     temp++) {
 				ap = skb_header_pointer(skb,
 							ptr
@@ -112,8 +113,6 @@ static bool rt_mt6(const struct sk_buff *skb, struct xt_action_param *par)
 
 				if (ipv6_addr_equal(ap, &rtinfo->addrs[i]))
 					i++;
-				if (i == rtinfo->addrnr)
-					break;
 			}
 			if (i == rtinfo->addrnr)
 				return ret;
@@ -162,6 +161,12 @@ static int rt_mt6_check(const struct xt_mtchk_param *par)
 		pr_info_ratelimited("too many addresses specified\n");
 		return -EINVAL;
 	}
+
+	if ((rtinfo->flags & IP6T_RT_FST_MASK) && !rtinfo->addrnr) {
+		pr_info_ratelimited("address list match requested but addrnr is 0\n");
+		return -EINVAL;
+	}
+
 	if ((rtinfo->flags & (IP6T_RT_RES | IP6T_RT_FST_MASK)) &&
 	    (!(rtinfo->flags & IP6T_RT_TYP) ||
 	     (rtinfo->rt_type != 0) ||
-- 
2.47.3

[PATCH net 05/10] netfilter: nft_synproxy: use the family-aware checksum helper

[Pablo Neira Ayuso]

From: Karl Mehltretter <kmehltretter@gmail.com>

nft_synproxy_do_eval() verifies the TCP checksum before it switches on
skb->protocol.  It uses nf_ip_checksum(), which constructs an IPv4
pseudo header and relies on the IPv4 header checksum when folding the
whole skb.  Neither operation is valid for an IPv6 packet.

A correctly checksummed IPv6 segment can therefore fail verification
when it reaches the hook as CHECKSUM_NONE or, at NF_INET_LOCAL_IN,
CHECKSUM_COMPLETE.  nft_synproxy_do_eval() returns NF_DROP before
nft_synproxy_eval_v6() can send a SYN-ACK.

nft_synproxy_validate() deliberately admits NFPROTO_IPV6 and
NFPROTO_INET, and the xtables counterpart ip6t_SYNPROXY.c already calls
nf_ip6_checksum().

Use nf_checksum() with nft_pf() so the checksum helper dispatches to the
packet family's implementation.

Fixes: ad49d86e07a4 ("netfilter: nf_tables: Add synproxy support")
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_synproxy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nft_synproxy.c b/net/netfilter/nft_synproxy.c
index 9ed288c9d168..554a96a000f4 100644
--- a/net/netfilter/nft_synproxy.c
+++ b/net/netfilter/nft_synproxy.c
@@ -118,7 +118,8 @@ static void nft_synproxy_do_eval(const struct nft_synproxy *priv,
 		return;
 	}
 
-	if (nf_ip_checksum(skb, nft_hook(pkt), thoff, IPPROTO_TCP)) {
+	if (nf_checksum(skb, nft_hook(pkt), thoff, IPPROTO_TCP,
+			nft_pf(pkt))) {
 		regs->verdict.code = NF_DROP;
 		return;
 	}
-- 
2.47.3

[PATCH net 06/10] ipvs: revalidate ihl before icmp_send

[Pablo Neira Ayuso]

From: Julian Anastasov <ja@ssi.bg>

While the outer IP header is already pulled into the skb head, we must
be careful and revalidate the embedded headers after reading them from
the skb frags to prevent possible out-of-bounds access.

One such place reported by Sashiko is ip_vs_in_icmp() where local
process can change the ihl field and after pskb_may_pull() we can see
larger value. Even if icmp_send() has checks to prevent out-of-bounds
access, play safe and add check to drop the packet if the ihl field is
changed.  As the outer headers are pulled, make sure the transport
header is updated too, it was used before commit 7fcc2fe39fed ("net:
icmp: avoid invalid transport header access in icmp_send tracepoint")

Fixes: f2edb9f7706d ("ipvs: implement passive PMTUD for IPIP packets")
Link: https://sashiko.dev/#/patchset/20260806105211.34622-1-ja%40ssi.bg
Signed-off-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/ipvs/ip_vs_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index ba0957798bad..fd503f0efb57 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -1960,6 +1960,12 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
 		/* Ensure the IP header is present in headroom */
 		if (!pskb_may_pull(skb, hlen_orig))
 			goto ignore_tunnel;
+		skb_set_transport_header(skb, hlen_orig);
+		/* Before now we may used ihl from skb frag, revalidate it after
+		 * copying it into skb head to prevent out-of-bounds access
+		 */
+		if (ip_hdr(skb)->ihl * 4 != hlen_orig)
+			goto ignore_tunnel;
 		IP_VS_DBG(12, "Sending ICMP for %pI4->%pI4: t=%u, c=%u, i=%u\n",
 			&ip_hdr(skb)->saddr, &ip_hdr(skb)->daddr,
 			type, code, ntohl(info));
-- 
2.47.3

[PATCH net 07/10] ipvs: filter some flags received in the backup server

[Pablo Neira Ayuso]

From: Julian Anastasov <ja@ssi.bg>

While the IPVS SYNC protocol is not secure by design
we can still protect the backup server from messages that
can wreak havoc.

This commit addresses problems from received connection flags
or their combinations. We now drop messages as follows:

1. the NO_CPORT+TEMPLATE combination allows lookups for normal
connections to hit template which can break in many ways

2. ONE_PACKET: it is not sent by master, so we do not
expect it in backup. Before now it was ignored by
IP_VS_CONN_F_BACKUP_MASK for protocol v1 while protocol
v0 created connections that are not hashed and dropped
immediately. Better to apply the IP_VS_CONN_F_BACKUP_MASK
also to the flags from v0 messages for consistency with v1.

Fixes: 87375ab47cd0 ("[IPVS]: ip_vs_ftp breaks connections using persistence")
Signed-off-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/ipvs/ip_vs_sync.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 5383aeafb0ae..69dc28153ec1 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -949,6 +949,21 @@ static void ip_vs_proc_conn(struct netns_ipvs *ipvs, struct ip_vs_conn_param *pa
 	ip_vs_conn_put(cp);
 }
 
+/* Check for incompatible flags */
+static bool ip_vs_sync_validate_flags(u32 flags)
+{
+	/* We do not expect NO_CPORT, especially to allow lookups
+	 * to hit templates
+	 */
+	if (flags & IP_VS_CONN_F_NO_CPORT) {
+		if (flags & IP_VS_CONN_F_TEMPLATE)
+			return false;
+	}
+	if (flags & IP_VS_CONN_F_ONE_PACKET)
+		return false;
+	return true;
+}
+
 /*
  *  Process received multicast message for Version 0
  */
@@ -972,8 +987,7 @@ static void ip_vs_process_message_v0(struct netns_ipvs *ipvs, const char *buffer
 			return;
 		}
 		s = (struct ip_vs_sync_conn_v0 *) p;
-		flags = ntohs(s->flags) | IP_VS_CONN_F_SYNC;
-		flags &= ~IP_VS_CONN_F_HASHED;
+		flags = ntohs(s->flags);
 		if (flags & IP_VS_CONN_F_SEQ_MASK) {
 			opt = (struct ip_vs_sync_conn_options *)&s[1];
 			p += FULL_CONN_SIZE;
@@ -986,6 +1000,13 @@ static void ip_vs_process_message_v0(struct netns_ipvs *ipvs, const char *buffer
 			p += SIMPLE_CONN_SIZE;
 		}
 
+		if (!ip_vs_sync_validate_flags(flags)) {
+			IP_VS_DBG(2, "BACKUP v0, Invalid flags 0x%X\n", flags);
+			continue;
+		}
+		flags &= IP_VS_CONN_F_BACKUP_MASK;
+		flags |= IP_VS_CONN_F_SYNC;
+
 		state = ntohs(s->state);
 		if (!(flags & IP_VS_CONN_F_TEMPLATE)) {
 			pp = ip_vs_proto_get(s->protocol);
@@ -1141,7 +1162,13 @@ static inline int ip_vs_proc_sync_conn(struct netns_ipvs *ipvs, __u8 *p, __u8 *m
 	}
 
 	/* Get flags and Mask off unsupported */
-	flags  = ntohl(s->v4.flags) & IP_VS_CONN_F_BACKUP_MASK;
+	flags = ntohl(s->v4.flags);
+	if (!ip_vs_sync_validate_flags(flags)) {
+		IP_VS_DBG(3, "BACKUP, Invalid flags 0x%X\n", flags);
+		retc = 25;
+		goto out;
+	}
+	flags &= IP_VS_CONN_F_BACKUP_MASK;
 	flags |= IP_VS_CONN_F_SYNC;
 	state = ntohs(s->v4.state);
 
-- 
2.47.3

Re: [PATCH net 07/10] ipvs: filter some flags received in the backup server

[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: filter some flags received in the backup server

This adds ip_vs_sync_validate_flags() to the backup receive path so that
sync records carrying the NO_CPORT+TEMPLATE combination or ONE_PACKET are
dropped, and it also applies IP_VS_CONN_F_BACKUP_MASK to the flags taken
from version 0 messages for consistency with version 1.

A few questions about the new validation below.

> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index 5383aeafb0aed..69dc28153ec12 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -949,6 +949,21 @@ static void ip_vs_proc_conn(struct netns_ipvs *ipvs, struct ip_vs_conn_param *pa
>  	ip_vs_conn_put(cp);
>  }
>  
> +/* Check for incompatible flags */
> +static bool ip_vs_sync_validate_flags(u32 flags)
> +{
> +	/* We do not expect NO_CPORT, especially to allow lookups
> +	 * to hit templates
> +	 */

[Severity: Low]
This isn't a bug, but does the comment describe what the code does?  The
text reads as an unconditional statement that NO_CPORT is not expected,
while the code below only rejects it when TEMPLATE is set, and both call
sites then keep the bit:

	flags &= IP_VS_CONN_F_BACKUP_MASK;

IP_VS_CONN_F_BACKUP_MASK in include/uapi/linux/ip_vs.h is documented as
"Initial bits allowed in backup server" and it lists
IP_VS_CONN_F_NO_CPORT, so bare NO_CPORT is expected here.  It is also
needed: ip_vs_conn_new() accounts NO_CPORT entries in
ipvs->no_cport_conns[] and ip_vs_conn_in_get() relies on that counter for
the cport-zero retry used by the FTP/persistence connections referenced by
the Fixes: tag.

Could the comment be reworded to say that it is the NO_CPORT+TEMPLATE
combination that is not expected?

> +	if (flags & IP_VS_CONN_F_NO_CPORT) {
> +		if (flags & IP_VS_CONN_F_TEMPLATE)
> +			return false;
> +	}

[Severity: High]
Does this close the case described in the changelog as "the NO_CPORT+TEMPLATE
combination allows lookups for normal connections to hit template"?

A record with IP_VS_CONN_F_TEMPLATE set, NO_CPORT clear and a non-zero
cport passes this test, survives flags &= IP_VS_CONN_F_BACKUP_MASK (TEMPLATE
is part of that mask) and reaches ip_vs_proc_conn() -> ip_vs_conn_new() with
the cport taken from the wire (ip_vs_conn_fill_param_sync() copies
s->v4.cport, ip_vs_process_message_v0() copies s->cport).

ip_vs_conn_hash() hashes templates into the same dir-0 key space as normal
connections, and __ip_vs_conn_in_get() has no TEMPLATE test:

net/netfilter/ipvs/ip_vs_conn.c:__ip_vs_conn_in_get() {
	...
			if (p->cport == cp->cport &&
			    p->vport == cp->vport && cp->af == p->af &&
			    ip_vs_addr_equal(p->af, p->caddr, &cp->caddr) &&
			    ip_vs_addr_equal(p->af, p->vaddr, &cp->vaddr) &&
			    (!p->cport ^
			     (!(cp->flags & IP_VS_CONN_F_NO_CPORT))) &&
			    p->protocol == cp->protocol) {
	...
}

For a packet whose client port matches the injected cport, the XOR term is
(0 ^ 1), so the template is returned as if it were a normal connection.
Legitimate templates never hit this only because ip_vs_sched_persist()
always creates them with cport == 0 and without NO_CPORT, which is the
invariant this helper does not enforce.

Once that happens, ip_vs_in() has no template guard, so live traffic is
forwarded through the template's packet_xmit and daddr/dport, and
set_tcp_state() then adjusts dest->activeconns:

net/netfilter/ipvs/ip_vs_proto_tcp.c:set_tcp_state() {
	...
		if (dest) {
			if (!(cp->flags & IP_VS_CONN_F_INACTIVE) &&
			    !tcp_state_active(new_state)) {
				atomic_dec(&dest->activeconns);
	...
}

while ip_vs_bind_dest() counted that entry under persistconns and
ip_vs_unbind_dest() only decrements persistconns for templates, so
dest->activeconns stays skewed and affects lc/wlc scheduling for the real
service.  The timeout is taken from the message (bounded only by
MAX_SCHEDULE_TIMEOUT / HZ in ip_vs_proc_conn()), so the bogus template can
be long lived.

Would rejecting TEMPLATE records with a non-zero cport, or excluding
templates in __ip_vs_conn_in_get(), be needed for the stated goal to hold?

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but since
the helper is the new place where flag combinations are screened: a record
with NO_CPORT set and a non-zero cport is still accepted, because only the
flags word is inspected and never cross-checked against the record's cport.

ip_vs_conn_new() hashes such an entry under the non-zero cport and bumps
ipvs->no_cport_conns[af]:

net/netfilter/ipvs/ip_vs_conn.c:ip_vs_conn_new() {
	...
	if (unlikely(flags & IP_VS_CONN_F_NO_CPORT)) {
		int af_id = ip_vs_af_index(cp->af);

		atomic_inc(&ipvs->no_cport_conns[af_id]);
	}
	...
}

The entry can then never be matched: the direct lookup fails the
(!p->cport ^ !(cp->flags & IP_VS_CONN_F_NO_CPORT)) test, and the cport-zero
retry in ip_vs_conn_in_get() hashes a different bucket than the one the
entry sits in, so ip_vs_conn_fill_cport() never runs and the counter stays
elevated until the entry expires.  While one such entry exists, every
conn-table miss in that address family does a second full hash lookup.

Is it worth also requiring cport == 0 when NO_CPORT is set?

> +	if (flags & IP_VS_CONN_F_ONE_PACKET)
> +		return false;
> +	return true;
> +}

[Severity: Low]
The changelog says:

  "2. ONE_PACKET: it is not sent by master, so we do not expect it in
  backup."

Is that accurate for controller connections?  ip_vs_sync_conn() tests
ONE_PACKET only for the connection it is entered with, and the test sits
above the sloop label:

net/netfilter/ipvs/ip_vs_sync.c:ip_vs_sync_conn() {
	...
	/* Do not sync ONE PACKET */
	if (cp->flags & IP_VS_CONN_F_ONE_PACKET)
		goto control;
sloop:
	...
	s->v4.flags = htonl(cp->flags & ~IP_VS_CONN_F_HASHED);
	...
control:
	/* synchronize its controller if it has */
	cp = cp->control;
	if (!cp)
		return;
	...
	goto sloop;
}

The controller path re-enters below the ONE_PACKET test and the serializer
clears only IP_VS_CONN_F_HASHED, so a controller carrying ONE_PACKET is
sent on the wire with that bit.  A persistence template can carry it:
ip_vs_bind_dest() strips the dest ONE_PACKET only for non-UDP and for
SYNC-created entries, and IP_VS_CONN_F_DEST_MASK includes
IP_VS_CONN_F_ONE_PACKET, which the legacy setsockopt path copies verbatim.

For a UDP persistent service configured that way, the master emits a
TEMPLATE|ONE_PACKET record which the backup now drops with retc = 25
instead of masking ONE_PACKET off and creating or refreshing the template,
so that template is no longer replicated.  The practical effect looks small
since ip_vs_conn_hash() returns early for ONE_PACKET on the master too, and
only the single record is skipped, but could the changelog wording be
adjusted, or the controller be masked in ip_vs_sync_conn()?

> @@ -972,8 +987,7 @@ static void ip_vs_process_message_v0(struct netns_ipvs *ipvs, const char *buffer
>  			return;
>  		}
>  		s = (struct ip_vs_sync_conn_v0 *) p;
> -		flags = ntohs(s->flags) | IP_VS_CONN_F_SYNC;
> -		flags &= ~IP_VS_CONN_F_HASHED;
> +		flags = ntohs(s->flags);
>  		if (flags & IP_VS_CONN_F_SEQ_MASK) {
>  			opt = (struct ip_vs_sync_conn_options *)&s[1];
>  			p += FULL_CONN_SIZE;
> @@ -986,6 +1000,13 @@ static void ip_vs_process_message_v0(struct netns_ipvs *ipvs, const char *buffer
>  			p += SIMPLE_CONN_SIZE;
>  		}
>  
> +		if (!ip_vs_sync_validate_flags(flags)) {
> +			IP_VS_DBG(2, "BACKUP v0, Invalid flags 0x%X\n", flags);
> +			continue;
> +		}
> +		flags &= IP_VS_CONN_F_BACKUP_MASK;
> +		flags |= IP_VS_CONN_F_SYNC;
> +

[ ... ]

> @@ -1141,7 +1162,13 @@ static inline int ip_vs_proc_sync_conn(struct netns_ipvs *ipvs, __u8 *p, __u8 *m
>  	}
>  
>  	/* Get flags and Mask off unsupported */
> -	flags  = ntohl(s->v4.flags) & IP_VS_CONN_F_BACKUP_MASK;
> +	flags = ntohl(s->v4.flags);
> +	if (!ip_vs_sync_validate_flags(flags)) {
> +		IP_VS_DBG(3, "BACKUP, Invalid flags 0x%X\n", flags);
> +		retc = 25;
> +		goto out;
> +	}
> +	flags &= IP_VS_CONN_F_BACKUP_MASK;
>  	flags |= IP_VS_CONN_F_SYNC;
>  	state = ntohs(s->v4.state);

Re: [PATCH net 07/10] ipvs: filter some flags received in the backup server

[Pablo Neira Ayuso]

On Thu, Sep 17, 2026 at 07:04:11PM -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: filter some flags received in the backup server
> 
> This adds ip_vs_sync_validate_flags() to the backup receive path so that
> sync records carrying the NO_CPORT+TEMPLATE combination or ONE_PACKET are
> dropped, and it also applies IP_VS_CONN_F_BACKUP_MASK to the flags taken
> from version 0 messages for consistency with version 1.
> 
> A few questions about the new validation below.

Julian wants to post a v2.

Re: [PATCH net 07/10] ipvs: filter some flags received in the backup server

[Julian Anastasov]

	Hello,

On Thu, 17 Sep 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: filter some flags received in the backup server
> 
> This adds ip_vs_sync_validate_flags() to the backup receive path so that
> sync records carrying the NO_CPORT+TEMPLATE combination or ONE_PACKET are
> dropped, and it also applies IP_VS_CONN_F_BACKUP_MASK to the flags taken
> from version 0 messages for consistency with version 1.
> 
> A few questions about the new validation below.

	I'll explain them before sending v2...

> 
> > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> > index 5383aeafb0aed..69dc28153ec12 100644
> > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > @@ -949,6 +949,21 @@ static void ip_vs_proc_conn(struct netns_ipvs *ipvs, struct ip_vs_conn_param *pa
> >  	ip_vs_conn_put(cp);
> >  }
> >  
> > +/* Check for incompatible flags */
> > +static bool ip_vs_sync_validate_flags(u32 flags)
> > +{
> > +	/* We do not expect NO_CPORT, especially to allow lookups
> > +	 * to hit templates
> > +	 */
> 
> [Severity: Low]
> This isn't a bug, but does the comment describe what the code does?  The
> text reads as an unconditional statement that NO_CPORT is not expected,
> while the code below only rejects it when TEMPLATE is set, and both call
> sites then keep the bit:
> 
> 	flags &= IP_VS_CONN_F_BACKUP_MASK;
> 
> IP_VS_CONN_F_BACKUP_MASK in include/uapi/linux/ip_vs.h is documented as
> "Initial bits allowed in backup server" and it lists
> IP_VS_CONN_F_NO_CPORT, so bare NO_CPORT is expected here.  It is also
> needed: ip_vs_conn_new() accounts NO_CPORT entries in
> ipvs->no_cport_conns[] and ip_vs_conn_in_get() relies on that counter for
> the cport-zero retry used by the FTP/persistence connections referenced by
> the Fixes: tag.
> 
> Could the comment be reworded to say that it is the NO_CPORT+TEMPLATE
> combination that is not expected?

	This is one of the things I want to explain. It is
ip_vs_sync_conn_needed() in the master that does not send
sync messages for TCP conn before it is established, so we do
not expect to see NO_CPORT on the wire. But we prefer not to
disable receiving NO_CPORT with cport=0 for normal connections.
It is nothing special to wait for initial SYN from unknown cport.

> 
> > +	if (flags & IP_VS_CONN_F_NO_CPORT) {
> > +		if (flags & IP_VS_CONN_F_TEMPLATE)
> > +			return false;
> > +	}
> 
> [Severity: High]
> Does this close the case described in the changelog as "the NO_CPORT+TEMPLATE
> combination allows lookups for normal connections to hit template"?
> 
> A record with IP_VS_CONN_F_TEMPLATE set, NO_CPORT clear and a non-zero
> cport passes this test, survives flags &= IP_VS_CONN_F_BACKUP_MASK (TEMPLATE
> is part of that mask) and reaches ip_vs_proc_conn() -> ip_vs_conn_new() with
> the cport taken from the wire (ip_vs_conn_fill_param_sync() copies
> s->v4.cport, ip_vs_process_message_v0() copies s->cport).

	This will be solved in separate patchset from
Axel Mierczuk.

> ip_vs_conn_hash() hashes templates into the same dir-0 key space as normal
> connections, and __ip_vs_conn_in_get() has no TEMPLATE test:
> 
> net/netfilter/ipvs/ip_vs_conn.c:__ip_vs_conn_in_get() {
> 	...
> 			if (p->cport == cp->cport &&
> 			    p->vport == cp->vport && cp->af == p->af &&
> 			    ip_vs_addr_equal(p->af, p->caddr, &cp->caddr) &&
> 			    ip_vs_addr_equal(p->af, p->vaddr, &cp->vaddr) &&
> 			    (!p->cport ^
> 			     (!(cp->flags & IP_VS_CONN_F_NO_CPORT))) &&
> 			    p->protocol == cp->protocol) {
> 	...
> }
> 
> For a packet whose client port matches the injected cport, the XOR term is
> (0 ^ 1), so the template is returned as if it were a normal connection.
> Legitimate templates never hit this only because ip_vs_sched_persist()
> always creates them with cport == 0 and without NO_CPORT, which is the
> invariant this helper does not enforce.
> 
> Once that happens, ip_vs_in() has no template guard, so live traffic is
> forwarded through the template's packet_xmit and daddr/dport, and
> set_tcp_state() then adjusts dest->activeconns:
> 
> net/netfilter/ipvs/ip_vs_proto_tcp.c:set_tcp_state() {
> 	...
> 		if (dest) {
> 			if (!(cp->flags & IP_VS_CONN_F_INACTIVE) &&
> 			    !tcp_state_active(new_state)) {
> 				atomic_dec(&dest->activeconns);
> 	...
> }
> 
> while ip_vs_bind_dest() counted that entry under persistconns and
> ip_vs_unbind_dest() only decrements persistconns for templates, so
> dest->activeconns stays skewed and affects lc/wlc scheduling for the real
> service.  The timeout is taken from the message (bounded only by
> MAX_SCHEDULE_TIMEOUT / HZ in ip_vs_proc_conn()), so the bogus template can
> be long lived.
> 
> Would rejecting TEMPLATE records with a non-zero cport, or excluding
> templates in __ip_vs_conn_in_get(), be needed for the stated goal to hold?

	Yep, the above problems will be solved...

> [Severity: Low]
> This is a pre-existing issue and not introduced by this patch, but since
> the helper is the new place where flag combinations are screened: a record
> with NO_CPORT set and a non-zero cport is still accepted, because only the
> flags word is inspected and never cross-checked against the record's cport.
> 
> ip_vs_conn_new() hashes such an entry under the non-zero cport and bumps
> ipvs->no_cport_conns[af]:
> 
> net/netfilter/ipvs/ip_vs_conn.c:ip_vs_conn_new() {
> 	...
> 	if (unlikely(flags & IP_VS_CONN_F_NO_CPORT)) {
> 		int af_id = ip_vs_af_index(cp->af);
> 
> 		atomic_inc(&ipvs->no_cport_conns[af_id]);
> 	}
> 	...
> }
> 
> The entry can then never be matched: the direct lookup fails the
> (!p->cport ^ !(cp->flags & IP_VS_CONN_F_NO_CPORT)) test, and the cport-zero
> retry in ip_vs_conn_in_get() hashes a different bucket than the one the
> entry sits in, so ip_vs_conn_fill_cport() never runs and the counter stays
> elevated until the entry expires.  While one such entry exists, every
> conn-table miss in that address family does a second full hash lookup.
> 
> Is it worth also requiring cport == 0 when NO_CPORT is set?

	It is harmsless but better to check it.

> 
> > +	if (flags & IP_VS_CONN_F_ONE_PACKET)
> > +		return false;
> > +	return true;
> > +}
> 
> [Severity: Low]
> The changelog says:
> 
>   "2. ONE_PACKET: it is not sent by master, so we do not expect it in
>   backup."
> 
> Is that accurate for controller connections?  ip_vs_sync_conn() tests
> ONE_PACKET only for the connection it is entered with, and the test sits
> above the sloop label:
> 
> net/netfilter/ipvs/ip_vs_sync.c:ip_vs_sync_conn() {
> 	...
> 	/* Do not sync ONE PACKET */
> 	if (cp->flags & IP_VS_CONN_F_ONE_PACKET)
> 		goto control;
> sloop:
> 	...
> 	s->v4.flags = htonl(cp->flags & ~IP_VS_CONN_F_HASHED);
> 	...
> control:
> 	/* synchronize its controller if it has */
> 	cp = cp->control;
> 	if (!cp)
> 		return;
> 	...
> 	goto sloop;
> }
> 
> The controller path re-enters below the ONE_PACKET test and the serializer
> clears only IP_VS_CONN_F_HASHED, so a controller carrying ONE_PACKET is
> sent on the wire with that bit.  A persistence template can carry it:
> ip_vs_bind_dest() strips the dest ONE_PACKET only for non-UDP and for
> SYNC-created entries, and IP_VS_CONN_F_DEST_MASK includes
> IP_VS_CONN_F_ONE_PACKET, which the legacy setsockopt path copies verbatim.
> 
> For a UDP persistent service configured that way, the master emits a
> TEMPLATE|ONE_PACKET record which the backup now drops with retc = 25
> instead of masking ONE_PACKET off and creating or refreshing the template,
> so that template is no longer replicated.  The practical effect looks small
> since ip_vs_conn_hash() returns early for ONE_PACKET on the master too, and
> only the single record is skipped, but could the changelog wording be
> adjusted, or the controller be masked in ip_vs_sync_conn()?

	Templates with ONE_PACKET will not be hashed. But before that
we can attach such template as controlling connection to some
data connection. Such template will be invisible (not hashed)
and will expire when its data connection expires. But it is better
to disallow ONE_PACKET for templates. Will post a patch for this.

Regards

--
Julian Anastasov <ja@ssi.bg>

[PATCH net 08/10] netfilter: ctnetlink: fix suspicious RCU usage in expect_iter_name

[Pablo Neira Ayuso]

From: Naman Gulati <namangulati@google.com>

expect_iter_name() is invoked by nf_ct_expect_iterate_net() under
spin_lock_bh(&nf_conntrack_expect_lock). It does not hold
rcu_read_lock().

When accessing exp->helper with rcu_dereference() in syzbot's report,
lockdep warns:

  =============================
  WARNING: suspicious RCU usage
  syzkaller #0 Not tainted
  -----------------------------
  net/netfilter/nf_conntrack_netlink.c:3393 suspicious rcu_dereference_check() usage!

  locks held by syz-executor381/5628: 2, last CPU#1:
   #0: ffffffff9aee42a0 (nfnl_subsys_ctnetlink_exp){+.+.}-{4:4},
       at: nfnetlink_rcv_msg+0xa69/0x12b0
   #1: ffffffff8ea74d58 (nf_conntrack_expect_lock){+...}-{3:3},
       at: nf_ct_expect_iterate_net+0x38/0x180

  Call Trace:
   <TASK>
   dump_stack_lvl+0xe8/0x150
   lockdep_rcu_suspicious+0x140/0x1d0
   expect_iter_name+0xfb/0x100
   nf_ct_expect_iterate_net+0xf2/0x180
   ctnetlink_del_expect+0x45d/0x640
   nfnetlink_rcv_msg+0xcc2/0x12b0
   netlink_rcv_skb+0x226/0x4a0
   nfnetlink_rcv+0x2b9/0x28c0
   netlink_unicast+0x7bd/0x940
   netlink_sendmsg+0x813/0xb40
   ____sys_sendmsg+0x54e/0x850
   ___sys_sendmsg+0x2a5/0x360
   __sys_sendmsg+0x2a5/0x360
   do_syscall_64+0x166/0x520
   entry_SYSCALL_64_after_hwframe+0x77/0x7f

Use rcu_dereference_protected() with lockdep_is_held() on
nf_conntrack_expect_lock instead, similar to expect_iter_me() in
nf_conntrack_helper.c.

Fixes: f01794106042 ("netfilter: nf_conntrack_expect: use expect->helper")
Reported-by: syzbot+4bd730aede2791e40bdf@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/6aa4a377.f81106d8.2ab401.0024.GAE@google.com/T/#u
Signed-off-by: Naman Gulati <namangulati@google.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_netlink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 579ada063b1b..4e5d7c701436 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -3392,7 +3392,8 @@ static bool expect_iter_name(struct nf_conntrack_expect *exp, void *data)
 	struct nf_conntrack_helper *helper;
 	const char *name = data;
 
-	helper = rcu_dereference(exp->helper);
+	helper = rcu_dereference_protected(exp->helper,
+					   lockdep_is_held(&nf_conntrack_expect_lock));
 	if (!helper)
 		return false;
 
-- 
2.47.3

[PATCH net 09/10] net: remove WARN_ON_ONCE() from the dev_fill_forward_path() loop check

[Pablo Neira Ayuso]

From: Farhad Alemi <farhad.alemi@berkeley.edu>

ipip_fill_forward_path() and ip6_tnl_fill_forward_path() look up the
route to the tunnel's remote endpoint and set ctx->dev to its device,
which is the tunnel itself when that route resolves back to the tunnel.
dev_fill_forward_path() then makes no progress and trips
WARN_ON_ONCE(last_dev == ctx->dev) as soon as a flowtable tries to
offload a flow through the tunnel. That routing loop is a configuration
any CAP_NET_ADMIN user can set up, and ip_tunnel_xmit() and
ip6_tnl_xmit() already treat it as a tx error, so remove the warning and
just fail the walk, as commit 008e7a7c293b ("net: remove WARN_ON_ONCE
when accessing forward path array") did for the path stack overflow.

Fixes: ab427db17885 ("netfilter: flowtable: Add IPIP rx sw acceleration")
Fixes: d98103575dcd ("netfilter: flowtable: Add IP6IP6 rx sw acceleration")
Closes: https://lore.kernel.org/all/CA+0ovCgaRvbd0Udj70b2xxG8Cx3CaCpNhnf1V4RWQuDveZYZhA@mail.gmail.com/
Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Assisted-by: Claude:claude-opus-5 syzkaller
Signed-off-by: Farhad Alemi <farhad.alemi@berkeley.edu>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index ecfbd72d5d1a..c67900354fa6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -789,7 +789,7 @@ int dev_fill_forward_path(struct net_device_path_ctx *ctx,
 			goto err_out;
 
 		stack->num_paths++;
-		if (WARN_ON_ONCE(last_dev == ctx->dev))
+		if (last_dev == ctx->dev)
 			goto err_out;
 	}
 
-- 
2.47.3

[PATCH net 10/10] netfilter: nf_tables: skip expired catchall elements on insert and delete

[Pablo Neira Ayuso]

From: Aohan Mei <henrymei@tencent.com>

nft_setelem_catchall_insert() looks up duplicates with
nft_set_elem_active() only, while nft_set_catchall_lookup() and the
dump path additionally skip expired elements.

Once a catchall element with a timeout expires, this predicate drift
makes it invisible to userspace dumps, yet it still blocks
re-insertion: with NLM_F_EXCL the request fails with -EEXIST, and
without it the request reports success but silently inserts nothing.
The stale entry only goes away when the (user-tunable) gc interval
elapses, so the catchall rule may silently stop matching for an
arbitrarily long time after its first expiration.

The delete path shows the same drift: nft_setelem_catchall_deactivate()
picks the first active-next entry in the catchall list, so with an
expired entry still pending GC it retires the stale entry instead of
the fresh one, and it deactivates an element that userspace no longer
sees instead of failing with -ENOENT.

Align both walks with the lookup and dump predicates: only an element
that is active and not expired counts as a duplicate or delete
candidate, using the per-netns timestamp taken at transaction start,
in line with the set backend .insert/.deactivate and catchall GC sync
paths.

Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Cc: stable@vger.kernel.org
Fixes: aaa31047a6d2 ("netfilter: nftables: add catch-all set element support")
Assisted-by: CodeBuddy:Kimi-K3
Signed-off-by: Aohan Mei <henrymei@tencent.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index c0b754a2d45b..800efb4d91eb 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -6995,11 +6995,13 @@ static int nft_setelem_catchall_insert(const struct net *net,
 {
 	struct nft_set_elem_catchall *catchall;
 	u8 genmask = nft_genmask_next(net);
+	u64 tstamp = nft_net_tstamp(net);
 	struct nft_set_ext *ext;
 
 	list_for_each_entry(catchall, &set->catchall_list, list) {
 		ext = nft_set_elem_ext(set, catchall->elem);
-		if (nft_set_elem_active(ext, genmask)) {
+		if (nft_set_elem_active(ext, genmask) &&
+		    !__nft_set_elem_expired(ext, tstamp)) {
 			*priv = catchall->elem;
 			return -EEXIST;
 		}
@@ -7092,11 +7094,13 @@ static int nft_setelem_catchall_deactivate(const struct net *net,
 					   struct nft_set_elem *elem)
 {
 	struct nft_set_elem_catchall *catchall;
+	u64 tstamp = nft_net_tstamp(net);
 	struct nft_set_ext *ext;
 
 	list_for_each_entry(catchall, &set->catchall_list, list) {
 		ext = nft_set_elem_ext(set, catchall->elem);
-		if (!nft_is_active_next(net, ext))
+		if (!nft_is_active_next(net, ext) ||
+		    __nft_set_elem_expired(ext, tstamp))
 			continue;
 
 		kfree(elem->priv);
-- 
2.47.3

Re: [PATCH net 10/10] netfilter: nf_tables: skip expired catchall elements on insert and delete

[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: nf_tables: skip expired catchall elements on insert and delete

This adds an expiry test to the catchall duplicate walk in
nft_setelem_catchall_insert() and to the delete-candidate walk in
nft_setelem_catchall_deactivate(), using the per-netns transaction
timestamp.  The stated goal is to make both control-plane walks agree with
the lookup and dump predicates.

A couple of questions on the new predicates and on the commit message.

> The delete path shows the same drift: nft_setelem_catchall_deactivate()
> picks the first active-next entry in the catchall list, so with an
> expired entry still pending GC it retires the stale entry instead of
> the fresh one, and it deactivates an element that userspace no longer
> sees instead of failing with -ENOENT.

[Severity: Low]
Is the first half of this paragraph reachable before this patch?  The
"expired entry still pending GC ... instead of the fresh one" wording
implies a stale active-next entry and a fresh active-next entry can coexist
in catchall_list.

Pre-patch nft_setelem_catchall_insert() rejected every entry that was
active in the next generation, expired or not:

	list_for_each_entry(catchall, &set->catchall_list, list) {
		ext = nft_set_elem_ext(set, catchall->elem);
		if (nft_set_elem_active(ext, genmask)) {
			*priv = catchall->elem;
			return -EEXIST;
		}
	}

The other ways an entry leaves active-next all unlink it from
catchall_list (nft_trans_gc_catchall_sync(), the async GC completion) or
happen on an explicit delete, and the non-EXCL add refreshed the existing
element through nft_trans_elem_update() rather than linking a second one.
So the "wrong entry retired" state seems to first become reachable because
of the insert hunk in this same patch.

The second half (deactivating an element userspace no longer sees instead of
returning -ENOENT) does look pre-existing.

Since the two hunks are interdependent, could the message say so explicitly
so the stable backport is not split?

> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index c0b754a2d45b0..800efb4d91ebe 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -6995,11 +6995,13 @@ static int nft_setelem_catchall_insert(const struct net *net,
>  {
>  	struct nft_set_elem_catchall *catchall;
>  	u8 genmask = nft_genmask_next(net);
> +	u64 tstamp = nft_net_tstamp(net);
>  	struct nft_set_ext *ext;
>  
>  	list_for_each_entry(catchall, &set->catchall_list, list) {
>  		ext = nft_set_elem_ext(set, catchall->elem);
> -		if (nft_set_elem_active(ext, genmask)) {
> +		if (nft_set_elem_active(ext, genmask) &&
> +		    !__nft_set_elem_expired(ext, tstamp)) {
>  			*priv = catchall->elem;
>  			return -EEXIST;
>  		}

[Severity: High]
Should this predicate also test nft_set_elem_is_dead()?

The commit message says the walk is aligned "with the lookup and dump
predicates" and is "in line with the set backend .insert/.deactivate", but
both of those references carry a dead test that is missing here.
nft_set_catchall_lookup() in the same file:

	if (nft_set_elem_active(ext, genmask) &&
	    !nft_set_elem_expired(ext) &&
	    !nft_set_elem_is_dead(ext))
		return ext;

and nft_rhash_cmp() in nft_set_hash.c, which is what the rhash
.insert/.deactivate comparisons go through:

	if (nft_set_elem_is_dead(&he->ext))
		return 1;
	if (__nft_set_elem_expired(&he->ext, x->tstamp))
		return 1;

The two predicates can disagree because the DEAD bit is set from the async
GC worker against real jiffies, while the control plane compares against
nft_net->tstamp, latched once per netlink batch in nf_tables_valid_genid():

	nft_net->tstamp = get_jiffies_64();

An element created inside the current batch always has
expiration = get_jiffies_64() + timeout, i.e. later than tstamp.  If its
timeout (which may be as small as 1 ms) elapses while the same batch is
still being processed, nft_trans_gc_catchall_async() marks it DEAD, yet
__nft_set_elem_expired(ext, tstamp) is still false here.

Does that leave the element in a state where nft_set_catchall_lookup()
skips it for every packet, but this walk still returns -EEXIST?  With
NLM_F_EXCL the re-add then fails for an element that no longer matches
anything.

Without NLM_F_EXCL, the -EEXIST path in nft_add_set_elem() queues
NFT_TRANS_UPD_TIMEOUT/EXPIRATION and nft_trans_elem_update() rewrites the
expiration:

	if (update->flags & NFT_TRANS_UPD_EXPIRATION)
		WRITE_ONCE(nft_set_ext_timeout(ext)->expiration, get_jiffies_64() + update->expiration);

Nothing clears NFT_SET_ELEM_DEAD_BIT anywhere (the only sites are set_bit()
in nft_trans_gc_catchall_async() and in nft_set_hash.c), and
nft_trans_gc_catchall_async() starts with

		if (!nft_set_elem_expired(ext))
			continue;

so after the refresh the element is neither matched by the datapath nor
reclaimed by GC.  A userspace loop that keeps refreshing the timeout would
keep it in that state indefinitely.  Is that not the same "catchall rule may
silently stop matching for an arbitrarily long time" symptom the patch sets
out to remove?  nft_set_catchall_dump() and nft_setelem_catchall_get() also
lack the dead test, so the element is still listed to userspace while never
matching traffic.

>  	}
>  
>  	catchall = kmalloc_obj(*catchall, GFP_KERNEL_ACCOUNT);
>  	if (!catchall)
>  		return -ENOMEM;
>  
>  	catchall->elem = elem->priv;
>  	list_add_tail_rcu(&catchall->list, &set->catchall_list);

[Severity: High]
Can this grow set->catchall_list without bound?

The skipped stale entry is neither unlinked nor reused, so the function
falls through to kmalloc_obj() + list_add_tail_rcu() and appends a new
entry while the expired one stays linked.  Repeating "wait for the element
timeout to elapse, then NFT_MSG_NEWSETELEM with NFT_SET_ELEM_CATCHALL"
appends one entry per cycle.

Nothing appears to cap that, since catchall elements are excluded from the
set size accounting in nft_add_set_elem():

	if (!(flags & NFT_SET_ELEM_CATCHALL)) {
		unsigned int max = nft_set_maxsize(set), nelems;

		nelems = atomic_inc_return(&set->nelems);
		if (nelems > max)
			set_full = true;
	}

and the decrement side skips them too, in nft_trans_gc_trans_free():

		if (!nft_setelem_is_catchall(trans->set, elem_priv))
			atomic_dec(&trans->set->nelems);

so nft_set_maxsize() and -ENFILE never apply here.  For timeout-capable
hash sets the only reaper is the delayed nft_rhash_gc() work:

	queue_delayed_work(system_power_efficient_wq, &priv->gc_work,
			   nft_set_gc_interval(set));

and gc_int comes straight from NFTA_SET_GC_INTERVAL in nf_tables_newset()
with no upper bound, so the same requester controls both how fast entries
accumulate (1 ms element timeout) and how long they stay linked.  The GC
batch is additionally dropped when gc_seq moves, so a stream of small
batches can keep starving reclamation.

Besides the memory, does this not also make the data path scale with the
number of retained entries?  nft_set_catchall_lookup() walks the whole list
under rcu_read_lock() for every packet, and
nft_setelem_catchall_get()/nft_set_catchall_dump() walk it as well.

Would retiring or reusing the skipped element in this walk, rather than
just skipping it, avoid both problems?

>  
>  	return 0;
>  }

[ ... ]

> @@ -7092,11 +7094,13 @@ static int nft_setelem_catchall_deactivate(const struct net *net,
>  					   struct nft_set_elem *elem)
>  {
>  	struct nft_set_elem_catchall *catchall;
> +	u64 tstamp = nft_net_tstamp(net);
>  	struct nft_set_ext *ext;
>  
>  	list_for_each_entry(catchall, &set->catchall_list, list) {
>  		ext = nft_set_elem_ext(set, catchall->elem);
> -		if (!nft_is_active_next(net, ext))
> +		if (!nft_is_active_next(net, ext) ||
> +		    __nft_set_elem_expired(ext, tstamp))
>  			continue;

[Severity: High]
Same question for the delete side: should this skip condition also include
nft_set_elem_is_dead(ext)?  A DEAD element that the datapath already ignores
in nft_set_catchall_lookup() is still selected here and deactivated, where
-ENOENT looks like the expected result.

>  
>  		kfree(elem->priv);

Re: [PATCH net 10/10] netfilter: nf_tables: skip expired catchall elements on insert and delete

[Pablo Neira Ayuso]

On Thu, Sep 17, 2026 at 07:04:12PM -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: nf_tables: skip expired catchall elements on insert and delete
> 
> This adds an expiry test to the catchall duplicate walk in
> nft_setelem_catchall_insert() and to the delete-candidate walk in
> nft_setelem_catchall_deactivate(), using the per-netns transaction
> timestamp.  The stated goal is to make both control-plane walks agree with
> the lookup and dump predicates.
> 
> A couple of questions on the new predicates and on the commit message.
> 
> > The delete path shows the same drift: nft_setelem_catchall_deactivate()
> > picks the first active-next entry in the catchall list, so with an
> > expired entry still pending GC it retires the stale entry instead of
> > the fresh one, and it deactivates an element that userspace no longer
> > sees instead of failing with -ENOENT.
> 
> [Severity: Low]
> Is the first half of this paragraph reachable before this patch?  The
> "expired entry still pending GC ... instead of the fresh one" wording
> implies a stale active-next entry and a fresh active-next entry can coexist
> in catchall_list.
> 
> Pre-patch nft_setelem_catchall_insert() rejected every entry that was
> active in the next generation, expired or not:
> 
> 	list_for_each_entry(catchall, &set->catchall_list, list) {
> 		ext = nft_set_elem_ext(set, catchall->elem);
> 		if (nft_set_elem_active(ext, genmask)) {
> 			*priv = catchall->elem;
> 			return -EEXIST;
> 		}
> 	}
> 
> The other ways an entry leaves active-next all unlink it from
> catchall_list (nft_trans_gc_catchall_sync(), the async GC completion) or
> happen on an explicit delete, and the non-EXCL add refreshed the existing
> element through nft_trans_elem_update() rather than linking a second one.
> So the "wrong entry retired" state seems to first become reachable because
> of the insert hunk in this same patch.
> 
> The second half (deactivating an element userspace no longer sees instead of
> returning -ENOENT) does look pre-existing.
> 
> Since the two hunks are interdependent, could the message say so explicitly
> so the stable backport is not split?

No need to split this stable backport...

> [Severity: High]
> Should this predicate also test nft_set_elem_is_dead()?

Yes, originally this patch checked for the dead and it is indeed need.