On Thu, Jun 06, 2019 at 10:36:12AM +0200, Pablo Neira Ayuso wrote: > 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. Now that there is a single cache_update() call, fetching a consistent cache like iptables-nft is doing should not too hard, I think this will be sufficient to fix this scenario.
