[PATCH nft,v2] cache: do not populate the cache in case of flush ruleset command

[PATCH nft,v2] cache: do not populate the cache in case of flush ruleset command

[Pablo Neira Ayuso]

__CMD_FLUSH_RULESET is a dummy definition that used to skip the netlink
dump to populate the cache. This patch is a workaround until we have a
better infrastructure to track the state of the cache objects.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: check for __CMD_FLUSH_RULESET before dumping tables via netlink.

 include/rule.h | 1 +
 src/cache.c    | 3 +++
 src/rule.c     | 3 +++
 3 files changed, 7 insertions(+)

diff --git a/include/rule.h b/include/rule.h
index dd9df9ec6dd8..b41825d000d6 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -462,6 +462,7 @@ enum cmd_ops {
 	CMD_EXPORT,
 	CMD_MONITOR,
 	CMD_DESCRIBE,
+	__CMD_FLUSH_RULESET,
 };
 
 /**
diff --git a/src/cache.c b/src/cache.c
index 532ef425906a..d7153f6f6b8f 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -54,6 +54,9 @@ static unsigned int evaluate_cache_flush(struct cmd *cmd)
 	unsigned int completeness = CMD_INVALID;
 
 	switch (cmd->obj) {
+	case CMD_OBJ_RULESET:
+		completeness = __CMD_FLUSH_RULESET;
+		break;
 	case CMD_OBJ_SET:
 	case CMD_OBJ_MAP:
 	case CMD_OBJ_METER:
diff --git a/src/rule.c b/src/rule.c
index 8de5aa62b94f..0c0fd07ec70c 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -210,6 +210,9 @@ static int cache_init(struct netlink_ctx *ctx, enum cmd_ops cmd)
 	};
 	int ret;
 
+	if (cmd == __CMD_FLUSH_RULESET)
+		return 0;
+
 	ret = cache_init_tables(ctx, &handle, &ctx->nft->cache);
 	if (ret < 0)
 		return ret;
-- 
2.11.0

Re: [PATCH nft,v2] cache: do not populate the cache in case of flush ruleset command

[Phil Sutter]

Hi Pablo,

On Fri, Jun 14, 2019 at 02:36:30PM +0200, Pablo Neira Ayuso wrote:
> __CMD_FLUSH_RULESET is a dummy definition that used to skip the netlink
> dump to populate the cache. This patch is a workaround until we have a
> better infrastructure to track the state of the cache objects.

I assumed the problem wouldn't exist anymore since we're populating the
cache just once. Can you maybe elaborate a bit on the problem you're
trying to solve with that workaround?

Cheers, Phil

Re: [PATCH nft,v2] cache: do not populate the cache in case of flush ruleset command

[Pablo Neira Ayuso]

On Fri, Jun 14, 2019 at 02:54:32PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Fri, Jun 14, 2019 at 02:36:30PM +0200, Pablo Neira Ayuso wrote:
> > __CMD_FLUSH_RULESET is a dummy definition that used to skip the netlink
> > dump to populate the cache. This patch is a workaround until we have a
> > better infrastructure to track the state of the cache objects.
> 
> I assumed the problem wouldn't exist anymore since we're populating the
> cache just once. Can you maybe elaborate a bit on the problem you're
> trying to solve with that workaround?

If nft segfaults to dump the cache, 'nft flush ruleset' will not work
since it always fetches the cache, it will segfault too.

The flush ruleset command was still dumping the cache before this
patch.

Re: [PATCH nft,v2] cache: do not populate the cache in case of flush ruleset command

[Pablo Neira Ayuso]

On Fri, Jun 14, 2019 at 02:59:10PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Jun 14, 2019 at 02:54:32PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Fri, Jun 14, 2019 at 02:36:30PM +0200, Pablo Neira Ayuso wrote:
> > > __CMD_FLUSH_RULESET is a dummy definition that used to skip the netlink
> > > dump to populate the cache. This patch is a workaround until we have a
> > > better infrastructure to track the state of the cache objects.
> > 
> > I assumed the problem wouldn't exist anymore since we're populating the
> > cache just once. Can you maybe elaborate a bit on the problem you're
> > trying to solve with that workaround?
> 
> If nft segfaults to dump the cache, 'nft flush ruleset' will not work
> since it always fetches the cache, it will segfault too.
> 
> The flush ruleset command was still dumping the cache before this
> patch.

In general, we still need to improve the cache logic, to make it finer
grain. Now that we have a single point to populate the cache, things
will get more simple. We need to replace the cache command level to
cache flags or our own cache level definitions. The existing approach
that uses of commands to define the cache level completeness has its
own limitations. We can discuss this during the NFWS :-).

Re: [PATCH nft,v2] cache: do not populate the cache in case of flush ruleset command

[Phil Sutter]

Hi Pablo,

On Fri, Jun 14, 2019 at 03:04:38PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Jun 14, 2019 at 02:59:10PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Jun 14, 2019 at 02:54:32PM +0200, Phil Sutter wrote:
> > > Hi Pablo,
> > > 
> > > On Fri, Jun 14, 2019 at 02:36:30PM +0200, Pablo Neira Ayuso wrote:
> > > > __CMD_FLUSH_RULESET is a dummy definition that used to skip the netlink
> > > > dump to populate the cache. This patch is a workaround until we have a
> > > > better infrastructure to track the state of the cache objects.
> > > 
> > > I assumed the problem wouldn't exist anymore since we're populating the
> > > cache just once. Can you maybe elaborate a bit on the problem you're
> > > trying to solve with that workaround?
> > 
> > If nft segfaults to dump the cache, 'nft flush ruleset' will not work
> > since it always fetches the cache, it will segfault too.
> > 
> > The flush ruleset command was still dumping the cache before this
> > patch.
> 
> In general, we still need to improve the cache logic, to make it finer
> grain. Now that we have a single point to populate the cache, things
> will get more simple. We need to replace the cache command level to
> cache flags or our own cache level definitions. The existing approach
> that uses of commands to define the cache level completeness has its
> own limitations. We can discuss this during the NFWS :-).

Yes, I had the same thought already. It is quite unintuitive how we link
cache completeness to certain commands. :)

Regarding your problem, maybe cache_update() should exit immediately if
passed cmd is CMD_INVALID? Unless I miss something, if cache_evaluate()
returns that value, we don't need a cache at all.

Cheers, Phil

Re: [PATCH nft,v2] cache: do not populate the cache in case of flush ruleset command

[Pablo Neira Ayuso]

Hi Phil,

On Fri, Jun 14, 2019 at 03:41:24PM +0200, Phil Sutter wrote:
[...]
> On Fri, Jun 14, 2019 at 03:04:38PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Jun 14, 2019 at 02:59:10PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Jun 14, 2019 at 02:54:32PM +0200, Phil Sutter wrote:
> > > > Hi Pablo,
> > > > 
> > > > On Fri, Jun 14, 2019 at 02:36:30PM +0200, Pablo Neira Ayuso wrote:
> > > > > __CMD_FLUSH_RULESET is a dummy definition that used to skip the netlink
> > > > > dump to populate the cache. This patch is a workaround until we have a
> > > > > better infrastructure to track the state of the cache objects.
> > > > 
> > > > I assumed the problem wouldn't exist anymore since we're populating the
> > > > cache just once. Can you maybe elaborate a bit on the problem you're
> > > > trying to solve with that workaround?
> > > 
> > > If nft segfaults to dump the cache, 'nft flush ruleset' will not work
> > > since it always fetches the cache, it will segfault too.
> > > 
> > > The flush ruleset command was still dumping the cache before this
> > > patch.
> > 
> > In general, we still need to improve the cache logic, to make it finer
> > grain. Now that we have a single point to populate the cache, things
> > will get more simple. We need to replace the cache command level to
> > cache flags or our own cache level definitions. The existing approach
> > that uses of commands to define the cache level completeness has its
> > own limitations. We can discuss this during the NFWS :-).
> 
> Yes, I had the same thought already. It is quite unintuitive how we link
> cache completeness to certain commands. :)

Just sent a follow up patch to introduce cache level flags [1]. The
existing approach is rather conservative in what we fetch from the
kernel (sometimes more than needed) but it should be possible to
review this logic incrementally.

> Regarding your problem, maybe cache_update() should exit immediately if
> passed cmd is CMD_INVALID? Unless I miss something, if cache_evaluate()
> returns that value, we don't need a cache at all.

Problem is that CMD_INVALID does not mean empty cache.

With the new cache level infrastructure, now there is a
NFT_CACHE_EMPTY that provides the semantics this workaround was
providing in a clearer way I think.

[1] https://patchwork.ozlabs.org/patch/1116973/