Hi Phil,
On Wed, Aug 29, 2018 at 02:40:18PM +0200, Phil Sutter wrote:
> On Wed, Aug 29, 2018 at 02:04:45PM +0200, Pablo Neira Ayuso wrote:
> > On Wed, Aug 29, 2018 at 01:16:25PM +0200, Pablo Neira Ayuso wrote:
> > > Hi Phil,
> > >
> > > On Wed, Aug 29, 2018 at 11:52:09AM +0200, Phil Sutter wrote:
> > > > Hi,
> > > >
> > > > I found constellations in which userspace ruleset cache maintenance
> > > > causes headaches. A "simple" case is this:
> > > >
> > > > | # nft flush ruleset
> > > > | # nft -i
> > > > | nft> add table ip t
> > > > | nft> list ruleset
> > > > | table ip t {
> > > > | }
> > > > | table ip t {
> > > > | }
> >
> > Hm, wait. See below.
> >
> > [...]
> > > > Here's a more complex one:
> > > >
> > > > | # ./src/nft -i
> > > > | nft> list ruleset
> > > > | nft> add table ip t
> > > > | nft> add table ip t2 ; add chain ip t2 c
> > > > | Error: Could not process rule: Table 't2' does not exist
> > > > | add table ip t2 ; add chain ip t2 c
> > > > | ^^^^^^^^^^^^^^^^^^
> > > >
> > > > The first call to 'list ruleset' causes cache->genid to be non-zero.
> > > > Adding the first table does not trigger a cache update, but kernel's
> > > > genid increments as a result.
> >
> > Looks like we should be flushing after 'list ruleset' in interactive
> > mode? In these two examples it looks like there's a missing flush
> > somewhere in the interactive mode to me.
> >
> > I mean, we don't know when the next command comes, so doing fine grain
> > tracking in this case makes no sense.
>
> I would avoid special treating for interactive mode as it increases
> complexity. BTW, I discovered the issue while testing for JSON, the CLI
> was just a simple way to reproduce it. But I agree that we should try to
> keep simple fire and forget use-cases as fast as possible. You do have
> scripts to detect performance regressions, right? Maybe I'll give
> incremental cache updates a try and we see how bad things become?
Sure, go ahead. Finer grain is good to have so we don't always need to
flush and resync. But please keep in mind a few things:
* Listing inside batch shows the existing ruleset context. Will send a
patch to expose the 'commit' statement so we provide similar
semantics to iptables. There are a few of bugzilla reports from
people that does list in a ruleset batch that is loaded via nft -f
which confuses users because it shows the old ruleset.
* For finer grain tracking in interactive mode, we will have to track
events and incrementally update the cache. Unless you come up with
anything better than that :-)
Thanks!
