Re: [nft PATCH v5 00/10] Cache update fix && intra-transaction rule references

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Wed, Jun 05, 2019 at 07:05:41PM +0200, Pablo Neira Ayuso wrote:
> Thanks a lot for working on this, I have a few comments.
> 
> On Tue, Jun 04, 2019 at 07:31:48PM +0200, Phil Sutter wrote:
> > Next round of combined cache update fix and intra-transaction rule
> > reference support.
> 
> Patch 1 looks good.
> 
> > Patch 2 is new, it avoids accidential cache updates when committing a
> > transaction containing flush ruleset command and kernel ruleset has
> > changed meanwhile.
> 
> Patch 2: Could you provide an example scenario for this new patch?

Given the sample nft input:

| flush ruleset
| add table t
| add chain t c
| ...

First command causes call of cache_flush(), which updates cache->genid
from kernel. Third command causes call to cache_update(). If in between
these two another process has committed a change to kernel, kernel's
genid has bumped and cache_update() will populate the cache because
cache_is_updated() returns false.

From the top of my head I don't see where these stray cache entries
would lead to spurious errors but it is clearly false behaviour in cache
update logic.

> > Patch 3 is also new: If a transaction fails in kernel, local cache is
> > incorrect - drop it.
> 
> Patch 3 looks good!
> 
> Regarding patches 4, 5 and 6. I think we can skip them if we follow
> the approach described by [1], given there is only one single
> cache_update() call after that patchset, we don't need to do the
> "Restore local entries after cache update" logic.
> 
> [1] https://marc.info/?l=netfilter-devel&m=155975322308042&w=2

The question is how we want to treat rule reference by index if cache is
outdated. We could be nice and recalculate the rule reference which
would require to rebuild the cache including own additions. We could
also just refer to the warning in nft.8 and do nothing about it. :)

> > Patch 9 is a new requirement for patch 10 due to relocation of new
> > functions.
> > 
> > Patch 10 was changed, changelog included.
> 
> Patch 10 looks fine. However, as said, I would like to avoid the patch
> dependencies 4, 5 and 6, they are adding more cache_update() calls and
> I think we should go in the opposite direction to end up with a more
> simple approach.

OK, so how to proceed from here? I see you've based your patch series
upon an earlier version of mine. If you provide me with a clean version,
I could apply the index reference stuff on top. Or something else?

Cheers, Phil