[PATCH nf] netfilter: nf_tables: fix oops during rule dump

[PATCH nf] netfilter: nf_tables: fix oops during rule dump

[Florian Westphal]

We can oops in nf_tables_fill_rule_info().

Its not possible to fetch previous element in rcu-protected lists
when deletions are not prevented somehow: list_del_rcu poisons
the ->prev pointer value.

Before rcu-conversion this was safe as dump operations did hold
nfnetlink mutex.

Pass previous rule as argument, obtained by keeping a pointer to
the previous rule during traversal.

Fixes: d9adf22a291883 ("netfilter: nf_tables: use call_rcu in netlink dumps")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_tables_api.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index aa5e7b00a581..0969f9647927 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2261,13 +2261,13 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, struct net *net,
 				    u32 flags, int family,
 				    const struct nft_table *table,
 				    const struct nft_chain *chain,
-				    const struct nft_rule *rule)
+				    const struct nft_rule *rule,
+				    const struct nft_rule *prule)
 {
 	struct nlmsghdr *nlh;
 	struct nfgenmsg *nfmsg;
 	const struct nft_expr *expr, *next;
 	struct nlattr *list;
-	const struct nft_rule *prule;
 	u16 type = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event);
 
 	nlh = nlmsg_put(skb, portid, seq, type, sizeof(struct nfgenmsg), flags);
@@ -2287,8 +2287,7 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, struct net *net,
 			 NFTA_RULE_PAD))
 		goto nla_put_failure;
 
-	if ((event != NFT_MSG_DELRULE) && (rule->list.prev != &chain->rules)) {
-		prule = list_prev_entry(rule, list);
+	if (event != NFT_MSG_DELRULE && prule) {
 		if (nla_put_be64(skb, NFTA_RULE_POSITION,
 				 cpu_to_be64(prule->handle),
 				 NFTA_RULE_PAD))
@@ -2335,7 +2334,7 @@ static void nf_tables_rule_notify(const struct nft_ctx *ctx,
 
 	err = nf_tables_fill_rule_info(skb, ctx->net, ctx->portid, ctx->seq,
 				       event, 0, ctx->family, ctx->table,
-				       ctx->chain, rule);
+				       ctx->chain, rule, NULL);
 	if (err < 0) {
 		kfree_skb(skb);
 		goto err;
@@ -2360,12 +2359,13 @@ static int __nf_tables_dump_rules(struct sk_buff *skb,
 				  const struct nft_chain *chain)
 {
 	struct net *net = sock_net(skb->sk);
+	const struct nft_rule *rule, *prule;
 	unsigned int s_idx = cb->args[0];
-	const struct nft_rule *rule;
 
+	prule = NULL;
 	list_for_each_entry_rcu(rule, &chain->rules, list) {
 		if (!nft_is_active(net, rule))
-			goto cont;
+			goto cont_skip;
 		if (*idx < s_idx)
 			goto cont;
 		if (*idx > s_idx) {
@@ -2377,11 +2377,13 @@ static int __nf_tables_dump_rules(struct sk_buff *skb,
 					NFT_MSG_NEWRULE,
 					NLM_F_MULTI | NLM_F_APPEND,
 					table->family,
-					table, chain, rule) < 0)
+					table, chain, rule, prule) < 0)
 			return 1;
 
 		nl_dump_check_consistent(cb, nlmsg_hdr(skb));
 cont:
+		prule = rule;
+cont_skip:
 		(*idx)++;
 	}
 	return 0;
@@ -2537,7 +2539,7 @@ static int nf_tables_getrule(struct net *net, struct sock *nlsk,
 
 	err = nf_tables_fill_rule_info(skb2, net, NETLINK_CB(skb).portid,
 				       nlh->nlmsg_seq, NFT_MSG_NEWRULE, 0,
-				       family, table, chain, rule);
+				       family, table, chain, rule, NULL);
 	if (err < 0)
 		goto err;
 
-- 
2.21.0

Re: [PATCH nf] netfilter: nf_tables: fix oops during rule dump

[Taehee Yoo]

On Tue, 30 Apr 2019 at 21:52, Florian Westphal <fw@strlen.de> wrote:
>
> We can oops in nf_tables_fill_rule_info().
>
> Its not possible to fetch previous element in rcu-protected lists
> when deletions are not prevented somehow: list_del_rcu poisons
> the ->prev pointer value.
>
> Before rcu-conversion this was safe as dump operations did hold
> nfnetlink mutex.
>
> Pass previous rule as argument, obtained by keeping a pointer to
> the previous rule during traversal.
>

Hi Florian,
I have tested this patch and I think this patch works well.
I would like to share my test method.

SHELL#1
while :
do
    nft flush ruleset
    nft -f test.nft
done

SHELL#2
while :
do
    nft list ruleset -a
    iptables-nft -vnL
done

This test method could make panic.

[ 1887.092089] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN PTI
[ 1887.100127] CPU: 0 PID: 1652 Comm: nft Not tainted 5.1.0-rc5+ #76
[ 1887.106982] Hardware name: To be filled by O.E.M. To be filled by
O.E.M./Aptio CRB, BIOS 5.6.5 07/08/2015
[ 1887.117769] RIP: 0010:nf_tables_fill_rule_info.isra.59+0x362/0x860
[nf_tables]
[ 1887.125892] Code: 8b 84 24 d8 00 00 00 4d 8b 65 08 48 83 c0 10 49
39 c4 74 5a 49 8d 7c 24 10 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48
c1 ea 03 <80> 3c 02 00 0f 85 9c 04 00 00 48 b8 ff ff ff ff ff 03 00 00
49 23
[ 1887.146996] RSP: 0018:ffff88810c0872a0 EFLAGS: 00010206
[ 1887.152878] RAX: dffffc0000000000 RBX: ffff88810ea0b1c0 RCX: 0000000000000000
[ 1887.160894] RDX: 1bd5a00000000042 RSI: ffff88810c0872f8 RDI: dead000000000210
[ 1887.168909] RBP: ffff8880b4b78000 R08: ffffed101696f008 R09: ffffed101696f008
[ 1887.176933] R10: 0000000000000002 R11: ffffed101696f007 R12: dead000000000200
[ 1887.184957] R13: ffff8880b8142688 R14: ffff8880b8142698 R15: ffff88810c0872f0
[ 1887.192973] FS:  00007fb475d8a740(0000) GS:ffff888116600000(0000)
knlGS:0000000000000000
[ 1887.202064] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1887.208527] CR2: 0000557d208c4088 CR3: 00000001102de000 CR4: 00000000001006f0
[ 1887.216548] Call Trace:
[ 1887.219344]  ? nft_expr_dump+0x4c0/0x4c0 [nf_tables]
[ 1887.224947]  ? debug_show_all_locks+0x2d0/0x2d0
[ 1887.230055]  ? rcu_read_lock_sched_held+0x114/0x130
[ 1887.235573]  __nf_tables_dump_rules+0x27d/0x620 [nf_tables]
[ 1887.241892]  nf_tables_dump_rules+0x4c4/0xc80 [nf_tables]
[ 1887.247967]  ? rcu_read_lock_sched_held+0x114/0x130
[ 1887.253466]  ? __kmalloc_node_track_caller+0x321/0x360
[ 1887.259285]  ? __nf_tables_dump_rules+0x620/0x620 [nf_tables]
[ 1887.265759]  ? __alloc_skb+0x314/0x500
[ 1887.269990]  ? sock_spd_release+0xf0/0xf0
[ 1887.274514]  ? check_flags.part.41+0x440/0x440
[ 1887.279532]  netlink_dump+0x476/0xea0
[ 1887.283669]  ? __netlink_sendskb+0xc0/0xc0
[ 1887.288283]  ? __netlink_dump_start+0x165/0x750
[ 1887.293408]  __netlink_dump_start+0x537/0x750
[ 1887.298345]  nft_netlink_dump_start_rcu.constprop.71+0x97/0x180 [nf_tables]
[ 1887.306205]  nf_tables_getrule+0x3b6/0x620 [nf_tables]
[ 1887.312019]  ? nf_tables_rule_notify+0x3e0/0x3e0 [nf_tables]
[ 1887.318407]  ? nf_tables_dump_obj_start+0x230/0x230 [nf_tables]
[ 1887.325086]  ? __nf_tables_dump_rules+0x620/0x620 [nf_tables]
[ 1887.331569]  ? nf_tables_dump_sets_done+0x40/0x40 [nf_tables]
[ 1887.338045]  ? __nla_parse+0x34/0x310
[ 1887.342198]  ? nf_tables_rule_notify+0x3e0/0x3e0 [nf_tables]
[ 1887.348576]  nfnetlink_rcv_msg+0x3f0/0xd0b [nfnetlink]
[ 1887.354366]  ? kernel_text_address+0x111/0x120
[ 1887.359391]  ? nfnetlink_bind+0x200/0x200 [nfnetlink]
[ 1887.365097]  ? sched_clock_cpu+0x18/0x170
[ 1887.369616]  ? __sys_sendto+0x263/0x300
[ 1887.373929]  ? sched_clock_cpu+0x18/0x170
[ 1887.378447]  ? do_syscall_64+0x9c/0x3e0
[ 1887.382779]  ? __lock_acquire+0xe7c/0x3bd0
[ 1887.387403]  ? check_flags.part.41+0x440/0x440
[ 1887.392416]  netlink_rcv_skb+0x112/0x330
[ 1887.396841]  ? nfnetlink_bind+0x200/0x200 [nfnetlink]
[ 1887.402538]  ? netlink_ack+0x940/0x940
[ 1887.406776]  ? ns_capable_common+0x5c/0xd0
[ 1887.411405]  nfnetlink_rcv+0x161/0x320 [nfnetlink]
[ 1887.416809]  ? nfnetlink_rcv_batch+0x1280/0x1280 [nfnetlink]
[ 1887.423200]  netlink_unicast+0x3ee/0x5a0

After this patch, I couldn't see this panic message.

Thanks!

> Fixes: d9adf22a291883 ("netfilter: nf_tables: use call_rcu in netlink dumps")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/netfilter/nf_tables_api.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index aa5e7b00a581..0969f9647927 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -2261,13 +2261,13 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, struct net *net,
>                                     u32 flags, int family,
>                                     const struct nft_table *table,
>                                     const struct nft_chain *chain,
> -                                   const struct nft_rule *rule)
> +                                   const struct nft_rule *rule,
> +                                   const struct nft_rule *prule)
>  {
>         struct nlmsghdr *nlh;
>         struct nfgenmsg *nfmsg;
>         const struct nft_expr *expr, *next;
>         struct nlattr *list;
> -       const struct nft_rule *prule;
>         u16 type = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event);
>
>         nlh = nlmsg_put(skb, portid, seq, type, sizeof(struct nfgenmsg), flags);
> @@ -2287,8 +2287,7 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, struct net *net,
>                          NFTA_RULE_PAD))
>                 goto nla_put_failure;
>
> -       if ((event != NFT_MSG_DELRULE) && (rule->list.prev != &chain->rules)) {
> -               prule = list_prev_entry(rule, list);
> +       if (event != NFT_MSG_DELRULE && prule) {
>                 if (nla_put_be64(skb, NFTA_RULE_POSITION,
>                                  cpu_to_be64(prule->handle),
>                                  NFTA_RULE_PAD))
> @@ -2335,7 +2334,7 @@ static void nf_tables_rule_notify(const struct nft_ctx *ctx,
>
>         err = nf_tables_fill_rule_info(skb, ctx->net, ctx->portid, ctx->seq,
>                                        event, 0, ctx->family, ctx->table,
> -                                      ctx->chain, rule);
> +                                      ctx->chain, rule, NULL);
>         if (err < 0) {
>                 kfree_skb(skb);
>                 goto err;
> @@ -2360,12 +2359,13 @@ static int __nf_tables_dump_rules(struct sk_buff *skb,
>                                   const struct nft_chain *chain)
>  {
>         struct net *net = sock_net(skb->sk);
> +       const struct nft_rule *rule, *prule;
>         unsigned int s_idx = cb->args[0];
> -       const struct nft_rule *rule;
>
> +       prule = NULL;
>         list_for_each_entry_rcu(rule, &chain->rules, list) {
>                 if (!nft_is_active(net, rule))
> -                       goto cont;
> +                       goto cont_skip;
>                 if (*idx < s_idx)
>                         goto cont;
>                 if (*idx > s_idx) {
> @@ -2377,11 +2377,13 @@ static int __nf_tables_dump_rules(struct sk_buff *skb,
>                                         NFT_MSG_NEWRULE,
>                                         NLM_F_MULTI | NLM_F_APPEND,
>                                         table->family,
> -                                       table, chain, rule) < 0)
> +                                       table, chain, rule, prule) < 0)
>                         return 1;
>
>                 nl_dump_check_consistent(cb, nlmsg_hdr(skb));
>  cont:
> +               prule = rule;
> +cont_skip:
>                 (*idx)++;
>         }
>         return 0;
> @@ -2537,7 +2539,7 @@ static int nf_tables_getrule(struct net *net, struct sock *nlsk,
>
>         err = nf_tables_fill_rule_info(skb2, net, NETLINK_CB(skb).portid,
>                                        nlh->nlmsg_seq, NFT_MSG_NEWRULE, 0,
> -                                      family, table, chain, rule);
> +                                      family, table, chain, rule, NULL);
>         if (err < 0)
>                 goto err;
>
> --
> 2.21.0
>

Re: [PATCH nf] netfilter: nf_tables: fix oops during rule dump

[Pablo Neira Ayuso]

On Tue, Apr 30, 2019 at 02:53:11PM +0200, Florian Westphal wrote:
> We can oops in nf_tables_fill_rule_info().
> 
> Its not possible to fetch previous element in rcu-protected lists
> when deletions are not prevented somehow: list_del_rcu poisons
> the ->prev pointer value.
> 
> Before rcu-conversion this was safe as dump operations did hold
> nfnetlink mutex.
> 
> Pass previous rule as argument, obtained by keeping a pointer to
> the previous rule during traversal.

Applied, thanks.

[PATCH nf-next 0/2] netfilter: nat: avoid long-running loops

[Florian Westphal]

If a majority of ports are in use, trying every available port may
take significant amounts of time.

Add a upper limit and cancel once we've exhausted all available
options.

First patch collapses the repetitive reserve-port loop into a helper,
second patch changes the helper to only make up to 128 attempts.

Florian Westphal (2):
  netfilter: nat: move repetitive nat port reserve loop to a helper
  netfilter: nat: avoid long-running port range loop

 include/net/netfilter/nf_nat_helper.h |  1 +
 net/ipv4/netfilter/nf_nat_h323.c      | 60 ++-------------------------
 net/netfilter/nf_nat_amanda.c         | 14 +------
 net/netfilter/nf_nat_ftp.c            | 17 +-------
 net/netfilter/nf_nat_helper.c         | 31 ++++++++++++++
 net/netfilter/nf_nat_irc.c            | 16 +------
 net/netfilter/nf_nat_sip.c            | 14 +------
 7 files changed, 42 insertions(+), 111 deletions(-)

-- 
2.35.1

[PATCH nf-next 1/2] netfilter: nat: move repetitive nat port reserve loop to a helper

[Florian Westphal]

Almost all nat helpers reserve an expecation port the same way:
Try the port inidcated by the peer, then move to next port if that
port is already in use.

We can squash this into a helper.

Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_nat_helper.h |  1 +
 net/ipv4/netfilter/nf_nat_h323.c      | 60 ++-------------------------
 net/netfilter/nf_nat_amanda.c         | 14 +------
 net/netfilter/nf_nat_ftp.c            | 17 +-------
 net/netfilter/nf_nat_helper.c         | 19 +++++++++
 net/netfilter/nf_nat_irc.c            | 16 +------
 net/netfilter/nf_nat_sip.c            | 14 +------
 7 files changed, 30 insertions(+), 111 deletions(-)

diff --git a/include/net/netfilter/nf_nat_helper.h b/include/net/netfilter/nf_nat_helper.h
index efae84646353..44c421b9be85 100644
--- a/include/net/netfilter/nf_nat_helper.h
+++ b/include/net/netfilter/nf_nat_helper.h
@@ -38,4 +38,5 @@ bool nf_nat_mangle_udp_packet(struct sk_buff *skb, struct nf_conn *ct,
  * to port ct->master->saved_proto. */
 void nf_nat_follow_master(struct nf_conn *ct, struct nf_conntrack_expect *this);
 
+u16 nf_nat_exp_find_port(struct nf_conntrack_expect *exp, u16 port);
 #endif
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index a334f0dcc2d0..faee20af4856 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -291,20 +291,7 @@ static int nat_t120(struct sk_buff *skb, struct nf_conn *ct,
 	exp->expectfn = nf_nat_follow_master;
 	exp->dir = !dir;
 
-	/* Try to get same port: if not, try to change it. */
-	for (; nated_port != 0; nated_port++) {
-		int ret;
-
-		exp->tuple.dst.u.tcp.port = htons(nated_port);
-		ret = nf_ct_expect_related(exp, 0);
-		if (ret == 0)
-			break;
-		else if (ret != -EBUSY) {
-			nated_port = 0;
-			break;
-		}
-	}
-
+	nated_port = nf_nat_exp_find_port(exp, nated_port);
 	if (nated_port == 0) {	/* No port available */
 		net_notice_ratelimited("nf_nat_h323: out of TCP ports\n");
 		return 0;
@@ -347,20 +334,7 @@ static int nat_h245(struct sk_buff *skb, struct nf_conn *ct,
 	if (info->sig_port[dir] == port)
 		nated_port = ntohs(info->sig_port[!dir]);
 
-	/* Try to get same port: if not, try to change it. */
-	for (; nated_port != 0; nated_port++) {
-		int ret;
-
-		exp->tuple.dst.u.tcp.port = htons(nated_port);
-		ret = nf_ct_expect_related(exp, 0);
-		if (ret == 0)
-			break;
-		else if (ret != -EBUSY) {
-			nated_port = 0;
-			break;
-		}
-	}
-
+	nated_port = nf_nat_exp_find_port(exp, nated_port);
 	if (nated_port == 0) {	/* No port available */
 		net_notice_ratelimited("nf_nat_q931: out of TCP ports\n");
 		return 0;
@@ -439,20 +413,7 @@ static int nat_q931(struct sk_buff *skb, struct nf_conn *ct,
 	if (info->sig_port[dir] == port)
 		nated_port = ntohs(info->sig_port[!dir]);
 
-	/* Try to get same port: if not, try to change it. */
-	for (; nated_port != 0; nated_port++) {
-		int ret;
-
-		exp->tuple.dst.u.tcp.port = htons(nated_port);
-		ret = nf_ct_expect_related(exp, 0);
-		if (ret == 0)
-			break;
-		else if (ret != -EBUSY) {
-			nated_port = 0;
-			break;
-		}
-	}
-
+	nated_port = nf_nat_exp_find_port(exp, nated_port);
 	if (nated_port == 0) {	/* No port available */
 		net_notice_ratelimited("nf_nat_ras: out of TCP ports\n");
 		return 0;
@@ -532,20 +493,7 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct,
 	exp->expectfn = ip_nat_callforwarding_expect;
 	exp->dir = !dir;
 
-	/* Try to get same port: if not, try to change it. */
-	for (nated_port = ntohs(port); nated_port != 0; nated_port++) {
-		int ret;
-
-		exp->tuple.dst.u.tcp.port = htons(nated_port);
-		ret = nf_ct_expect_related(exp, 0);
-		if (ret == 0)
-			break;
-		else if (ret != -EBUSY) {
-			nated_port = 0;
-			break;
-		}
-	}
-
+	nated_port = nf_nat_exp_find_port(exp, ntohs(port));
 	if (nated_port == 0) {	/* No port available */
 		net_notice_ratelimited("nf_nat_q931: out of TCP ports\n");
 		return 0;
diff --git a/net/netfilter/nf_nat_amanda.c b/net/netfilter/nf_nat_amanda.c
index 3bc7e0854efe..98deef6cde69 100644
--- a/net/netfilter/nf_nat_amanda.c
+++ b/net/netfilter/nf_nat_amanda.c
@@ -44,19 +44,7 @@ static unsigned int help(struct sk_buff *skb,
 	exp->expectfn = nf_nat_follow_master;
 
 	/* Try to get same port: if not, try to change it. */
-	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
-		int res;
-
-		exp->tuple.dst.u.tcp.port = htons(port);
-		res = nf_ct_expect_related(exp, 0);
-		if (res == 0)
-			break;
-		else if (res != -EBUSY) {
-			port = 0;
-			break;
-		}
-	}
-
+	port = nf_nat_exp_find_port(exp, ntohs(exp->saved_proto.tcp.port));
 	if (port == 0) {
 		nf_ct_helper_log(skb, exp->master, "all ports in use");
 		return NF_DROP;
diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c
index aace6768a64e..c92a436d9c48 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -86,22 +86,9 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
 	 * this one. */
 	exp->expectfn = nf_nat_follow_master;
 
-	/* Try to get same port: if not, try to change it. */
-	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
-		int ret;
-
-		exp->tuple.dst.u.tcp.port = htons(port);
-		ret = nf_ct_expect_related(exp, 0);
-		if (ret == 0)
-			break;
-		else if (ret != -EBUSY) {
-			port = 0;
-			break;
-		}
-	}
-
+	port = nf_nat_exp_find_port(exp, ntohs(exp->saved_proto.tcp.port));
 	if (port == 0) {
-		nf_ct_helper_log(skb, ct, "all ports in use");
+		nf_ct_helper_log(skb, exp->master, "all ports in use");
 		return NF_DROP;
 	}
 
diff --git a/net/netfilter/nf_nat_helper.c b/net/netfilter/nf_nat_helper.c
index a263505455fc..067d6d6f6b7d 100644
--- a/net/netfilter/nf_nat_helper.c
+++ b/net/netfilter/nf_nat_helper.c
@@ -198,3 +198,22 @@ void nf_nat_follow_master(struct nf_conn *ct,
 	nf_nat_setup_info(ct, &range, NF_NAT_MANIP_DST);
 }
 EXPORT_SYMBOL(nf_nat_follow_master);
+
+u16 nf_nat_exp_find_port(struct nf_conntrack_expect *exp, u16 port)
+{
+	/* Try to get same port: if not, try to change it. */
+	for (; port != 0; port++) {
+		int res;
+
+		exp->tuple.dst.u.tcp.port = htons(port);
+		res = nf_ct_expect_related(exp, 0);
+		if (res == 0)
+			return port;
+
+		if (res != -EBUSY)
+			break;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(nf_nat_exp_find_port);
diff --git a/net/netfilter/nf_nat_irc.c b/net/netfilter/nf_nat_irc.c
index c691ab8d234c..19c4fcc60c50 100644
--- a/net/netfilter/nf_nat_irc.c
+++ b/net/netfilter/nf_nat_irc.c
@@ -48,20 +48,8 @@ static unsigned int help(struct sk_buff *skb,
 	exp->dir = IP_CT_DIR_REPLY;
 	exp->expectfn = nf_nat_follow_master;
 
-	/* Try to get same port: if not, try to change it. */
-	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
-		int ret;
-
-		exp->tuple.dst.u.tcp.port = htons(port);
-		ret = nf_ct_expect_related(exp, 0);
-		if (ret == 0)
-			break;
-		else if (ret != -EBUSY) {
-			port = 0;
-			break;
-		}
-	}
-
+	port = nf_nat_exp_find_port(exp,
+				    ntohs(exp->saved_proto.tcp.port));
 	if (port == 0) {
 		nf_ct_helper_log(skb, ct, "all ports in use");
 		return NF_DROP;
diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
index f0a735e86851..cf4aeb299bde 100644
--- a/net/netfilter/nf_nat_sip.c
+++ b/net/netfilter/nf_nat_sip.c
@@ -410,19 +410,7 @@ static unsigned int nf_nat_sip_expect(struct sk_buff *skb, unsigned int protoff,
 	exp->dir = !dir;
 	exp->expectfn = nf_nat_sip_expected;
 
-	for (; port != 0; port++) {
-		int ret;
-
-		exp->tuple.dst.u.udp.port = htons(port);
-		ret = nf_ct_expect_related(exp, NF_CT_EXP_F_SKIP_MASTER);
-		if (ret == 0)
-			break;
-		else if (ret != -EBUSY) {
-			port = 0;
-			break;
-		}
-	}
-
+	port = nf_nat_exp_find_port(exp, port);
 	if (port == 0) {
 		nf_ct_helper_log(skb, ct, "all ports in use for SIP");
 		return NF_DROP;
-- 
2.35.1

[PATCH nf-next 2/2] netfilter: nat: avoid long-running port range loop

[Florian Westphal]

Looping a large port range takes too long. Instead select a random
offset within [ntohs(exp->saved_proto.tcp.port), 65535] and try 128
ports.

This is a rehash of an erlier patch to do the same, but generalized
to handle other helpers as well.

Link: https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210920204439.13179-2-Cole.Dishington@alliedtelesis.co.nz/
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_nat_helper.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_nat_helper.c b/net/netfilter/nf_nat_helper.c
index 067d6d6f6b7d..a95a25196943 100644
--- a/net/netfilter/nf_nat_helper.c
+++ b/net/netfilter/nf_nat_helper.c
@@ -201,8 +201,18 @@ EXPORT_SYMBOL(nf_nat_follow_master);
 
 u16 nf_nat_exp_find_port(struct nf_conntrack_expect *exp, u16 port)
 {
+	static const unsigned int max_attempts = 128;
+	int range, attempts_left;
+	u16 min = port;
+
+	range = USHRT_MAX - port;
+	attempts_left = range;
+
+	if (attempts_left > max_attempts)
+		attempts_left = max_attempts;
+
 	/* Try to get same port: if not, try to change it. */
-	for (; port != 0; port++) {
+	for (;;) {
 		int res;
 
 		exp->tuple.dst.u.tcp.port = htons(port);
@@ -210,8 +220,10 @@ u16 nf_nat_exp_find_port(struct nf_conntrack_expect *exp, u16 port)
 		if (res == 0)
 			return port;
 
-		if (res != -EBUSY)
+		if (res != -EBUSY || (--attempts_left < 0))
 			break;
+
+		port = min + prandom_u32_max(range);
 	}
 
 	return 0;
-- 
2.35.1

[PATCH nf-next] netfilter: conntrack: simplify nf_conntrack_alter_reply

[Florian Westphal]

nf_conntrack_alter_reply doesn't do helper reassignment anymore.
Remove the comments that make this claim.

Furthermore, remove dead code from the function and place ot
in nf_conntrack.h.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_conntrack.h | 14 ++++++++++----
 net/netfilter/nf_conntrack_core.c    | 18 ------------------
 net/netfilter/nf_conntrack_helper.c  |  7 +------
 3 files changed, 11 insertions(+), 28 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index 4085765c3370..cba3ccf03fcc 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -160,10 +160,6 @@ static inline struct net *nf_ct_net(const struct nf_conn *ct)
 	return read_pnet(&ct->ct_net);
 }
 
-/* Alter reply tuple (maybe alter helper). */
-void nf_conntrack_alter_reply(struct nf_conn *ct,
-			      const struct nf_conntrack_tuple *newreply);
-
 /* Is this tuple taken? (ignoring any belonging to the given
    conntrack). */
 int nf_conntrack_tuple_taken(const struct nf_conntrack_tuple *tuple,
@@ -284,6 +280,16 @@ static inline bool nf_is_loopback_packet(const struct sk_buff *skb)
 	return skb->dev && skb->skb_iif && skb->dev->flags & IFF_LOOPBACK;
 }
 
+static inline void nf_conntrack_alter_reply(struct nf_conn *ct,
+					    const struct nf_conntrack_tuple *newreply)
+{
+	/* Must be unconfirmed, so not in hash table yet */
+	if (WARN_ON(nf_ct_is_confirmed(ct)))
+		return;
+
+	ct->tuplehash[IP_CT_DIR_REPLY].tuple = *newreply;
+}
+
 #define nfct_time_stamp ((u32)(jiffies))
 
 /* jiffies until ct expires, 0 if already expired */
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 9f6f2e643575..124136b5a79a 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2042,24 +2042,6 @@ nf_conntrack_in(struct sk_buff *skb, const struct nf_hook_state *state)
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_in);
 
-/* Alter reply tuple (maybe alter helper).  This is for NAT, and is
-   implicitly racy: see __nf_conntrack_confirm */
-void nf_conntrack_alter_reply(struct nf_conn *ct,
-			      const struct nf_conntrack_tuple *newreply)
-{
-	struct nf_conn_help *help = nfct_help(ct);
-
-	/* Should be unconfirmed, so not in hash table yet */
-	WARN_ON(nf_ct_is_confirmed(ct));
-
-	nf_ct_dump_tuple(newreply);
-
-	ct->tuplehash[IP_CT_DIR_REPLY].tuple = *newreply;
-	if (ct->master || (help && !hlist_empty(&help->expectations)))
-		return;
-}
-EXPORT_SYMBOL_GPL(nf_conntrack_alter_reply);
-
 /* Refresh conntrack for this many jiffies and do accounting if do_acct is 1 */
 void __nf_ct_refresh_acct(struct nf_conn *ct,
 			  enum ip_conntrack_info ctinfo,
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index f22691f83853..4ed5878cb25b 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -194,12 +194,7 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
 	struct nf_conntrack_helper *helper = NULL;
 	struct nf_conn_help *help;
 
-	/* We already got a helper explicitly attached. The function
-	 * nf_conntrack_alter_reply - in case NAT is in use - asks for looking
-	 * the helper up again. Since now the user is in full control of
-	 * making consistent helper configurations, skip this automatic
-	 * re-lookup, otherwise we'll lose the helper.
-	 */
+	/* We already got a helper explicitly attached (e.g. nft_ct) */
 	if (test_bit(IPS_HELPER_BIT, &ct->status))
 		return 0;
 
-- 
2.41.0

[PATCH nft] parser_bison: fix memory leak when parsing flowtable hook declaration

[Florian Westphal]

When the hook location is invalid we error out but we do leak both
the priority expression and the flowtable name.  Example:

valgrind --leak-check=full nft -f flowtable-parser-err-memleak
[..] Error: unknown chain hook
hook enoent priority filter + 10
     ^^^^^^
[..]
2 bytes in 1 blocks are definitely lost in loss record 1 of 3
   at: malloc (vg_replace_malloc.c:446)
   by: strdup (in libc.so.6)
   by: xstrdup (in libnftables.so.1.1.0)
   by: nft_lex (in libnftables.so.1.1.0)
   by: nft_parse (in libnftables.so.1.1.0)
   by: __nft_run_cmd_from_filename (in libnftables.so.1.1.0)
   by: nft_run_cmd_from_filename (in libnftables.so.1.1.0)

First two reports are due to the priority expression: this needs to call
expr_free().  Third report is due to the flowtable name, the destructor
was missing so add one.

After fix:
All heap blocks were freed -- no leaks are possible

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/parser_bison.y                                           | 3 ++-
 .../testcases/bogons/nft-f/flowtable-parser-err-memleak      | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/flowtable-parser-err-memleak

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 5b84331f220d..aacfa2917917 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -719,7 +719,7 @@ int nft_lex(void *, void *, void *);
 %destructor { handle_free(&$$); } obj_spec objid_spec obj_or_id_spec
 
 %type <handle>			set_identifier flowtableid_spec flowtable_identifier obj_identifier
-%destructor { handle_free(&$$); } set_identifier flowtableid_spec obj_identifier
+%destructor { handle_free(&$$); } set_identifier flowtableid_spec flowtable_identifier obj_identifier
 
 %type <handle>			basehook_spec
 %destructor { handle_free(&$$); } basehook_spec
@@ -2427,6 +2427,7 @@ flowtable_block		:	/* empty */	{ $$ = $<flowtable>-1; }
 					erec_queue(error(&@3, "unknown chain hook"),
 						   state->msgs);
 					free_const($3);
+					expr_free($4.expr);
 					YYERROR;
 				}
 				free_const($3);
diff --git a/tests/shell/testcases/bogons/nft-f/flowtable-parser-err-memleak b/tests/shell/testcases/bogons/nft-f/flowtable-parser-err-memleak
new file mode 100644
index 000000000000..ca0480bfc943
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/flowtable-parser-err-memleak
@@ -0,0 +1,5 @@
+table ip t {
+        flowtable f {
+                hook enoent priority filter + 10
+        }
+}
-- 
2.49.1

Re: [PATCH nft] parser_bison: fix memory leak when parsing flowtable hook declaration

[Pablo Neira Ayuso]

On Wed, Jul 23, 2025 at 05:00:11PM +0200, Florian Westphal wrote:
> When the hook location is invalid we error out but we do leak both
> the priority expression and the flowtable name.  Example:
> 
> valgrind --leak-check=full nft -f flowtable-parser-err-memleak
> [..] Error: unknown chain hook
> hook enoent priority filter + 10
>      ^^^^^^
> [..]
> 2 bytes in 1 blocks are definitely lost in loss record 1 of 3
>    at: malloc (vg_replace_malloc.c:446)
>    by: strdup (in libc.so.6)
>    by: xstrdup (in libnftables.so.1.1.0)
>    by: nft_lex (in libnftables.so.1.1.0)
>    by: nft_parse (in libnftables.so.1.1.0)
>    by: __nft_run_cmd_from_filename (in libnftables.so.1.1.0)
>    by: nft_run_cmd_from_filename (in libnftables.so.1.1.0)
> 
> First two reports are due to the priority expression: this needs to call
> expr_free().  Third report is due to the flowtable name, the destructor
> was missing so add one.
> 
> After fix:
> All heap blocks were freed -- no leaks are possible
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

Thanks