Re: [nft PATCH 2/4] Revert "monitor: fix double cache update with --echo"

[Phil Sutter <phil@nwl.cc>]

On Thu, Oct 17, 2019 at 01:29:17PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 17, 2019 at 12:36:49PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Thu, Oct 17, 2019 at 11:07:38AM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote:
> > > > On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote:
> > > > > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754.
> > > > >
> > > > > While it is true that a cache exists, we still need to capture new sets
> > > > > and their elements if they are anonymous. This is because the name
> > > > > changes and rules will refer to them by name.
> > > 
> > > Please, tell me how I can reproduce this here with a simple snippet
> > > and I will have a look. Thanks!
> > 
> > Just run tests/monitor testsuite, echo testing simple.t will fail.
> > Alternatively, add a rule with anonymous set like so:
> > | # nft --echo add rule inet t c tcp dport '{ 22, 80 }'
> > 
> > > > > Given that there is no easy way to identify the anonymous set in cache
> > > > > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name,
> > > > > just go with cache updates. Assuming that echo option is typically used
> > > > > for single commands, there is not much cache updating happening anyway.
> > > > 
> > > > This was fixing a real bug, if this is breaking anything, then I think
> > > > we are not getting to the root cause.
> > > > 
> > > > But reverting it does not make things any better.
> > 
> > With all respect, this wasn't obvious. There is no test case covering
> > it, commit message reads like it is an optimization (apart from the
> > subject containing 'fix').
> 
> After reverting:
> 
> # ./run-tests.sh testcases/sets/0036add_set_element_expiration_0
> I: using nft binary ./../../src/nft
> 
> W: [FAILED]     testcases/sets/0036add_set_element_expiration_0: got 139
> 
> I: results: [OK] 0 [FAILED] 1 [TOTAL] 1
> 
> so I made the test for this fix.
> 
> commit 44348edfb9fa414152d53bcf705db882899ddc4e
> Author: Pablo Neira Ayuso <pablo@netfilter.org>
> Date:   Mon Jul 1 18:34:42 2019 +0200
> 
>     tests: shell: restore element expiration
> 
>     This patch adds a test for 24f33c710e8c ("src: enable set expiration
>     date for set elements").
> 
>     This is also implicitly testing for a cache corruption bug that is fixed
>     by 9b032cd6477b ("monitor: fix double cache update with --echo").

Ah, thanks for the pointer! I'll check what's going wrong there.

Cheers, Phil