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

[Phil Sutter <phil@nwl.cc>]

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