Re: [nft PATCH v2] Implement 'reset rule' and 'reset rules' commands

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Tue, Jan 17, 2023 at 03:58:04PM +0100, Phil Sutter wrote:
> On Mon, Jan 16, 2023 at 01:30:42PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Jan 12, 2023 at 05:23:42PM +0100, Phil Sutter wrote:
> > > Reset rule counters and quotas in kernel, i.e. without having to reload
> > > them. Requires respective kernel patch to support NFT_MSG_GETRULE_RESET
> > > message type.
> > 
> > Only thing to mention: This adds a new rule_cache_dump() call, this
> > was consolidated on top of the cache infrastructure, to have a single
> > spot in the code to fetch kernel objects via netlink. This triggers to
> > netlink dumps, one to populate the cache and another for the reset,
> > right?
> 
> Well, rule_cache_dump() becomes dual-use, because the rule reset call is
> so similar to the regular rule dump one.
> 
> I also changed it to allow turning the actual dump off (by sending
> NLM_F_ACK instead of NLM_F_DUMP to kernel). This is used for resetting a
> single rule (identified by rule-handle). Same functionality as with
> resetting (a single) quota instead of "quotas".

Makes sense.

> If dump is active, the kernel returns the rules that have been reset to
> user space, so users see the last state data. To correctly print this,
> nft needs a table and chain cache. My v2 has this wrong, it needlessly
> requests a rule cache also. Thanks for making me validate this aspect of
> the implementation! :)

Ok.

> The only caveat I just noticed with reduced cache is that "whole ruleset
> reset" does not print the rules because list_rule_cb() expects h->family
> to be set. Making it accept NFPROTO_UNSPEC is trivial, though.

I guess you refer to this chunk.

@@ -591,8 +615,8 @@  static int list_rule_cb(struct nftnl_rule *nlr, void *data)
 	table  = nftnl_rule_get_str(nlr, NFTNL_RULE_TABLE);
 	chain  = nftnl_rule_get_str(nlr, NFTNL_RULE_CHAIN);
 
-	if (h->family != family ||
-	    strcmp(table, h->table.name) != 0 ||
+	if ((h->family != NFPROTO_UNSPEC && h->family != family) ||
+	    (h->table.name && strcmp(table, h->table.name) != 0) ||
 	    (h->chain.name && strcmp(chain, h->chain.name) != 0))
 		return 0;

There is one more thing that will need a re-visit sooner or later.

NLM_F_DUMP might hit EINTR, meaning that while dumping-and-reset,
there was a ruleset update from the transaction path.

In this case, the listing would not be consistent. This would require
to trigger a bit more complicated path, that is: 1) keep previous list
of rules, 2) re-start dump with empty list, 3) look up via handle to
check if the rule in the previous list, to fetch the counter into the
new rule.

I think this problem already exists for the counter/quota/... reset.

I still have to extend the cache infrastructure to support for rules.
It is on my TODO list for monitor infrastructure. Then probably this
reset command can be reworked on top of the cache.

Anyway, your patch is a good starter. Better cache integration and
this corner case can possibly wait.