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
