All of lore.kernel.org
 help / color / mirror / Atom feed
* [iptables PATCH 0/4] Speed up iptables-nft-save
@ 2022-03-02 15:18 Phil Sutter
  2022-03-02 15:18 ` [iptables PATCH 1/4] nft: Simplify immediate parsing Phil Sutter
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Phil Sutter @ 2022-03-02 15:18 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

OpenShift tends to create huge rulesets[*] and consequently wonders why
iptables is slow. Anyway, iptables-nft-save really does odd things:

* For each jump target, it checks if such extension exists, despite
  already knowing whether the target is a chain or not. This is solved
  in patch 2, with patch 1 preparing nft_parse_immediate() to make the
  change simpler.

* Every l4proto match causes a call to getprotobynumber() which in turn
  opens /etc/protocols. One could cache the lookups, but preferring the
  builtin fallback list of protocol names and numbers is much simpler.
  Do that in patch 3, aligning said list with /etc/protocols to not
  change output unexpectedly.

[*] I have a "real-life" dump adding 50k chains and 130k rules. Dumping
    it to /dev/null took 45s on my testing VM, with these patches
    applied I'm down to 2.7s.

The last patch is fallout from the above, simplifying family ops
callbacks a bit given that there is only a single *tables_command_state
struct left.

Phil Sutter (4):
  nft: Simplify immediate parsing
  nft: Speed up immediate parsing
  xshared: Prefer xtables_chain_protos lookup over getprotoent
  nft: Don't pass command state opaque to family ops callbacks

 iptables/nft-arp.c    | 32 ++++++------------
 iptables/nft-bridge.c | 55 +++++++++++++------------------
 iptables/nft-ipv4.c   | 40 ++++++++---------------
 iptables/nft-ipv6.c   | 40 ++++++++---------------
 iptables/nft-shared.c | 75 ++++++++++++++++++++-----------------------
 iptables/nft-shared.h | 35 ++++++++++----------
 iptables/nft.c        |  4 +--
 iptables/nft.h        |  2 +-
 iptables/xshared.c    |  8 ++---
 libxtables/xtables.c  | 19 ++++-------
 10 files changed, 126 insertions(+), 184 deletions(-)

-- 
2.34.1


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

* [iptables PATCH 1/4] nft: Simplify immediate parsing
  2022-03-02 15:18 [iptables PATCH 0/4] Speed up iptables-nft-save Phil Sutter
@ 2022-03-02 15:18 ` Phil Sutter
  2022-03-10 12:09   ` Florian Westphal
  2022-03-02 15:18 ` [iptables PATCH 2/4] nft: Speed up " Phil Sutter
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Phil Sutter @ 2022-03-02 15:18 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Implementations of parse_immediate callback are mostly trivial, the only
relevant part is access to family-specific parts of struct
iptables_command_state when setting goto flag for iptables and
ip6tables. Refactor them into simple set_goto_flag callbacks.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-arp.c    |  9 ---------
 iptables/nft-bridge.c |  9 ---------
 iptables/nft-ipv4.c   | 12 +++---------
 iptables/nft-ipv6.c   | 12 +++---------
 iptables/nft-shared.c | 17 +++++++----------
 iptables/nft-shared.h |  2 +-
 6 files changed, 14 insertions(+), 47 deletions(-)

diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 1472b11543239..78509ce9d87e8 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -182,14 +182,6 @@ static void nft_arp_parse_meta(struct nft_xt_ctx *ctx, struct nftnl_expr *e,
 	fw->arp.invflags |= flags;
 }
 
-static void nft_arp_parse_immediate(const char *jumpto, bool nft_goto,
-				    void *data)
-{
-	struct iptables_command_state *cs = data;
-
-	cs->jumpto = jumpto;
-}
-
 static void parse_mask_ipv4(struct nft_xt_ctx *ctx, struct in_addr *mask)
 {
 	mask->s_addr = ctx->bitwise.mask[0];
@@ -797,7 +789,6 @@ struct nft_family_ops nft_family_ops_arp = {
 	.print_payload		= NULL,
 	.parse_meta		= nft_arp_parse_meta,
 	.parse_payload		= nft_arp_parse_payload,
-	.parse_immediate	= nft_arp_parse_immediate,
 	.print_header		= nft_arp_print_header,
 	.print_rule		= nft_arp_print_rule,
 	.save_rule		= nft_arp_save_rule,
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 90d55e441ab95..d6a0d6e518fcb 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -251,14 +251,6 @@ static void nft_bridge_parse_payload(struct nft_xt_ctx *ctx,
 	}
 }
 
-static void nft_bridge_parse_immediate(const char *jumpto, bool nft_goto,
-				       void *data)
-{
-	struct iptables_command_state *cs = data;
-
-	cs->jumpto = jumpto;
-}
-
 /* return 0 if saddr, 1 if daddr, -1 on error */
 static int
 lookup_check_ether_payload(uint32_t base, uint32_t offset, uint32_t len)
@@ -891,7 +883,6 @@ struct nft_family_ops nft_family_ops_bridge = {
 	.print_payload		= NULL,
 	.parse_meta		= nft_bridge_parse_meta,
 	.parse_payload		= nft_bridge_parse_payload,
-	.parse_immediate	= nft_bridge_parse_immediate,
 	.parse_lookup		= nft_bridge_parse_lookup,
 	.parse_match		= nft_bridge_parse_match,
 	.parse_target		= nft_bridge_parse_target,
diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c
index f374d468d2ff4..bdb105f8eb683 100644
--- a/iptables/nft-ipv4.c
+++ b/iptables/nft-ipv4.c
@@ -215,15 +215,9 @@ static void nft_ipv4_parse_payload(struct nft_xt_ctx *ctx,
 	}
 }
 
-static void nft_ipv4_parse_immediate(const char *jumpto, bool nft_goto,
-				     void *data)
+static void nft_ipv4_set_goto_flag(struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
-
-	cs->jumpto = jumpto;
-
-	if (nft_goto)
-		cs->fw.ip.flags |= IPT_F_GOTO;
+	cs->fw.ip.flags |= IPT_F_GOTO;
 }
 
 static void nft_ipv4_print_rule(struct nft_handle *h, struct nftnl_rule *r,
@@ -450,7 +444,7 @@ struct nft_family_ops nft_family_ops_ipv4 = {
 	.is_same		= nft_ipv4_is_same,
 	.parse_meta		= nft_ipv4_parse_meta,
 	.parse_payload		= nft_ipv4_parse_payload,
-	.parse_immediate	= nft_ipv4_parse_immediate,
+	.set_goto_flag		= nft_ipv4_set_goto_flag,
 	.print_header		= print_header,
 	.print_rule		= nft_ipv4_print_rule,
 	.save_rule		= nft_ipv4_save_rule,
diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c
index 9ecc754f37805..a5323171bb4bb 100644
--- a/iptables/nft-ipv6.c
+++ b/iptables/nft-ipv6.c
@@ -180,15 +180,9 @@ static void nft_ipv6_parse_payload(struct nft_xt_ctx *ctx,
 	}
 }
 
-static void nft_ipv6_parse_immediate(const char *jumpto, bool nft_goto,
-				     void *data)
+static void nft_ipv6_set_goto_flag(struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
-
-	cs->jumpto = jumpto;
-
-	if (nft_goto)
-		cs->fw6.ipv6.flags |= IP6T_F_GOTO;
+	cs->fw6.ipv6.flags |= IP6T_F_GOTO;
 }
 
 static void nft_ipv6_print_rule(struct nft_handle *h, struct nftnl_rule *r,
@@ -418,7 +412,7 @@ struct nft_family_ops nft_family_ops_ipv6 = {
 	.is_same		= nft_ipv6_is_same,
 	.parse_meta		= nft_ipv6_parse_meta,
 	.parse_payload		= nft_ipv6_parse_payload,
-	.parse_immediate	= nft_ipv6_parse_immediate,
+	.set_goto_flag		= nft_ipv6_set_goto_flag,
 	.print_header		= print_header,
 	.print_rule		= nft_ipv6_print_rule,
 	.save_rule		= nft_ipv6_save_rule,
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 746a948ccf96d..daa251ae0982a 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -906,9 +906,7 @@ static void nft_parse_counter(struct nftnl_expr *e, struct xt_counters *counters
 static void nft_parse_immediate(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 {
 	const char *chain = nftnl_expr_get_str(e, NFTNL_EXPR_IMM_CHAIN);
-	const char *jumpto = NULL;
-	bool nft_goto = false;
-	void *data = ctx->cs;
+	struct iptables_command_state *cs = ctx->cs;
 	int verdict;
 
 	if (nftnl_expr_is_set(e, NFTNL_EXPR_IMM_DATA)) {
@@ -931,23 +929,22 @@ static void nft_parse_immediate(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 	/* Standard target? */
 	switch(verdict) {
 	case NF_ACCEPT:
-		jumpto = "ACCEPT";
+		cs->jumpto = "ACCEPT";
 		break;
 	case NF_DROP:
-		jumpto = "DROP";
+		cs->jumpto = "DROP";
 		break;
 	case NFT_RETURN:
-		jumpto = "RETURN";
+		cs->jumpto = "RETURN";
 		break;;
 	case NFT_GOTO:
-		nft_goto = true;
+		if (ctx->h->ops->set_goto_flag)
+			ctx->h->ops->set_goto_flag(cs);
 		/* fall through */
 	case NFT_JUMP:
-		jumpto = chain;
+		cs->jumpto = chain;
 		break;
 	}
-
-	ctx->h->ops->parse_immediate(jumpto, nft_goto, data);
 }
 
 static void nft_parse_limit(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index 0788e98a9f93a..04b1d97f950d5 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -93,7 +93,7 @@ struct nft_family_ops {
 			  void *data);
 	void (*parse_lookup)(struct nft_xt_ctx *ctx, struct nftnl_expr *e,
 			     void *data);
-	void (*parse_immediate)(const char *jumpto, bool nft_goto, void *data);
+	void (*set_goto_flag)(struct iptables_command_state *cs);
 
 	void (*print_table_header)(const char *tablename);
 	void (*print_header)(unsigned int format, const char *chain,
-- 
2.34.1


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

* [iptables PATCH 2/4] nft: Speed up immediate parsing
  2022-03-02 15:18 [iptables PATCH 0/4] Speed up iptables-nft-save Phil Sutter
  2022-03-02 15:18 ` [iptables PATCH 1/4] nft: Simplify immediate parsing Phil Sutter
@ 2022-03-02 15:18 ` Phil Sutter
  2022-03-02 15:18 ` [iptables PATCH 3/4] xshared: Prefer xtables_chain_protos lookup over getprotoent Phil Sutter
  2022-03-02 15:18 ` [iptables PATCH 4/4] nft: Don't pass command state opaque to family ops callbacks Phil Sutter
  3 siblings, 0 replies; 10+ messages in thread
From: Phil Sutter @ 2022-03-02 15:18 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Parsing of rules which jump to a chain pointlessly causes a call to
xtables_find_target() despite the code already knowing the outcome.

Avoid the significant delay for rulesets with many chain jumps by
performing the (standard) target lookup only for accept/drop/return
verdicts.

From a biased test-case on my VM:

| # iptables-nft-save | grep -c -- '-j'
| 133943
| # time ./old/iptables-nft-save >/dev/null
| real	0m45.566s
| user	0m1.308s
| sys	0m8.430s
| # time ./new/iptables-nft-save >/dev/null
| real	0m3.547s
| user	0m0.762s
| sys	0m2.476s

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
The same benchmark completes in 1.7s with legacy, in case anyone
wonders.
---
 iptables/nft-bridge.c |  1 +
 iptables/nft-shared.c | 37 ++++++++++++++++++-------------------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index d6a0d6e518fcb..d342858e1d9d8 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -502,6 +502,7 @@ static void nft_bridge_parse_target(struct xtables_target *t, void *data)
 	}
 
 	cs->target = t;
+	cs->jumpto = t->name;
 }
 
 static void nft_rule_to_ebtables_command_state(struct nft_handle *h,
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index daa251ae0982a..861aa0642061e 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -907,6 +907,8 @@ static void nft_parse_immediate(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 {
 	const char *chain = nftnl_expr_get_str(e, NFTNL_EXPR_IMM_CHAIN);
 	struct iptables_command_state *cs = ctx->cs;
+	struct xt_entry_target *t;
+	uint32_t size;
 	int verdict;
 
 	if (nftnl_expr_is_set(e, NFTNL_EXPR_IMM_DATA)) {
@@ -943,8 +945,21 @@ static void nft_parse_immediate(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 		/* fall through */
 	case NFT_JUMP:
 		cs->jumpto = chain;
-		break;
+		/* fall through */
+	default:
+		return;
 	}
+
+	cs->target = xtables_find_target(cs->jumpto, XTF_TRY_LOAD);
+	if (!cs->target)
+		return;
+
+	size = XT_ALIGN(sizeof(struct xt_entry_target)) + cs->target->size;
+	t = xtables_calloc(1, size);
+	t->u.target_size = size;
+	t->u.user.revision = cs->target->revision;
+	strcpy(t->u.user.name, cs->jumpto);
+	cs->target->t = t;
 }
 
 static void nft_parse_limit(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
@@ -1143,25 +1158,8 @@ void nft_rule_to_iptables_command_state(struct nft_handle *h,
 		}
 	}
 
-	if (cs->target != NULL) {
-		cs->jumpto = cs->target->name;
-	} else if (cs->jumpto != NULL) {
-		struct xt_entry_target *t;
-		uint32_t size;
-
-		cs->target = xtables_find_target(cs->jumpto, XTF_TRY_LOAD);
-		if (!cs->target)
-			return;
-
-		size = XT_ALIGN(sizeof(struct xt_entry_target)) + cs->target->size;
-		t = xtables_calloc(1, size);
-		t->u.target_size = size;
-		t->u.user.revision = cs->target->revision;
-		strcpy(t->u.user.name, cs->jumpto);
-		cs->target->t = t;
-	} else {
+	if (!cs->jumpto)
 		cs->jumpto = "";
-	}
 }
 
 void nft_clear_iptables_command_state(struct iptables_command_state *cs)
@@ -1326,6 +1324,7 @@ void nft_ipv46_parse_target(struct xtables_target *t, void *data)
 	struct iptables_command_state *cs = data;
 
 	cs->target = t;
+	cs->jumpto = t->name;
 }
 
 void nft_check_xt_legacy(int family, bool is_ipt_save)
-- 
2.34.1


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

* [iptables PATCH 3/4] xshared: Prefer xtables_chain_protos lookup over getprotoent
  2022-03-02 15:18 [iptables PATCH 0/4] Speed up iptables-nft-save Phil Sutter
  2022-03-02 15:18 ` [iptables PATCH 1/4] nft: Simplify immediate parsing Phil Sutter
  2022-03-02 15:18 ` [iptables PATCH 2/4] nft: Speed up " Phil Sutter
@ 2022-03-02 15:18 ` Phil Sutter
  2022-03-10 12:11   ` Florian Westphal
  2022-03-02 15:18 ` [iptables PATCH 4/4] nft: Don't pass command state opaque to family ops callbacks Phil Sutter
  3 siblings, 1 reply; 10+ messages in thread
From: Phil Sutter @ 2022-03-02 15:18 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

When dumping a large ruleset, common protocol matches such as for TCP
port number significantly slow down rule printing due to repeated calls
for getprotobynumber(). The latter does not involve any caching, so
/etc/protocols is consulted over and over again.

As a simple countermeasure, make functions converting between proto
number and name prefer the built-in list of "well-known" protocols. This
is not a perfect solution, repeated rules for protocol names libxtables
does not cache (e.g. igmp or dccp) will still be slow. Implementing
getprotoent() result caching could solve this.

As a side-effect, explicit check for pseudo-protocol "all" may be
dropped as it is contained in the built-in list and therefore immutable.

Also update xtables_chain_protos entries a bit to align with typical
/etc/protocols contents. The testsuite assumes those names, so the
preferred ones prior to this patch are indeed uncommon nowadays.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xshared.c   |  8 ++++----
 libxtables/xtables.c | 19 ++++++-------------
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/iptables/xshared.c b/iptables/xshared.c
index 50a1d48a55ebe..43321d3b5358c 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -53,16 +53,16 @@ proto_to_name(uint16_t proto, int nolookup)
 {
 	unsigned int i;
 
+	for (i = 0; xtables_chain_protos[i].name != NULL; ++i)
+		if (xtables_chain_protos[i].num == proto)
+			return xtables_chain_protos[i].name;
+
 	if (proto && !nolookup) {
 		struct protoent *pent = getprotobynumber(proto);
 		if (pent)
 			return pent->p_name;
 	}
 
-	for (i = 0; xtables_chain_protos[i].name != NULL; ++i)
-		if (xtables_chain_protos[i].num == proto)
-			return xtables_chain_protos[i].name;
-
 	return NULL;
 }
 
diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 87424d045466b..094cbd87ec1ed 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -2101,10 +2101,11 @@ const struct xtables_pprot xtables_chain_protos[] = {
 	{"udp",       IPPROTO_UDP},
 	{"udplite",   IPPROTO_UDPLITE},
 	{"icmp",      IPPROTO_ICMP},
-	{"icmpv6",    IPPROTO_ICMPV6},
 	{"ipv6-icmp", IPPROTO_ICMPV6},
+	{"icmpv6",    IPPROTO_ICMPV6},
 	{"esp",       IPPROTO_ESP},
 	{"ah",        IPPROTO_AH},
+	{"mobility-header", IPPROTO_MH},
 	{"ipv6-mh",   IPPROTO_MH},
 	{"mh",        IPPROTO_MH},
 	{"all",       0},
@@ -2120,23 +2121,15 @@ xtables_parse_protocol(const char *s)
 	if (xtables_strtoui(s, NULL, &proto, 0, UINT8_MAX))
 		return proto;
 
-	/* first deal with the special case of 'all' to prevent
-	 * people from being able to redefine 'all' in nsswitch
-	 * and/or provoke expensive [not working] ldap/nis/...
-	 * lookups */
-	if (strcmp(s, "all") == 0)
-		return 0;
+	for (i = 0; xtables_chain_protos[i].name != NULL; ++i) {
+		if (strcmp(s, xtables_chain_protos[i].name) == 0)
+			return xtables_chain_protos[i].num;
+	}
 
 	pent = getprotobyname(s);
 	if (pent != NULL)
 		return pent->p_proto;
 
-	for (i = 0; i < ARRAY_SIZE(xtables_chain_protos); ++i) {
-		if (xtables_chain_protos[i].name == NULL)
-			continue;
-		if (strcmp(s, xtables_chain_protos[i].name) == 0)
-			return xtables_chain_protos[i].num;
-	}
 	xt_params->exit_err(PARAMETER_PROBLEM,
 		"unknown protocol \"%s\" specified", s);
 	return -1;
-- 
2.34.1


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

* [iptables PATCH 4/4] nft: Don't pass command state opaque to family ops callbacks
  2022-03-02 15:18 [iptables PATCH 0/4] Speed up iptables-nft-save Phil Sutter
                   ` (2 preceding siblings ...)
  2022-03-02 15:18 ` [iptables PATCH 3/4] xshared: Prefer xtables_chain_protos lookup over getprotoent Phil Sutter
@ 2022-03-02 15:18 ` Phil Sutter
  2022-03-10 12:14   ` Florian Westphal
  3 siblings, 1 reply; 10+ messages in thread
From: Phil Sutter @ 2022-03-02 15:18 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

There are no family-specific versions of struct iptables_command_state
anymore, so no need to hide it behind void pointer. Pass the type as-is
and save a few casts.

While at it, drop unused callbacks parse_bitwise and parse_cmp.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-arp.c    | 23 ++++++++++------------
 iptables/nft-bridge.c | 45 +++++++++++++++++++++----------------------
 iptables/nft-ipv4.c   | 28 +++++++++++----------------
 iptables/nft-ipv6.c   | 28 +++++++++++----------------
 iptables/nft-shared.c | 23 ++++++++++------------
 iptables/nft-shared.h | 33 ++++++++++++++++---------------
 iptables/nft.c        |  4 ++--
 iptables/nft.h        |  2 +-
 8 files changed, 84 insertions(+), 102 deletions(-)

diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 78509ce9d87e8..028b06a608e4e 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -54,9 +54,9 @@ static bool need_devaddr(struct arpt_devaddr_info *info)
 	return false;
 }
 
-static int nft_arp_add(struct nft_handle *h, struct nftnl_rule *r, void *data)
+static int nft_arp_add(struct nft_handle *h, struct nftnl_rule *r,
+		       struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
 	struct arpt_entry *fw = &cs->arp;
 	uint32_t op;
 	int ret = 0;
@@ -169,9 +169,8 @@ static int nft_arp_add(struct nft_handle *h, struct nftnl_rule *r, void *data)
 }
 
 static void nft_arp_parse_meta(struct nft_xt_ctx *ctx, struct nftnl_expr *e,
-			       void *data)
+			       struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
 	struct arpt_entry *fw = &cs->arp;
 	uint8_t flags = 0;
 
@@ -213,9 +212,9 @@ static bool nft_arp_parse_devaddr(struct nft_xt_ctx *ctx,
 }
 
 static void nft_arp_parse_payload(struct nft_xt_ctx *ctx,
-				  struct nftnl_expr *e, void *data)
+				  struct nftnl_expr *e,
+				  struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
 	struct arpt_entry *fw = &cs->arp;
 	struct in_addr addr;
 	uint16_t ar_hrd, ar_pro, ar_op;
@@ -464,10 +463,8 @@ after_devdst:
 }
 
 static void
-nft_arp_save_rule(const void *data, unsigned int format)
+nft_arp_save_rule(const struct iptables_command_state *cs, unsigned int format)
 {
-	const struct iptables_command_state *cs = data;
-
 	format |= FMT_NUMERIC;
 
 	printf(" ");
@@ -504,11 +501,11 @@ nft_arp_print_rule(struct nft_handle *h, struct nftnl_rule *r,
 	nft_clear_iptables_command_state(&cs);
 }
 
-static bool nft_arp_is_same(const void *data_a,
-			    const void *data_b)
+static bool nft_arp_is_same(const struct iptables_command_state *cs_a,
+			    const struct iptables_command_state *cs_b)
 {
-	const struct arpt_entry *a = data_a;
-	const struct arpt_entry *b = data_b;
+	const struct arpt_entry *a = &cs_a->arp;
+	const struct arpt_entry *b = &cs_b->arp;
 
 	if (a->arp.src.s_addr != b->arp.src.s_addr
 	    || a->arp.tgt.s_addr != b->arp.tgt.s_addr
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index d342858e1d9d8..d4b66a25c740e 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -97,9 +97,9 @@ static int _add_action(struct nftnl_rule *r, struct iptables_command_state *cs)
 }
 
 static int nft_bridge_add(struct nft_handle *h,
-			  struct nftnl_rule *r, void *data)
+			  struct nftnl_rule *r,
+			  struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
 	struct ebt_match *iter;
 	struct ebt_entry *fw = &cs->eb;
 	uint32_t op;
@@ -164,9 +164,9 @@ static int nft_bridge_add(struct nft_handle *h,
 }
 
 static void nft_bridge_parse_meta(struct nft_xt_ctx *ctx,
-				  struct nftnl_expr *e, void *data)
+				  struct nftnl_expr *e,
+				  struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
 	struct ebt_entry *fw = &cs->eb;
 	uint8_t invflags = 0;
 	char iifname[IFNAMSIZ] = {}, oifname[IFNAMSIZ] = {};
@@ -200,9 +200,9 @@ static void nft_bridge_parse_meta(struct nft_xt_ctx *ctx,
 }
 
 static void nft_bridge_parse_payload(struct nft_xt_ctx *ctx,
-				     struct nftnl_expr *e, void *data)
+				     struct nftnl_expr *e,
+				     struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
 	struct ebt_entry *fw = &cs->eb;
 	unsigned char addr[ETH_ALEN];
 	unsigned short int ethproto;
@@ -397,7 +397,7 @@ static struct nftnl_set *set_from_lookup_expr(struct nft_xt_ctx *ctx,
 }
 
 static void nft_bridge_parse_lookup(struct nft_xt_ctx *ctx,
-				    struct nftnl_expr *e, void *data)
+				    struct nftnl_expr *e)
 {
 	struct xtables_match *match = NULL;
 	struct nft_among_data *among_data;
@@ -483,17 +483,15 @@ static void parse_watcher(void *object, struct ebt_match **match_list,
 		(*match_list)->next = m;
 }
 
-static void nft_bridge_parse_match(struct xtables_match *m, void *data)
+static void nft_bridge_parse_match(struct xtables_match *m,
+				   struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
-
 	parse_watcher(m, &cs->match_list, true);
 }
 
-static void nft_bridge_parse_target(struct xtables_target *t, void *data)
+static void nft_bridge_parse_target(struct xtables_target *t,
+				    struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
-
 	/* harcoded names :-( */
 	if (strcmp(t->name, "log") == 0 ||
 	    strcmp(t->name, "nflog") == 0) {
@@ -594,10 +592,9 @@ static void print_protocol(uint16_t ethproto, bool invert, unsigned int bitmask)
 		printf("%s ", ent->e_name);
 }
 
-static void __nft_bridge_save_rule(const void *data, unsigned int format)
+static void __nft_bridge_save_rule(const struct iptables_command_state *cs,
+				   unsigned int format)
 {
-	const struct iptables_command_state *cs = data;
-
 	if (cs->eb.ethproto)
 		print_protocol(cs->eb.ethproto, cs->eb.invflags & EBT_IPROTO,
 			       cs->eb.bitmask);
@@ -645,10 +642,11 @@ static void __nft_bridge_save_rule(const void *data, unsigned int format)
 		fputc('\n', stdout);
 }
 
-static void nft_bridge_save_rule(const void *data, unsigned int format)
+static void nft_bridge_save_rule(const struct iptables_command_state *cs,
+				 unsigned int format)
 {
 	printf(" ");
-	__nft_bridge_save_rule(data, format);
+	__nft_bridge_save_rule(cs, format);
 }
 
 static void nft_bridge_print_rule(struct nft_handle *h, struct nftnl_rule *r,
@@ -672,10 +670,11 @@ static void nft_bridge_save_chain(const struct nftnl_chain *c,
 	printf(":%s %s\n", chain, policy ?: "ACCEPT");
 }
 
-static bool nft_bridge_is_same(const void *data_a, const void *data_b)
+static bool nft_bridge_is_same(const struct iptables_command_state *cs_a,
+			       const struct iptables_command_state *cs_b)
 {
-	const struct ebt_entry *a = data_a;
-	const struct ebt_entry *b = data_b;
+	const struct ebt_entry *a = &cs_a->eb;
+	const struct ebt_entry *b = &cs_b->eb;
 	int i;
 
 	if (a->ethproto != b->ethproto ||
@@ -821,9 +820,9 @@ static void nft_bridge_xlate_mac(struct xt_xlate *xl, const char *type, bool inv
 	xt_xlate_add(xl, " ");
 }
 
-static int nft_bridge_xlate(const void *data, struct xt_xlate *xl)
+static int nft_bridge_xlate(const struct iptables_command_state *cs,
+			    struct xt_xlate *xl)
 {
-	const struct iptables_command_state *cs = data;
 	int ret;
 
 	xlate_ifname(xl, "iifname", cs->eb.in,
diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c
index bdb105f8eb683..af3d0c98b7989 100644
--- a/iptables/nft-ipv4.c
+++ b/iptables/nft-ipv4.c
@@ -26,9 +26,9 @@
 #include "nft.h"
 #include "nft-shared.h"
 
-static int nft_ipv4_add(struct nft_handle *h, struct nftnl_rule *r, void *data)
+static int nft_ipv4_add(struct nft_handle *h, struct nftnl_rule *r,
+			struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
 	struct xtables_rule_match *matchp;
 	uint32_t op;
 	int ret;
@@ -93,12 +93,9 @@ static int nft_ipv4_add(struct nft_handle *h, struct nftnl_rule *r, void *data)
 	return add_action(r, cs, !!(cs->fw.ip.flags & IPT_F_GOTO));
 }
 
-static bool nft_ipv4_is_same(const void *data_a,
-			     const void *data_b)
+static bool nft_ipv4_is_same(const struct iptables_command_state *a,
+			     const struct iptables_command_state *b)
 {
-	const struct iptables_command_state *a = data_a;
-	const struct iptables_command_state *b = data_b;
-
 	if (a->fw.ip.src.s_addr != b->fw.ip.src.s_addr
 	    || a->fw.ip.dst.s_addr != b->fw.ip.dst.s_addr
 	    || a->fw.ip.smsk.s_addr != b->fw.ip.smsk.s_addr
@@ -135,10 +132,8 @@ static void get_frag(struct nft_xt_ctx *ctx, struct nftnl_expr *e, bool *inv)
 }
 
 static void nft_ipv4_parse_meta(struct nft_xt_ctx *ctx, struct nftnl_expr *e,
-				void *data)
+				struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
-
 	switch (ctx->meta.key) {
 	case NFT_META_L4PROTO:
 		cs->fw.ip.proto = nftnl_expr_get_u8(e, NFTNL_EXPR_CMP_DATA);
@@ -160,9 +155,9 @@ static void parse_mask_ipv4(struct nft_xt_ctx *ctx, struct in_addr *mask)
 }
 
 static void nft_ipv4_parse_payload(struct nft_xt_ctx *ctx,
-				   struct nftnl_expr *e, void *data)
+				   struct nftnl_expr *e,
+				   struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
 	struct in_addr addr;
 	uint8_t proto;
 	bool inv;
@@ -250,10 +245,9 @@ static void nft_ipv4_print_rule(struct nft_handle *h, struct nftnl_rule *r,
 	nft_clear_iptables_command_state(&cs);
 }
 
-static void nft_ipv4_save_rule(const void *data, unsigned int format)
+static void nft_ipv4_save_rule(const struct iptables_command_state *cs,
+			       unsigned int format)
 {
-	const struct iptables_command_state *cs = data;
-
 	save_ipv4_addr('s', &cs->fw.ip.src, &cs->fw.ip.smsk,
 		       cs->fw.ip.invflags & IPT_INV_SRCIP);
 	save_ipv4_addr('d', &cs->fw.ip.dst, &cs->fw.ip.dmsk,
@@ -296,9 +290,9 @@ static void xlate_ipv4_addr(const char *selector, const struct in_addr *addr,
 	}
 }
 
-static int nft_ipv4_xlate(const void *data, struct xt_xlate *xl)
+static int nft_ipv4_xlate(const struct iptables_command_state *cs,
+			  struct xt_xlate *xl)
 {
-	const struct iptables_command_state *cs = data;
 	const char *comment;
 	int ret;
 
diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c
index a5323171bb4bb..892a485415933 100644
--- a/iptables/nft-ipv6.c
+++ b/iptables/nft-ipv6.c
@@ -25,9 +25,9 @@
 #include "nft.h"
 #include "nft-shared.h"
 
-static int nft_ipv6_add(struct nft_handle *h, struct nftnl_rule *r, void *data)
+static int nft_ipv6_add(struct nft_handle *h, struct nftnl_rule *r,
+			struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
 	struct xtables_rule_match *matchp;
 	uint32_t op;
 	int ret;
@@ -82,12 +82,9 @@ static int nft_ipv6_add(struct nft_handle *h, struct nftnl_rule *r, void *data)
 	return add_action(r, cs, !!(cs->fw6.ipv6.flags & IP6T_F_GOTO));
 }
 
-static bool nft_ipv6_is_same(const void *data_a,
-			     const void *data_b)
+static bool nft_ipv6_is_same(const struct iptables_command_state *a,
+			     const struct iptables_command_state *b)
 {
-	const struct iptables_command_state *a = data_a;
-	const struct iptables_command_state *b = data_b;
-
 	if (memcmp(a->fw6.ipv6.src.s6_addr, b->fw6.ipv6.src.s6_addr,
 		   sizeof(struct in6_addr)) != 0
 	    || memcmp(a->fw6.ipv6.dst.s6_addr, b->fw6.ipv6.dst.s6_addr,
@@ -108,10 +105,8 @@ static bool nft_ipv6_is_same(const void *data_a,
 }
 
 static void nft_ipv6_parse_meta(struct nft_xt_ctx *ctx, struct nftnl_expr *e,
-				void *data)
+				struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
-
 	switch (ctx->meta.key) {
 	case NFT_META_L4PROTO:
 		cs->fw6.ipv6.proto = nftnl_expr_get_u8(e, NFTNL_EXPR_CMP_DATA);
@@ -133,9 +128,9 @@ static void parse_mask_ipv6(struct nft_xt_ctx *ctx, struct in6_addr *mask)
 }
 
 static void nft_ipv6_parse_payload(struct nft_xt_ctx *ctx,
-				   struct nftnl_expr *e, void *data)
+				   struct nftnl_expr *e,
+				   struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
 	struct in6_addr addr;
 	uint8_t proto;
 	bool inv;
@@ -213,10 +208,9 @@ static void nft_ipv6_print_rule(struct nft_handle *h, struct nftnl_rule *r,
 	nft_clear_iptables_command_state(&cs);
 }
 
-static void nft_ipv6_save_rule(const void *data, unsigned int format)
+static void nft_ipv6_save_rule(const struct iptables_command_state *cs,
+			       unsigned int format)
 {
-	const struct iptables_command_state *cs = data;
-
 	save_ipv6_addr('s', &cs->fw6.ipv6.src, &cs->fw6.ipv6.smsk,
 		       cs->fw6.ipv6.invflags & IP6T_INV_SRCIP);
 	save_ipv6_addr('d', &cs->fw6.ipv6.dst, &cs->fw6.ipv6.dmsk,
@@ -257,9 +251,9 @@ static void xlate_ipv6_addr(const char *selector, const struct in6_addr *addr,
 	}
 }
 
-static int nft_ipv6_xlate(const void *data, struct xt_xlate *xl)
+static int nft_ipv6_xlate(const struct iptables_command_state *cs,
+			  struct xt_xlate *xl)
 {
-	const struct iptables_command_state *cs = data;
 	const char *comment;
 	int ret;
 
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 861aa0642061e..c57218542c964 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -319,7 +319,6 @@ static void nft_parse_target(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 	struct xtables_target *target;
 	struct xt_entry_target *t;
 	size_t size;
-	void *data = ctx->cs;
 
 	target = xtables_find_target(targname, XTF_TRY_LOAD);
 	if (target == NULL)
@@ -335,7 +334,7 @@ static void nft_parse_target(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 
 	target->t = t;
 
-	ctx->h->ops->parse_target(target, data);
+	ctx->h->ops->parse_target(target, ctx->cs);
 }
 
 static void nft_parse_match(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
@@ -767,9 +766,9 @@ static void nft_parse_tcp_flags(struct nft_xt_ctx *ctx,
 }
 
 static void nft_parse_transport(struct nft_xt_ctx *ctx,
-				struct nftnl_expr *e, void *data)
+				struct nftnl_expr *e,
+				struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
 	uint32_t sdport;
 	uint16_t port;
 	uint8_t proto, op;
@@ -869,7 +868,6 @@ static void nft_parse_transport_range(struct nft_xt_ctx *ctx,
 
 static void nft_parse_cmp(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 {
-	void *data = ctx->cs;
 	uint32_t reg;
 
 	reg = nftnl_expr_get_u32(e, NFTNL_EXPR_CMP_SREG);
@@ -877,7 +875,7 @@ static void nft_parse_cmp(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 		return;
 
 	if (ctx->flags & NFT_XT_CTX_META) {
-		ctx->h->ops->parse_meta(ctx, e, data);
+		ctx->h->ops->parse_meta(ctx, e, ctx->cs);
 		ctx->flags &= ~NFT_XT_CTX_META;
 	}
 	/* bitwise context is interpreted from payload */
@@ -885,13 +883,13 @@ static void nft_parse_cmp(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 		switch (ctx->payload.base) {
 		case NFT_PAYLOAD_LL_HEADER:
 			if (ctx->h->family == NFPROTO_BRIDGE)
-				ctx->h->ops->parse_payload(ctx, e, data);
+				ctx->h->ops->parse_payload(ctx, e, ctx->cs);
 			break;
 		case NFT_PAYLOAD_NETWORK_HEADER:
-			ctx->h->ops->parse_payload(ctx, e, data);
+			ctx->h->ops->parse_payload(ctx, e, ctx->cs);
 			break;
 		case NFT_PAYLOAD_TRANSPORT_HEADER:
-			nft_parse_transport(ctx, e, data);
+			nft_parse_transport(ctx, e, ctx->cs);
 			break;
 		}
 	}
@@ -1055,7 +1053,7 @@ static void nft_parse_lookup(struct nft_xt_ctx *ctx, struct nft_handle *h,
 			     struct nftnl_expr *e)
 {
 	if (ctx->h->ops->parse_lookup)
-		ctx->h->ops->parse_lookup(ctx, e, NULL);
+		ctx->h->ops->parse_lookup(ctx, e);
 }
 
 static void nft_parse_range(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
@@ -1319,10 +1317,9 @@ bool compare_targets(struct xtables_target *tg1, struct xtables_target *tg2)
 	return true;
 }
 
-void nft_ipv46_parse_target(struct xtables_target *t, void *data)
+void nft_ipv46_parse_target(struct xtables_target *t,
+			    struct iptables_command_state *cs)
 {
-	struct iptables_command_state *cs = data;
-
 	cs->target = t;
 	cs->jumpto = t->name;
 }
diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index 04b1d97f950d5..7b337943836a4 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -78,21 +78,17 @@ struct nft_xt_ctx {
 };
 
 struct nft_family_ops {
-	int (*add)(struct nft_handle *h, struct nftnl_rule *r, void *data);
-	bool (*is_same)(const void *data_a,
-			const void *data_b);
+	int (*add)(struct nft_handle *h, struct nftnl_rule *r,
+		   struct iptables_command_state *cs);
+	bool (*is_same)(const struct iptables_command_state *cs_a,
+			const struct iptables_command_state *cs_b);
 	void (*print_payload)(struct nftnl_expr *e,
 			      struct nftnl_expr_iter *iter);
 	void (*parse_meta)(struct nft_xt_ctx *ctx, struct nftnl_expr *e,
-			   void *data);
+			   struct iptables_command_state *cs);
 	void (*parse_payload)(struct nft_xt_ctx *ctx, struct nftnl_expr *e,
-			      void *data);
-	void (*parse_bitwise)(struct nft_xt_ctx *ctx, struct nftnl_expr *e,
-			      void *data);
-	void (*parse_cmp)(struct nft_xt_ctx *ctx, struct nftnl_expr *e,
-			  void *data);
-	void (*parse_lookup)(struct nft_xt_ctx *ctx, struct nftnl_expr *e,
-			     void *data);
+			      struct iptables_command_state *cs);
+	void (*parse_lookup)(struct nft_xt_ctx *ctx, struct nftnl_expr *e);
 	void (*set_goto_flag)(struct iptables_command_state *cs);
 
 	void (*print_table_header)(const char *tablename);
@@ -102,16 +98,20 @@ struct nft_family_ops {
 			     int refs, uint32_t entries);
 	void (*print_rule)(struct nft_handle *h, struct nftnl_rule *r,
 			   unsigned int num, unsigned int format);
-	void (*save_rule)(const void *data, unsigned int format);
+	void (*save_rule)(const struct iptables_command_state *cs,
+			  unsigned int format);
 	void (*save_chain)(const struct nftnl_chain *c, const char *policy);
 	struct xt_cmd_parse_ops cmd_parse;
-	void (*parse_match)(struct xtables_match *m, void *data);
-	void (*parse_target)(struct xtables_target *t, void *data);
+	void (*parse_match)(struct xtables_match *m,
+			    struct iptables_command_state *cs);
+	void (*parse_target)(struct xtables_target *t,
+			     struct iptables_command_state *cs);
 	void (*init_cs)(struct iptables_command_state *cs);
 	void (*rule_to_cs)(struct nft_handle *h, const struct nftnl_rule *r,
 			   struct iptables_command_state *cs);
 	void (*clear_cs)(struct iptables_command_state *cs);
-	int (*xlate)(const void *data, struct xt_xlate *xl);
+	int (*xlate)(const struct iptables_command_state *cs,
+		     struct xt_xlate *xl);
 	int (*add_entry)(struct nft_handle *h,
 			 const char *chain, const char *table,
 			 struct iptables_command_state *cs,
@@ -173,7 +173,8 @@ void save_matches_and_target(const struct iptables_command_state *cs,
 
 struct nft_family_ops *nft_family_ops_lookup(int family);
 
-void nft_ipv46_parse_target(struct xtables_target *t, void *data);
+void nft_ipv46_parse_target(struct xtables_target *t,
+			    struct iptables_command_state *cs);
 
 bool compare_matches(struct xtables_rule_match *mt1, struct xtables_rule_match *mt2);
 bool compare_targets(struct xtables_target *tg1, struct xtables_target *tg2);
diff --git a/iptables/nft.c b/iptables/nft.c
index d011d7c88da12..6883662fa28d2 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1648,7 +1648,7 @@ void add_compat(struct nftnl_rule *r, uint32_t proto, bool inv)
 
 struct nftnl_rule *
 nft_rule_new(struct nft_handle *h, const char *chain, const char *table,
-	     void *data)
+	     struct iptables_command_state *cs)
 {
 	struct nftnl_rule *r;
 
@@ -1660,7 +1660,7 @@ nft_rule_new(struct nft_handle *h, const char *chain, const char *table,
 	nftnl_rule_set_str(r, NFTNL_RULE_TABLE, table);
 	nftnl_rule_set_str(r, NFTNL_RULE_CHAIN, chain);
 
-	if (h->ops->add(h, r, data) < 0)
+	if (h->ops->add(h, r, cs) < 0)
 		goto err;
 
 	return r;
diff --git a/iptables/nft.h b/iptables/nft.h
index fd116c2e3e198..68b0910c8e182 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -173,7 +173,7 @@ struct nftnl_set *nft_set_batch_lookup_byid(struct nft_handle *h,
  */
 struct nftnl_rule;
 
-struct nftnl_rule *nft_rule_new(struct nft_handle *h, const char *chain, const char *table, void *data);
+struct nftnl_rule *nft_rule_new(struct nft_handle *h, const char *chain, const char *table, struct iptables_command_state *cs);
 int nft_rule_append(struct nft_handle *h, const char *chain, const char *table, struct nftnl_rule *r, struct nftnl_rule *ref, bool verbose);
 int nft_rule_insert(struct nft_handle *h, const char *chain, const char *table, struct nftnl_rule *r, int rulenum, bool verbose);
 int nft_rule_check(struct nft_handle *h, const char *chain, const char *table, struct nftnl_rule *r, bool verbose);
-- 
2.34.1


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

* Re: [iptables PATCH 1/4] nft: Simplify immediate parsing
  2022-03-02 15:18 ` [iptables PATCH 1/4] nft: Simplify immediate parsing Phil Sutter
@ 2022-03-10 12:09   ` Florian Westphal
  0 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2022-03-10 12:09 UTC (permalink / raw)
  To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel

Phil Sutter <phil@nwl.cc> wrote:
> Implementations of parse_immediate callback are mostly trivial, the only
> relevant part is access to family-specific parts of struct
> iptables_command_state when setting goto flag for iptables and
> ip6tables. Refactor them into simple set_goto_flag callbacks.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Reviewed-by: Florian Westphal <fw@strlen.de>

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

* Re: [iptables PATCH 3/4] xshared: Prefer xtables_chain_protos lookup over getprotoent
  2022-03-02 15:18 ` [iptables PATCH 3/4] xshared: Prefer xtables_chain_protos lookup over getprotoent Phil Sutter
@ 2022-03-10 12:11   ` Florian Westphal
  2022-03-10 12:20     ` Phil Sutter
  0 siblings, 1 reply; 10+ messages in thread
From: Florian Westphal @ 2022-03-10 12:11 UTC (permalink / raw)
  To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel

Phil Sutter <phil@nwl.cc> wrote:
> When dumping a large ruleset, common protocol matches such as for TCP
> port number significantly slow down rule printing due to repeated calls
> for getprotobynumber(). The latter does not involve any caching, so
> /etc/protocols is consulted over and over again.

> As a simple countermeasure, make functions converting between proto
> number and name prefer the built-in list of "well-known" protocols. This
> is not a perfect solution, repeated rules for protocol names libxtables
> does not cache (e.g. igmp or dccp) will still be slow. Implementing
> getprotoent() result caching could solve this.

Hmm, I think we could just extend xtables_chain_protos[].
Anyway, this looks safe to me, so

Acked-by: Florian Westphal <fw@strlen.de>

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

* Re: [iptables PATCH 4/4] nft: Don't pass command state opaque to family ops callbacks
  2022-03-02 15:18 ` [iptables PATCH 4/4] nft: Don't pass command state opaque to family ops callbacks Phil Sutter
@ 2022-03-10 12:14   ` Florian Westphal
  0 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2022-03-10 12:14 UTC (permalink / raw)
  To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel

Phil Sutter <phil@nwl.cc> wrote:
> There are no family-specific versions of struct iptables_command_state
> anymore, so no need to hide it behind void pointer. Pass the type as-is
> and save a few casts.

Thanks, that is a good improvement to have.

Acked-by: Florian Westphal <fw@strlen.de>

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

* Re: [iptables PATCH 3/4] xshared: Prefer xtables_chain_protos lookup over getprotoent
  2022-03-10 12:11   ` Florian Westphal
@ 2022-03-10 12:20     ` Phil Sutter
       [not found]       ` <20220310122303.GC13772@breakpoint.cc>
  0 siblings, 1 reply; 10+ messages in thread
From: Phil Sutter @ 2022-03-10 12:20 UTC (permalink / raw)
  To: Florian Westphal; +Cc: Pablo Neira Ayuso, netfilter-devel

On Thu, Mar 10, 2022 at 01:11:55PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > When dumping a large ruleset, common protocol matches such as for TCP
> > port number significantly slow down rule printing due to repeated calls
> > for getprotobynumber(). The latter does not involve any caching, so
> > /etc/protocols is consulted over and over again.
> 
> > As a simple countermeasure, make functions converting between proto
> > number and name prefer the built-in list of "well-known" protocols. This
> > is not a perfect solution, repeated rules for protocol names libxtables
> > does not cache (e.g. igmp or dccp) will still be slow. Implementing
> > getprotoent() result caching could solve this.
> 
> Hmm, I think we could just extend xtables_chain_protos[].

Statically, i.e. add more entries based on "usual" /etc/protocols
contents or dynamically from getprotoent() results?

> Anyway, this looks safe to me, so
> 
> Acked-by: Florian Westphal <fw@strlen.de>

Thanks!

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

* Re: [iptables PATCH 3/4] xshared: Prefer xtables_chain_protos lookup over getprotoent
       [not found]       ` <20220310122303.GC13772@breakpoint.cc>
@ 2022-03-10 12:54         ` Phil Sutter
  0 siblings, 0 replies; 10+ messages in thread
From: Phil Sutter @ 2022-03-10 12:54 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel, Pablo Neira Ayuso

[restored Cc list]

On Thu, Mar 10, 2022 at 01:23:03PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > On Thu, Mar 10, 2022 at 01:11:55PM +0100, Florian Westphal wrote:
> > > Phil Sutter <phil@nwl.cc> wrote:
> > > > When dumping a large ruleset, common protocol matches such as for TCP
> > > > port number significantly slow down rule printing due to repeated calls
> > > > for getprotobynumber(). The latter does not involve any caching, so
> > > > /etc/protocols is consulted over and over again.
> > > 
> > > > As a simple countermeasure, make functions converting between proto
> > > > number and name prefer the built-in list of "well-known" protocols. This
> > > > is not a perfect solution, repeated rules for protocol names libxtables
> > > > does not cache (e.g. igmp or dccp) will still be slow. Implementing
> > > > getprotoent() result caching could solve this.
> > > 
> > > Hmm, I think we could just extend xtables_chain_protos[].
> > 
> > Statically, i.e. add more entries based on "usual" /etc/protocols
> > contents or dynamically from getprotoent() results?
> 
> I meant statically, I don't see why you'd need to do that for igmp or
> dccp (or any other well-known protocol for that matter).

I hesitated because we take users' ability to override the definitions.
Yet giving it another thought, you're right:

When translating name to number, it is very unlikely users would reuse
a common name ('tcp' for instance) for another protocol value. They'll
probably just add new ones.

In reverse direction, it is inconvenient at most: People may prefer
'ipv6-icmp' over 'icmpv6', but whatever name libxtables has stored will
at least parse OK later.

Thanks, Phil

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

end of thread, other threads:[~2022-03-10 12:55 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-02 15:18 [iptables PATCH 0/4] Speed up iptables-nft-save Phil Sutter
2022-03-02 15:18 ` [iptables PATCH 1/4] nft: Simplify immediate parsing Phil Sutter
2022-03-10 12:09   ` Florian Westphal
2022-03-02 15:18 ` [iptables PATCH 2/4] nft: Speed up " Phil Sutter
2022-03-02 15:18 ` [iptables PATCH 3/4] xshared: Prefer xtables_chain_protos lookup over getprotoent Phil Sutter
2022-03-10 12:11   ` Florian Westphal
2022-03-10 12:20     ` Phil Sutter
     [not found]       ` <20220310122303.GC13772@breakpoint.cc>
2022-03-10 12:54         ` Phil Sutter
2022-03-02 15:18 ` [iptables PATCH 4/4] nft: Don't pass command state opaque to family ops callbacks Phil Sutter
2022-03-10 12:14   ` Florian Westphal

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.