Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested

[Phil Sutter <phil@nwl.cc>]

On Mon, Jul 22, 2019 at 11:25:56PM +0200, Pablo Neira Ayuso wrote:
> On Sun, Jul 21, 2019 at 08:50:40PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Sun, Jul 21, 2019 at 02:14:07AM +0200, Florian Westphal wrote:
> > > > This makes nft behave like 0.9.0 -- the ruleset
> > > > 
> > > > flush ruleset
> > > > table inet filter {
> > > > }
> > > > table inet filter {
> > > >       chain test {
> > > >         counter
> > > >     }
> > > > }
> > > > 
> > > > loads again without generating an error message.
> > > > I've added a test case for this, without this it will create an error,
> > > > and with a checkout of the 'fixes' tag we get crash.
> > > > 
> > > > Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1351
> > > > Fixes: e5382c0d08e3c ("src: Support intra-transaction rule references")
> > > 
> > > This one is causing the cache corruption, right?
> > 
> > There is no cache corruption.  This patch makes us enter a code
> > path that we did not take before.
> 
> Sorry, I mean, this is a cache bug :-)
> 
> cache_flush() is cheating, it sets flags to CACHE_FULL, hence this
> enters this codepath we dit not take before. This propagates from the
> previous logic, as a workaround.
> 
> I made this patch, to skip the cache in case "flush ruleset" is
> requested.
> 
> This breaks testcases/transactions/0024rule_0, which is a recent test
> from Phil to check for intra-transaction references, I don't know yet
> what makes this code unhappy with my changes.
> 
> Phil, would you help me have a look at what assumption breaks? Thanks.

Sorry, I don't get it. What is happening in the first place? Florian
writes, a lookup happens in the wrong table and it seems
chain_evaluate() doesn't add the chain to cache. Yet I don't understand
why given patch fixes the problem.

Cheers, Phil