[PATCH nft 1/2,v2] cache: check for NFT_CACHE_REFRESH in current requested cache too

[PATCH nft 1/2,v2] cache: check for NFT_CACHE_REFRESH in current requested cache too

[Pablo Neira Ayuso]

NFT_CACHE_REFRESH is set on inconditionally by ruleset list commands to
deal with stateful information in this ruleset. This flag results in
dropping the existing cache and fully fetching all objects from the
kernel.

Set on this flag for reset commands too, this is missing.

List/reset commands allow for filtering by specific family and object,
therefore, NFT_CACHE_REFRESH also signals that the cache is partially
populated.

Check if this flag is requested by the current list/reset command, as
well as cache->flags which represents the cache after the _previous_
list of commands.

A follow up patch allows to recycle the existing cache if the flags
report that the same objects are already available in the cache,
NFT_CACHE_REFRESH is useful to report that cache cannot be recycled.

Fixes: 407c54f71255 ("src: cache gets out of sync in interactive mode")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: cache filtering, eg. list table inet test, could result in partial
    cache that cannot be recycle, use NFT_CACHE_REFRESH to signal that
    cache cannot be reused.

 src/cache.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/cache.c b/src/cache.c
index c000e32c497f..e88cbae2ad95 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -297,6 +297,7 @@ static unsigned int evaluate_cache_reset(struct cmd *cmd, unsigned int flags,
 		flags |= NFT_CACHE_TABLE;
 		break;
 	}
+	flags |= NFT_CACHE_REFRESH;
 
 	return flags;
 }
@@ -1177,9 +1178,10 @@ static bool nft_cache_is_complete(struct nft_cache *cache, unsigned int flags)
 	return (cache->flags & flags) == flags;
 }
 
-static bool nft_cache_needs_refresh(struct nft_cache *cache)
+static bool nft_cache_needs_refresh(struct nft_cache *cache, unsigned int flags)
 {
-	return cache->flags & NFT_CACHE_REFRESH;
+	return (cache->flags & NFT_CACHE_REFRESH) ||
+	       (flags & NFT_CACHE_REFRESH);
 }
 
 static bool nft_cache_is_updated(struct nft_cache *cache, uint16_t genid)
@@ -1207,7 +1209,7 @@ int nft_cache_update(struct nft_ctx *nft, unsigned int flags,
 replay:
 	ctx.seqnum = cache->seqnum++;
 	genid = mnl_genid_get(&ctx);
-	if (!nft_cache_needs_refresh(cache) &&
+	if (!nft_cache_needs_refresh(cache, flags) &&
 	    nft_cache_is_complete(cache, flags) &&
 	    nft_cache_is_updated(cache, genid))
 		return 0;
-- 
2.30.2

[PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Pablo Neira Ayuso]

Cache tracking has improved over time by incrementally adding/deleting
objects when evaluating commands that are going to be sent to the kernel.

nft_cache_is_complete() already checks that the cache contains objects
that are required to handle this batch of commands by comparing cache
flags.

Infer from the current generation ID if no other transaction has
invalidated the existing cache, this allows to skip unnecessary cache
flush then refill situations which slow down incremental updates.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: no changes

    nft_slew.c provided by Neels (on Cc) show better numbers now after this
    series.

 src/cache.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/cache.c b/src/cache.c
index e88cbae2ad95..4b797ec79ae5 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -1184,9 +1184,21 @@ static bool nft_cache_needs_refresh(struct nft_cache *cache, unsigned int flags)
 	       (flags & NFT_CACHE_REFRESH);
 }
 
-static bool nft_cache_is_updated(struct nft_cache *cache, uint16_t genid)
+static bool nft_cache_is_updated(struct nft_cache *cache, unsigned int flags,
+				 uint16_t genid)
 {
-	return genid && genid == cache->genid;
+	if (!genid)
+		return false;
+
+	if (genid == cache->genid)
+		return true;
+
+	if (genid == cache->genid + 1) {
+		cache->genid++;
+		return true;
+	}
+
+	return false;
 }
 
 bool nft_cache_needs_update(struct nft_cache *cache)
@@ -1211,7 +1223,7 @@ replay:
 	genid = mnl_genid_get(&ctx);
 	if (!nft_cache_needs_refresh(cache, flags) &&
 	    nft_cache_is_complete(cache, flags) &&
-	    nft_cache_is_updated(cache, genid))
+	    nft_cache_is_updated(cache, flags, genid))
 		return 0;
 
 	if (cache->genid)
-- 
2.30.2

Re: [PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Eric Garver]

On Tue, May 28, 2024 at 05:28:17PM +0200, Pablo Neira Ayuso wrote:
> Cache tracking has improved over time by incrementally adding/deleting
> objects when evaluating commands that are going to be sent to the kernel.
> 
> nft_cache_is_complete() already checks that the cache contains objects
> that are required to handle this batch of commands by comparing cache
> flags.
> 
> Infer from the current generation ID if no other transaction has
> invalidated the existing cache, this allows to skip unnecessary cache
> flush then refill situations which slow down incremental updates.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> v2: no changes

Hi Pablo,

This patch introduced a regression with the index keyword. It seems to
be triggered by adding a rule with "insert", then referencing the new
rule with by "add"-ing another rule using index.

https://github.com/firewalld/firewalld/issues/1366#issuecomment-2243772215

I'm happy to test any fixes.

Thanks.
Eric.


--->8---


# cat /tmp/foo2
add table inet foo
add chain inet foo bar { type filter hook input priority filter; }
add rule inet foo bar accept
insert rule inet foo bar index 0 accept
add rule inet foo bar index 0 accept

# nft delete table inet foo; nft -i < /tmp/foo2 ; nft list table inet foo
Error: Could not process rule: No such file or directory
add rule inet foo bar index 0 accept
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
table inet foo {
        chain bar {
                type filter hook input priority filter; policy accept;
                accept
                accept
        }
}

# git revert e791dbe109b6dd891a63a4236df5dc29d7a4b863
[master 30ae3c684990] Revert "cache: recycle existing cache with incremental updates"
 1 file changed, 3 insertions(+), 15 deletions(-)

# make install
[..]

# nft delete table inet foo; nft -i < /tmp/foo2 ; nft list table inet foo
table inet foo {
        chain bar {
                type filter hook input priority filter; policy accept;
                accept
                accept
                accept
        }
}

Re: [PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Pablo Neira Ayuso]

On Mon, Jul 22, 2024 at 04:48:40PM -0400, Eric Garver wrote:
> On Tue, May 28, 2024 at 05:28:17PM +0200, Pablo Neira Ayuso wrote:
> > Cache tracking has improved over time by incrementally adding/deleting
> > objects when evaluating commands that are going to be sent to the kernel.
> > 
> > nft_cache_is_complete() already checks that the cache contains objects
> > that are required to handle this batch of commands by comparing cache
> > flags.
> > 
> > Infer from the current generation ID if no other transaction has
> > invalidated the existing cache, this allows to skip unnecessary cache
> > flush then refill situations which slow down incremental updates.
> > 
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> > v2: no changes
> 
> Hi Pablo,
> 
> This patch introduced a regression with the index keyword. It seems to
> be triggered by adding a rule with "insert", then referencing the new
> rule with by "add"-ing another rule using index.
> 
> https://github.com/firewalld/firewalld/issues/1366#issuecomment-2243772215

I can reproduce it:

# nft -i
nft> add table inet foo
nft> add chain inet foo bar { type filter hook input priority filter; }
nft> add rule inet foo bar accept
nft> insert rule inet foo bar index 0 accept
nft> add rule inet foo bar index 0 accept
Error: Could not process rule: No such file or directory
add rule inet foo bar index 0 accept
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Cache woes. Maybe a bug in

commit e5382c0d08e3c6d8246afa95b7380f0d6b8c1826
Author: Phil Sutter <phil@nwl.cc>
Date:   Fri Jun 7 19:21:21 2019 +0200

    src: Support intra-transaction rule references

that uncover now that cache is not flushed and sync with kernel so often?

Re: [PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Phil Sutter]

On Mon, Jul 22, 2024 at 11:34:01PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jul 22, 2024 at 04:48:40PM -0400, Eric Garver wrote:
> > On Tue, May 28, 2024 at 05:28:17PM +0200, Pablo Neira Ayuso wrote:
> > > Cache tracking has improved over time by incrementally adding/deleting
> > > objects when evaluating commands that are going to be sent to the kernel.
> > > 
> > > nft_cache_is_complete() already checks that the cache contains objects
> > > that are required to handle this batch of commands by comparing cache
> > > flags.
> > > 
> > > Infer from the current generation ID if no other transaction has
> > > invalidated the existing cache, this allows to skip unnecessary cache
> > > flush then refill situations which slow down incremental updates.
> > > 
> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > ---
> > > v2: no changes
> > 
> > Hi Pablo,
> > 
> > This patch introduced a regression with the index keyword. It seems to
> > be triggered by adding a rule with "insert", then referencing the new
> > rule with by "add"-ing another rule using index.
> > 
> > https://github.com/firewalld/firewalld/issues/1366#issuecomment-2243772215
> 
> I can reproduce it:
> 
> # nft -i
> nft> add table inet foo
> nft> add chain inet foo bar { type filter hook input priority filter; }
> nft> add rule inet foo bar accept
> nft> insert rule inet foo bar index 0 accept
> nft> add rule inet foo bar index 0 accept
> Error: Could not process rule: No such file or directory
> add rule inet foo bar index 0 accept
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Thanks for providing instructions.

> Cache woes. Maybe a bug in
> 
> commit e5382c0d08e3c6d8246afa95b7380f0d6b8c1826
> Author: Phil Sutter <phil@nwl.cc>
> Date:   Fri Jun 7 19:21:21 2019 +0200

I'll have a look later today.

Cheers, Phil

Re: [PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Phil Sutter]

Some digging and lots of printf's later:

On Mon, Jul 22, 2024 at 11:34:01PM +0200, Pablo Neira Ayuso wrote:
[...]
> I can reproduce it:
> 
> # nft -i
> nft> add table inet foo
> nft> add chain inet foo bar { type filter hook input priority filter; }
> nft> add rule inet foo bar accept

This bumps cache->flags from 0 to 0x1f (no cache -> NFT_CACHE_OBJECT).

> nft> insert rule inet foo bar index 0 accept

This adds NFT_CACHE_RULE_BIT and NFT_CACHE_UPDATE, cache is updated (to
fetch rules).

> nft> add rule inet foo bar index 0 accept

No new flags for this one, so the code hits the 'genid == cache->genid +
1' case in nft_cache_is_updated() which bumps the local genid and skips
a cache update. The new rule then references the cached copy of the
previously commited one which still does not have a handle. Therefore
link_rules() does it's thing for references to  uncommitted rules which
later fails.

Pablo: Could you please explain the logic around this cache->genid
increment? Commit e791dbe109b6d ("cache: recycle existing cache with
incremental updates") is not clear to me in this regard. How can the
local process know it doesn't need whatever has changed in the kernel?

> Error: Could not process rule: No such file or directory

BTW: There are surprisingly many spots which emit a "Could not process
rule:" error. I'm sure it may be provoked for non-rule-related commands
(e.g. the calls in nft_netlink()).

> add rule inet foo bar index 0 accept
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Cache woes. Maybe a bug in
> 
> commit e5382c0d08e3c6d8246afa95b7380f0d6b8c1826
> Author: Phil Sutter <phil@nwl.cc>
> Date:   Fri Jun 7 19:21:21 2019 +0200
> 
>     src: Support intra-transaction rule references
> 
> that uncover now that cache is not flushed and sync with kernel so often?

The commit by itself is fine, as long as the cache is up to date. The
problem is we have this previously inserted rule in cache which does not
have a handle although it was committed to kernel already. This is
something I don't see covered by e791dbe109b6d at all.

Cheers, Phil

Re: [PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Pablo Neira Ayuso]

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

On Tue, Jul 23, 2024 at 01:56:46PM +0200, Phil Sutter wrote:
> Some digging and lots of printf's later:
> 
> On Mon, Jul 22, 2024 at 11:34:01PM +0200, Pablo Neira Ayuso wrote:
> [...]
> > I can reproduce it:
> > 
> > # nft -i
> > nft> add table inet foo
> > nft> add chain inet foo bar { type filter hook input priority filter; }
> > nft> add rule inet foo bar accept
> 
> This bumps cache->flags from 0 to 0x1f (no cache -> NFT_CACHE_OBJECT).
> 
> > nft> insert rule inet foo bar index 0 accept
> 
> This adds NFT_CACHE_RULE_BIT and NFT_CACHE_UPDATE, cache is updated (to
> fetch rules).
> 
> > nft> add rule inet foo bar index 0 accept
> 
> No new flags for this one, so the code hits the 'genid == cache->genid +
> 1' case in nft_cache_is_updated() which bumps the local genid and skips
> a cache update. The new rule then references the cached copy of the
> previously commited one which still does not have a handle. Therefore
> link_rules() does it's thing for references to  uncommitted rules which
> later fails.
> 
> Pablo: Could you please explain the logic around this cache->genid
> increment? Commit e791dbe109b6d ("cache: recycle existing cache with
> incremental updates") is not clear to me in this regard. How can the
> local process know it doesn't need whatever has changed in the kernel?

The idea is to use the ruleset generation ID as a hint to infer if the
existing cache can be recycled, to speed up incremental updates. This
is not sufficient for the index cache, see below.

> > Error: Could not process rule: No such file or directory
> 
> BTW: There are surprisingly many spots which emit a "Could not process
> rule:" error. I'm sure it may be provoked for non-rule-related commands
> (e.g. the calls in nft_netlink()).
> 
> > add rule inet foo bar index 0 accept
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > Cache woes. Maybe a bug in
> > 
> > commit e5382c0d08e3c6d8246afa95b7380f0d6b8c1826
> > Author: Phil Sutter <phil@nwl.cc>
> > Date:   Fri Jun 7 19:21:21 2019 +0200
> > 
> >     src: Support intra-transaction rule references
> > 
> > that uncover now that cache is not flushed and sync with kernel so often?
> 
> The commit by itself is fine, as long as the cache is up to date. The
> problem is we have this previously inserted rule in cache which does not
> have a handle although it was committed to kernel already. This is
> something I don't see covered by e791dbe109b6d at all.

Yes, your commit relies on the frequent flush and resync at that time,
which is correct.

See attached patch, it sets on NFT_CACHE_REFRESH so a fresh cache from
the kernel is fetched in each run, it fixes the issue for me.

A quick recap:

Step #1 add rule inet foo bar accept

No cache is updated, because no NFT_CACHE_UPDATE is ever set on in
this case. It is not possible to know that the that the next command
will use the index feature.

After this point, the cache is inconsistent for this index feature.

Generation ID is checked, if cache is current, the previous rule in
step #1 is not there in the cache.

It should be possible to improve this logic to indicate that the cache
supports updates via index, instead of forcing a cache dump from the
kernel on each new index command.

So NFT_CACHE_UPDATE is only useful for intra-batch updates as you
commit describes.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 421 bytes --]

diff --git a/src/cache.c b/src/cache.c
index 4b797ec79ae5..1fda3f6939f5 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -68,7 +68,7 @@ static unsigned int evaluate_cache_add(struct cmd *cmd, unsigned int flags)
 
 		if (cmd->handle.index.id ||
 		    cmd->handle.position.id)
-			flags |= NFT_CACHE_RULE | NFT_CACHE_UPDATE;
+			flags |= NFT_CACHE_RULE | NFT_CACHE_UPDATE | NFT_CACHE_REFRESH;
 		break;
 	default:
 		break;

Re: [PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Pablo Neira Ayuso]

On Tue, Jul 23, 2024 at 02:19:25PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jul 23, 2024 at 01:56:46PM +0200, Phil Sutter wrote:
> > Some digging and lots of printf's later:
> > 
> > On Mon, Jul 22, 2024 at 11:34:01PM +0200, Pablo Neira Ayuso wrote:
> > [...]
> > > I can reproduce it:
> > > 
> > > # nft -i
> > > nft> add table inet foo
> > > nft> add chain inet foo bar { type filter hook input priority filter; }
> > > nft> add rule inet foo bar accept
> > 
> > This bumps cache->flags from 0 to 0x1f (no cache -> NFT_CACHE_OBJECT).
> > 
> > > nft> insert rule inet foo bar index 0 accept
> > 
> > This adds NFT_CACHE_RULE_BIT and NFT_CACHE_UPDATE, cache is updated (to
> > fetch rules).
> > 
> > > nft> add rule inet foo bar index 0 accept
> > 
> > No new flags for this one, so the code hits the 'genid == cache->genid +
> > 1' case in nft_cache_is_updated() which bumps the local genid and skips
> > a cache update. The new rule then references the cached copy of the
> > previously commited one which still does not have a handle. Therefore
> > link_rules() does it's thing for references to  uncommitted rules which
> > later fails.
> > 
> > Pablo: Could you please explain the logic around this cache->genid
> > increment? Commit e791dbe109b6d ("cache: recycle existing cache with
> > incremental updates") is not clear to me in this regard. How can the
> > local process know it doesn't need whatever has changed in the kernel?
> 
> The idea is to use the ruleset generation ID as a hint to infer if the
> existing cache can be recycled, to speed up incremental updates. This
> is not sufficient for the index cache, see below.

I have to revisit e791dbe109b6d, another process could race to bump
the generation ID incrementally and I incorrectly assumed cache is
consistent.

Re: [PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Phil Sutter]

On Tue, Jul 23, 2024 at 02:57:07PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jul 23, 2024 at 02:19:25PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Jul 23, 2024 at 01:56:46PM +0200, Phil Sutter wrote:
> > > Some digging and lots of printf's later:
> > > 
> > > On Mon, Jul 22, 2024 at 11:34:01PM +0200, Pablo Neira Ayuso wrote:
> > > [...]
> > > > I can reproduce it:
> > > > 
> > > > # nft -i
> > > > nft> add table inet foo
> > > > nft> add chain inet foo bar { type filter hook input priority filter; }
> > > > nft> add rule inet foo bar accept
> > > 
> > > This bumps cache->flags from 0 to 0x1f (no cache -> NFT_CACHE_OBJECT).
> > > 
> > > > nft> insert rule inet foo bar index 0 accept
> > > 
> > > This adds NFT_CACHE_RULE_BIT and NFT_CACHE_UPDATE, cache is updated (to
> > > fetch rules).
> > > 
> > > > nft> add rule inet foo bar index 0 accept
> > > 
> > > No new flags for this one, so the code hits the 'genid == cache->genid +
> > > 1' case in nft_cache_is_updated() which bumps the local genid and skips
> > > a cache update. The new rule then references the cached copy of the
> > > previously commited one which still does not have a handle. Therefore
> > > link_rules() does it's thing for references to  uncommitted rules which
> > > later fails.
> > > 
> > > Pablo: Could you please explain the logic around this cache->genid
> > > increment? Commit e791dbe109b6d ("cache: recycle existing cache with
> > > incremental updates") is not clear to me in this regard. How can the
> > > local process know it doesn't need whatever has changed in the kernel?
> > 
> > The idea is to use the ruleset generation ID as a hint to infer if the
> > existing cache can be recycled, to speed up incremental updates. This
> > is not sufficient for the index cache, see below.
> 
> I have to revisit e791dbe109b6d, another process could race to bump
> the generation ID incrementally and I incorrectly assumed cache is
> consistent.

It might be fine, because cache->genid != 0 means we have fetched from
kernel previously and thus also committed a change (list commands set
CACHE_REFRESH). Kernel genid is expectedly cache->genid + 1, a
concurrent commit would bump again.

I don't like the commit because it breaks with the assumption that
kernel genid matching cache genid means cache is up to date. It may
indeed be, but I think it's thin ice and caching code is pretty complex
as-is. :/

Cheers, Phil

Re: [PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Pablo Neira Ayuso]

On Tue, Jul 23, 2024 at 05:09:12PM +0200, Phil Sutter wrote:
[...]
> I don't like the commit because it breaks with the assumption that
> kernel genid matching cache genid means cache is up to date. It may
> indeed be, but I think it's thin ice and caching code is pretty complex
> as-is. :/

Right.

It is possible to retrieve the generation ID from the batch via
NLM_F_ECHO and the NFT_MSG_GETGEN to answer the question: "Was it
myself that has updated the ruleset last time?".

And this needs a lot more tests for -i/--interactive which is a
similar path to what daemons will exercise to ensure cache
consistency.

Re: [PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Phil Sutter]

On Tue, Jul 23, 2024 at 02:19:19PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jul 23, 2024 at 01:56:46PM +0200, Phil Sutter wrote:
> > Some digging and lots of printf's later:
> > 
> > On Mon, Jul 22, 2024 at 11:34:01PM +0200, Pablo Neira Ayuso wrote:
> > [...]
> > > I can reproduce it:
> > > 
> > > # nft -i
> > > nft> add table inet foo
> > > nft> add chain inet foo bar { type filter hook input priority filter; }
> > > nft> add rule inet foo bar accept
> > 
> > This bumps cache->flags from 0 to 0x1f (no cache -> NFT_CACHE_OBJECT).
> > 
> > > nft> insert rule inet foo bar index 0 accept
> > 
> > This adds NFT_CACHE_RULE_BIT and NFT_CACHE_UPDATE, cache is updated (to
> > fetch rules).
> > 
> > > nft> add rule inet foo bar index 0 accept
> > 
> > No new flags for this one, so the code hits the 'genid == cache->genid +
> > 1' case in nft_cache_is_updated() which bumps the local genid and skips
> > a cache update. The new rule then references the cached copy of the
> > previously commited one which still does not have a handle. Therefore
> > link_rules() does it's thing for references to  uncommitted rules which
> > later fails.
> > 
> > Pablo: Could you please explain the logic around this cache->genid
> > increment? Commit e791dbe109b6d ("cache: recycle existing cache with
> > incremental updates") is not clear to me in this regard. How can the
> > local process know it doesn't need whatever has changed in the kernel?
> 
> The idea is to use the ruleset generation ID as a hint to infer if the
> existing cache can be recycled, to speed up incremental updates. This
> is not sufficient for the index cache, see below.

So the assumption is that any changes to the ruleset in kernel were also
done to the cache, so we may just bump cache->genid instead of
refetching. In interactive nft, this requires for the last command to
have manually updated the cache as needed though. This is what confused
me. E.g., adding some chains:

- add chain t c2
  - no cache yet, fetch initially, store current genid
  - commit the new chain (kernel bumps genid)

- add cahin t c3
  - local genid == kernel's genid + 1, so skip cache update and bump
    local genid instead
  - commit the new chain (kernel bumps genid)

So I think if NFT_CACHE_RULE_BIT is set, NFT_CACHE_UPDATE must be set as
well to be sure that a previous command updated the rule cache if if was
a rule-related one. Therefore the local genid bump trick should be
allowed only if !NFT_CACHE_RULE_BIT || NFT_CACHE_UPDATE.

This still does not fix for the case at hand, because it doesn't cover
for the fact that cache had been updated but the contained items were
modified in-kernel.

I think 'delete rule' command needs a fix, too: cmd_evaluate_delete()
does not drop the rule from cache. I can't come up with something that
breaks due to that, though.

> > > Error: Could not process rule: No such file or directory
> > 
> > BTW: There are surprisingly many spots which emit a "Could not process
> > rule:" error. I'm sure it may be provoked for non-rule-related commands
> > (e.g. the calls in nft_netlink()).
> > 
> > > add rule inet foo bar index 0 accept
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > 
> > > Cache woes. Maybe a bug in
> > > 
> > > commit e5382c0d08e3c6d8246afa95b7380f0d6b8c1826
> > > Author: Phil Sutter <phil@nwl.cc>
> > > Date:   Fri Jun 7 19:21:21 2019 +0200
> > > 
> > >     src: Support intra-transaction rule references
> > > 
> > > that uncover now that cache is not flushed and sync with kernel so often?
> > 
> > The commit by itself is fine, as long as the cache is up to date. The
> > problem is we have this previously inserted rule in cache which does not
> > have a handle although it was committed to kernel already. This is
> > something I don't see covered by e791dbe109b6d at all.
> 
> Yes, your commit relies on the frequent flush and resync at that time,
> which is correct.
> 
> See attached patch, it sets on NFT_CACHE_REFRESH so a fresh cache from
> the kernel is fetched in each run, it fixes the issue for me.

Yes, NFT_CACHE_REFRESH disables the conditional cache update skipping.
It is overly agressive though: If kernel's genid matches the cache, no
update should be needed.

> A quick recap:
> 
> Step #1 add rule inet foo bar accept
> 
> No cache is updated, because no NFT_CACHE_UPDATE is ever set on in
> this case. It is not possible to know that the that the next command
> will use the index feature.
> 
> After this point, the cache is inconsistent for this index feature.

This is expected: A simple 'add rule' command does not need a rule
cache, so even setting NFT_CACHE_UPDATE would not lead to a consistent
cache. This is fine, we should avoid fetching rules unless really
needed.

> Generation ID is checked, if cache is current, the previous rule in
> step #1 is not there in the cache.

Actually no rules at all, as NFT_CACHE_RULE_BIT remains unset.

> It should be possible to improve this logic to indicate that the cache
> supports updates via index, instead of forcing a cache dump from the
> kernel on each new index command.

Only if we start fetching rules for non index/handle 'rule add'/'rule
insert' commands, right? Also, new rules have to be replaced by their
kernel versions in cache to get the handles.

> So NFT_CACHE_UPDATE is only useful for intra-batch updates as you
> commit describes.

> diff --git a/src/cache.c b/src/cache.c
> index 4b797ec79ae5..1fda3f6939f5 100644
> --- a/src/cache.c
> +++ b/src/cache.c
> @@ -68,7 +68,7 @@ static unsigned int evaluate_cache_add(struct cmd *cmd, unsigned int flags)
>  
>  		if (cmd->handle.index.id ||
>  		    cmd->handle.position.id)
> -			flags |= NFT_CACHE_RULE | NFT_CACHE_UPDATE;
> +			flags |= NFT_CACHE_RULE | NFT_CACHE_UPDATE | NFT_CACHE_REFRESH;
>  		break;
>  	default:
>  		break;

Acked-by: Phil Sutter <phil@nwl.cc>

Thanks, Phil

Re: [PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Eric Garver]

On Tue, Jul 23, 2024 at 04:34:17PM +0200, Phil Sutter wrote:
> On Tue, Jul 23, 2024 at 02:19:19PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Jul 23, 2024 at 01:56:46PM +0200, Phil Sutter wrote:
> > > Some digging and lots of printf's later:
> > > 
> > > On Mon, Jul 22, 2024 at 11:34:01PM +0200, Pablo Neira Ayuso wrote:
> > > [...]
> > > > I can reproduce it:
> > > > 
> > > > # nft -i
> > > > nft> add table inet foo
> > > > nft> add chain inet foo bar { type filter hook input priority filter; }
> > > > nft> add rule inet foo bar accept
> > > 
> > > This bumps cache->flags from 0 to 0x1f (no cache -> NFT_CACHE_OBJECT).
> > > 
> > > > nft> insert rule inet foo bar index 0 accept
> > > 
> > > This adds NFT_CACHE_RULE_BIT and NFT_CACHE_UPDATE, cache is updated (to
> > > fetch rules).
> > > 
> > > > nft> add rule inet foo bar index 0 accept
> > > 
> > > No new flags for this one, so the code hits the 'genid == cache->genid +
> > > 1' case in nft_cache_is_updated() which bumps the local genid and skips
> > > a cache update. The new rule then references the cached copy of the
> > > previously commited one which still does not have a handle. Therefore
> > > link_rules() does it's thing for references to  uncommitted rules which
> > > later fails.
> > > 
> > > Pablo: Could you please explain the logic around this cache->genid
> > > increment? Commit e791dbe109b6d ("cache: recycle existing cache with
> > > incremental updates") is not clear to me in this regard. How can the
> > > local process know it doesn't need whatever has changed in the kernel?
> > 
> > The idea is to use the ruleset generation ID as a hint to infer if the
> > existing cache can be recycled, to speed up incremental updates. This
> > is not sufficient for the index cache, see below.
> 
> So the assumption is that any changes to the ruleset in kernel were also
> done to the cache, so we may just bump cache->genid instead of
> refetching. In interactive nft, this requires for the last command to
> have manually updated the cache as needed though. This is what confused
> me. E.g., adding some chains:
> 
> - add chain t c2
>   - no cache yet, fetch initially, store current genid
>   - commit the new chain (kernel bumps genid)
> 
> - add cahin t c3
>   - local genid == kernel's genid + 1, so skip cache update and bump
>     local genid instead
>   - commit the new chain (kernel bumps genid)
> 
> So I think if NFT_CACHE_RULE_BIT is set, NFT_CACHE_UPDATE must be set as
> well to be sure that a previous command updated the rule cache if if was
> a rule-related one. Therefore the local genid bump trick should be
> allowed only if !NFT_CACHE_RULE_BIT || NFT_CACHE_UPDATE.
> 
> This still does not fix for the case at hand, because it doesn't cover
> for the fact that cache had been updated but the contained items were
> modified in-kernel.
> 
> I think 'delete rule' command needs a fix, too: cmd_evaluate_delete()
> does not drop the rule from cache. I can't come up with something that
> breaks due to that, though.
> 
> > > > Error: Could not process rule: No such file or directory
> > > 
> > > BTW: There are surprisingly many spots which emit a "Could not process
> > > rule:" error. I'm sure it may be provoked for non-rule-related commands
> > > (e.g. the calls in nft_netlink()).
> > > 
> > > > add rule inet foo bar index 0 accept
> > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > 
> > > > Cache woes. Maybe a bug in
> > > > 
> > > > commit e5382c0d08e3c6d8246afa95b7380f0d6b8c1826
> > > > Author: Phil Sutter <phil@nwl.cc>
> > > > Date:   Fri Jun 7 19:21:21 2019 +0200
> > > > 
> > > >     src: Support intra-transaction rule references
> > > > 
> > > > that uncover now that cache is not flushed and sync with kernel so often?
> > > 
> > > The commit by itself is fine, as long as the cache is up to date. The
> > > problem is we have this previously inserted rule in cache which does not
> > > have a handle although it was committed to kernel already. This is
> > > something I don't see covered by e791dbe109b6d at all.
> > 
> > Yes, your commit relies on the frequent flush and resync at that time,
> > which is correct.
> > 
> > See attached patch, it sets on NFT_CACHE_REFRESH so a fresh cache from
> > the kernel is fetched in each run, it fixes the issue for me.
> 
> Yes, NFT_CACHE_REFRESH disables the conditional cache update skipping.
> It is overly agressive though: If kernel's genid matches the cache, no
> update should be needed.
> 
> > A quick recap:
> > 
> > Step #1 add rule inet foo bar accept
> > 
> > No cache is updated, because no NFT_CACHE_UPDATE is ever set on in
> > this case. It is not possible to know that the that the next command
> > will use the index feature.
> > 
> > After this point, the cache is inconsistent for this index feature.
> 
> This is expected: A simple 'add rule' command does not need a rule
> cache, so even setting NFT_CACHE_UPDATE would not lead to a consistent
> cache. This is fine, we should avoid fetching rules unless really
> needed.
> 
> > Generation ID is checked, if cache is current, the previous rule in
> > step #1 is not there in the cache.
> 
> Actually no rules at all, as NFT_CACHE_RULE_BIT remains unset.
> 
> > It should be possible to improve this logic to indicate that the cache
> > supports updates via index, instead of forcing a cache dump from the
> > kernel on each new index command.
> 
> Only if we start fetching rules for non index/handle 'rule add'/'rule
> insert' commands, right? Also, new rules have to be replaced by their
> kernel versions in cache to get the handles.
> 
> > So NFT_CACHE_UPDATE is only useful for intra-batch updates as you
> > commit describes.
> 
> > diff --git a/src/cache.c b/src/cache.c
> > index 4b797ec79ae5..1fda3f6939f5 100644
> > --- a/src/cache.c
> > +++ b/src/cache.c
> > @@ -68,7 +68,7 @@ static unsigned int evaluate_cache_add(struct cmd *cmd, unsigned int flags)
> >  
> >  		if (cmd->handle.index.id ||
> >  		    cmd->handle.position.id)
> > -			flags |= NFT_CACHE_RULE | NFT_CACHE_UPDATE;
> > +			flags |= NFT_CACHE_RULE | NFT_CACHE_UPDATE | NFT_CACHE_REFRESH;
> >  		break;
> >  	default:
> >  		break;
> 
> Acked-by: Phil Sutter <phil@nwl.cc>

This patch fixes the failures around the index keyword. I see one more
issue around set entries.

Notably, if the set add and element add are on separate lines (and thus
round trips to the kernel) then the issue is not seen. Perhaps there are
more instances with other stateful objects.

--->8---

# cat /tmp/foo
add table inet foo
add set inet foo bar { type ipv4_addr; flags interval; }; add element inet foo bar { 10.1.1.1/32 }
add element inet foo bar { 10.1.1.2/32 }

# nft -i < /tmp/foo
internal:0:0-0: Error: Could not process rule: File exists

Re: [PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Phil Sutter]

On Tue, Jul 23, 2024 at 03:30:07PM -0400, Eric Garver wrote:
> On Tue, Jul 23, 2024 at 04:34:17PM +0200, Phil Sutter wrote:
> > On Tue, Jul 23, 2024 at 02:19:19PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Jul 23, 2024 at 01:56:46PM +0200, Phil Sutter wrote:
> > > > Some digging and lots of printf's later:
> > > > 
> > > > On Mon, Jul 22, 2024 at 11:34:01PM +0200, Pablo Neira Ayuso wrote:
> > > > [...]
> > > > > I can reproduce it:
> > > > > 
> > > > > # nft -i
> > > > > nft> add table inet foo
> > > > > nft> add chain inet foo bar { type filter hook input priority filter; }
> > > > > nft> add rule inet foo bar accept
> > > > 
> > > > This bumps cache->flags from 0 to 0x1f (no cache -> NFT_CACHE_OBJECT).
> > > > 
> > > > > nft> insert rule inet foo bar index 0 accept
> > > > 
> > > > This adds NFT_CACHE_RULE_BIT and NFT_CACHE_UPDATE, cache is updated (to
> > > > fetch rules).
> > > > 
> > > > > nft> add rule inet foo bar index 0 accept
> > > > 
> > > > No new flags for this one, so the code hits the 'genid == cache->genid +
> > > > 1' case in nft_cache_is_updated() which bumps the local genid and skips
> > > > a cache update. The new rule then references the cached copy of the
> > > > previously commited one which still does not have a handle. Therefore
> > > > link_rules() does it's thing for references to  uncommitted rules which
> > > > later fails.
> > > > 
> > > > Pablo: Could you please explain the logic around this cache->genid
> > > > increment? Commit e791dbe109b6d ("cache: recycle existing cache with
> > > > incremental updates") is not clear to me in this regard. How can the
> > > > local process know it doesn't need whatever has changed in the kernel?
> > > 
> > > The idea is to use the ruleset generation ID as a hint to infer if the
> > > existing cache can be recycled, to speed up incremental updates. This
> > > is not sufficient for the index cache, see below.
> > 
> > So the assumption is that any changes to the ruleset in kernel were also
> > done to the cache, so we may just bump cache->genid instead of
> > refetching. In interactive nft, this requires for the last command to
> > have manually updated the cache as needed though. This is what confused
> > me. E.g., adding some chains:
> > 
> > - add chain t c2
> >   - no cache yet, fetch initially, store current genid
> >   - commit the new chain (kernel bumps genid)
> > 
> > - add cahin t c3
> >   - local genid == kernel's genid + 1, so skip cache update and bump
> >     local genid instead
> >   - commit the new chain (kernel bumps genid)
> > 
> > So I think if NFT_CACHE_RULE_BIT is set, NFT_CACHE_UPDATE must be set as
> > well to be sure that a previous command updated the rule cache if if was
> > a rule-related one. Therefore the local genid bump trick should be
> > allowed only if !NFT_CACHE_RULE_BIT || NFT_CACHE_UPDATE.
> > 
> > This still does not fix for the case at hand, because it doesn't cover
> > for the fact that cache had been updated but the contained items were
> > modified in-kernel.
> > 
> > I think 'delete rule' command needs a fix, too: cmd_evaluate_delete()
> > does not drop the rule from cache. I can't come up with something that
> > breaks due to that, though.
> > 
> > > > > Error: Could not process rule: No such file or directory
> > > > 
> > > > BTW: There are surprisingly many spots which emit a "Could not process
> > > > rule:" error. I'm sure it may be provoked for non-rule-related commands
> > > > (e.g. the calls in nft_netlink()).
> > > > 
> > > > > add rule inet foo bar index 0 accept
> > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > 
> > > > > Cache woes. Maybe a bug in
> > > > > 
> > > > > commit e5382c0d08e3c6d8246afa95b7380f0d6b8c1826
> > > > > Author: Phil Sutter <phil@nwl.cc>
> > > > > Date:   Fri Jun 7 19:21:21 2019 +0200
> > > > > 
> > > > >     src: Support intra-transaction rule references
> > > > > 
> > > > > that uncover now that cache is not flushed and sync with kernel so often?
> > > > 
> > > > The commit by itself is fine, as long as the cache is up to date. The
> > > > problem is we have this previously inserted rule in cache which does not
> > > > have a handle although it was committed to kernel already. This is
> > > > something I don't see covered by e791dbe109b6d at all.
> > > 
> > > Yes, your commit relies on the frequent flush and resync at that time,
> > > which is correct.
> > > 
> > > See attached patch, it sets on NFT_CACHE_REFRESH so a fresh cache from
> > > the kernel is fetched in each run, it fixes the issue for me.
> > 
> > Yes, NFT_CACHE_REFRESH disables the conditional cache update skipping.
> > It is overly agressive though: If kernel's genid matches the cache, no
> > update should be needed.
> > 
> > > A quick recap:
> > > 
> > > Step #1 add rule inet foo bar accept
> > > 
> > > No cache is updated, because no NFT_CACHE_UPDATE is ever set on in
> > > this case. It is not possible to know that the that the next command
> > > will use the index feature.
> > > 
> > > After this point, the cache is inconsistent for this index feature.
> > 
> > This is expected: A simple 'add rule' command does not need a rule
> > cache, so even setting NFT_CACHE_UPDATE would not lead to a consistent
> > cache. This is fine, we should avoid fetching rules unless really
> > needed.
> > 
> > > Generation ID is checked, if cache is current, the previous rule in
> > > step #1 is not there in the cache.
> > 
> > Actually no rules at all, as NFT_CACHE_RULE_BIT remains unset.
> > 
> > > It should be possible to improve this logic to indicate that the cache
> > > supports updates via index, instead of forcing a cache dump from the
> > > kernel on each new index command.
> > 
> > Only if we start fetching rules for non index/handle 'rule add'/'rule
> > insert' commands, right? Also, new rules have to be replaced by their
> > kernel versions in cache to get the handles.
> > 
> > > So NFT_CACHE_UPDATE is only useful for intra-batch updates as you
> > > commit describes.
> > 
> > > diff --git a/src/cache.c b/src/cache.c
> > > index 4b797ec79ae5..1fda3f6939f5 100644
> > > --- a/src/cache.c
> > > +++ b/src/cache.c
> > > @@ -68,7 +68,7 @@ static unsigned int evaluate_cache_add(struct cmd *cmd, unsigned int flags)
> > >  
> > >  		if (cmd->handle.index.id ||
> > >  		    cmd->handle.position.id)
> > > -			flags |= NFT_CACHE_RULE | NFT_CACHE_UPDATE;
> > > +			flags |= NFT_CACHE_RULE | NFT_CACHE_UPDATE | NFT_CACHE_REFRESH;
> > >  		break;
> > >  	default:
> > >  		break;
> > 
> > Acked-by: Phil Sutter <phil@nwl.cc>
> 
> This patch fixes the failures around the index keyword. I see one more
> issue around set entries.
> 
> Notably, if the set add and element add are on separate lines (and thus
> round trips to the kernel) then the issue is not seen. Perhaps there are
> more instances with other stateful objects.
> 
> --->8---
> 
> # cat /tmp/foo
> add table inet foo
> add set inet foo bar { type ipv4_addr; flags interval; }; add element inet foo bar { 10.1.1.1/32 }
> add element inet foo bar { 10.1.1.2/32 }
> 
> # nft -i < /tmp/foo
> internal:0:0-0: Error: Could not process rule: File exists

Netlink debug output indicates what's going on:

| nft> add set inet foo bar { type ipv4_addr; flags interval; }; add element inet foo bar { 10.1.1.1/32 }
| bar foo 4
| bar foo 0
| 	element 0101010a  : 0 [end]	element 0201010a  : 1 [end]
| nft> add element inet foo bar { 10.1.1.2/32 }
| bar foo 0
| 	element 00000000  : 1 [end]	element 01000000  : 1 [end]
| 	element 0201010a  : 1 [end]	element 0301010a  : 1 [end]
| 	element 0201010a  : 0 [end]	element 0301010a  : 1 [end]

In contrast, the same with separate 'add set' and 'add element'
commands:

| nft> add set inet foo bar { type ipv4_addr; flags interval; };
| bar foo 4
| nft> add element inet foo bar { 10.1.1.1/32 }
| bar foo 0
| 	element 00000000  : 1 [end]	element 0101010a  : 0 [end]
| 	element 0201010a  : 1 [end]
| nft> add element inet foo bar { 10.1.1.2/32 }
| bar foo 0
| 	element 00000000  : 1 [end]	element 0201010a  : 0 [end]
| 	element 0301010a  : 1 [end]

So I assume the code does not recognize the previously committed
elements as such and tries to commit them again.

Reverting commit e791dbe109b6d helps here as well.

BTW: It's funny how your second reproducer proves my suspicion that
nft_cmd_error() may print "Could not process rule:" errors for non-rule
commands.

Cheers, Phil

Re: [PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Pablo Neira Ayuso]

On Tue, Jul 23, 2024 at 03:30:07PM -0400, Eric Garver wrote:
> This patch fixes the failures around the index keyword. I see one more
> issue around set entries.
> 
> Notably, if the set add and element add are on separate lines (and thus
> round trips to the kernel) then the issue is not seen. Perhaps there are
> more instances with other stateful objects.
> 
> --->8---
> 
> # cat /tmp/foo
> add table inet foo
> add set inet foo bar { type ipv4_addr; flags interval; }; add element inet foo bar { 10.1.1.1/32 }
> add element inet foo bar { 10.1.1.2/32 }

Thanks for your reproducer.

I have reverted it:

https://git.netfilter.org/nftables/commit/?id=93560d0117639c8685fc287128ab06dec9950fbd

This needs more work and tests.

Re: [PATCH nft 2/2,v2] cache: recycle existing cache with incremental updates

[Eric Garver]

On Wed, Jul 24, 2024 at 09:44:23AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jul 23, 2024 at 03:30:07PM -0400, Eric Garver wrote:
> > This patch fixes the failures around the index keyword. I see one more
> > issue around set entries.
> > 
> > Notably, if the set add and element add are on separate lines (and thus
> > round trips to the kernel) then the issue is not seen. Perhaps there are
> > more instances with other stateful objects.
> > 
> > --->8---
> > 
> > # cat /tmp/foo
> > add table inet foo
> > add set inet foo bar { type ipv4_addr; flags interval; }; add element inet foo bar { 10.1.1.1/32 }
> > add element inet foo bar { 10.1.1.2/32 }
> 
> Thanks for your reproducer.
> 
> I have reverted it:
> 
> https://git.netfilter.org/nftables/commit/?id=93560d0117639c8685fc287128ab06dec9950fbd
> 
> This needs more work and tests.

Thanks Pablo. I'll keep my eyes open for future cache patches.