On Wed, Jun 05, 2019 at 09:24:29PM +0200, Phil Sutter wrote: > 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. Thanks for explaining, so this is resulting from lack of consistency in the cache handling. > > > 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. :) We can set the generation ID of reference that is used for this batch, the kernel then bails out with ERESTART so we can rebuild the cache. However, I think the problem with the index is that it is unreliable itself for concurrent updates. In that case the rule handle should be used. > > > 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? That would be great. I'm going to send a v2 for: http://patchwork.ozlabs.org/patch/1110586/ addressing your feedback, then you can rebase on top. Thanks!
