Re: [PATCH nft,v2 5/7] cache: consolidate reset command

[Phil Sutter <phil@nwl.cc>]

On Thu, Sep 26, 2024 at 04:34:46PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Sep 26, 2024 at 12:47:30AM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Mon, Aug 26, 2024 at 10:54:53AM +0200, Pablo Neira Ayuso wrote:
> > > Reset command does not utilize the cache infrastructure.
> > 
> > This commit changes audit log output for some reason. At least I see
> > tools/testing/selftests/net/netfilter/nft_audit.sh failing and git
> > bisect pointed at it. The relevant kselftest output is:
> > 
> > # testing for cmd: nft reset rules ... FAIL
> > #  table=t1 family=2 entries=3 op=nft_reset_rule
> > #  table=t2 family=2 entries=3 op=nft_reset_rule
> > #  table=t2 family=2 entries=3 op=nft_reset_rule
> > # -table=t2 family=2 entries=180 op=nft_reset_rule
> > # +table=t2 family=2 entries=186 op=nft_reset_rule
> > #  table=t2 family=2 entries=188 op=nft_reset_rule
> > # -table=t2 family=2 entries=135 op=nft_reset_rule
> > # +table=t2 family=2 entries=129 op=nft_reset_rule
> > 
> > I don't know why entries value changes and whether it is expected or
> > not. Could you perhaps have a look?
> 
> Before my patch, there is a single dump request to the kernel:
> 
>   rule_cache_dump table (null) chain (null) rule_handle 0 dump 1 reset 1
> 
> the skbuff already contains 6 entries for t1/c1 and t1/c2, which why
> 180 entries of t2 fit into the skbuff is delivered to userspace:
> 
>   table=t2 family=2 entries=180 op=nft_reset_rule
> 
> (it seems 186 rule entries can fit into the skbuff).
> 
> after my patch, there is one for each table:
> 
>   rule_cache_dump table t1 chain (null) rule_handle 0 dump 1 reset 1
>   rule_cache_dump table t2 chain (null) rule_handle 0 dump 1 reset 1
> 
> the skbuff is empty when dumping t2, so we can fit 6 more entries:
> 
> table=t2 family=2 entries=186 op=nft_reset_rule
> 
> If you take the number, before patch:
> 
> 180+188+135=503
> 
> after patch:
> 
> 186+188+129=503
> 
> show that is the same number of entries. Behaviour is correct.

Thanks for investigating!

> I don't know how to fix this test without removing the check for
> 'reset rules', because it will break between different nftables
> versions (due to the different strategies to dump all rules vs. dump
> rules per table).
> 
> And I don't think it makes sense to revert my userspace update just to
> make this test happy.

Maybe a valid measure is to sum up entries there to make sure the total
remains the same (just like you did above). I'll have a look at making
the test tolerant to both variants.

Thanks, Phil