[PATCH net-next 00/12] Netfilter updates for net

[PATCH net-next 00/12] Netfilter updates for net

[Pablo Neira Ayuso]

Hi,

The following patchset contains Netfilter updates for net-next.
This includes an enhancement to detect ct memleaks easier via
DEBUG_NET and flowtable preparation patches for IPv4 over IPV6
and vice-versa. This also includes a fix for the nft_ct custom
expectation support.

1) Add DEBUG_NET_WARN_ON_ONCE to nf_ct_set() to spot ct memleaks.

2) Pass struct net_device_path_ctx to dev_fill_forward_path() to
   make it easier to pass more parameters to this function.
   From Lorenzo Bianconi.

3) Add ether_type field to net_device_path context structucture.

4) Rename tun.l3_proto field to tun.inner_proto.

5) Rename ctx.tun.proto to ctx.tun.inner_proto.

6) Store ether_type in flowtable context.

7) Move IPv4 and IPv6 xmit path to a helper function.

8) Move encapsulation header parser out of the flowtable lookup
   function.

9) Rework nft_ct custom expectation support to address a possible
   reallocation of ct extension area while expectation list also
   contains expectations. Move datapath to a ct helper to fix it.

10) Ensure timeout is always lowered for the non-closing RST case
    in the TCP connection tracking.

11) Bail out when inserting already dead expectation, this should
    not ever happen, hence report it via DEBUG_NET.

12) Comestic updates for improving the conntrack selftest dump and
    flush userspace program, from Qingshuang Fu.

Please, pull these changes from:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf-next.git nf-next-26-08-10

Thanks.

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

The following changes since commit 001b5d347d8ba39b2dccaefcc57967b18caec8fe:

  Merge branch 'net-devmem-allow-rx-buf-size-page_size-per-binding' (2026-08-07 18:32:41 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf-next.git tags/nf-next-26-08-10

for you to fetch changes up to 736fb8632217bd27da6b2e3f1f8cbbe3193fc2d8:

  selftests: netfilter: conntrack_dump_flush: remove unused variables and fix typo (2026-08-10 21:20:44 +0200)

----------------------------------------------------------------
netfilter pull request 26-08-10

----------------------------------------------------------------
Lorenzo Bianconi (1):
      net: pass net_device_path_ctx to dev_fill_forward_path()

Pablo Neira Ayuso (10):
      netfilter: add DEBUG_NET_WARN_ON_ONCE to skb_set_nfct()
      net: netfilter: add ether_type to net_device_path_ctx and use it
      netfilter: flowtable: rename tun.l3_proto to tun.inner_proto
      netfilter: flowtable: rename ctx.tun.proto to ctx.tun.inner_proto
      netfilter: flowtable: store ethertype in flowtable context
      netfilter: flowtable: move ipv4 and ipv6 xmit path to function
      netfilter: flowtable: detach layer 2 encapsulation parser from lookup
      netfilter: nft_ct: move custom expectation support to helper
      netfilter: conntrack: always lower timeout for non-closing RST packets
      netfilter: nf_conntrack_expect: bail out on insert dead expectations

Qingshuang Fu (1):
      selftests: netfilter: conntrack_dump_flush: remove unused variables and fix typo

 drivers/net/ethernet/airoha/airoha_ppe.c           |   7 +-
 drivers/net/ethernet/mediatek/mtk_ppe_offload.c    |   7 +-
 include/linux/netdevice.h                          |   5 +-
 include/linux/skbuff.h                             |   1 +
 include/net/ip_vs.h                                |   2 +-
 include/net/netfilter/nf_conntrack_helper.h        |   1 +
 include/net/netfilter/nf_flow_table.h              |   2 +-
 net/core/dev.c                                     |  18 +--
 net/ipv4/ipip.c                                    |   5 +-
 net/ipv6/ip6_tunnel.c                              |   5 +-
 net/netfilter/nf_conntrack_core.c                  |   2 +-
 net/netfilter/nf_conntrack_expect.c                |   6 +
 net/netfilter/nf_conntrack_helper.c                |  14 +-
 net/netfilter/nf_conntrack_proto_tcp.c             |   3 +-
 net/netfilter/nf_flow_table_ip.c                   | 167 ++++++++++++---------
 net/netfilter/nf_flow_table_path.c                 |  17 ++-
 net/netfilter/nft_ct.c                             | 167 +++++++++++++++------
 net/openvswitch/conntrack.c                        |  12 +-
 net/sched/act_ct.c                                 |   6 +-
 .../selftests/net/netfilter/conntrack_dump_flush.c |  31 ++--
 20 files changed, 305 insertions(+), 173 deletions(-)

[PATCH net-next 01/12] netfilter: add DEBUG_NET_WARN_ON_ONCE to skb_set_nfct()

[Pablo Neira Ayuso]

Trigger a warning if nf_ct_set() overlaps an existing ct object leading
to refcount leak. Add this warning to skb_set_nfct() whose only user is
nf_ct_set() instead.

Update existing nf_ct_set() callers to use nf_reset_ct() first to clean
up stale pointer to conntrack object which migh trigger false positive
warnings.

Reviewed-by: Fernando Fernandez Mancera <fmancera@suse.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/skbuff.h            |  1 +
 include/net/ip_vs.h               |  2 +-
 net/netfilter/nf_conntrack_core.c |  2 +-
 net/openvswitch/conntrack.c       | 12 +++---------
 net/sched/act_ct.c                |  6 +++---
 5 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 22eda1d54a0e..95184183180f 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -5004,6 +5004,7 @@ static inline unsigned long skb_get_nfct(const struct sk_buff *skb)
 static inline void skb_set_nfct(struct sk_buff *skb, unsigned long nfct)
 {
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
+	DEBUG_NET_WARN_ON_ONCE(skb->_nfct & NFCT_PTRMASK);
 	skb->slow_gro |= !!nfct;
 	skb->_nfct = nfct;
 #endif
diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index b3bb228ad75c..3dca7d387dd0 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -2121,7 +2121,7 @@ static inline void ip_vs_notrack(struct sk_buff *skb)
 	struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
 
 	if (ct) {
-		nf_conntrack_put(&ct->ct_general);
+		nf_reset_ct(skb);
 		nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
 	}
 #endif
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 784bd1d7a9bf..d0d9e5ea84a0 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1031,7 +1031,7 @@ static int __nf_ct_resolve_clash(struct sk_buff *skb,
 		nf_conntrack_get(&ct->ct_general);
 
 		nf_ct_acct_merge(ct, ctinfo, loser_ct);
-		nf_ct_put(loser_ct);
+		nf_reset_ct(skb);
 		nf_ct_set(skb, ct, ctinfo);
 
 		NF_CT_STAT_INC(net, clash_resolve);
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 95697d4e16e6..4dd82c4e87d3 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -603,7 +603,7 @@ static bool skb_nfct_cached(struct net *net,
 		if (nf_ct_is_confirmed(ct))
 			nf_ct_delete(ct, 0, 0);
 
-		nf_ct_put(ct);
+		nf_reset_ct(skb);
 		nf_ct_set(skb, NULL, 0);
 		return false;
 	}
@@ -745,8 +745,7 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 
 		/* Associate skb with specified zone. */
 		if (tmpl) {
-			ct = nf_ct_get(skb, &ctinfo);
-			nf_ct_put(ct);
+			nf_reset_ct(skb);
 			nf_conntrack_get(&tmpl->ct_general);
 			nf_ct_set(skb, tmpl, IP_CT_NEW);
 		}
@@ -1075,12 +1074,7 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
 
 int ovs_ct_clear(struct sk_buff *skb, struct sw_flow_key *key)
 {
-	enum ip_conntrack_info ctinfo;
-	struct nf_conn *ct;
-
-	ct = nf_ct_get(skb, &ctinfo);
-
-	nf_ct_put(ct);
+	nf_reset_ct(skb);
 	nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
 
 	if (key)
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 4ca7964e83c8..7f54fb4e4ec9 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -782,7 +782,7 @@ static bool tcf_ct_skb_nfct_cached(struct net *net, struct sk_buff *skb,
 	return true;
 
 drop_ct:
-	nf_ct_put(ct);
+	nf_reset_ct(skb);
 	nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
 
 	return false;
@@ -996,7 +996,7 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 		qdisc_skb_cb(skb)->post_ct = false;
 		ct = nf_ct_get(skb, &ctinfo);
 		if (ct) {
-			nf_ct_put(ct);
+			nf_reset_ct(skb);
 			nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
 		}
 
@@ -1034,7 +1034,7 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 
 		/* Associate skb with specified zone. */
 		if (tmpl) {
-			nf_conntrack_put(skb_nfct(skb));
+			nf_reset_ct(skb);
 			nf_conntrack_get(&tmpl->ct_general);
 			nf_ct_set(skb, tmpl, IP_CT_NEW);
 		}
-- 
2.47.3

Re: [PATCH net-next 01/12] netfilter: add DEBUG_NET_WARN_ON_ONCE to skb_set_nfct()

[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: add DEBUG_NET_WARN_ON_ONCE to skb_set_nfct()

This adds a DEBUG_NET_WARN_ON_ONCE() to skb_set_nfct() that fires when
skb->_nfct already holds a conntrack pointer, so that nf_ct_set() calls
overwriting a live ct object are caught.  Several existing nf_ct_set()
callers in ipvs, conntrack clash resolution, openvswitch and act_ct are
converted to call nf_reset_ct() first.

> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 22eda1d54a0e8..95184183180f6 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -5004,6 +5004,7 @@ static inline unsigned long skb_get_nfct(const struct sk_buff *skb)
>  static inline void skb_set_nfct(struct sk_buff *skb, unsigned long nfct)
>  {
>  #if IS_ENABLED(CONFIG_NF_CONNTRACK)
> +	DEBUG_NET_WARN_ON_ONCE(skb->_nfct & NFCT_PTRMASK);
>  	skb->slow_gro |= !!nfct;
>  	skb->_nfct = nfct;
>  #endif

[Severity: High]
Was nf_ct_bridge_pre() in net/bridge/netfilter/nf_conntrack_bridge.c meant
to be converted too?

That hook deliberately lets an skb carrying a conntrack template through its
entry check:

net/bridge/netfilter/nf_conntrack_bridge.c:nf_ct_bridge_pre() {
	ct = nf_ct_get(skb, &ctinfo);
	if ((ct && !nf_ct_is_template(ct)) ||
	    ctinfo == IP_CT_UNTRACKED)
		return NF_ACCEPT;

The ETH_P_IP and ETH_P_IPV6 arms then hand the template to
nf_conntrack_in(), which zeroes skb->_nfct and drops the template reference
with nf_ct_put(tmpl) at its out: label, so those paths satisfy the new
precondition.  The default: arm does not:

	default:
		nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
		return NF_ACCEPT;
	}

Since the new warning tests the old value of skb->_nfct regardless of the
new value, does nf_ct_set(skb, NULL, IP_CT_UNTRACKED) here trip
DEBUG_NET_WARN_ON_ONCE() whenever a template is still attached?

Can a template be attached before this hook runs?  An nft bridge base chain
in NF_BR_PRE_ROUTING at a priority below NF_IP_PRI_CONNTRACK (-200) appears
to be accepted, as nf_tables_api.c only restricts priorities at or below
NF_IP_PRI_CONNTRACK for NAT-type chains.  A rule with ct zone set in such a
chain reaches nft_ct_set_zone_eval():

net/netfilter/nft_ct.c:nft_ct_set_zone_eval() {
	...
	nf_ct_set(skb, ct, IP_CT_NEW);

which takes a reference on the per-CPU template (or allocates a fresh one
via nf_ct_tmpl_alloc() when that one is busy).  nft_do_chain_bridge()
evaluates rules for every ethertype, including ARP, PPPoE and LLDP frames,
which then land in the default: arm above.

With CONFIG_DEBUG_NET=y, does this turn any received non-IPv4/IPv6 frame on
such a bridge into a warning splat, and a panic when panic_on_warn=1?

Separately, the reference drop at that site is not new: the overwrite has
always discarded the template reference without nf_conntrack_put().  Does
that pin the per-CPU template refcount above 1 permanently, so
nft_ct_set_zone_eval() takes the nf_ct_tmpl_alloc() path for every later
frame, and each of those allocations is leaked by the same
nf_ct_set(skb, NULL, IP_CT_UNTRACKED)?

For reference, the same shape was converted in this patch:

> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index b3bb228ad75cb..3dca7d387dd08 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -2121,7 +2121,7 @@ static inline void ip_vs_notrack(struct sk_buff *skb)
>  	struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
>  
>  	if (ct) {
> -		nf_conntrack_put(&ct->ct_general);
> +		nf_reset_ct(skb);
>  		nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
>  	}
>  #endif

[ ... ]

Would adding nf_reset_ct(skb) before the nf_ct_set() in the default: arm of
nf_ct_bridge_pre() be the right fix?  If it is intentionally left for a
separate change, could the commit message mention it so that a new warning
in bridge plus conntrack zone setups is not a surprise?

Checking the tip of the series (839e6caf13c7332db9b40817696bb7a326b810d5),
nf_ct_bridge_pre() is still unchanged and the warning in skb_set_nfct() is
still present.

Two nearby nf_ct_set() users also set a ct on an skb without clearing a
possible pre-existing one: nf_conntrack_attach() in
net/netfilter/nf_conntrack_core.c and tcf_ct_flow_table_restore_skb() in
include/net/tc_act/tc_ct.h.  Both rely on the caller supplying a fresh skb.
Is that assumption worth documenting alongside the new invariant?

Re: [PATCH net-next 01/12] netfilter: add DEBUG_NET_WARN_ON_ONCE to skb_set_nfct()

[Pablo Neira Ayuso]

On Tue, Aug 11, 2026 at 06:34:41PM -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: add DEBUG_NET_WARN_ON_ONCE to skb_set_nfct()
> 
> This adds a DEBUG_NET_WARN_ON_ONCE() to skb_set_nfct() that fires when
> skb->_nfct already holds a conntrack pointer, so that nf_ct_set() calls
> overwriting a live ct object are caught.  Several existing nf_ct_set()
> callers in ipvs, conntrack clash resolution, openvswitch and act_ct are
> converted to call nf_reset_ct() first.
> 
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 22eda1d54a0e8..95184183180f6 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -5004,6 +5004,7 @@ static inline unsigned long skb_get_nfct(const struct sk_buff *skb)
> >  static inline void skb_set_nfct(struct sk_buff *skb, unsigned long nfct)
> >  {
> >  #if IS_ENABLED(CONFIG_NF_CONNTRACK)
> > +	DEBUG_NET_WARN_ON_ONCE(skb->_nfct & NFCT_PTRMASK);
> >  	skb->slow_gro |= !!nfct;
> >  	skb->_nfct = nfct;
> >  #endif
> 
> [Severity: High]
> Was nf_ct_bridge_pre() in net/bridge/netfilter/nf_conntrack_bridge.c meant
> to be converted too?

This particular issue is fixed in the nf.git PR.


BTW, I can see these reports from here:

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260810194015.932627-1-pablo%40netfilter.org

Thanks.

[PATCH net-next 02/12] net: pass net_device_path_ctx to dev_fill_forward_path()

[Pablo Neira Ayuso]

From: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

Refactor dev_fill_forward_path() to take a struct net_device_path_ctx
pointer instead of a (dev, daddr) pair, so the caller can build and
populate the context up front and keep it after the forward path walk.

This allows additional fields (e.g. vlan and ether_type) to be carried
in the context and shared with ndo_fill_forward_path implementations,
instead of being reconstructed on the stack inside the core helper.

Update the mtk_ppe_offload, airoha_ppe and nf_flow_table_path callers to
allocate and fill the context before invoking dev_fill_forward_path().
The network topology resolution behaviour is unchanged.

This is a preliminary patch to enable HW flowtable offload for IPv4
over IPv6 tunnels.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 drivers/net/ethernet/airoha/airoha_ppe.c       |  7 ++++++-
 .../net/ethernet/mediatek/mtk_ppe_offload.c    |  7 ++++++-
 include/linux/netdevice.h                      |  2 +-
 net/core/dev.c                                 | 18 +++++++-----------
 net/netfilter/nf_flow_table_path.c             |  7 ++++++-
 5 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c
index a03af9750573..92611802801e 100644
--- a/drivers/net/ethernet/airoha/airoha_ppe.c
+++ b/drivers/net/ethernet/airoha/airoha_ppe.c
@@ -283,14 +283,19 @@ static int airoha_ppe_get_wdma_info(struct net_device *dev, const u8 *addr,
 				    struct airoha_wdma_info *info)
 {
 	struct net_device_path_stack stack;
+	struct net_device_path_ctx ctx = {
+		.dev = dev,
+	};
 	struct net_device_path *path;
 	int err;
 
 	if (!dev)
 		return -ENODEV;
 
+	ether_addr_copy(ctx.daddr, addr);
+
 	rcu_read_lock();
-	err = dev_fill_forward_path(dev, addr, &stack);
+	err = dev_fill_forward_path(&ctx, &stack);
 	rcu_read_unlock();
 	if (err)
 		return err;
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
index 771d9118f94a..99b28aaa7cc4 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
@@ -92,6 +92,9 @@ static int
 mtk_flow_get_wdma_info(struct net_device *dev, const u8 *addr, struct mtk_wdma_info *info)
 {
 	struct net_device_path_stack stack;
+	struct net_device_path_ctx ctx = {
+		.dev = dev,
+	};
 	struct net_device_path *path;
 	int err;
 
@@ -101,8 +104,10 @@ mtk_flow_get_wdma_info(struct net_device *dev, const u8 *addr, struct mtk_wdma_i
 	if (!IS_ENABLED(CONFIG_NET_MEDIATEK_SOC_WED))
 		return -1;
 
+	ether_addr_copy(ctx.daddr, addr);
+
 	rcu_read_lock();
-	err = dev_fill_forward_path(dev, addr, &stack);
+	err = dev_fill_forward_path(&ctx, &stack);
 	rcu_read_unlock();
 	if (err)
 		return err;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index db9dce7f0aa6..17d28adb029b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3428,7 +3428,7 @@ void dev_remove_offload(struct packet_offload *po);
 
 int dev_get_iflink(const struct net_device *dev);
 int dev_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb);
-int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
+int dev_fill_forward_path(struct net_device_path_ctx *ctx,
 			  struct net_device_path_stack *stack);
 void dev_fill_forward_path_release(struct net_device_path_stack *stack);
 struct net_device *dev_get_by_name(struct net *net, const char *name);
diff --git a/net/core/dev.c b/net/core/dev.c
index fd0b445f5d38..1755dd0b2a92 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -769,35 +769,31 @@ void dev_fill_forward_path_release(struct net_device_path_stack *stack)
 }
 EXPORT_SYMBOL_GPL(dev_fill_forward_path_release);
 
-int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
+int dev_fill_forward_path(struct net_device_path_ctx *ctx,
 			  struct net_device_path_stack *stack)
 {
 	const struct net_device *last_dev;
-	struct net_device_path_ctx ctx = {
-		.dev	= dev,
-	};
 	struct net_device_path *path;
 	int ret = 0;
 
-	memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
 	stack->num_paths = 0;
-	while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) {
-		last_dev = ctx.dev;
+	while (ctx->dev && ctx->dev->netdev_ops->ndo_fill_forward_path) {
+		last_dev = ctx->dev;
 		path = dev_fwd_path(stack);
 		if (!path)
 			goto err_out;
 
 		memset(path, 0, sizeof(struct net_device_path));
-		ret = ctx.dev->netdev_ops->ndo_fill_forward_path(&ctx, path);
+		ret = ctx->dev->netdev_ops->ndo_fill_forward_path(ctx, path);
 		if (ret < 0)
 			goto err_out;
 
 		stack->num_paths++;
-		if (WARN_ON_ONCE(last_dev == ctx.dev))
+		if (WARN_ON_ONCE(last_dev == ctx->dev))
 			goto err_out;
 	}
 
-	if (!ctx.dev)
+	if (!ctx->dev)
 		return ret;
 
 	path = dev_fwd_path(stack);
@@ -805,7 +801,7 @@ int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
 		goto err_out;
 
 	path->type = DEV_PATH_ETHERNET;
-	path->dev = ctx.dev;
+	path->dev = ctx->dev;
 	stack->num_paths++;
 
 	return 0;
diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
index 56219b02e122..0cbde535b8ba 100644
--- a/net/netfilter/nf_flow_table_path.c
+++ b/net/netfilter/nf_flow_table_path.c
@@ -49,6 +49,9 @@ static int nft_dev_fill_forward_path(const struct dst_entry *dst_cache,
 {
 	const void *daddr = &ct->tuplehash[!dir].tuple.src.u3;
 	struct net_device *dev = dst_cache->dev;
+	struct net_device_path_ctx ctx = {
+		.dev = dev,
+	};
 	struct neighbour *n;
 	u8 nud_state;
 
@@ -71,7 +74,9 @@ static int nft_dev_fill_forward_path(const struct dst_entry *dst_cache,
 		return -1;
 
 out:
-	return dev_fill_forward_path(dev, ha, stack);
+	ether_addr_copy(ctx.daddr, ha);
+
+	return dev_fill_forward_path(&ctx, stack);
 }
 
 struct nft_forward_info {
-- 
2.47.3

[PATCH net-next 03/12] net: netfilter: add ether_type to net_device_path_ctx and use it

[Pablo Neira Ayuso]

Add an ether_type field to struct net_device_path_ctx to reject IPv4
over IPv6 and vice-versa, this is currently not support. Otherwise,
incorrect dst_entry family can be reached from datapath.

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netdevice.h          | 1 +
 net/ipv4/ipip.c                    | 3 +++
 net/ipv6/ip6_tunnel.c              | 3 +++
 net/netfilter/nf_flow_table_path.c | 6 ++++--
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 17d28adb029b..2327a2703b83 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -941,6 +941,7 @@ struct net_device_path_stack {
 struct net_device_path_ctx {
 	const struct net_device *dev;
 	u8			daddr[ETH_ALEN];
+	__be16			ether_type;
 
 	int			num_vlans;
 	struct {
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index fb7d96f99b06..62a374079bfc 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -360,6 +360,9 @@ static int ipip_fill_forward_path(struct net_device_path_ctx *ctx,
 	const struct iphdr *tiph = &tunnel->parms.iph;
 	struct rtable *rt;
 
+	if (ctx->ether_type != cpu_to_be16(ETH_P_IP))
+		return -EOPNOTSUPP;
+
 	if (tunnel->collect_md)
 		return -EOPNOTSUPP;
 
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 042d743edb6c..d063add01f52 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1852,6 +1852,9 @@ static int ip6_tnl_fill_forward_path(struct net_device_path_ctx *ctx,
 	struct flowi6 fl6;
 	int err;
 
+	if (ctx->ether_type != cpu_to_be16(ETH_P_IPV6))
+		return -EOPNOTSUPP;
+
 	if (t->parms.flags & (IP6_TNL_F_USE_ORIG_TCLASS |
 			      IP6_TNL_F_USE_ORIG_FLOWLABEL |
 			      IP6_TNL_F_USE_ORIG_FWMARK))
diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
index 0cbde535b8ba..5f166da3b09b 100644
--- a/net/netfilter/nf_flow_table_path.c
+++ b/net/netfilter/nf_flow_table_path.c
@@ -44,13 +44,15 @@ static bool nft_is_valid_ether_device(const struct net_device *dev)
 
 static int nft_dev_fill_forward_path(const struct dst_entry *dst_cache,
 				     const struct nf_conn *ct,
-				     enum ip_conntrack_dir dir, u8 *ha,
+				     enum ip_conntrack_dir dir,
+				     u8 *ha, __be16 ether_type,
 				     struct net_device_path_stack *stack)
 {
 	const void *daddr = &ct->tuplehash[!dir].tuple.src.u3;
 	struct net_device *dev = dst_cache->dev;
 	struct net_device_path_ctx ctx = {
 		.dev = dev,
+		.ether_type = ether_type,
 	};
 	struct neighbour *n;
 	u8 nud_state;
@@ -228,7 +230,7 @@ static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
 	unsigned char ha[ETH_ALEN];
 	int i;
 
-	if (nft_dev_fill_forward_path(dst, ct, dir, ha, &stack) < 0 ||
+	if (nft_dev_fill_forward_path(dst, ct, dir, ha, pkt->ethertype, &stack) < 0 ||
 	    nft_dev_path_info(&stack, &info, ha, ft) < 0)
 		return -ENOENT;
 
-- 
2.47.3

Re: [PATCH net-next 03/12] net: netfilter: add ether_type to net_device_path_ctx and use it

[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.
---
net: netfilter: add ether_type to net_device_path_ctx and use it

This adds an ether_type field to struct net_device_path_ctx and makes the
ipip and ip6tnl ndo_fill_forward_path callbacks reject a context whose
ether_type does not match their own address family.  The netfilter
flowtable path builder fills the new field from pkt->ethertype.

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 17d28adb029bb..2327a2703b835 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -941,6 +941,7 @@ struct net_device_path_stack {
>  struct net_device_path_ctx {
>  	const struct net_device *dev;
>  	u8			daddr[ETH_ALEN];
> +	__be16			ether_type;
>  
>  	int			num_vlans;
>  	struct {

[Severity: Medium]
This new field becomes a mandatory input for the tunnel
ndo_fill_forward_path callbacks, but only the netfilter context builder is
updated here.  The two other in-tree constructors of the context still use
designated initializers that set only .dev, so ether_type stays zero:

drivers/net/ethernet/mediatek/mtk_ppe_offload.c:mtk_flow_get_wdma_info() {
	struct net_device_path_ctx ctx = {
		.dev = dev,
	};
	...
}

drivers/net/ethernet/airoha/airoha_ppe.c:airoha_ppe_get_wdma_info() {
	struct net_device_path_ctx ctx = {
		.dev = dev,
	};
	...
}

Zero matches neither ETH_P_IP nor ETH_P_IPV6, and dev_fill_forward_path()
turns any negative return from the callback into a full abort:

net/core/dev.c:dev_fill_forward_path() {
	...
		ret = ctx->dev->netdev_ops->ndo_fill_forward_path(ctx, path);
		if (ret < 0)
			goto err_out;
	...
}

So does an ipip or ip6tnl device anywhere in the walk now always fail path
resolution for those two drivers, even for the same-family case that used
to work, leaving the PPE/WED entry unprogrammed?  Both drivers know the
flow's L3 protocol at that point, so could they populate ether_type as
well?

Would it also help to document in the struct that 0 means "unknown" and is
treated as unsupported by the tunnel callbacks?

> diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
> index fb7d96f99b06b..62a374079bfcb 100644
> --- a/net/ipv4/ipip.c
> +++ b/net/ipv4/ipip.c
> @@ -360,6 +360,9 @@ static int ipip_fill_forward_path(struct net_device_path_ctx *ctx,
>  	const struct iphdr *tiph = &tunnel->parms.iph;
>  	struct rtable *rt;
>  
> +	if (ctx->ether_type != cpu_to_be16(ETH_P_IP))
> +		return -EOPNOTSUPP;
> +
>  	if (tunnel->collect_md)
>  		return -EOPNOTSUPP;
>  

[Severity: Low]
The commit message describes a datapath defect ("incorrect dst_entry family
can be reached from datapath"), but there is no Fixes: tag.  Without this
gate, an IPv6 flow routed out of an ipip device makes
ipip_fill_forward_path() succeed and return an IPv4 rtable in
path->tun.dst, after which nft_dev_path_info() reads the v6 members of the
address union and nft_dev_forward_path() installs that dst:

net/netfilter/nf_flow_table_path.c:nft_dev_forward_path() {
	...
		dst_release(route->tuple[dir].dst);
		route->tuple[dir].dst = info.tun_dst;
	...
}

The two handlers were added by ab427db17885 ("netfilter: flowtable: Add
IPIP rx sw acceleration") and d98103575dcd, and the tunnel dst hand-off was
last touched by 806273fcaffb82 ("netfilter: flowtable: release tunnel route
on error when building forward path").  Could a Fixes: tag against one of
these be added so a backport can be keyed off it?

It would also be worth noting in the message that this is not
standalone-backportable: it depends on the preceding commit 635bf8763d7e
("net: pass net_device_path_ctx to dev_fill_forward_path()"), since before
that the context was built inside net/core/dev.c and the netfilter caller
had no way to pass an ethertype.

[Severity: Medium]
This isn't a bug introduced by this patch, but the new gate only validates
the packet's L3 protocol and still leaves the tunnel device's configured
payload protocol unchecked.  An ipip link can be configured with
iph.protocol == IPPROTO_MPLS (mplsip), and such a device shares
ipip_netdev_ops and therefore this callback, yet the callback never consults
tunnel->parms.iph.protocol and unconditionally sets
path->tun.inner_proto = IPPROTO_IPIP.

The device's own paths do enforce it:

net/ipv4/ipip.c:ipip_tunnel_xmit() {
	...
	if (tiph->protocol != ipproto && tiph->protocol != 0)
		goto tx_error;
	...
}

net/ipv4/ipip.c:ipip_rcv() {
	...
		if (tunnel->parms.iph.protocol != ipproto &&
		    tunnel->parms.iph.protocol != 0)
			goto drop;
	...
}

The offload path then writes the hardcoded value itself:

net/netfilter/nf_flow_table_ip.c:nf_flow_tunnel_ipip_push() {
	...
	iph->protocol	= tuple->tun.l3_proto;
	...
}

So for an mplsip ipip device, does the slow path drop the first packet
while every subsequent packet of the offloaded flow is encapsulated (and
matching ingress packets decapsulated) with a protocol number that
contradicts the device configuration?  Should the gate also compare against
tunnel->parms.iph.protocol?

> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index 042d743edb6c0..d063add01f523 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1852,6 +1852,9 @@ static int ip6_tnl_fill_forward_path(struct net_device_path_ctx *ctx,
>  	struct flowi6 fl6;
>  	int err;
>  
> +	if (ctx->ether_type != cpu_to_be16(ETH_P_IPV6))
> +		return -EOPNOTSUPP;
> +
>  	if (t->parms.flags & (IP6_TNL_F_USE_ORIG_TCLASS |
>  			      IP6_TNL_F_USE_ORIG_FLOWLABEL |
>  			      IP6_TNL_F_USE_ORIG_FWMARK))

[Severity: Medium]
Same pre-existing observation on the IPv6 side, not something this patch
introduces: t->parms.proto is ignored while path->tun.inner_proto is always
set to IPPROTO_IPV6, whereas the software paths reject a mismatch:

net/ipv6/ip6_tunnel.c:ipxip6_tnl_xmit() {
	...
	tproto = READ_ONCE(t->parms.proto);
	if (tproto != protocol && tproto != 0)
		return -1;
	...
}

ip6_tnl_rcv() applies the same check on receive.  For an ip6tnl in ip4ip6
mode, does the offload path end up forwarding traffic that the tunnel
device itself would drop?

> diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
> index 0cbde535b8bae..5f166da3b09be 100644
> --- a/net/netfilter/nf_flow_table_path.c
> +++ b/net/netfilter/nf_flow_table_path.c
> @@ -44,13 +44,15 @@ static bool nft_is_valid_ether_device(const struct net_device *dev)
>  
>  static int nft_dev_fill_forward_path(const struct dst_entry *dst_cache,
>  				     const struct nf_conn *ct,
> -				     enum ip_conntrack_dir dir, u8 *ha,
> +				     enum ip_conntrack_dir dir,
> +				     u8 *ha, __be16 ether_type,
>  				     struct net_device_path_stack *stack)
>  {
>  	const void *daddr = &ct->tuplehash[!dir].tuple.src.u3;
>  	struct net_device *dev = dst_cache->dev;
>  	struct net_device_path_ctx ctx = {
>  		.dev = dev,
> +		.ether_type = ether_type,
>  	};
>  	struct neighbour *n;
>  	u8 nud_state;

[ ... ]

> @@ -228,7 +230,7 @@ static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
>  	unsigned char ha[ETH_ALEN];
>  	int i;
>  
> -	if (nft_dev_fill_forward_path(dst, ct, dir, ha, &stack) < 0 ||
> +	if (nft_dev_fill_forward_path(dst, ct, dir, ha, pkt->ethertype, &stack) < 0 ||
>  	    nft_dev_path_info(&stack, &info, ha, ft) < 0)
>  		return -ENOENT;
>

[PATCH net-next 04/12] netfilter: flowtable: rename tun.l3_proto to tun.inner_proto

[Pablo Neira Ayuso]

This field refers to the inner protocol that is encapsulated by the
tunnel header, just a comestic change. No functional changes are
expected.

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netdevice.h             | 2 +-
 include/net/netfilter/nf_flow_table.h | 2 +-
 net/ipv4/ipip.c                       | 2 +-
 net/ipv6/ip6_tunnel.c                 | 2 +-
 net/netfilter/nf_flow_table_ip.c      | 6 +++---
 net/netfilter/nf_flow_table_path.c    | 4 ++--
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2327a2703b83..7f5c2323146d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -904,7 +904,7 @@ struct net_device_path {
 				struct in6_addr	dst_v6;
 			};
 
-			u8	l3_proto;
+			u8	inner_proto;
 		} tun;
 		struct {
 			enum {
diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
index a090ec3ffef2..f2e2771f188f 100644
--- a/include/net/netfilter/nf_flow_table.h
+++ b/include/net/netfilter/nf_flow_table.h
@@ -117,7 +117,7 @@ struct flow_offload_tunnel {
 		struct in6_addr	dst_v6;
 	};
 
-	u8	l3_proto;
+	u8	inner_proto;
 };
 
 struct flow_offload_tuple {
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index 62a374079bfc..1630325c77d3 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -378,7 +378,7 @@ static int ipip_fill_forward_path(struct net_device_path_ctx *ctx,
 	path->type = DEV_PATH_TUN;
 	path->tun.src_v4.s_addr = tiph->saddr;
 	path->tun.dst_v4.s_addr = tiph->daddr;
-	path->tun.l3_proto = IPPROTO_IPIP;
+	path->tun.inner_proto = IPPROTO_IPIP;
 	path->tun.dst = &rt->dst;
 	path->dev = ctx->dev;
 
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index d063add01f52..143d061fb48c 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1875,7 +1875,7 @@ static int ip6_tnl_fill_forward_path(struct net_device_path_ctx *ctx,
 		path->type = DEV_PATH_TUN;
 		path->tun.src_v6 = fl6.saddr;
 		path->tun.dst_v6 = fl6.daddr;
-		path->tun.l3_proto = IPPROTO_IPV6;
+		path->tun.inner_proto = IPPROTO_IPV6;
 		path->tun.dst = dst;
 		path->dev = ctx->dev;
 		ctx->dev = dst->dev;
diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index 78d1862ce783..a03946a5c2e7 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -197,7 +197,7 @@ static void nf_flow_tuple_encap(struct nf_flowtable_ctx *ctx,
 		if (ctx->tun.proto == IPPROTO_IPIP) {
 			tuple->tun.dst_v4.s_addr = iph->daddr;
 			tuple->tun.src_v4.s_addr = iph->saddr;
-			tuple->tun.l3_proto = IPPROTO_IPIP;
+			tuple->tun.inner_proto = IPPROTO_IPIP;
 		}
 		break;
 	case htons(ETH_P_IPV6):
@@ -205,7 +205,7 @@ static void nf_flow_tuple_encap(struct nf_flowtable_ctx *ctx,
 		if (ctx->tun.proto == IPPROTO_IPV6) {
 			tuple->tun.dst_v6 = ip6h->daddr;
 			tuple->tun.src_v6 = ip6h->saddr;
-			tuple->tun.l3_proto = IPPROTO_IPV6;
+			tuple->tun.inner_proto = IPPROTO_IPV6;
 		}
 		break;
 	default:
@@ -612,7 +612,7 @@ static int nf_flow_tunnel_ipip_push(struct net *net, struct sk_buff *skb,
 	iph->version	= 4;
 	iph->ihl	= sizeof(*iph) >> 2;
 	iph->frag_off	= ip_mtu_locked(&rt->dst) ? 0 : frag_off;
-	iph->protocol	= tuple->tun.l3_proto;
+	iph->protocol	= tuple->tun.inner_proto;
 	iph->tos	= tos;
 	iph->daddr	= tuple->tun.src_v4.s_addr;
 	iph->saddr	= tuple->tun.dst_v4.s_addr;
diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
index 5f166da3b09b..1e55644f2edb 100644
--- a/net/netfilter/nf_flow_table_path.c
+++ b/net/netfilter/nf_flow_table_path.c
@@ -133,7 +133,7 @@ static int nft_dev_path_info(struct net_device_path_stack *stack,
 
 				info->tun.src_v6 = path->tun.src_v6;
 				info->tun.dst_v6 = path->tun.dst_v6;
-				info->tun.l3_proto = path->tun.l3_proto;
+				info->tun.inner_proto = path->tun.inner_proto;
 				info->tun_dst = path->tun.dst;
 				info->num_tuns++;
 			} else {
@@ -245,7 +245,7 @@ static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
 	if (info.num_tuns) {
 		route->tuple[!dir].in.tun.src_v6 = info.tun.dst_v6;
 		route->tuple[!dir].in.tun.dst_v6 = info.tun.src_v6;
-		route->tuple[!dir].in.tun.l3_proto = info.tun.l3_proto;
+		route->tuple[!dir].in.tun.inner_proto = info.tun.inner_proto;
 		route->tuple[!dir].in.num_tuns = info.num_tuns;
 		dst_release(route->tuple[dir].dst);
 		route->tuple[dir].dst = info.tun_dst;
-- 
2.47.3

[PATCH net-next 05/12] netfilter: flowtable: rename ctx.tun.proto to ctx.tun.inner_proto

[Pablo Neira Ayuso]

For consistency with the tun.l3proto rename, use same name field.
No functional changes are intended.

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_ip.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index a03946a5c2e7..7692ae7aa853 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -153,7 +153,7 @@ struct nf_flowtable_ctx {
 		/* Tunnel IP header size */
 		u32 hdr_size;
 		/* IP tunnel protocol */
-		u8 proto;
+		u8 inner_proto;
 	} tun;
 };
 
@@ -194,7 +194,7 @@ static void nf_flow_tuple_encap(struct nf_flowtable_ctx *ctx,
 	switch (inner_proto) {
 	case htons(ETH_P_IP):
 		iph = (struct iphdr *)(skb_network_header(skb) + offset);
-		if (ctx->tun.proto == IPPROTO_IPIP) {
+		if (ctx->tun.inner_proto == IPPROTO_IPIP) {
 			tuple->tun.dst_v4.s_addr = iph->daddr;
 			tuple->tun.src_v4.s_addr = iph->saddr;
 			tuple->tun.inner_proto = IPPROTO_IPIP;
@@ -202,7 +202,7 @@ static void nf_flow_tuple_encap(struct nf_flowtable_ctx *ctx,
 		break;
 	case htons(ETH_P_IPV6):
 		ip6h = (struct ipv6hdr *)(skb_network_header(skb) + offset);
-		if (ctx->tun.proto == IPPROTO_IPV6) {
+		if (ctx->tun.inner_proto == IPPROTO_IPV6) {
 			tuple->tun.dst_v6 = ip6h->daddr;
 			tuple->tun.src_v6 = ip6h->saddr;
 			tuple->tun.inner_proto = IPPROTO_IPV6;
@@ -329,7 +329,7 @@ static bool nf_flow_ip4_tunnel_proto(struct nf_flowtable_ctx *ctx,
 		return false;
 
 	if (iph->protocol == IPPROTO_IPIP) {
-		ctx->tun.proto = iph->protocol;
+		ctx->tun.inner_proto = iph->protocol;
 		ctx->tun.hdr_size = size;
 		ctx->offset += ctx->tun.hdr_size;
 	}
@@ -354,7 +354,7 @@ static bool nf_flow_ip6_tunnel_proto(struct nf_flowtable_ctx *ctx,
 		return false;
 
 	if (ip6h->nexthdr == IPPROTO_IPV6) {
-		ctx->tun.proto = ip6h->nexthdr;
+		ctx->tun.inner_proto = ip6h->nexthdr;
 		ctx->tun.hdr_size = sizeof(*ip6h);
 		ctx->offset += ctx->tun.hdr_size;
 	}
@@ -368,8 +368,8 @@ static bool nf_flow_ip6_tunnel_proto(struct nf_flowtable_ctx *ctx,
 static void nf_flow_ip_tunnel_pop(struct nf_flowtable_ctx *ctx,
 				  struct sk_buff *skb)
 {
-	if (ctx->tun.proto != IPPROTO_IPIP &&
-	    ctx->tun.proto != IPPROTO_IPV6)
+	if (ctx->tun.inner_proto != IPPROTO_IPIP &&
+	    ctx->tun.inner_proto != IPPROTO_IPV6)
 		return;
 
 	skb_pull(skb, ctx->tun.hdr_size);
-- 
2.47.3

[PATCH net-next 06/12] netfilter: flowtable: store ethertype in flowtable context

[Pablo Neira Ayuso]

Add a new field to store the ethertype of the packet, skipping layer 2
encapsulation. Store the ether_type in the context after parsing the
layer 2 header for the first time and then use it later on.

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_ip.c | 47 +++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index 7692ae7aa853..3f417a43bd12 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -147,6 +147,7 @@ static bool ip_has_options(unsigned int thoff)
 
 struct nf_flowtable_ctx {
 	const struct net_device	*in;
+	__be16			ether_type;
 	u32			offset;
 	u32			hdrsize;
 	struct {
@@ -161,7 +162,6 @@ static void nf_flow_tuple_encap(struct nf_flowtable_ctx *ctx,
 				struct sk_buff *skb,
 				struct flow_offload_tuple *tuple)
 {
-	__be16 inner_proto = skb->protocol;
 	struct vlan_ethhdr *veth;
 	struct pppoe_hdr *phdr;
 	struct ipv6hdr *ip6h;
@@ -179,19 +179,17 @@ static void nf_flow_tuple_encap(struct nf_flowtable_ctx *ctx,
 		veth = (struct vlan_ethhdr *)skb_mac_header(skb);
 		tuple->encap[i].id = ntohs(veth->h_vlan_TCI);
 		tuple->encap[i].proto = skb->protocol;
-		inner_proto = veth->h_vlan_encapsulated_proto;
 		offset += VLAN_HLEN;
 		break;
 	case htons(ETH_P_PPP_SES):
 		phdr = (struct pppoe_hdr *)skb_network_header(skb);
 		tuple->encap[i].id = ntohs(phdr->sid);
 		tuple->encap[i].proto = skb->protocol;
-		inner_proto = *((__be16 *)(phdr + 1));
 		offset += PPPOE_SES_HLEN;
 		break;
 	}
 
-	switch (inner_proto) {
+	switch (ctx->ether_type) {
 	case htons(ETH_P_IP):
 		iph = (struct iphdr *)(skb_network_header(skb) + offset);
 		if (ctx->tun.inner_proto == IPPROTO_IPIP) {
@@ -377,10 +375,10 @@ static void nf_flow_ip_tunnel_pop(struct nf_flowtable_ctx *ctx,
 }
 
 static bool nf_flow_skb_encap_protocol(struct nf_flowtable_ctx *ctx,
-				       struct sk_buff *skb, __be16 proto)
+				       struct sk_buff *skb)
 {
-	__be16 inner_proto = skb->protocol;
 	struct vlan_ethhdr *veth;
+	__be16 ether_type;
 	bool ret = false;
 
 	switch (skb->protocol) {
@@ -389,22 +387,27 @@ static bool nf_flow_skb_encap_protocol(struct nf_flowtable_ctx *ctx,
 			return false;
 
 		veth = (struct vlan_ethhdr *)skb_mac_header(skb);
-		if (veth->h_vlan_encapsulated_proto == proto) {
-			ctx->offset += VLAN_HLEN;
-			inner_proto = proto;
-			ret = true;
-		}
+		ctx->ether_type = veth->h_vlan_encapsulated_proto;
+		ctx->offset += VLAN_HLEN;
+		ret = true;
 		break;
 	case htons(ETH_P_PPP_SES):
-		if (nf_flow_pppoe_proto(skb, &inner_proto) &&
-		    inner_proto == proto) {
-			ctx->offset += PPPOE_SES_HLEN;
-			ret = true;
-		}
+		if (!nf_flow_pppoe_proto(skb, &ether_type))
+			return false;
+
+		ctx->ether_type = ether_type;
+		ctx->offset += PPPOE_SES_HLEN;
+		ret = true;
+		break;
+	case htons(ETH_P_IP):
+	case htons(ETH_P_IPV6):
+		ctx->ether_type = skb->protocol;
 		break;
+	default:
+		return false;
 	}
 
-	switch (inner_proto) {
+	switch (ctx->ether_type) {
 	case htons(ETH_P_IP):
 		ret = nf_flow_ip4_tunnel_proto(ctx, skb);
 		break;
@@ -456,7 +459,10 @@ nf_flow_offload_lookup(struct nf_flowtable_ctx *ctx,
 {
 	struct flow_offload_tuple tuple = {};
 
-	if (!nf_flow_skb_encap_protocol(ctx, skb, htons(ETH_P_IP)))
+	if (!nf_flow_skb_encap_protocol(ctx, skb))
+		return NULL;
+
+	if (unlikely(ctx->ether_type != htons(ETH_P_IP)))
 		return NULL;
 
 	if (nf_flow_tuple_ip(ctx, skb, &tuple) < 0)
@@ -1103,7 +1109,10 @@ nf_flow_offload_ipv6_lookup(struct nf_flowtable_ctx *ctx,
 {
 	struct flow_offload_tuple tuple = {};
 
-	if (!nf_flow_skb_encap_protocol(ctx, skb, htons(ETH_P_IPV6)))
+	if (!nf_flow_skb_encap_protocol(ctx, skb))
+		return NULL;
+
+	if (unlikely(ctx->ether_type != htons(ETH_P_IPV6)))
 		return NULL;
 
 	if (nf_flow_tuple_ipv6(ctx, skb, &tuple) < 0)
-- 
2.47.3

Re: [PATCH net-next 06/12] netfilter: flowtable: store ethertype in flowtable context

[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: store ethertype in flowtable context

This adds an ether_type field to struct nf_flowtable_ctx, sets it while
parsing the layer 2 header in nf_flow_skb_encap_protocol(), and replaces
the local inner_proto computations in nf_flow_tuple_encap() and
nf_flow_skb_encap_protocol() with reads of ctx->ether_type.  The IPv4 and
IPv6 lookup helpers now compare ctx->ether_type themselves instead of
passing an expected protocol down.

No Link: tags in the commit header.

> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index 7692ae7aa8539..3f417a43bd122 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -161,7 +162,6 @@ static void nf_flow_tuple_encap(struct nf_flowtable_ctx *ctx,
>  				struct sk_buff *skb,
>  				struct flow_offload_tuple *tuple)
>  {
> -	__be16 inner_proto = skb->protocol;
>  	struct vlan_ethhdr *veth;
>  	struct pppoe_hdr *phdr;
>  	struct ipv6hdr *ip6h;
> @@ -179,19 +179,17 @@ static void nf_flow_tuple_encap(struct nf_flowtable_ctx *ctx,
>  		veth = (struct vlan_ethhdr *)skb_mac_header(skb);
>  		tuple->encap[i].id = ntohs(veth->h_vlan_TCI);
>  		tuple->encap[i].proto = skb->protocol;
> -		inner_proto = veth->h_vlan_encapsulated_proto;
>  		offset += VLAN_HLEN;
>  		break;
>  	case htons(ETH_P_PPP_SES):
>  		phdr = (struct pppoe_hdr *)skb_network_header(skb);
>  		tuple->encap[i].id = ntohs(phdr->sid);
>  		tuple->encap[i].proto = skb->protocol;
> -		inner_proto = *((__be16 *)(phdr + 1));
>  		offset += PPPOE_SES_HLEN;
>  		break;
>  	}
>  
> -	switch (inner_proto) {
> +	switch (ctx->ether_type) {
>  	case htons(ETH_P_IP):
>  		iph = (struct iphdr *)(skb_network_header(skb) + offset);
>  		if (ctx->tun.inner_proto == IPPROTO_IPIP) {

[Severity: Medium]

Does this hunk change behaviour for PPPoE frames rather than just moving
the ethertype into the context?

Before this patch, the PPPoE arm fed the raw PPP protocol id into the
switch:

net/netfilter/nf_flow_table_ip.c:nf_flow_tuple_encap() {
	case htons(ETH_P_PPP_SES):
		...
		inner_proto = *((__be16 *)(phdr + 1));
		offset += PPPOE_SES_HLEN;
		break;
	}

	switch (inner_proto) {
	case htons(ETH_P_IP):
	...
}

PPP_IP is 0x0021 and PPP_IPV6 is 0x0057, so inner_proto could never equal
htons(ETH_P_IP) (0x0800) or htons(ETH_P_IPV6) (0x86dd), and PPPoE frames
always landed in the default: arm.  That left
tuple->tun.{src_v4,dst_v4,src_v6,dst_v6} and tuple->tun.inner_proto zero
even when nf_flow_skb_encap_protocol() -> nf_flow_ip4_tunnel_proto() had
already set ctx->tun.inner_proto = IPPROTO_IPIP.

After the patch, ctx->ether_type for PPPoE is the value normalised by
__nf_flow_pppoe_proto() in include/net/netfilter/nf_flow_table.h:

	switch (proto) {
	case htons(PPP_IP):
		return htons(ETH_P_IP);
	case htons(PPP_IPV6):
		return htons(ETH_P_IPV6);
	}

so these tunnel arms now execute for PPPoE traffic and populate
tuple->tun.

struct flow_offload_tunnel tun sits before the __hash marker in struct
flow_offload_tuple, so it is part of the rhashtable lookup key, and the
control path always installs non-zero endpoints for tunnel flows:

net/netfilter/nf_flow_table_core.c:flow_offload_fill_tuple() {
	flow_tuple->tun = route->tuple[dir].in.tun;
	flow_tuple->encap_num = route->tuple[dir].in.num_encaps;
}

filled from DEV_PATH_TUN in nft_dev_path_info(), which can be walked
together with DEV_PATH_PPPOE in the same path stack (ipip or ip6tnl over a
PPPoE underlay).  That means PPPoE encapsulated IPIP/IP6IP6 rx flows
previously missed their flowtable entry on every packet and now start
matching.

Should the commit message mention this, and should a Fixes: tag be added
pointing at ab427db17885 ("netfilter: flowtable: Add IPIP rx sw
acceleration") and d98103575dcd ("netfilter: flowtable: Add IP6IP6 rx sw
acceleration"), which introduced the raw PPP id versus ETH_P_* comparison?
As written, the message only says "Store the ether_type in the context
after parsing the layer 2 header for the first time and then use it later
on", which reads as a pure cleanup.

The behaviour persists to the end of the series (nf_flow_tuple_encap() at
839e6caf13c7 still switches on ctx->ether_type), so no later patch
documents or reverts it.

> @@ -389,22 +387,27 @@ static bool nf_flow_skb_encap_protocol(struct nf_flowtable_ctx *ctx,
>  			return false;
>  
>  		veth = (struct vlan_ethhdr *)skb_mac_header(skb);
> -		if (veth->h_vlan_encapsulated_proto == proto) {
> -			ctx->offset += VLAN_HLEN;
> -			inner_proto = proto;
> -			ret = true;
> -		}
> +		ctx->ether_type = veth->h_vlan_encapsulated_proto;
> +		ctx->offset += VLAN_HLEN;
> +		ret = true;
>  		break;
>  	case htons(ETH_P_PPP_SES):
> -		if (nf_flow_pppoe_proto(skb, &inner_proto) &&
> -		    inner_proto == proto) {
> -			ctx->offset += PPPOE_SES_HLEN;
> -			ret = true;
> -		}
> +		if (!nf_flow_pppoe_proto(skb, &ether_type))
> +			return false;
> +
> +		ctx->ether_type = ether_type;
> +		ctx->offset += PPPOE_SES_HLEN;
> +		ret = true;
> +		break;

This is the store of the normalised ETH_P_* value that nf_flow_tuple_encap()
later consumes for PPPoE frames.

[ ... ]

[PATCH net-next 07/12] netfilter: flowtable: move ipv4 and ipv6 xmit path to function

[Pablo Neira Ayuso]

Move the existing ipv4 and ipv6 transmit path to functions in
preparation of the IPv4 over IPv6 and SIT support.

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_ip.c | 92 +++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 38 deletions(-)

diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index 3f417a43bd12..ff45f17f3c4a 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -801,33 +801,17 @@ static unsigned int nf_flow_queue_xmit(struct net *net, struct sk_buff *skb,
 	return NF_STOLEN;
 }
 
-unsigned int
-nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
-			const struct nf_hook_state *state)
+static int nf_flow_queue_xmit4(struct sk_buff *skb,
+			       struct flow_offload_tuple_rhash *tuplehash,
+			       const struct nf_hook_state *state)
 {
-	struct flow_offload_tuple_rhash *tuplehash;
-	struct nf_flowtable *flow_table = priv;
 	struct flow_offload_tuple *other_tuple;
 	enum flow_offload_tuple_dir dir;
-	struct nf_flowtable_ctx ctx = {
-		.in	= state->in,
-	};
 	struct nf_flow_xmit xmit = {};
 	struct flow_offload *flow;
 	struct neighbour *neigh;
 	struct rtable *rt;
 	__be32 ip_daddr;
-	int ret;
-
-	tuplehash = nf_flow_offload_lookup(&ctx, flow_table, skb);
-	if (!tuplehash)
-		return NF_ACCEPT;
-
-	ret = nf_flow_offload_forward(&ctx, flow_table, tuplehash, skb);
-	if (ret < 0)
-		return NF_DROP;
-	else if (ret == 0)
-		return NF_ACCEPT;
 
 	if (unlikely(tuplehash->tuple.xmit_type == FLOW_OFFLOAD_XMIT_XFRM)) {
 		rt = dst_rtable(tuplehash->tuple.dst_cache);
@@ -881,6 +865,30 @@ nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
 
 	return nf_flow_queue_xmit(state->net, skb, &xmit);
 }
+
+unsigned int
+nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
+			const struct nf_hook_state *state)
+{
+	struct flow_offload_tuple_rhash *tuplehash;
+	struct nf_flowtable *flow_table = priv;
+	struct nf_flowtable_ctx ctx = {
+		.in	= state->in,
+	};
+	int ret;
+
+	tuplehash = nf_flow_offload_lookup(&ctx, flow_table, skb);
+	if (!tuplehash)
+		return NF_ACCEPT;
+
+	ret = nf_flow_offload_forward(&ctx, flow_table, tuplehash, skb);
+	if (ret < 0)
+		return NF_DROP;
+	else if (ret == 0)
+		return NF_ACCEPT;
+
+	return nf_flow_queue_xmit4(skb, tuplehash, state);
+}
 EXPORT_SYMBOL_GPL(nf_flow_offload_ip_hook);
 
 static void nf_flow_nat_ipv6_tcp(struct sk_buff *skb, unsigned int thoff,
@@ -1121,33 +1129,17 @@ nf_flow_offload_ipv6_lookup(struct nf_flowtable_ctx *ctx,
 	return flow_offload_lookup(flow_table, &tuple);
 }
 
-unsigned int
-nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
-			  const struct nf_hook_state *state)
+static int nf_flow_queue_xmit6(struct sk_buff *skb,
+			       struct flow_offload_tuple_rhash *tuplehash,
+			       const struct nf_hook_state *state)
 {
-	struct flow_offload_tuple_rhash *tuplehash;
-	struct nf_flowtable *flow_table = priv;
 	struct flow_offload_tuple *other_tuple;
 	enum flow_offload_tuple_dir dir;
-	struct nf_flowtable_ctx ctx = {
-		.in	= state->in,
-	};
 	struct nf_flow_xmit xmit = {};
 	struct in6_addr *ip6_daddr;
 	struct flow_offload *flow;
 	struct neighbour *neigh;
 	struct rt6_info *rt;
-	int ret;
-
-	tuplehash = nf_flow_offload_ipv6_lookup(&ctx, flow_table, skb);
-	if (tuplehash == NULL)
-		return NF_ACCEPT;
-
-	ret = nf_flow_offload_ipv6_forward(&ctx, flow_table, tuplehash, skb);
-	if (ret < 0)
-		return NF_DROP;
-	else if (ret == 0)
-		return NF_ACCEPT;
 
 	if (unlikely(tuplehash->tuple.xmit_type == FLOW_OFFLOAD_XMIT_XFRM)) {
 		rt = dst_rt6_info(tuplehash->tuple.dst_cache);
@@ -1202,4 +1194,28 @@ nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
 
 	return nf_flow_queue_xmit(state->net, skb, &xmit);
 }
+
+unsigned int
+nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
+			  const struct nf_hook_state *state)
+{
+	struct flow_offload_tuple_rhash *tuplehash;
+	struct nf_flowtable *flow_table = priv;
+	struct nf_flowtable_ctx ctx = {
+		.in	= state->in,
+	};
+	int ret;
+
+	tuplehash = nf_flow_offload_ipv6_lookup(&ctx, flow_table, skb);
+	if (!tuplehash)
+		return NF_ACCEPT;
+
+	ret = nf_flow_offload_ipv6_forward(&ctx, flow_table, tuplehash, skb);
+	if (ret < 0)
+		return NF_DROP;
+	else if (ret == 0)
+		return NF_ACCEPT;
+
+	return nf_flow_queue_xmit6(skb, tuplehash, state);
+}
 EXPORT_SYMBOL_GPL(nf_flow_offload_ipv6_hook);
-- 
2.47.3

[PATCH net-next 08/12] netfilter: flowtable: detach layer 2 encapsulation parser from lookup

[Pablo Neira Ayuso]

Move the layer 2 encapsulation header parser out of the lookup function
to prepare for IPv4 over IPv6 and SIT.

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_ip.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index ff45f17f3c4a..c8c29a9a1684 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -459,12 +459,6 @@ nf_flow_offload_lookup(struct nf_flowtable_ctx *ctx,
 {
 	struct flow_offload_tuple tuple = {};
 
-	if (!nf_flow_skb_encap_protocol(ctx, skb))
-		return NULL;
-
-	if (unlikely(ctx->ether_type != htons(ETH_P_IP)))
-		return NULL;
-
 	if (nf_flow_tuple_ip(ctx, skb, &tuple) < 0)
 		return NULL;
 
@@ -877,6 +871,12 @@ nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
 	};
 	int ret;
 
+	if (!nf_flow_skb_encap_protocol(&ctx, skb))
+		return NF_ACCEPT;
+
+	if (unlikely(ctx.ether_type != htons(ETH_P_IP)))
+		return NF_ACCEPT;
+
 	tuplehash = nf_flow_offload_lookup(&ctx, flow_table, skb);
 	if (!tuplehash)
 		return NF_ACCEPT;
@@ -1117,12 +1117,6 @@ nf_flow_offload_ipv6_lookup(struct nf_flowtable_ctx *ctx,
 {
 	struct flow_offload_tuple tuple = {};
 
-	if (!nf_flow_skb_encap_protocol(ctx, skb))
-		return NULL;
-
-	if (unlikely(ctx->ether_type != htons(ETH_P_IPV6)))
-		return NULL;
-
 	if (nf_flow_tuple_ipv6(ctx, skb, &tuple) < 0)
 		return NULL;
 
@@ -1206,6 +1200,12 @@ nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
 	};
 	int ret;
 
+	if (!nf_flow_skb_encap_protocol(&ctx, skb))
+		return NF_ACCEPT;
+
+	if (unlikely(ctx.ether_type != htons(ETH_P_IPV6)))
+		return NF_ACCEPT;
+
 	tuplehash = nf_flow_offload_ipv6_lookup(&ctx, flow_table, skb);
 	if (!tuplehash)
 		return NF_ACCEPT;
-- 
2.47.3

[PATCH net-next 09/12] 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 unconfirmed 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.

I'd rather not increase the size of the struct nf_conn for this feature
has very limited scope: only one expectation can be created at a time
given expect_clash() will make nf_ct_expect_related() reports EBUSY.
For this reason, relax nf_ct_expect_related() not to drop packets in
case expectation creation fails, therefore, expectation creation becomes
best effort.

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_release() function to release the
internal helper that is allocated and attached to the conntrack entry to
create the custom expectations. The nft_ct module removal always waits
for rcu grace period, then the NULL helper callback is observed after
this.

This patch also restricts the creation of expectations to different
helpers other than this custom helper that is created for this type of
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 |   1 +
 net/netfilter/nf_conntrack_helper.c         |  14 +-
 net/netfilter/nft_ct.c                      | 167 +++++++++++++++-----
 3 files changed, 135 insertions(+), 47 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index bc5427d239f4..335b8c43694f 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -106,6 +106,7 @@ void nf_ct_helper_init(struct nf_conntrack_helper *helper,
 int nf_conntrack_helper_register(struct nf_conntrack_helper *, struct nf_conntrack_helper **);
 int __nf_conntrack_helper_register(struct nf_conntrack_helper *);
 void nf_conntrack_helper_unregister(struct nf_conntrack_helper *);
+void nf_conntrack_helper_release(struct nf_conntrack_helper *);
 
 int nf_conntrack_helpers_register(struct nf_conntrack_helper *, unsigned int,
 				  struct nf_conntrack_helper **);
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 506c58034761..c30ae3f203be 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -448,6 +448,15 @@ static bool expect_iter_me(struct nf_conntrack_expect *exp, void *data)
 	return this == me;
 }
 
+void nf_conntrack_helper_release(struct nf_conntrack_helper *me)
+{
+	nf_ct_expect_iterate_destroy(expect_iter_me, me);
+
+	if (refcount_dec_and_test(&me->ct_refcnt))
+		kfree_rcu(me, rcu);
+}
+EXPORT_SYMBOL_GPL(nf_conntrack_helper_release);
+
 void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
 {
 	mutex_lock(&nf_ct_helper_mutex);
@@ -463,10 +472,7 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
 	 */
 	synchronize_rcu();
 
-	nf_ct_expect_iterate_destroy(expect_iter_me, me);
-
-	if (refcount_dec_and_test(&me->ct_refcnt))
-		kfree_rcu(me, rcu);
+	nf_conntrack_helper_release(me);
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_helper_unregister);
 
diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index 358b9287e12e..9dbf127df9c8 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,93 @@ static int nft_ct_expect_timeout_get(const struct nlattr *attr, u32 *val)
 	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;
+};
+
+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;
+	u16 l3num;
+
+	if (nf_ct_is_confirmed(ct))
+		return NF_ACCEPT;
+
+	expect_data = nfct_help_data(ct);
+	if (!expect_data)
+		return NF_ACCEPT;
+
+	if (expect_data->dir != dir)
+		return NF_ACCEPT;
+
+	exp = nf_ct_expect_alloc(ct);
+	if (!exp)
+		return NF_DROP;
+
+	if (expect_data->obj.l3num == NFPROTO_INET)
+		l3num = nf_ct_l3num(ct);
+	else
+		l3num = expect_data->obj.l3num;
+
+	nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT, l3num,
+			  &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)
+		ret = NF_ACCEPT;
+
+	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,
+				       GFP_KERNEL_ACCOUNT);
+	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 +1322,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] ||
@@ -1272,13 +1363,31 @@ 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]);
+	if (!priv->size)
+		priv->size = NF_CT_EXPECT_MAX_CNT;
+
+	err = nf_ct_netns_get(ctx->net, ctx->family);
+	if (err < 0)
+		return err;
 
-	return nf_ct_netns_get(ctx->net, ctx->family);
+	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);
+	struct nf_conntrack_helper *me = priv->helper;
+
+	/* This helper is going away, disable it. */
+	rcu_assign_pointer(me->help, NULL);
+	nf_conntrack_helper_release(me);
 	nf_ct_netns_put(ctx->net, ctx->family);
 }
 
@@ -1297,27 +1406,14 @@ static int nft_ct_expect_obj_dump(struct sk_buff *skb,
 	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
-
 static void nft_ct_expect_obj_eval(struct nft_object *obj,
 				   struct nft_regs *regs,
 				   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);
@@ -1325,45 +1421,30 @@ static void nft_ct_expect_obj_eval(struct nft_object *obj,
 		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 IS_ENABLED(CONFIG_NF_NAT)
-	if (ct->status & IPS_NAT_MASK) {
-		exp->saved_proto.tcp.port = priv->dport;
-		exp->dir = !dir;
-		exp->expectfn = nft_ct_nat_follow_master;
+	expect_data = nfct_help_data(ct);
+	if (!expect_data) {
+		regs->verdict.code = NFT_BREAK;
+		return;
 	}
-#endif
-	if (nf_ct_expect_related(exp, 0) != 0)
-		regs->verdict.code = NF_DROP;
+	expect_data->obj = *priv;
+	expect_data->obj.helper = NULL;
+	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-next 09/12] 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 moves the nft ct expectation support from creating the expectation
directly in the object eval path to allocating an internal (unregistered)
conntrack helper that is attached to the conntrack, so the expectation is
created from the helper callback at confirmation time. A new exported
nf_conntrack_helper_release() is added to tear down that internal helper.

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

A few questions below.

> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> index 506c580347615..c30ae3f203bee 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c
> @@ -448,6 +448,15 @@ static bool expect_iter_me(struct nf_conntrack_expect *exp, void *data)
>  	return this == me;
>  }
>  
> +void nf_conntrack_helper_release(struct nf_conntrack_helper *me)
> +{
> +	nf_ct_expect_iterate_destroy(expect_iter_me, me);
> +
> +	if (refcount_dec_and_test(&me->ct_refcnt))
> +		kfree_rcu(me, rcu);
> +}
> +EXPORT_SYMBOL_GPL(nf_conntrack_helper_release);
> +
>  void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
>  {
>  	mutex_lock(&nf_ct_helper_mutex);
> @@ -463,10 +472,7 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
>  	 */
>  	synchronize_rcu();
>  
> -	nf_ct_expect_iterate_destroy(expect_iter_me, me);
> -
> -	if (refcount_dec_and_test(&me->ct_refcnt))
> -		kfree_rcu(me, rcu);
> +	nf_conntrack_helper_release(me);
>  }
>  EXPORT_SYMBOL_GPL(nf_conntrack_helper_unregister);
>  
> diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> index 358b9287e12e8..9dbf127df9c81 100644
> --- a/net/netfilter/nft_ct.c
> +++ b/net/netfilter/nft_ct.c

[ ... ]

> @@ -1226,6 +1228,93 @@ static int nft_ct_expect_timeout_get(const struct nlattr *attr, u32 *val)
>  	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;
> +};
> +
> +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;
> +	u16 l3num;
> +
> +	if (nf_ct_is_confirmed(ct))
> +		return NF_ACCEPT;
> +
> +	expect_data = nfct_help_data(ct);
> +	if (!expect_data)
> +		return NF_ACCEPT;
> +
> +	if (expect_data->dir != dir)
> +		return NF_ACCEPT;
> +
> +	exp = nf_ct_expect_alloc(ct);
> +	if (!exp)
> +		return NF_DROP;
> +
> +	if (expect_data->obj.l3num == NFPROTO_INET)
> +		l3num = nf_ct_l3num(ct);
> +	else
> +		l3num = expect_data->obj.l3num;

[Severity: Low]
This isn't a bug introduced by this patch, but can obj.l3num ever be
NFPROTO_INET here?  nft_ct_expect_obj_init() rejects it:

	case NFPROTO_INET: /* tuple.src.l3num supports NFPROTO_IPV4/6 only */
	default:
		return -EAFNOSUPPORT;

so priv->l3num, and therefore the copy in expect_data->obj.l3num, can never
hold NFPROTO_INET and the nf_ct_l3num(ct) branch looks unreachable.  The same
code existed in the old nft_ct_expect_obj_eval() and was moved verbatim, so
this is only a cleanup note: either drop the branch or let init accept
NFPROTO_INET and resolve it per packet.

> +
> +	nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT, l3num,
> +			  &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)
> +		ret = NF_ACCEPT;
> +
> +	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,
> +				       GFP_KERNEL_ACCOUNT);
> +	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;
> +}
> +

[ ... ]

> @@ -1272,13 +1363,31 @@ 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]);
> +	if (!priv->size)
> +		priv->size = NF_CT_EXPECT_MAX_CNT;

[Severity: Medium]
Does this break the round-trip of NFTA_CT_EXPECT_SIZE?  priv->size is what
nft_ct_expect_obj_dump() echoes back:

	    nla_put_u8(skb, NFTA_CT_EXPECT_SIZE, priv->size))

so an object created with size 0 is now listed as size 255.  The policy
[NFTA_CT_EXPECT_SIZE] = { .type = NLA_U8 } accepts 0, so this is reachable
from userspace.

The meaning of size 0 also flips: the pre-patch eval tested

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

which is always true for 0, i.e. size 0 meant "never create expectations",
whereas it now means "allow the maximum".

Would it work to apply the substitution only where it is needed, i.e. to
expect_policy[NF_CT_EXPECT_CLASS_DEFAULT].max_expected in
nft_ct_expect_helper_alloc(), and keep the user-supplied value in
priv->size?  That is what __nf_conntrack_helper_register() does, but there
the clamped value lives in an internal policy struct that is never sent to
userspace.

Related: with the new scheme ct_expect_help() only runs while the conntrack
is unconfirmed and only for the single recorded direction, so at most one
expectation per conntrack is created and the size attribute has no
observable effect anymore.

> +
> +	err = nf_ct_netns_get(ctx->net, ctx->family);
> +	if (err < 0)
> +		return err;
>  
> -	return nf_ct_netns_get(ctx->net, ctx->family);
> +	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);
> +	struct nf_conntrack_helper *me = priv->helper;
> +
> +	/* This helper is going away, disable it. */
> +	rcu_assign_pointer(me->help, NULL);
> +	nf_conntrack_helper_release(me);
>  	nf_ct_netns_put(ctx->net, ctx->family);
>  }

[Severity: High]
Is a grace period needed between clearing me->help and sweeping the
expectation table here?  nf_conntrack_helper_unregister() does exactly these
two steps with a synchronize_rcu() in between:

	rcu_assign_pointer(me->help, NULL);

	/* Make sure every nothing is still using the helper unless its a
	 * connection in the hash.
	 */
	synchronize_rcu();

	nf_conntrack_helper_release(me);

The refactor left the grace period in the unregister wrapper, so the new
external caller does not get it.

Can an in-flight ct_expect_help() then insert an expectation after the sweep
has finished?

CPU A (softirq), nf_confirm():
	helper_cb = rcu_dereference(helper->help);	/* still ct_expect_help */
	...
	ret = helper_cb(skb, protoff, ct, ctinfo);
	  ct_expect_help()
	    nf_ct_expect_alloc() / nf_ct_expect_init()

CPU B (process context), object delete:
	nft_ct_expect_obj_destroy()
	  rcu_assign_pointer(me->help, NULL);
	  nf_conntrack_helper_release()
	    nf_ct_expect_iterate_destroy(expect_iter_me, me);  /* sweeps */

CPU A continues:
	    nf_ct_expect_related(exp, 0)	/* takes nf_conntrack_expect_lock
						   after the sweep, inserts */

rcu_assign_pointer() does not wait for a reader that already loaded the old
pointer, and the callback is reached through the conntrack's own
help->helper in nf_confirm() (and nf_ct_helper() in nf_conntrack_ovs.c), not
through the nft object, so the nf_tables commit-time synchronize_rcu() that
precedes ops->destroy() does not fence it.

Since this sweep is the only teardown for these expectations, would the
escaped expectation (exp->helper == me, and exp->expectfn ==
nft_ct_nat_follow_master when NAT is in use) then keep admitting a RELATED
connection after the object/rule is gone, until its timeout expires or the
master conntrack dies?

The commit message says:

    The nft_ct module removal always waits for rcu grace period, then the
    NULL helper callback is observed after this.

but nft_ct_expect_obj_destroy() itself does not wait; object destruction is
not module removal.  Would keeping the unregister ordering here (or folding
the synchronize_rcu() into nf_conntrack_helper_release()) be preferable?

>  
> @@ -1297,27 +1406,14 @@ static int nft_ct_expect_obj_dump(struct sk_buff *skb,

[ ... ]

>  static void nft_ct_expect_obj_eval(struct nft_object *obj,
>  				   struct nft_regs *regs,
>  				   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);
> @@ -1325,45 +1421,30 @@ static void nft_ct_expect_obj_eval(struct nft_object *obj,
>  		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: Medium]
Is this change of behaviour intended?  The old code reused an existing
nf_conn_help extension; now any conntrack that already carries one gives up
with NFT_BREAK and no expectation is created at all.

A help extension can already be present on an unconfirmed conntrack from
several places, for example:

  - nft_ct_helper_obj_eval() calling nf_ct_helper_ext_add() for a preceding
    "ct helper set" statement
  - __nf_ct_try_assign_helper() from init_conntrack()
  - the exp->assign_helper path in init_conntrack():

	assign_helper = rcu_dereference(exp->assign_helper);
	if (assign_helper) {
		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);

  - ctnetlink created conntracks
  - a second "ct expectation set" statement

Since NFT_BREAK aborts the whole rule, do the statements following
"ct expectation set" (counter/log/accept/jump) get skipped too, so such
packets now fall through to the next rule or the chain policy?

Also, on the commit message:

    I'd rather not increase the size of the struct nf_conn for this feature
    has very limited scope: only one expectation can be created at a time
    given expect_clash() will make nf_ct_expect_related() reports EBUSY.

Is that accurate for the pre-patch code?  expect_clash() goes through
nf_ct_tuple_mask_cmp(), which compares the destination tuple including
dst.u.all, so two expectation objects with different dports on the same
master did not clash and could coexist up to priv->size.  Should the commit
message describe the single-expectation limit as something this patch
introduces rather than as existing behaviour?

> -	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;
>  	}

[ ... ]

> +	expect_data = nfct_help_data(ct);
> +	if (!expect_data) {
> +		regs->verdict.code = NFT_BREAK;
> +		return;
>  	}
> -#endif
> -	if (nf_ct_expect_related(exp, 0) != 0)
> -		regs->verdict.code = NF_DROP;
> +	expect_data->obj = *priv;
> +	expect_data->obj.helper = NULL;
> +	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);
>  }

[PATCH net-next 10/12] netfilter: conntrack: always lower timeout for non-closing RST packets

[Pablo Neira Ayuso]

The existing check might extend the timeout if the ESTABLISHED timeout
has been tuned to be lower than UNACK via sysctl. Reported by sashiko.

Fixes: bf80e6802273 ("netfilter: conntrack: tcp: use UNACK timeout for non-closing RST packets")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_proto_tcp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 723e946a78f4..1300c236dd41 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -1282,7 +1282,8 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 	    timeouts[new_state] > timeouts[TCP_CONNTRACK_RETRANS])
 		timeout = timeouts[TCP_CONNTRACK_RETRANS];
 	else if (unlikely(index == TCP_RST_SET &&
-			  new_state == TCP_CONNTRACK_ESTABLISHED))
+			  new_state == TCP_CONNTRACK_ESTABLISHED) &&
+		 timeouts[new_state] > timeouts[TCP_CONNTRACK_UNACK])
 		timeout = timeouts[TCP_CONNTRACK_UNACK];
 	else if ((ct->proto.tcp.seen[0].flags | ct->proto.tcp.seen[1].flags) &
 		 IP_CT_TCP_FLAG_DATA_UNACKNOWLEDGED &&
-- 
2.47.3

[PATCH net-next 11/12] netfilter: nf_conntrack_expect: bail out on insert dead expectations

[Pablo Neira Ayuso]

If the NF_CT_EXPECT_DEAD expectation flag is set on, bail out on
insertion. Moreover, add also DEBUG_NET_WARN_ON_ONCE() since this should
not ever happen. This is hardening commit b8b09dc2bf35 ("netfilter:
nf_conntrack_expect: use conntrack GC to reap expectations").

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_expect.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 10b130a7b230..f1f0c582db5d 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -528,6 +528,12 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
 	int ret;
 
 	spin_lock_bh(&nf_conntrack_expect_lock);
+	if (expect->flags & NF_CT_EXPECT_DEAD) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		ret = -EINVAL;
+		goto out;
+	}
+
 	master_help = nfct_help(expect->master);
 	if (!master_help) {
 		ret = -ESHUTDOWN;
-- 
2.47.3

Re: [PATCH net-next 11/12] netfilter: nf_conntrack_expect: bail out on insert dead expectations

[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_conntrack_expect: bail out on insert dead expectations

This adds a check in nf_ct_expect_related_report() that refuses insertion
of an expectation carrying NF_CT_EXPECT_DEAD, returning -EINVAL and
triggering DEBUG_NET_WARN_ON_ONCE(). It is described as hardening for
b8b09dc2bf35 ("netfilter: nf_conntrack_expect: use conntrack GC to reap
expectations").

A couple of questions about the placement of the new check and about the
DEAD flag itself.

> diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
> index 10b130a7b230b..f1f0c582db5df 100644
> --- a/net/netfilter/nf_conntrack_expect.c
> +++ b/net/netfilter/nf_conntrack_expect.c
> @@ -528,6 +528,12 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
>  	int ret;
>  
>  	spin_lock_bh(&nf_conntrack_expect_lock);
> +	if (expect->flags & NF_CT_EXPECT_DEAD) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +

[Severity: Low]
Would it make sense to put this check in __nf_ct_expect_check() instead?

The other exported insertion entry point, nf_ct_expect_related_pair(),
goes straight from the lock to __nf_ct_expect_check() and
nf_ct_expect_insert() with no NF_CT_EXPECT_DEAD test:

net/netfilter/nf_conntrack_expect.c:nf_ct_expect_related_pair() {
	spin_lock_bh(&nf_conntrack_expect_lock);
	master_help = nfct_help(expect[0]->master);
	if (!master_help || master_help != nfct_help(expect[1]->master)) {
		ret = -EINVAL;
		goto out;
	}

	for (i = 0; i < 2; i++) {
		ret = __nf_ct_expect_check(expect[i], master_help, flags);
	...
}

Since nf_ct_unexpect_related() now only sets the bit and leaves the
expectation linked on hnode/lnode until GC runs:

net/netfilter/nf_conntrack_expect.c:nf_ct_unexpect_related() {
	spin_lock_bh(&nf_conntrack_expect_lock);
	WRITE_ONCE(exp->flags, exp->flags | NF_CT_EXPECT_DEAD);
	spin_unlock_bh(&nf_conntrack_expect_lock);
}

a resubmission of a DEAD-but-still-linked expectation on the pair path
would reach nf_ct_expect_insert() and call hlist_add_head_rcu() on
already-linked nodes, plus double-count cnet->expect_count and
master_help->expecting[class].

The gap in the pair path is pre-existing and not introduced by this patch,
and I could not find any in-tree caller that actually reaches it. The two
pair callers, nf_nat_sdp_media() in net/netfilter/nf_nat_sip.c and
nat_rtp_rtcp() in net/ipv4/netfilter/nf_nat_h323.c, allocate the
expectations per packet with flags cleared, retry only on -EBUSY (before
anything is published), and call nf_ct_unexpect_related() only on the
terminal error exits that return immediately. When the tuple hash is
unchanged, __nf_ct_expect_check()'s own expired-entry loop would also
unlink the stale entry first.

Would it still be worth making the invariant symmetric, given the commit
message says this should not ever happen?

Separately, on the DEAD flag itself:

[Severity: Medium]
This is a pre-existing issue rather than something introduced here, but
since this patch is tightening the semantics of the same flag: is the
kernel-private NF_CT_EXPECT_DEAD bit supposed to be visible over ctnetlink?

The uAPI header keeps it out of the userspace-visible mask:

include/uapi/linux/netfilter/nf_conntrack_common.h {
#ifdef __KERNEL__
#define NF_CT_EXPECT_DEAD		0x8
#define NF_CT_EXPECT_MASK	(NF_CT_EXPECT_PERMANENT | NF_CT_EXPECT_INACTIVE | \
				 NF_CT_EXPECT_USERSPACE)
#endif
}

and the input policy enforces that mask, so a request carrying 0x8 is
rejected with -EINVAL:

net/netfilter/nf_conntrack_netlink.c:exp_nla_policy {
	[CTA_EXPECT_FLAGS]	= NLA_POLICY_MASK(NLA_BE32, NF_CT_EXPECT_MASK),
}

But the dump path exports the raw value:

net/netfilter/nf_conntrack_netlink.c:ctnetlink_exp_dump_expect() {
	    nla_put_be32(skb, CTA_EXPECT_FLAGS, htonl(exp->flags)) ||
}

Because the expectation now stays linked after nf_ct_unexpect_related()
sets the bit, the value can be observed with 0x8 set through the
IPEXP_DESTROY event from nf_ct_unlink_expect_report(), and through
ctnetlink_exp_ct_dump_table(), which walks help->expectations without the
nf_ct_exp_is_expired() filter that ctnetlink_exp_dump_table() and
__nf_ct_expect_find() apply:

net/netfilter/nf_conntrack_netlink.c:ctnetlink_exp_ct_dump_table() {
	hlist_for_each_entry_rcu(exp, &help->expectations, lnode) {
		if (l3proto && exp->tuple.src.l3num != l3proto)
			continue;
	...
}

A tool that reads CTA_EXPECT_FLAGS and replays it gets a value the same
kernel refuses on input. Would masking with NF_CT_EXPECT_MASK in
ctnetlink_exp_dump_expect() be appropriate here?

This behaviour dates back to b8b09dc2bf35 and is unchanged through the end
of the series.

[PATCH net-next 12/12] selftests: netfilter: conntrack_dump_flush: remove unused variables and fix typo

[Pablo Neira Ayuso]

From: Qingshuang Fu <fuqingshuang@kylinos.cn>

Remove unused 'rplnlh' in conntrack_data_insert(), and remove unused
'rplnlh' and 'nest' variables in conntrack_count_zone() and
conntrack_flush_zone(). These variables were declared but never used
since their introduction.

Also fix typo: rename misspelled conntracK_count_zone() to
conntrack_count_zone().

Signed-off-by: Qingshuang Fu <fuqingshuang@kylinos.cn>
Reviewed-by: Fernando Fernandez Mancera <fmancera@suse.de>
Reviewed-by: Hangbin Liu <liuhangbin@kylinos.cn>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 .../net/netfilter/conntrack_dump_flush.c      | 31 +++++++++----------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/net/netfilter/conntrack_dump_flush.c b/tools/testing/selftests/net/netfilter/conntrack_dump_flush.c
index 5cecb8a1bc94..31b8250ddc53 100644
--- a/tools/testing/selftests/net/netfilter/conntrack_dump_flush.c
+++ b/tools/testing/selftests/net/netfilter/conntrack_dump_flush.c
@@ -102,7 +102,6 @@ static int conntrack_data_insert(struct mnl_socket *sock, struct nlmsghdr *nlh,
 				 uint16_t zone)
 {
 	char buf[MNL_SOCKET_BUFFER_SIZE];
-	struct nlmsghdr *rplnlh;
 	unsigned int portid;
 	int ret;
 
@@ -216,12 +215,11 @@ static int count_entries(const struct nlmsghdr *nlh, void *data)
 	return MNL_CB_OK;
 }
 
-static int conntracK_count_zone(struct mnl_socket *sock, uint16_t zone)
+static int conntrack_count_zone(struct mnl_socket *sock, uint16_t zone)
 {
 	char buf[MNL_SOCKET_BUFFER_SIZE];
-	struct nlmsghdr *nlh, *rplnlh;
+	struct nlmsghdr *nlh;
 	struct nfgenmsg *nfh;
-	struct nlattr *nest;
 	unsigned int portid;
 	int ret;
 
@@ -266,9 +264,8 @@ static int conntracK_count_zone(struct mnl_socket *sock, uint16_t zone)
 static int conntrack_flush_zone(struct mnl_socket *sock, uint16_t zone)
 {
 	char buf[MNL_SOCKET_BUFFER_SIZE];
-	struct nlmsghdr *nlh, *rplnlh;
+	struct nlmsghdr *nlh;
 	struct nfgenmsg *nfh;
-	struct nlattr *nest;
 	unsigned int portid;
 	int ret;
 
@@ -326,7 +323,7 @@ FIXTURE_SETUP(conntrack_dump_flush)
 	ret = mnl_socket_bind(self->sock, 0, MNL_SOCKET_AUTOPID);
 	EXPECT_EQ(ret, 0);
 
-	ret = conntracK_count_zone(self->sock, TEST_ZONE_ID);
+	ret = conntrack_count_zone(self->sock, TEST_ZONE_ID);
 	if (ret < 0 && errno == EPERM)
 		SKIP(return, "Needs to be run as root");
 	else if (ret < 0 && errno == EOPNOTSUPP)
@@ -423,7 +420,7 @@ FIXTURE_SETUP(conntrack_dump_flush)
 					 NF_CT_DEFAULT_ZONE_ID);
 	EXPECT_EQ(ret, 0);
 
-	ret = conntracK_count_zone(self->sock, TEST_ZONE_ID);
+	ret = conntrack_count_zone(self->sock, TEST_ZONE_ID);
 	EXPECT_GE(ret, 2);
 	if (ret > 2)
 		SKIP(return, "kernel does not support filtering by zone");
@@ -437,7 +434,7 @@ TEST_F(conntrack_dump_flush, test_dump_by_zone)
 {
 	int ret;
 
-	ret = conntracK_count_zone(self->sock, TEST_ZONE_ID);
+	ret = conntrack_count_zone(self->sock, TEST_ZONE_ID);
 	EXPECT_EQ(ret, 2);
 }
 
@@ -447,13 +444,13 @@ TEST_F(conntrack_dump_flush, test_flush_by_zone)
 
 	ret = conntrack_flush_zone(self->sock, TEST_ZONE_ID);
 	EXPECT_EQ(ret, 0);
-	ret = conntracK_count_zone(self->sock, TEST_ZONE_ID);
+	ret = conntrack_count_zone(self->sock, TEST_ZONE_ID);
 	EXPECT_EQ(ret, 0);
-	ret = conntracK_count_zone(self->sock, TEST_ZONE_ID + 1);
+	ret = conntrack_count_zone(self->sock, TEST_ZONE_ID + 1);
 	EXPECT_EQ(ret, 2);
-	ret = conntracK_count_zone(self->sock, TEST_ZONE_ID + 2);
+	ret = conntrack_count_zone(self->sock, TEST_ZONE_ID + 2);
 	EXPECT_EQ(ret, 2);
-	ret = conntracK_count_zone(self->sock, NF_CT_DEFAULT_ZONE_ID);
+	ret = conntrack_count_zone(self->sock, NF_CT_DEFAULT_ZONE_ID);
 	EXPECT_EQ(ret, 2);
 }
 
@@ -463,13 +460,13 @@ TEST_F(conntrack_dump_flush, test_flush_by_zone_default)
 
 	ret = conntrack_flush_zone(self->sock, NF_CT_DEFAULT_ZONE_ID);
 	EXPECT_EQ(ret, 0);
-	ret = conntracK_count_zone(self->sock, TEST_ZONE_ID);
+	ret = conntrack_count_zone(self->sock, TEST_ZONE_ID);
 	EXPECT_EQ(ret, 2);
-	ret = conntracK_count_zone(self->sock, TEST_ZONE_ID + 1);
+	ret = conntrack_count_zone(self->sock, TEST_ZONE_ID + 1);
 	EXPECT_EQ(ret, 2);
-	ret = conntracK_count_zone(self->sock, TEST_ZONE_ID + 2);
+	ret = conntrack_count_zone(self->sock, TEST_ZONE_ID + 2);
 	EXPECT_EQ(ret, 2);
-	ret = conntracK_count_zone(self->sock, NF_CT_DEFAULT_ZONE_ID);
+	ret = conntrack_count_zone(self->sock, NF_CT_DEFAULT_ZONE_ID);
 	EXPECT_EQ(ret, 0);
 }
 
-- 
2.47.3