[iptables PATCH 0/2] Restore rule counter zeroing

[iptables PATCH 0/2] Restore rule counter zeroing

[Phil Sutter]

Zeroing rule counters was broken in two ways: On one hand, cache
optimizations went a little too far (actually I missed that rule cache
is required for CMD_ZERO). On the other, rule replace logic was
insufficient with regards to NFTA_RULE_COMPAT attribute (elaborate
details in second patch).

Phil Sutter (2):
  nft: CMD_ZERO needs a rule cache
  nft: Fix -Z for rules with NFTA_RULE_COMPAT

 iptables/nft.c             | 41 ++++++++++++++++++++++++++++++++++++++
 iptables/xtables-restore.c |  1 +
 2 files changed, 42 insertions(+)

-- 
2.24.0

[iptables PATCH 1/2] nft: CMD_ZERO needs a rule cache

[Phil Sutter]

In order to zero rule counters, they have to be fetched from kernel. Fix
this for both standalone calls as well as xtables-restore --noflush.

Fixes: b5cb6e631c828 ("nft-cache: Fetch only chains in nft_chain_list_get()")
Fixes: 09cb517949e69 ("xtables-restore: Improve performance of --noflush operation")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c             | 2 ++
 iptables/xtables-restore.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/iptables/nft.c b/iptables/nft.c
index 3c230c121f8b9..83cf5fb703d3e 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2922,6 +2922,8 @@ static int __nft_chain_zero_counters(struct nftnl_chain *c, void *data)
 			return -1;
 	}
 
+	nft_build_cache(h, c);
+
 	iter = nftnl_rule_iter_create(c);
 	if (iter == NULL)
 		return -1;
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 282aa153b1599..2f0fe7d439d94 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -268,6 +268,7 @@ static bool cmd_needs_full_cache(char *cmd)
 	case 'C':
 	case 'S':
 	case 'L':
+	case 'Z':
 		return true;
 	}
 
-- 
2.24.0

[iptables PATCH 2/2] nft: Fix -Z for rules with NFTA_RULE_COMPAT

[Phil Sutter]

The special nested attribute NFTA_RULE_COMPAT holds information about
any present l4proto match (given via '-p' parameter) in input. The match
is contained as meta expression as well, but some xtables extensions
explicitly check it's value (see e.g. xt_TPROXY).

This nested attribute is input only, the information is lost after
parsing (and initialization of compat extensions). So in order to feed a
rule back to kernel with zeroed counters, the attribute has to be
reconstructed based on the rule's expressions.

Other code paths are not affected since rule_to_cs() callback will
populate respective fields in struct iptables_command_state and 'add'
callback (which is the inverse to rule_to_cs()) calls add_compat() in
any case.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/iptables/nft.c b/iptables/nft.c
index 83cf5fb703d3e..599c2f7e4c087 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2897,6 +2897,44 @@ const char *nft_strerror(int err)
 	return strerror(err);
 }
 
+static int recover_rule_compat(struct nftnl_rule *r)
+{
+	struct nftnl_expr_iter *iter;
+	struct nftnl_expr *e;
+	uint32_t reg;
+	int ret = -1;
+
+	iter = nftnl_expr_iter_create(r);
+	if (!iter)
+		return -1;
+
+next_expr:
+	e = nftnl_expr_iter_next(iter);
+	if (!e)
+		goto out;
+
+	if (strcmp("meta", nftnl_expr_get_str(e, NFTNL_EXPR_NAME)) ||
+	    nftnl_expr_get_u32(e, NFTNL_EXPR_META_KEY) != NFT_META_L4PROTO)
+		goto next_expr;
+
+	reg = nftnl_expr_get_u32(e, NFTNL_EXPR_META_DREG);
+
+	e = nftnl_expr_iter_next(iter);
+	if (!e)
+		goto out;
+
+	if (strcmp("cmp", nftnl_expr_get_str(e, NFTNL_EXPR_NAME)) ||
+	    reg != nftnl_expr_get_u32(e, NFTNL_EXPR_CMP_SREG))
+		goto next_expr;
+
+	add_compat(r, nftnl_expr_get_u8(e, NFTNL_EXPR_CMP_DATA),
+		   nftnl_expr_get_u32(e, NFTNL_EXPR_CMP_OP) == NFT_CMP_NEQ);
+	ret = 0;
+out:
+	nftnl_expr_iter_destroy(iter);
+	return ret;
+}
+
 struct chain_zero_data {
 	struct nft_handle	*handle;
 	bool			verbose;
@@ -2961,6 +2999,7 @@ static int __nft_chain_zero_counters(struct nftnl_chain *c, void *data)
 			 * Unset RULE_POSITION for older kernels, we want to replace
 			 * rule based on its handle only.
 			 */
+			recover_rule_compat(r);
 			nftnl_rule_unset(r, NFTNL_RULE_POSITION);
 			if (!batch_rule_add(h, NFT_COMPAT_RULE_REPLACE, r)) {
 				nftnl_rule_iter_destroy(iter);
-- 
2.24.0

Re: [iptables PATCH 1/2] nft: CMD_ZERO needs a rule cache

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> In order to zero rule counters, they have to be fetched from kernel. Fix
> this for both standalone calls as well as xtables-restore --noflush.

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

Re: [iptables PATCH 2/2] nft: Fix -Z for rules with NFTA_RULE_COMPAT

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> The special nested attribute NFTA_RULE_COMPAT holds information about
> any present l4proto match (given via '-p' parameter) in input. The match
> is contained as meta expression as well, but some xtables extensions
> explicitly check it's value (see e.g. xt_TPROXY).
> 
> This nested attribute is input only, the information is lost after
> parsing (and initialization of compat extensions). So in order to feed a
> rule back to kernel with zeroed counters, the attribute has to be
> reconstructed based on the rule's expressions.
> 
> Other code paths are not affected since rule_to_cs() callback will
> populate respective fields in struct iptables_command_state and 'add'
> callback (which is the inverse to rule_to_cs()) calls add_compat() in
> any case.

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

Re: [iptables PATCH 1/2] nft: CMD_ZERO needs a rule cache

[Pablo Neira Ayuso]

On Fri, Nov 15, 2019 at 11:29:07AM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > In order to zero rule counters, they have to be fetched from kernel. Fix
> > this for both standalone calls as well as xtables-restore --noflush.
> 
> Reviewed-by: Florian Westphal <fw@strlen.de>

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

Re: [iptables PATCH 2/2] nft: Fix -Z for rules with NFTA_RULE_COMPAT

[Pablo Neira Ayuso]

On Fri, Nov 15, 2019 at 11:29:22AM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > The special nested attribute NFTA_RULE_COMPAT holds information about
> > any present l4proto match (given via '-p' parameter) in input. The match
> > is contained as meta expression as well, but some xtables extensions
> > explicitly check it's value (see e.g. xt_TPROXY).
> > 
> > This nested attribute is input only, the information is lost after
> > parsing (and initialization of compat extensions). So in order to feed a
> > rule back to kernel with zeroed counters, the attribute has to be
> > reconstructed based on the rule's expressions.
> > 
> > Other code paths are not affected since rule_to_cs() callback will
> > populate respective fields in struct iptables_command_state and 'add'
> > callback (which is the inverse to rule_to_cs()) calls add_compat() in
> > any case.
> 
> Reviewed-by: Florian Westphal <fw@strlen.de>

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