netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH nf-next 0/3] NF NAT deduplication refactoring
@ 2023-03-13 13:46 Jeremy Sowden
  2023-03-13 13:46 ` [PATCH nf-next 1/3] netfilter: nf_nat_redirect: use `struct nf_nat_range2` in ipv4 API Jeremy Sowden
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Jeremy Sowden @ 2023-03-13 13:46 UTC (permalink / raw)
  To: Netfilter Devel

These three patches perform refactoring in NF NAT modules to remove
duplicate code.

Jeremy Sowden (3):
  netfilter: nf_nat_redirect: use `struct nf_nat_range2` in ipv4 API
  netfilter: nft_masq: deduplicate eval call-backs
  netfilter: nft_redir: deduplicate eval call-backs

 include/net/netfilter/nf_nat_redirect.h |  3 +-
 net/netfilter/nf_nat_redirect.c         | 58 ++++++++---------
 net/netfilter/nft_masq.c                | 75 +++++++++-------------
 net/netfilter/nft_redir.c               | 84 +++++++++----------------
 net/netfilter/xt_REDIRECT.c             | 10 ++-
 5 files changed, 96 insertions(+), 134 deletions(-)

-- 
2.39.2


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

* [PATCH nf-next 1/3] netfilter: nf_nat_redirect: use `struct nf_nat_range2` in ipv4 API
  2023-03-13 13:46 [PATCH nf-next 0/3] NF NAT deduplication refactoring Jeremy Sowden
@ 2023-03-13 13:46 ` Jeremy Sowden
  2023-03-13 13:46 ` [PATCH nf-next 2/3] netfilter: nft_masq: deduplicate eval call-backs Jeremy Sowden
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Jeremy Sowden @ 2023-03-13 13:46 UTC (permalink / raw)
  To: Netfilter Devel; +Cc: Florian Westphal

`nf_nat_redirect_ipv4` takes a `struct nf_nat_ipv4_multi_range_compat`,
but converts it internally to a `struct nf_nat_range2`.  Change the
function to take the latter, factor out the code now shared with
`nf_nat_redirect_ipv6`, move the conversion to the xt_REDIRECT module,
and update the ipv4 range initialization in the nft_redir module.

Replace a bare hex constant for 127.0.0.1 with a macro.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
Reviewed-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_nat_redirect.h |  3 +-
 net/netfilter/nf_nat_redirect.c         | 58 ++++++++++++-------------
 net/netfilter/nft_redir.c               | 15 ++++---
 net/netfilter/xt_REDIRECT.c             | 10 ++++-
 4 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/include/net/netfilter/nf_nat_redirect.h b/include/net/netfilter/nf_nat_redirect.h
index 2418653a66db..279380de904c 100644
--- a/include/net/netfilter/nf_nat_redirect.h
+++ b/include/net/netfilter/nf_nat_redirect.h
@@ -6,8 +6,7 @@
 #include <uapi/linux/netfilter/nf_nat.h>
 
 unsigned int
-nf_nat_redirect_ipv4(struct sk_buff *skb,
-		     const struct nf_nat_ipv4_multi_range_compat *mr,
+nf_nat_redirect_ipv4(struct sk_buff *skb, const struct nf_nat_range2 *range,
 		     unsigned int hooknum);
 unsigned int
 nf_nat_redirect_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
diff --git a/net/netfilter/nf_nat_redirect.c b/net/netfilter/nf_nat_redirect.c
index f91579c821e9..54ce8e6113ed 100644
--- a/net/netfilter/nf_nat_redirect.c
+++ b/net/netfilter/nf_nat_redirect.c
@@ -10,6 +10,7 @@
 
 #include <linux/if.h>
 #include <linux/inetdevice.h>
+#include <linux/in.h>
 #include <linux/ip.h>
 #include <linux/kernel.h>
 #include <linux/netdevice.h>
@@ -24,25 +25,38 @@
 #include <net/netfilter/nf_nat.h>
 #include <net/netfilter/nf_nat_redirect.h>
 
+static unsigned int
+nf_nat_redirect(struct sk_buff *skb, const struct nf_nat_range2 *range,
+		const union nf_inet_addr *newdst)
+{
+	struct nf_nat_range2 newrange;
+	enum ip_conntrack_info ctinfo;
+	struct nf_conn *ct;
+
+	ct = nf_ct_get(skb, &ctinfo);
+	WARN_ON(!(ct && (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));
+
+	newrange.flags		= range->flags | NF_NAT_RANGE_MAP_IPS;
+	newrange.min_addr	= *newdst;
+	newrange.max_addr	= *newdst;
+	newrange.min_proto	= range->min_proto;
+	newrange.max_proto	= range->max_proto;
+
+	return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_DST);
+}
+
 unsigned int
-nf_nat_redirect_ipv4(struct sk_buff *skb,
-		     const struct nf_nat_ipv4_multi_range_compat *mr,
+nf_nat_redirect_ipv4(struct sk_buff *skb, const struct nf_nat_range2 *range,
 		     unsigned int hooknum)
 {
-	struct nf_conn *ct;
-	enum ip_conntrack_info ctinfo;
 	__be32 newdst;
-	struct nf_nat_range2 newrange;
 
 	WARN_ON(hooknum != NF_INET_PRE_ROUTING &&
 		hooknum != NF_INET_LOCAL_OUT);
 
-	ct = nf_ct_get(skb, &ctinfo);
-	WARN_ON(!(ct && (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));
-
 	/* Local packets: make them go to loopback */
 	if (hooknum == NF_INET_LOCAL_OUT) {
-		newdst = htonl(0x7F000001);
+		newdst = htonl(INADDR_LOOPBACK);
 	} else {
 		const struct in_device *indev;
 
@@ -61,17 +75,8 @@ nf_nat_redirect_ipv4(struct sk_buff *skb,
 			return NF_DROP;
 	}
 
-	/* Transfer from original range. */
-	memset(&newrange.min_addr, 0, sizeof(newrange.min_addr));
-	memset(&newrange.max_addr, 0, sizeof(newrange.max_addr));
-	newrange.flags	     = mr->range[0].flags | NF_NAT_RANGE_MAP_IPS;
-	newrange.min_addr.ip = newdst;
-	newrange.max_addr.ip = newdst;
-	newrange.min_proto   = mr->range[0].min;
-	newrange.max_proto   = mr->range[0].max;
-
-	/* Hand modified range to generic setup. */
-	return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_DST);
+	return nf_nat_redirect(skb, range,
+			       &(union nf_inet_addr) { .ip = newdst });
 }
 EXPORT_SYMBOL_GPL(nf_nat_redirect_ipv4);
 
@@ -81,12 +86,8 @@ unsigned int
 nf_nat_redirect_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
 		     unsigned int hooknum)
 {
-	struct nf_nat_range2 newrange;
 	struct in6_addr newdst;
-	enum ip_conntrack_info ctinfo;
-	struct nf_conn *ct;
 
-	ct = nf_ct_get(skb, &ctinfo);
 	if (hooknum == NF_INET_LOCAL_OUT) {
 		newdst = loopback_addr;
 	} else {
@@ -109,12 +110,7 @@ nf_nat_redirect_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
 			return NF_DROP;
 	}
 
-	newrange.flags		= range->flags | NF_NAT_RANGE_MAP_IPS;
-	newrange.min_addr.in6	= newdst;
-	newrange.max_addr.in6	= newdst;
-	newrange.min_proto	= range->min_proto;
-	newrange.max_proto	= range->max_proto;
-
-	return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_DST);
+	return nf_nat_redirect(skb, range,
+			       &(union nf_inet_addr) { .in6 = newdst });
 }
 EXPORT_SYMBOL_GPL(nf_nat_redirect_ipv6);
diff --git a/net/netfilter/nft_redir.c b/net/netfilter/nft_redir.c
index 5f7739987559..77a459470cb7 100644
--- a/net/netfilter/nft_redir.c
+++ b/net/netfilter/nft_redir.c
@@ -104,20 +104,21 @@ static void nft_redir_ipv4_eval(const struct nft_expr *expr,
 				const struct nft_pktinfo *pkt)
 {
 	struct nft_redir *priv = nft_expr_priv(expr);
-	struct nf_nat_ipv4_multi_range_compat mr;
+	struct nf_nat_range2 range;
 
-	memset(&mr, 0, sizeof(mr));
+	memset(&range, 0, sizeof(range));
 	if (priv->sreg_proto_min) {
-		mr.range[0].min.all = (__force __be16)nft_reg_load16(
+		range.min_proto.all = (__force __be16)nft_reg_load16(
 			&regs->data[priv->sreg_proto_min]);
-		mr.range[0].max.all = (__force __be16)nft_reg_load16(
+		range.max_proto.all = (__force __be16)nft_reg_load16(
 			&regs->data[priv->sreg_proto_max]);
-		mr.range[0].flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
+		range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
 	}
 
-	mr.range[0].flags |= priv->flags;
+	range.flags |= priv->flags;
 
-	regs->verdict.code = nf_nat_redirect_ipv4(pkt->skb, &mr, nft_hook(pkt));
+	regs->verdict.code =
+		nf_nat_redirect_ipv4(pkt->skb, &range, nft_hook(pkt));
 }
 
 static void
diff --git a/net/netfilter/xt_REDIRECT.c b/net/netfilter/xt_REDIRECT.c
index 353ca7801251..ff66b56a3f97 100644
--- a/net/netfilter/xt_REDIRECT.c
+++ b/net/netfilter/xt_REDIRECT.c
@@ -46,7 +46,6 @@ static void redirect_tg_destroy(const struct xt_tgdtor_param *par)
 	nf_ct_netns_put(par->net, par->family);
 }
 
-/* FIXME: Take multiple ranges --RR */
 static int redirect_tg4_check(const struct xt_tgchk_param *par)
 {
 	const struct nf_nat_ipv4_multi_range_compat *mr = par->targinfo;
@@ -65,7 +64,14 @@ static int redirect_tg4_check(const struct xt_tgchk_param *par)
 static unsigned int
 redirect_tg4(struct sk_buff *skb, const struct xt_action_param *par)
 {
-	return nf_nat_redirect_ipv4(skb, par->targinfo, xt_hooknum(par));
+	const struct nf_nat_ipv4_multi_range_compat *mr = par->targinfo;
+	struct nf_nat_range2 range = {
+		.flags       = mr->range[0].flags,
+		.min_proto   = mr->range[0].min,
+		.max_proto   = mr->range[0].max,
+	};
+
+	return nf_nat_redirect_ipv4(skb, &range, xt_hooknum(par));
 }
 
 static struct xt_target redirect_tg_reg[] __read_mostly = {
-- 
2.39.2


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

* [PATCH nf-next 2/3] netfilter: nft_masq: deduplicate eval call-backs
  2023-03-13 13:46 [PATCH nf-next 0/3] NF NAT deduplication refactoring Jeremy Sowden
  2023-03-13 13:46 ` [PATCH nf-next 1/3] netfilter: nf_nat_redirect: use `struct nf_nat_range2` in ipv4 API Jeremy Sowden
@ 2023-03-13 13:46 ` Jeremy Sowden
  2023-03-13 13:46 ` [PATCH nf-next 3/3] netfilter: nft_redir: " Jeremy Sowden
  2023-03-14 10:52 ` [PATCH nf-next 0/3] NF NAT deduplication refactoring Pablo Neira Ayuso
  3 siblings, 0 replies; 8+ messages in thread
From: Jeremy Sowden @ 2023-03-13 13:46 UTC (permalink / raw)
  To: Netfilter Devel

nft_masq has separate ipv4 and ipv6 call-backs which share much of their
code, and an inet one switch containing a switch that calls one of the
others based on the family of the packet.  Merge the ipv4 and ipv6 ones
into the inet one in order to get rid of the duplicate code.

Const-qualify the `priv` pointer since we don't need to write through
it.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 net/netfilter/nft_masq.c | 75 ++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 46 deletions(-)

diff --git a/net/netfilter/nft_masq.c b/net/netfilter/nft_masq.c
index e55e455275c4..4bd79306eebd 100644
--- a/net/netfilter/nft_masq.c
+++ b/net/netfilter/nft_masq.c
@@ -96,23 +96,39 @@ static int nft_masq_dump(struct sk_buff *skb,
 	return -1;
 }
 
-static void nft_masq_ipv4_eval(const struct nft_expr *expr,
-			       struct nft_regs *regs,
-			       const struct nft_pktinfo *pkt)
+static void nft_masq_eval(const struct nft_expr *expr,
+			  struct nft_regs *regs,
+			  const struct nft_pktinfo *pkt)
 {
-	struct nft_masq *priv = nft_expr_priv(expr);
+	const struct nft_masq *priv = nft_expr_priv(expr);
 	struct nf_nat_range2 range;
 
 	memset(&range, 0, sizeof(range));
 	range.flags = priv->flags;
 	if (priv->sreg_proto_min) {
-		range.min_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_min]);
-		range.max_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_max]);
+		range.min_proto.all = (__force __be16)
+			nft_reg_load16(&regs->data[priv->sreg_proto_min]);
+		range.max_proto.all = (__force __be16)
+			nft_reg_load16(&regs->data[priv->sreg_proto_max]);
+	}
+
+	switch (nft_pf(pkt)) {
+	case NFPROTO_IPV4:
+		regs->verdict.code = nf_nat_masquerade_ipv4(pkt->skb,
+							    nft_hook(pkt),
+							    &range,
+							    nft_out(pkt));
+		break;
+#ifdef CONFIG_NF_TABLES_IPV6
+	case NFPROTO_IPV6:
+		regs->verdict.code = nf_nat_masquerade_ipv6(pkt->skb, &range,
+							    nft_out(pkt));
+		break;
+#endif
+	default:
+		WARN_ON_ONCE(1);
+		break;
 	}
-	regs->verdict.code = nf_nat_masquerade_ipv4(pkt->skb, nft_hook(pkt),
-						    &range, nft_out(pkt));
 }
 
 static void
@@ -125,7 +141,7 @@ static struct nft_expr_type nft_masq_ipv4_type;
 static const struct nft_expr_ops nft_masq_ipv4_ops = {
 	.type		= &nft_masq_ipv4_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_masq)),
-	.eval		= nft_masq_ipv4_eval,
+	.eval		= nft_masq_eval,
 	.init		= nft_masq_init,
 	.destroy	= nft_masq_ipv4_destroy,
 	.dump		= nft_masq_dump,
@@ -143,25 +159,6 @@ static struct nft_expr_type nft_masq_ipv4_type __read_mostly = {
 };
 
 #ifdef CONFIG_NF_TABLES_IPV6
-static void nft_masq_ipv6_eval(const struct nft_expr *expr,
-			       struct nft_regs *regs,
-			       const struct nft_pktinfo *pkt)
-{
-	struct nft_masq *priv = nft_expr_priv(expr);
-	struct nf_nat_range2 range;
-
-	memset(&range, 0, sizeof(range));
-	range.flags = priv->flags;
-	if (priv->sreg_proto_min) {
-		range.min_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_min]);
-		range.max_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_max]);
-	}
-	regs->verdict.code = nf_nat_masquerade_ipv6(pkt->skb, &range,
-						    nft_out(pkt));
-}
-
 static void
 nft_masq_ipv6_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr)
 {
@@ -172,7 +169,7 @@ static struct nft_expr_type nft_masq_ipv6_type;
 static const struct nft_expr_ops nft_masq_ipv6_ops = {
 	.type		= &nft_masq_ipv6_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_masq)),
-	.eval		= nft_masq_ipv6_eval,
+	.eval		= nft_masq_eval,
 	.init		= nft_masq_init,
 	.destroy	= nft_masq_ipv6_destroy,
 	.dump		= nft_masq_dump,
@@ -204,20 +201,6 @@ static inline void nft_masq_module_exit_ipv6(void) {}
 #endif
 
 #ifdef CONFIG_NF_TABLES_INET
-static void nft_masq_inet_eval(const struct nft_expr *expr,
-			       struct nft_regs *regs,
-			       const struct nft_pktinfo *pkt)
-{
-	switch (nft_pf(pkt)) {
-	case NFPROTO_IPV4:
-		return nft_masq_ipv4_eval(expr, regs, pkt);
-	case NFPROTO_IPV6:
-		return nft_masq_ipv6_eval(expr, regs, pkt);
-	}
-
-	WARN_ON_ONCE(1);
-}
-
 static void
 nft_masq_inet_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr)
 {
@@ -228,7 +211,7 @@ static struct nft_expr_type nft_masq_inet_type;
 static const struct nft_expr_ops nft_masq_inet_ops = {
 	.type		= &nft_masq_inet_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_masq)),
-	.eval		= nft_masq_inet_eval,
+	.eval		= nft_masq_eval,
 	.init		= nft_masq_init,
 	.destroy	= nft_masq_inet_destroy,
 	.dump		= nft_masq_dump,
-- 
2.39.2


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

* [PATCH nf-next 3/3] netfilter: nft_redir: deduplicate eval call-backs
  2023-03-13 13:46 [PATCH nf-next 0/3] NF NAT deduplication refactoring Jeremy Sowden
  2023-03-13 13:46 ` [PATCH nf-next 1/3] netfilter: nf_nat_redirect: use `struct nf_nat_range2` in ipv4 API Jeremy Sowden
  2023-03-13 13:46 ` [PATCH nf-next 2/3] netfilter: nft_masq: deduplicate eval call-backs Jeremy Sowden
@ 2023-03-13 13:46 ` Jeremy Sowden
  2023-03-14 10:52 ` [PATCH nf-next 0/3] NF NAT deduplication refactoring Pablo Neira Ayuso
  3 siblings, 0 replies; 8+ messages in thread
From: Jeremy Sowden @ 2023-03-13 13:46 UTC (permalink / raw)
  To: Netfilter Devel; +Cc: Florian Westphal

nft_redir has separate ipv4 and ipv6 call-backs which share much of
their code, and an inet one switch containing a switch that calls one of
the others based on the family of the packet.  Merge the ipv4 and ipv6
ones into the inet one in order to get rid of the duplicate code.

Const-qualify the `priv` pointer since we don't need to write through
it.

Assign `priv->flags` to the range instead of OR-ing it in.

Set the `NF_NAT_RANGE_PROTO_SPECIFIED` flag once during init, rather
than on every eval.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
Reviewed-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nft_redir.c | 81 ++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 52 deletions(-)

diff --git a/net/netfilter/nft_redir.c b/net/netfilter/nft_redir.c
index 77a459470cb7..1d52a05a8b03 100644
--- a/net/netfilter/nft_redir.c
+++ b/net/netfilter/nft_redir.c
@@ -64,6 +64,8 @@ static int nft_redir_init(const struct nft_ctx *ctx,
 		} else {
 			priv->sreg_proto_max = priv->sreg_proto_min;
 		}
+
+		priv->flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
 	}
 
 	if (tb[NFTA_REDIR_FLAGS]) {
@@ -99,26 +101,37 @@ static int nft_redir_dump(struct sk_buff *skb,
 	return -1;
 }
 
-static void nft_redir_ipv4_eval(const struct nft_expr *expr,
-				struct nft_regs *regs,
-				const struct nft_pktinfo *pkt)
+static void nft_redir_eval(const struct nft_expr *expr,
+			   struct nft_regs *regs,
+			   const struct nft_pktinfo *pkt)
 {
-	struct nft_redir *priv = nft_expr_priv(expr);
+	const struct nft_redir *priv = nft_expr_priv(expr);
 	struct nf_nat_range2 range;
 
 	memset(&range, 0, sizeof(range));
+	range.flags = priv->flags;
 	if (priv->sreg_proto_min) {
-		range.min_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_min]);
-		range.max_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_max]);
-		range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
+		range.min_proto.all = (__force __be16)
+			nft_reg_load16(&regs->data[priv->sreg_proto_min]);
+		range.max_proto.all = (__force __be16)
+			nft_reg_load16(&regs->data[priv->sreg_proto_max]);
 	}
 
-	range.flags |= priv->flags;
-
-	regs->verdict.code =
-		nf_nat_redirect_ipv4(pkt->skb, &range, nft_hook(pkt));
+	switch (nft_pf(pkt)) {
+	case NFPROTO_IPV4:
+		regs->verdict.code = nf_nat_redirect_ipv4(pkt->skb, &range,
+							  nft_hook(pkt));
+		break;
+#ifdef CONFIG_NF_TABLES_IPV6
+	case NFPROTO_IPV6:
+		regs->verdict.code = nf_nat_redirect_ipv6(pkt->skb, &range,
+							  nft_hook(pkt));
+		break;
+#endif
+	default:
+		WARN_ON_ONCE(1);
+		break;
+	}
 }
 
 static void
@@ -131,7 +144,7 @@ static struct nft_expr_type nft_redir_ipv4_type;
 static const struct nft_expr_ops nft_redir_ipv4_ops = {
 	.type		= &nft_redir_ipv4_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_redir)),
-	.eval		= nft_redir_ipv4_eval,
+	.eval		= nft_redir_eval,
 	.init		= nft_redir_init,
 	.destroy	= nft_redir_ipv4_destroy,
 	.dump		= nft_redir_dump,
@@ -149,28 +162,6 @@ static struct nft_expr_type nft_redir_ipv4_type __read_mostly = {
 };
 
 #ifdef CONFIG_NF_TABLES_IPV6
-static void nft_redir_ipv6_eval(const struct nft_expr *expr,
-				struct nft_regs *regs,
-				const struct nft_pktinfo *pkt)
-{
-	struct nft_redir *priv = nft_expr_priv(expr);
-	struct nf_nat_range2 range;
-
-	memset(&range, 0, sizeof(range));
-	if (priv->sreg_proto_min) {
-		range.min_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_min]);
-		range.max_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_max]);
-		range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
-	}
-
-	range.flags |= priv->flags;
-
-	regs->verdict.code =
-		nf_nat_redirect_ipv6(pkt->skb, &range, nft_hook(pkt));
-}
-
 static void
 nft_redir_ipv6_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr)
 {
@@ -181,7 +172,7 @@ static struct nft_expr_type nft_redir_ipv6_type;
 static const struct nft_expr_ops nft_redir_ipv6_ops = {
 	.type		= &nft_redir_ipv6_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_redir)),
-	.eval		= nft_redir_ipv6_eval,
+	.eval		= nft_redir_eval,
 	.init		= nft_redir_init,
 	.destroy	= nft_redir_ipv6_destroy,
 	.dump		= nft_redir_dump,
@@ -200,20 +191,6 @@ static struct nft_expr_type nft_redir_ipv6_type __read_mostly = {
 #endif
 
 #ifdef CONFIG_NF_TABLES_INET
-static void nft_redir_inet_eval(const struct nft_expr *expr,
-				struct nft_regs *regs,
-				const struct nft_pktinfo *pkt)
-{
-	switch (nft_pf(pkt)) {
-	case NFPROTO_IPV4:
-		return nft_redir_ipv4_eval(expr, regs, pkt);
-	case NFPROTO_IPV6:
-		return nft_redir_ipv6_eval(expr, regs, pkt);
-	}
-
-	WARN_ON_ONCE(1);
-}
-
 static void
 nft_redir_inet_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr)
 {
@@ -224,7 +201,7 @@ static struct nft_expr_type nft_redir_inet_type;
 static const struct nft_expr_ops nft_redir_inet_ops = {
 	.type		= &nft_redir_inet_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_redir)),
-	.eval		= nft_redir_inet_eval,
+	.eval		= nft_redir_eval,
 	.init		= nft_redir_init,
 	.destroy	= nft_redir_inet_destroy,
 	.dump		= nft_redir_dump,
-- 
2.39.2


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

* Re: [PATCH nf-next 0/3] NF NAT deduplication refactoring
  2023-03-13 13:46 [PATCH nf-next 0/3] NF NAT deduplication refactoring Jeremy Sowden
                   ` (2 preceding siblings ...)
  2023-03-13 13:46 ` [PATCH nf-next 3/3] netfilter: nft_redir: " Jeremy Sowden
@ 2023-03-14 10:52 ` Pablo Neira Ayuso
  2023-03-15 10:25   ` Pablo Neira Ayuso
  3 siblings, 1 reply; 8+ messages in thread
From: Pablo Neira Ayuso @ 2023-03-14 10:52 UTC (permalink / raw)
  To: Jeremy Sowden; +Cc: Netfilter Devel

Hi,

On Mon, Mar 13, 2023 at 01:46:46PM +0000, Jeremy Sowden wrote:
> These three patches perform refactoring in NF NAT modules to remove
> duplicate code.

Thanks for consolidating this.

I collapsed patch 1 and 3, in this particular case I think it is easy
to review to do these two things at once, I can see this:

+static unsigned int
+nf_nat_redirect(struct sk_buff *skb, const struct nf_nat_range2 *range,
+               const union nf_inet_addr *newdst)
+{
+       struct nf_nat_range2 newrange;
+       enum ip_conntrack_info ctinfo;
+       struct nf_conn *ct;
+
+       ct = nf_ct_get(skb, &ctinfo);
+       WARN_ON(!(ct && (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));

nf_nat_setup_info() calls _is_confirmed() which expect ct to be
non-null, so crash will happen right after this WARN_ON.

and _is_confirmed() implicitly means that this is either IP_CT_NEW or
IP_CT_RELATED.

This comes from 44d6e2f27328b25411 which replaced an assertion, quite
straight forward.

I'd suggest to remove this.

+       newrange.flags          = range->flags | NF_NAT_RANGE_MAP_IPS;
+       newrange.min_addr       = *newdst;
+       newrange.max_addr       = *newdst;
+       newrange.min_proto      = range->min_proto;
+       newrange.max_proto      = range->max_proto;

This newrange is missing memset(), other existing nat code calls
memset(), I think it might be related to iptables which is exposing
this structure to userspace through blob. I would need to check the
NAT core to check if this memset is really required.

+       return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_DST);
+}

May you submit v2 with these two changes?

Thanks.

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

* Re: [PATCH nf-next 0/3] NF NAT deduplication refactoring
  2023-03-14 10:52 ` [PATCH nf-next 0/3] NF NAT deduplication refactoring Pablo Neira Ayuso
@ 2023-03-15 10:25   ` Pablo Neira Ayuso
  2023-03-15 10:59     ` Jeremy Sowden
  0 siblings, 1 reply; 8+ messages in thread
From: Pablo Neira Ayuso @ 2023-03-15 10:25 UTC (permalink / raw)
  To: Jeremy Sowden; +Cc: Netfilter Devel

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

On Tue, Mar 14, 2023 at 11:52:49AM +0100, Pablo Neira Ayuso wrote:
> May you submit v2 with these two changes?

Something like this attached. It is doing all at once, but the patch
looks relatively easier to follow.

I can also document the removal of WARN_ON() and the use of
union nf_inet_addr newdst = {};

I am adding the memset() on range, but that is defensive. Probably all
memset() can just go away from the nftables nat code, but I need to
double check.

Thanks.

[-- Attachment #2: 0001-netfilter-nft_redir-deduplicate-eval-call-backs.patch --]
[-- Type: text/x-diff, Size: 11606 bytes --]

From b4e6d901cdf7f6adf43a66ec35829f6d90196326 Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Mon, 13 Mar 2023 13:46:47 +0000
Subject: [PATCH nf-next 1/2] netfilter: nft_redir: deduplicate eval call-backs

`nf_nat_redirect_ipv4` takes a `struct nf_nat_ipv4_multi_range_compat`,
but converts it internally to a `struct nf_nat_range2`.  Change the
function to take the latter, factor out the code now shared with
`nf_nat_redirect_ipv6`, move the conversion to the xt_REDIRECT module,
and update the ipv4 range initialization in the nft_redir module.

Replace a bare hex constant for 127.0.0.1 with a macro.

nft_redir has separate ipv4 and ipv6 call-backs which share much of
their code, and an inet one switch containing a switch that calls one of
the others based on the family of the packet.  Merge the ipv4 and ipv6
ones into the inet one in order to get rid of the duplicate code.

Const-qualify the `priv` pointer since we don't need to write through
it.

Assign `priv->flags` to the range instead of OR-ing it in.

Set the `NF_NAT_RANGE_PROTO_SPECIFIED` flag once during init, rather
than on every eval.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
Reviewed-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_nat_redirect.h |  3 +-
 net/netfilter/nf_nat_redirect.c         | 70 +++++++++------------
 net/netfilter/nft_redir.c               | 84 +++++++++----------------
 net/netfilter/xt_REDIRECT.c             | 10 ++-
 4 files changed, 71 insertions(+), 96 deletions(-)

diff --git a/include/net/netfilter/nf_nat_redirect.h b/include/net/netfilter/nf_nat_redirect.h
index 2418653a66db..279380de904c 100644
--- a/include/net/netfilter/nf_nat_redirect.h
+++ b/include/net/netfilter/nf_nat_redirect.h
@@ -6,8 +6,7 @@
 #include <uapi/linux/netfilter/nf_nat.h>
 
 unsigned int
-nf_nat_redirect_ipv4(struct sk_buff *skb,
-		     const struct nf_nat_ipv4_multi_range_compat *mr,
+nf_nat_redirect_ipv4(struct sk_buff *skb, const struct nf_nat_range2 *range,
 		     unsigned int hooknum);
 unsigned int
 nf_nat_redirect_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
diff --git a/net/netfilter/nf_nat_redirect.c b/net/netfilter/nf_nat_redirect.c
index f91579c821e9..083e534bded0 100644
--- a/net/netfilter/nf_nat_redirect.c
+++ b/net/netfilter/nf_nat_redirect.c
@@ -10,6 +10,7 @@
 
 #include <linux/if.h>
 #include <linux/inetdevice.h>
+#include <linux/in.h>
 #include <linux/ip.h>
 #include <linux/kernel.h>
 #include <linux/netdevice.h>
@@ -24,54 +25,55 @@
 #include <net/netfilter/nf_nat.h>
 #include <net/netfilter/nf_nat_redirect.h>
 
+static unsigned int
+nf_nat_redirect(struct sk_buff *skb, const struct nf_nat_range2 *range,
+		const union nf_inet_addr *newdst)
+{
+	struct nf_nat_range2 newrange;
+	enum ip_conntrack_info ctinfo;
+	struct nf_conn *ct;
+
+	ct = nf_ct_get(skb, &ctinfo);
+
+	memset(&newrange, 0, sizeof(newrange));
+	newrange.flags		= range->flags | NF_NAT_RANGE_MAP_IPS;
+	newrange.min_addr	= *newdst;
+	newrange.max_addr	= *newdst;
+	newrange.min_proto	= range->min_proto;
+	newrange.max_proto	= range->max_proto;
+
+	return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_DST);
+}
+
 unsigned int
-nf_nat_redirect_ipv4(struct sk_buff *skb,
-		     const struct nf_nat_ipv4_multi_range_compat *mr,
+nf_nat_redirect_ipv4(struct sk_buff *skb, const struct nf_nat_range2 *range,
 		     unsigned int hooknum)
 {
-	struct nf_conn *ct;
-	enum ip_conntrack_info ctinfo;
-	__be32 newdst;
-	struct nf_nat_range2 newrange;
+	union nf_inet_addr newdst = {};
 
 	WARN_ON(hooknum != NF_INET_PRE_ROUTING &&
 		hooknum != NF_INET_LOCAL_OUT);
 
-	ct = nf_ct_get(skb, &ctinfo);
-	WARN_ON(!(ct && (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));
-
 	/* Local packets: make them go to loopback */
 	if (hooknum == NF_INET_LOCAL_OUT) {
-		newdst = htonl(0x7F000001);
+		newdst.ip = htonl(INADDR_LOOPBACK);
 	} else {
 		const struct in_device *indev;
 
-		newdst = 0;
-
 		indev = __in_dev_get_rcu(skb->dev);
 		if (indev) {
 			const struct in_ifaddr *ifa;
 
 			ifa = rcu_dereference(indev->ifa_list);
 			if (ifa)
-				newdst = ifa->ifa_local;
+				newdst.ip = ifa->ifa_local;
 		}
 
-		if (!newdst)
+		if (!newdst.ip)
 			return NF_DROP;
 	}
 
-	/* Transfer from original range. */
-	memset(&newrange.min_addr, 0, sizeof(newrange.min_addr));
-	memset(&newrange.max_addr, 0, sizeof(newrange.max_addr));
-	newrange.flags	     = mr->range[0].flags | NF_NAT_RANGE_MAP_IPS;
-	newrange.min_addr.ip = newdst;
-	newrange.max_addr.ip = newdst;
-	newrange.min_proto   = mr->range[0].min;
-	newrange.max_proto   = mr->range[0].max;
-
-	/* Hand modified range to generic setup. */
-	return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_DST);
+	return nf_nat_redirect(skb, range, &newdst);
 }
 EXPORT_SYMBOL_GPL(nf_nat_redirect_ipv4);
 
@@ -81,14 +83,10 @@ unsigned int
 nf_nat_redirect_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
 		     unsigned int hooknum)
 {
-	struct nf_nat_range2 newrange;
-	struct in6_addr newdst;
-	enum ip_conntrack_info ctinfo;
-	struct nf_conn *ct;
+	union nf_inet_addr newdst = {};
 
-	ct = nf_ct_get(skb, &ctinfo);
 	if (hooknum == NF_INET_LOCAL_OUT) {
-		newdst = loopback_addr;
+		newdst.in6 = loopback_addr;
 	} else {
 		struct inet6_dev *idev;
 		struct inet6_ifaddr *ifa;
@@ -98,7 +96,7 @@ nf_nat_redirect_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
 		if (idev != NULL) {
 			read_lock_bh(&idev->lock);
 			list_for_each_entry(ifa, &idev->addr_list, if_list) {
-				newdst = ifa->addr;
+				newdst.in6 = ifa->addr;
 				addr = true;
 				break;
 			}
@@ -109,12 +107,6 @@ nf_nat_redirect_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
 			return NF_DROP;
 	}
 
-	newrange.flags		= range->flags | NF_NAT_RANGE_MAP_IPS;
-	newrange.min_addr.in6	= newdst;
-	newrange.max_addr.in6	= newdst;
-	newrange.min_proto	= range->min_proto;
-	newrange.max_proto	= range->max_proto;
-
-	return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_DST);
+	return nf_nat_redirect(skb, range, &newdst);
 }
 EXPORT_SYMBOL_GPL(nf_nat_redirect_ipv6);
diff --git a/net/netfilter/nft_redir.c b/net/netfilter/nft_redir.c
index 5f7739987559..1d52a05a8b03 100644
--- a/net/netfilter/nft_redir.c
+++ b/net/netfilter/nft_redir.c
@@ -64,6 +64,8 @@ static int nft_redir_init(const struct nft_ctx *ctx,
 		} else {
 			priv->sreg_proto_max = priv->sreg_proto_min;
 		}
+
+		priv->flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
 	}
 
 	if (tb[NFTA_REDIR_FLAGS]) {
@@ -99,25 +101,37 @@ static int nft_redir_dump(struct sk_buff *skb,
 	return -1;
 }
 
-static void nft_redir_ipv4_eval(const struct nft_expr *expr,
-				struct nft_regs *regs,
-				const struct nft_pktinfo *pkt)
+static void nft_redir_eval(const struct nft_expr *expr,
+			   struct nft_regs *regs,
+			   const struct nft_pktinfo *pkt)
 {
-	struct nft_redir *priv = nft_expr_priv(expr);
-	struct nf_nat_ipv4_multi_range_compat mr;
+	const struct nft_redir *priv = nft_expr_priv(expr);
+	struct nf_nat_range2 range;
 
-	memset(&mr, 0, sizeof(mr));
+	memset(&range, 0, sizeof(range));
+	range.flags = priv->flags;
 	if (priv->sreg_proto_min) {
-		mr.range[0].min.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_min]);
-		mr.range[0].max.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_max]);
-		mr.range[0].flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
+		range.min_proto.all = (__force __be16)
+			nft_reg_load16(&regs->data[priv->sreg_proto_min]);
+		range.max_proto.all = (__force __be16)
+			nft_reg_load16(&regs->data[priv->sreg_proto_max]);
 	}
 
-	mr.range[0].flags |= priv->flags;
-
-	regs->verdict.code = nf_nat_redirect_ipv4(pkt->skb, &mr, nft_hook(pkt));
+	switch (nft_pf(pkt)) {
+	case NFPROTO_IPV4:
+		regs->verdict.code = nf_nat_redirect_ipv4(pkt->skb, &range,
+							  nft_hook(pkt));
+		break;
+#ifdef CONFIG_NF_TABLES_IPV6
+	case NFPROTO_IPV6:
+		regs->verdict.code = nf_nat_redirect_ipv6(pkt->skb, &range,
+							  nft_hook(pkt));
+		break;
+#endif
+	default:
+		WARN_ON_ONCE(1);
+		break;
+	}
 }
 
 static void
@@ -130,7 +144,7 @@ static struct nft_expr_type nft_redir_ipv4_type;
 static const struct nft_expr_ops nft_redir_ipv4_ops = {
 	.type		= &nft_redir_ipv4_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_redir)),
-	.eval		= nft_redir_ipv4_eval,
+	.eval		= nft_redir_eval,
 	.init		= nft_redir_init,
 	.destroy	= nft_redir_ipv4_destroy,
 	.dump		= nft_redir_dump,
@@ -148,28 +162,6 @@ static struct nft_expr_type nft_redir_ipv4_type __read_mostly = {
 };
 
 #ifdef CONFIG_NF_TABLES_IPV6
-static void nft_redir_ipv6_eval(const struct nft_expr *expr,
-				struct nft_regs *regs,
-				const struct nft_pktinfo *pkt)
-{
-	struct nft_redir *priv = nft_expr_priv(expr);
-	struct nf_nat_range2 range;
-
-	memset(&range, 0, sizeof(range));
-	if (priv->sreg_proto_min) {
-		range.min_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_min]);
-		range.max_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_max]);
-		range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
-	}
-
-	range.flags |= priv->flags;
-
-	regs->verdict.code =
-		nf_nat_redirect_ipv6(pkt->skb, &range, nft_hook(pkt));
-}
-
 static void
 nft_redir_ipv6_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr)
 {
@@ -180,7 +172,7 @@ static struct nft_expr_type nft_redir_ipv6_type;
 static const struct nft_expr_ops nft_redir_ipv6_ops = {
 	.type		= &nft_redir_ipv6_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_redir)),
-	.eval		= nft_redir_ipv6_eval,
+	.eval		= nft_redir_eval,
 	.init		= nft_redir_init,
 	.destroy	= nft_redir_ipv6_destroy,
 	.dump		= nft_redir_dump,
@@ -199,20 +191,6 @@ static struct nft_expr_type nft_redir_ipv6_type __read_mostly = {
 #endif
 
 #ifdef CONFIG_NF_TABLES_INET
-static void nft_redir_inet_eval(const struct nft_expr *expr,
-				struct nft_regs *regs,
-				const struct nft_pktinfo *pkt)
-{
-	switch (nft_pf(pkt)) {
-	case NFPROTO_IPV4:
-		return nft_redir_ipv4_eval(expr, regs, pkt);
-	case NFPROTO_IPV6:
-		return nft_redir_ipv6_eval(expr, regs, pkt);
-	}
-
-	WARN_ON_ONCE(1);
-}
-
 static void
 nft_redir_inet_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr)
 {
@@ -223,7 +201,7 @@ static struct nft_expr_type nft_redir_inet_type;
 static const struct nft_expr_ops nft_redir_inet_ops = {
 	.type		= &nft_redir_inet_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_redir)),
-	.eval		= nft_redir_inet_eval,
+	.eval		= nft_redir_eval,
 	.init		= nft_redir_init,
 	.destroy	= nft_redir_inet_destroy,
 	.dump		= nft_redir_dump,
diff --git a/net/netfilter/xt_REDIRECT.c b/net/netfilter/xt_REDIRECT.c
index 353ca7801251..ff66b56a3f97 100644
--- a/net/netfilter/xt_REDIRECT.c
+++ b/net/netfilter/xt_REDIRECT.c
@@ -46,7 +46,6 @@ static void redirect_tg_destroy(const struct xt_tgdtor_param *par)
 	nf_ct_netns_put(par->net, par->family);
 }
 
-/* FIXME: Take multiple ranges --RR */
 static int redirect_tg4_check(const struct xt_tgchk_param *par)
 {
 	const struct nf_nat_ipv4_multi_range_compat *mr = par->targinfo;
@@ -65,7 +64,14 @@ static int redirect_tg4_check(const struct xt_tgchk_param *par)
 static unsigned int
 redirect_tg4(struct sk_buff *skb, const struct xt_action_param *par)
 {
-	return nf_nat_redirect_ipv4(skb, par->targinfo, xt_hooknum(par));
+	const struct nf_nat_ipv4_multi_range_compat *mr = par->targinfo;
+	struct nf_nat_range2 range = {
+		.flags       = mr->range[0].flags,
+		.min_proto   = mr->range[0].min,
+		.max_proto   = mr->range[0].max,
+	};
+
+	return nf_nat_redirect_ipv4(skb, &range, xt_hooknum(par));
 }
 
 static struct xt_target redirect_tg_reg[] __read_mostly = {
-- 
2.30.2


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

* Re: [PATCH nf-next 0/3] NF NAT deduplication refactoring
  2023-03-15 10:25   ` Pablo Neira Ayuso
@ 2023-03-15 10:59     ` Jeremy Sowden
  0 siblings, 0 replies; 8+ messages in thread
From: Jeremy Sowden @ 2023-03-15 10:59 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Netfilter Devel

[-- Attachment #1: Type: text/plain, Size: 13577 bytes --]

On 2023-03-15, at 11:25:02 +0100, Pablo Neira Ayuso wrote:
> On Tue, Mar 14, 2023 at 11:52:49AM +0100, Pablo Neira Ayuso wrote:
> > May you submit v2 with these two changes?
> 
> Something like this attached. It is doing all at once, but the patch
> looks relatively easier to follow.

Thanks for this.  I did make a start on v2, but yesterday was not very
productive.  I'll incorporate your suggestions and send it out later
to-day.

> I can also document the removal of WARN_ON() and the use of
> union nf_inet_addr newdst = {};
> 
> I am adding the memset() on range, but that is defensive. Probably all
> memset() can just go away from the nftables nat code, but I need to
> double check.

Yeah, I didn't see anywhere that appeared to require the memset, which
is why I took it out, but I didn't trace the whole lifecycle of the
variable, so I may have missed something.

J.

> From b4e6d901cdf7f6adf43a66ec35829f6d90196326 Mon Sep 17 00:00:00 2001
> From: Jeremy Sowden <jeremy@azazel.net>
> Date: Mon, 13 Mar 2023 13:46:47 +0000
> Subject: [PATCH nf-next 1/2] netfilter: nft_redir: deduplicate eval call-backs
> 
> `nf_nat_redirect_ipv4` takes a `struct nf_nat_ipv4_multi_range_compat`,
> but converts it internally to a `struct nf_nat_range2`.  Change the
> function to take the latter, factor out the code now shared with
> `nf_nat_redirect_ipv6`, move the conversion to the xt_REDIRECT module,
> and update the ipv4 range initialization in the nft_redir module.
> 
> Replace a bare hex constant for 127.0.0.1 with a macro.
> 
> nft_redir has separate ipv4 and ipv6 call-backs which share much of
> their code, and an inet one switch containing a switch that calls one of
> the others based on the family of the packet.  Merge the ipv4 and ipv6
> ones into the inet one in order to get rid of the duplicate code.
> 
> Const-qualify the `priv` pointer since we don't need to write through
> it.
> 
> Assign `priv->flags` to the range instead of OR-ing it in.
> 
> Set the `NF_NAT_RANGE_PROTO_SPECIFIED` flag once during init, rather
> than on every eval.
> 
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> Reviewed-by: Florian Westphal <fw@strlen.de>
> ---
>  include/net/netfilter/nf_nat_redirect.h |  3 +-
>  net/netfilter/nf_nat_redirect.c         | 70 +++++++++------------
>  net/netfilter/nft_redir.c               | 84 +++++++++----------------
>  net/netfilter/xt_REDIRECT.c             | 10 ++-
>  4 files changed, 71 insertions(+), 96 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_nat_redirect.h b/include/net/netfilter/nf_nat_redirect.h
> index 2418653a66db..279380de904c 100644
> --- a/include/net/netfilter/nf_nat_redirect.h
> +++ b/include/net/netfilter/nf_nat_redirect.h
> @@ -6,8 +6,7 @@
>  #include <uapi/linux/netfilter/nf_nat.h>
>  
>  unsigned int
> -nf_nat_redirect_ipv4(struct sk_buff *skb,
> -		     const struct nf_nat_ipv4_multi_range_compat *mr,
> +nf_nat_redirect_ipv4(struct sk_buff *skb, const struct nf_nat_range2 *range,
>  		     unsigned int hooknum);
>  unsigned int
>  nf_nat_redirect_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
> diff --git a/net/netfilter/nf_nat_redirect.c b/net/netfilter/nf_nat_redirect.c
> index f91579c821e9..083e534bded0 100644
> --- a/net/netfilter/nf_nat_redirect.c
> +++ b/net/netfilter/nf_nat_redirect.c
> @@ -10,6 +10,7 @@
>  
>  #include <linux/if.h>
>  #include <linux/inetdevice.h>
> +#include <linux/in.h>
>  #include <linux/ip.h>
>  #include <linux/kernel.h>
>  #include <linux/netdevice.h>
> @@ -24,54 +25,55 @@
>  #include <net/netfilter/nf_nat.h>
>  #include <net/netfilter/nf_nat_redirect.h>
>  
> +static unsigned int
> +nf_nat_redirect(struct sk_buff *skb, const struct nf_nat_range2 *range,
> +		const union nf_inet_addr *newdst)
> +{
> +	struct nf_nat_range2 newrange;
> +	enum ip_conntrack_info ctinfo;
> +	struct nf_conn *ct;
> +
> +	ct = nf_ct_get(skb, &ctinfo);
> +
> +	memset(&newrange, 0, sizeof(newrange));
> +	newrange.flags		= range->flags | NF_NAT_RANGE_MAP_IPS;
> +	newrange.min_addr	= *newdst;
> +	newrange.max_addr	= *newdst;
> +	newrange.min_proto	= range->min_proto;
> +	newrange.max_proto	= range->max_proto;
> +
> +	return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_DST);
> +}
> +
>  unsigned int
> -nf_nat_redirect_ipv4(struct sk_buff *skb,
> -		     const struct nf_nat_ipv4_multi_range_compat *mr,
> +nf_nat_redirect_ipv4(struct sk_buff *skb, const struct nf_nat_range2 *range,
>  		     unsigned int hooknum)
>  {
> -	struct nf_conn *ct;
> -	enum ip_conntrack_info ctinfo;
> -	__be32 newdst;
> -	struct nf_nat_range2 newrange;
> +	union nf_inet_addr newdst = {};
>  
>  	WARN_ON(hooknum != NF_INET_PRE_ROUTING &&
>  		hooknum != NF_INET_LOCAL_OUT);
>  
> -	ct = nf_ct_get(skb, &ctinfo);
> -	WARN_ON(!(ct && (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));
> -
>  	/* Local packets: make them go to loopback */
>  	if (hooknum == NF_INET_LOCAL_OUT) {
> -		newdst = htonl(0x7F000001);
> +		newdst.ip = htonl(INADDR_LOOPBACK);
>  	} else {
>  		const struct in_device *indev;
>  
> -		newdst = 0;
> -
>  		indev = __in_dev_get_rcu(skb->dev);
>  		if (indev) {
>  			const struct in_ifaddr *ifa;
>  
>  			ifa = rcu_dereference(indev->ifa_list);
>  			if (ifa)
> -				newdst = ifa->ifa_local;
> +				newdst.ip = ifa->ifa_local;
>  		}
>  
> -		if (!newdst)
> +		if (!newdst.ip)
>  			return NF_DROP;
>  	}
>  
> -	/* Transfer from original range. */
> -	memset(&newrange.min_addr, 0, sizeof(newrange.min_addr));
> -	memset(&newrange.max_addr, 0, sizeof(newrange.max_addr));
> -	newrange.flags	     = mr->range[0].flags | NF_NAT_RANGE_MAP_IPS;
> -	newrange.min_addr.ip = newdst;
> -	newrange.max_addr.ip = newdst;
> -	newrange.min_proto   = mr->range[0].min;
> -	newrange.max_proto   = mr->range[0].max;
> -
> -	/* Hand modified range to generic setup. */
> -	return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_DST);
> +	return nf_nat_redirect(skb, range, &newdst);
>  }
>  EXPORT_SYMBOL_GPL(nf_nat_redirect_ipv4);
>  
> @@ -81,14 +83,10 @@ unsigned int
>  nf_nat_redirect_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
>  		     unsigned int hooknum)
>  {
> -	struct nf_nat_range2 newrange;
> -	struct in6_addr newdst;
> -	enum ip_conntrack_info ctinfo;
> -	struct nf_conn *ct;
> +	union nf_inet_addr newdst = {};
>  
> -	ct = nf_ct_get(skb, &ctinfo);
>  	if (hooknum == NF_INET_LOCAL_OUT) {
> -		newdst = loopback_addr;
> +		newdst.in6 = loopback_addr;
>  	} else {
>  		struct inet6_dev *idev;
>  		struct inet6_ifaddr *ifa;
> @@ -98,7 +96,7 @@ nf_nat_redirect_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
>  		if (idev != NULL) {
>  			read_lock_bh(&idev->lock);
>  			list_for_each_entry(ifa, &idev->addr_list, if_list) {
> -				newdst = ifa->addr;
> +				newdst.in6 = ifa->addr;
>  				addr = true;
>  				break;
>  			}
> @@ -109,12 +107,6 @@ nf_nat_redirect_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
>  			return NF_DROP;
>  	}
>  
> -	newrange.flags		= range->flags | NF_NAT_RANGE_MAP_IPS;
> -	newrange.min_addr.in6	= newdst;
> -	newrange.max_addr.in6	= newdst;
> -	newrange.min_proto	= range->min_proto;
> -	newrange.max_proto	= range->max_proto;
> -
> -	return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_DST);
> +	return nf_nat_redirect(skb, range, &newdst);
>  }
>  EXPORT_SYMBOL_GPL(nf_nat_redirect_ipv6);
> diff --git a/net/netfilter/nft_redir.c b/net/netfilter/nft_redir.c
> index 5f7739987559..1d52a05a8b03 100644
> --- a/net/netfilter/nft_redir.c
> +++ b/net/netfilter/nft_redir.c
> @@ -64,6 +64,8 @@ static int nft_redir_init(const struct nft_ctx *ctx,
>  		} else {
>  			priv->sreg_proto_max = priv->sreg_proto_min;
>  		}
> +
> +		priv->flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
>  	}
>  
>  	if (tb[NFTA_REDIR_FLAGS]) {
> @@ -99,25 +101,37 @@ static int nft_redir_dump(struct sk_buff *skb,
>  	return -1;
>  }
>  
> -static void nft_redir_ipv4_eval(const struct nft_expr *expr,
> -				struct nft_regs *regs,
> -				const struct nft_pktinfo *pkt)
> +static void nft_redir_eval(const struct nft_expr *expr,
> +			   struct nft_regs *regs,
> +			   const struct nft_pktinfo *pkt)
>  {
> -	struct nft_redir *priv = nft_expr_priv(expr);
> -	struct nf_nat_ipv4_multi_range_compat mr;
> +	const struct nft_redir *priv = nft_expr_priv(expr);
> +	struct nf_nat_range2 range;
>  
> -	memset(&mr, 0, sizeof(mr));
> +	memset(&range, 0, sizeof(range));
> +	range.flags = priv->flags;
>  	if (priv->sreg_proto_min) {
> -		mr.range[0].min.all = (__force __be16)nft_reg_load16(
> -			&regs->data[priv->sreg_proto_min]);
> -		mr.range[0].max.all = (__force __be16)nft_reg_load16(
> -			&regs->data[priv->sreg_proto_max]);
> -		mr.range[0].flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
> +		range.min_proto.all = (__force __be16)
> +			nft_reg_load16(&regs->data[priv->sreg_proto_min]);
> +		range.max_proto.all = (__force __be16)
> +			nft_reg_load16(&regs->data[priv->sreg_proto_max]);
>  	}
>  
> -	mr.range[0].flags |= priv->flags;
> -
> -	regs->verdict.code = nf_nat_redirect_ipv4(pkt->skb, &mr, nft_hook(pkt));
> +	switch (nft_pf(pkt)) {
> +	case NFPROTO_IPV4:
> +		regs->verdict.code = nf_nat_redirect_ipv4(pkt->skb, &range,
> +							  nft_hook(pkt));
> +		break;
> +#ifdef CONFIG_NF_TABLES_IPV6
> +	case NFPROTO_IPV6:
> +		regs->verdict.code = nf_nat_redirect_ipv6(pkt->skb, &range,
> +							  nft_hook(pkt));
> +		break;
> +#endif
> +	default:
> +		WARN_ON_ONCE(1);
> +		break;
> +	}
>  }
>  
>  static void
> @@ -130,7 +144,7 @@ static struct nft_expr_type nft_redir_ipv4_type;
>  static const struct nft_expr_ops nft_redir_ipv4_ops = {
>  	.type		= &nft_redir_ipv4_type,
>  	.size		= NFT_EXPR_SIZE(sizeof(struct nft_redir)),
> -	.eval		= nft_redir_ipv4_eval,
> +	.eval		= nft_redir_eval,
>  	.init		= nft_redir_init,
>  	.destroy	= nft_redir_ipv4_destroy,
>  	.dump		= nft_redir_dump,
> @@ -148,28 +162,6 @@ static struct nft_expr_type nft_redir_ipv4_type __read_mostly = {
>  };
>  
>  #ifdef CONFIG_NF_TABLES_IPV6
> -static void nft_redir_ipv6_eval(const struct nft_expr *expr,
> -				struct nft_regs *regs,
> -				const struct nft_pktinfo *pkt)
> -{
> -	struct nft_redir *priv = nft_expr_priv(expr);
> -	struct nf_nat_range2 range;
> -
> -	memset(&range, 0, sizeof(range));
> -	if (priv->sreg_proto_min) {
> -		range.min_proto.all = (__force __be16)nft_reg_load16(
> -			&regs->data[priv->sreg_proto_min]);
> -		range.max_proto.all = (__force __be16)nft_reg_load16(
> -			&regs->data[priv->sreg_proto_max]);
> -		range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
> -	}
> -
> -	range.flags |= priv->flags;
> -
> -	regs->verdict.code =
> -		nf_nat_redirect_ipv6(pkt->skb, &range, nft_hook(pkt));
> -}
> -
>  static void
>  nft_redir_ipv6_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr)
>  {
> @@ -180,7 +172,7 @@ static struct nft_expr_type nft_redir_ipv6_type;
>  static const struct nft_expr_ops nft_redir_ipv6_ops = {
>  	.type		= &nft_redir_ipv6_type,
>  	.size		= NFT_EXPR_SIZE(sizeof(struct nft_redir)),
> -	.eval		= nft_redir_ipv6_eval,
> +	.eval		= nft_redir_eval,
>  	.init		= nft_redir_init,
>  	.destroy	= nft_redir_ipv6_destroy,
>  	.dump		= nft_redir_dump,
> @@ -199,20 +191,6 @@ static struct nft_expr_type nft_redir_ipv6_type __read_mostly = {
>  #endif
>  
>  #ifdef CONFIG_NF_TABLES_INET
> -static void nft_redir_inet_eval(const struct nft_expr *expr,
> -				struct nft_regs *regs,
> -				const struct nft_pktinfo *pkt)
> -{
> -	switch (nft_pf(pkt)) {
> -	case NFPROTO_IPV4:
> -		return nft_redir_ipv4_eval(expr, regs, pkt);
> -	case NFPROTO_IPV6:
> -		return nft_redir_ipv6_eval(expr, regs, pkt);
> -	}
> -
> -	WARN_ON_ONCE(1);
> -}
> -
>  static void
>  nft_redir_inet_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr)
>  {
> @@ -223,7 +201,7 @@ static struct nft_expr_type nft_redir_inet_type;
>  static const struct nft_expr_ops nft_redir_inet_ops = {
>  	.type		= &nft_redir_inet_type,
>  	.size		= NFT_EXPR_SIZE(sizeof(struct nft_redir)),
> -	.eval		= nft_redir_inet_eval,
> +	.eval		= nft_redir_eval,
>  	.init		= nft_redir_init,
>  	.destroy	= nft_redir_inet_destroy,
>  	.dump		= nft_redir_dump,
> diff --git a/net/netfilter/xt_REDIRECT.c b/net/netfilter/xt_REDIRECT.c
> index 353ca7801251..ff66b56a3f97 100644
> --- a/net/netfilter/xt_REDIRECT.c
> +++ b/net/netfilter/xt_REDIRECT.c
> @@ -46,7 +46,6 @@ static void redirect_tg_destroy(const struct xt_tgdtor_param *par)
>  	nf_ct_netns_put(par->net, par->family);
>  }
>  
> -/* FIXME: Take multiple ranges --RR */
>  static int redirect_tg4_check(const struct xt_tgchk_param *par)
>  {
>  	const struct nf_nat_ipv4_multi_range_compat *mr = par->targinfo;
> @@ -65,7 +64,14 @@ static int redirect_tg4_check(const struct xt_tgchk_param *par)
>  static unsigned int
>  redirect_tg4(struct sk_buff *skb, const struct xt_action_param *par)
>  {
> -	return nf_nat_redirect_ipv4(skb, par->targinfo, xt_hooknum(par));
> +	const struct nf_nat_ipv4_multi_range_compat *mr = par->targinfo;
> +	struct nf_nat_range2 range = {
> +		.flags       = mr->range[0].flags,
> +		.min_proto   = mr->range[0].min,
> +		.max_proto   = mr->range[0].max,
> +	};
> +
> +	return nf_nat_redirect_ipv4(skb, &range, xt_hooknum(par));
>  }
>  
>  static struct xt_target redirect_tg_reg[] __read_mostly = {
> -- 
> 2.30.2
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* [PATCH nf-next 2/3] netfilter: nft_masq: deduplicate eval call-backs
  2023-03-15 21:47 Jeremy Sowden
@ 2023-03-15 21:47 ` Jeremy Sowden
  0 siblings, 0 replies; 8+ messages in thread
From: Jeremy Sowden @ 2023-03-15 21:47 UTC (permalink / raw)
  To: Netfilter Devel

nft_masq has separate ipv4 and ipv6 call-backs which share much of their
code, and an inet one switch containing a switch that calls one of the
others based on the family of the packet.  Merge the ipv4 and ipv6 ones
into the inet one in order to get rid of the duplicate code.

Const-qualify the `priv` pointer since we don't need to write through
it.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 net/netfilter/nft_masq.c | 75 ++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 46 deletions(-)

diff --git a/net/netfilter/nft_masq.c b/net/netfilter/nft_masq.c
index e55e455275c4..4bd79306eebd 100644
--- a/net/netfilter/nft_masq.c
+++ b/net/netfilter/nft_masq.c
@@ -96,23 +96,39 @@ static int nft_masq_dump(struct sk_buff *skb,
 	return -1;
 }
 
-static void nft_masq_ipv4_eval(const struct nft_expr *expr,
-			       struct nft_regs *regs,
-			       const struct nft_pktinfo *pkt)
+static void nft_masq_eval(const struct nft_expr *expr,
+			  struct nft_regs *regs,
+			  const struct nft_pktinfo *pkt)
 {
-	struct nft_masq *priv = nft_expr_priv(expr);
+	const struct nft_masq *priv = nft_expr_priv(expr);
 	struct nf_nat_range2 range;
 
 	memset(&range, 0, sizeof(range));
 	range.flags = priv->flags;
 	if (priv->sreg_proto_min) {
-		range.min_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_min]);
-		range.max_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_max]);
+		range.min_proto.all = (__force __be16)
+			nft_reg_load16(&regs->data[priv->sreg_proto_min]);
+		range.max_proto.all = (__force __be16)
+			nft_reg_load16(&regs->data[priv->sreg_proto_max]);
+	}
+
+	switch (nft_pf(pkt)) {
+	case NFPROTO_IPV4:
+		regs->verdict.code = nf_nat_masquerade_ipv4(pkt->skb,
+							    nft_hook(pkt),
+							    &range,
+							    nft_out(pkt));
+		break;
+#ifdef CONFIG_NF_TABLES_IPV6
+	case NFPROTO_IPV6:
+		regs->verdict.code = nf_nat_masquerade_ipv6(pkt->skb, &range,
+							    nft_out(pkt));
+		break;
+#endif
+	default:
+		WARN_ON_ONCE(1);
+		break;
 	}
-	regs->verdict.code = nf_nat_masquerade_ipv4(pkt->skb, nft_hook(pkt),
-						    &range, nft_out(pkt));
 }
 
 static void
@@ -125,7 +141,7 @@ static struct nft_expr_type nft_masq_ipv4_type;
 static const struct nft_expr_ops nft_masq_ipv4_ops = {
 	.type		= &nft_masq_ipv4_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_masq)),
-	.eval		= nft_masq_ipv4_eval,
+	.eval		= nft_masq_eval,
 	.init		= nft_masq_init,
 	.destroy	= nft_masq_ipv4_destroy,
 	.dump		= nft_masq_dump,
@@ -143,25 +159,6 @@ static struct nft_expr_type nft_masq_ipv4_type __read_mostly = {
 };
 
 #ifdef CONFIG_NF_TABLES_IPV6
-static void nft_masq_ipv6_eval(const struct nft_expr *expr,
-			       struct nft_regs *regs,
-			       const struct nft_pktinfo *pkt)
-{
-	struct nft_masq *priv = nft_expr_priv(expr);
-	struct nf_nat_range2 range;
-
-	memset(&range, 0, sizeof(range));
-	range.flags = priv->flags;
-	if (priv->sreg_proto_min) {
-		range.min_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_min]);
-		range.max_proto.all = (__force __be16)nft_reg_load16(
-			&regs->data[priv->sreg_proto_max]);
-	}
-	regs->verdict.code = nf_nat_masquerade_ipv6(pkt->skb, &range,
-						    nft_out(pkt));
-}
-
 static void
 nft_masq_ipv6_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr)
 {
@@ -172,7 +169,7 @@ static struct nft_expr_type nft_masq_ipv6_type;
 static const struct nft_expr_ops nft_masq_ipv6_ops = {
 	.type		= &nft_masq_ipv6_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_masq)),
-	.eval		= nft_masq_ipv6_eval,
+	.eval		= nft_masq_eval,
 	.init		= nft_masq_init,
 	.destroy	= nft_masq_ipv6_destroy,
 	.dump		= nft_masq_dump,
@@ -204,20 +201,6 @@ static inline void nft_masq_module_exit_ipv6(void) {}
 #endif
 
 #ifdef CONFIG_NF_TABLES_INET
-static void nft_masq_inet_eval(const struct nft_expr *expr,
-			       struct nft_regs *regs,
-			       const struct nft_pktinfo *pkt)
-{
-	switch (nft_pf(pkt)) {
-	case NFPROTO_IPV4:
-		return nft_masq_ipv4_eval(expr, regs, pkt);
-	case NFPROTO_IPV6:
-		return nft_masq_ipv6_eval(expr, regs, pkt);
-	}
-
-	WARN_ON_ONCE(1);
-}
-
 static void
 nft_masq_inet_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr)
 {
@@ -228,7 +211,7 @@ static struct nft_expr_type nft_masq_inet_type;
 static const struct nft_expr_ops nft_masq_inet_ops = {
 	.type		= &nft_masq_inet_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_masq)),
-	.eval		= nft_masq_inet_eval,
+	.eval		= nft_masq_eval,
 	.init		= nft_masq_init,
 	.destroy	= nft_masq_inet_destroy,
 	.dump		= nft_masq_dump,
-- 
2.39.2


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

end of thread, other threads:[~2023-03-15 21:47 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-13 13:46 [PATCH nf-next 0/3] NF NAT deduplication refactoring Jeremy Sowden
2023-03-13 13:46 ` [PATCH nf-next 1/3] netfilter: nf_nat_redirect: use `struct nf_nat_range2` in ipv4 API Jeremy Sowden
2023-03-13 13:46 ` [PATCH nf-next 2/3] netfilter: nft_masq: deduplicate eval call-backs Jeremy Sowden
2023-03-13 13:46 ` [PATCH nf-next 3/3] netfilter: nft_redir: " Jeremy Sowden
2023-03-14 10:52 ` [PATCH nf-next 0/3] NF NAT deduplication refactoring Pablo Neira Ayuso
2023-03-15 10:25   ` Pablo Neira Ayuso
2023-03-15 10:59     ` Jeremy Sowden
2023-03-15 21:47 Jeremy Sowden
2023-03-15 21:47 ` [PATCH nf-next 2/3] netfilter: nft_masq: deduplicate eval call-backs Jeremy Sowden

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