Re: [iptables PATCH 3/3] nft: Fix for concurrent noflush restore calls

[Phil Sutter <phil@nwl.cc>]

On Tue, Oct 13, 2020 at 12:15:02PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 13, 2020 at 12:08:03PM +0200, Phil Sutter wrote:
> [...]
> > On Mon, Oct 12, 2020 at 02:54:50PM +0200, Pablo Neira Ayuso wrote:
> > [...]
> > > Patch LGTM, thanks Phil.
> > > 
> > > What I don't clearly see yet is what scenario is triggering the bug in
> > > the existing code, if you don't mind to explain.
> > 
> > See the test case attached to the patch: An other iptables-restore
> > process may add references (i.e., jumps) to a chain the own
> > iptables-restore process wants to delete. This should not be a problem
> > because these references are added to a chain that is being flushed by
> > the own process as well. But if that chain doesn't exist while the own
> > process fetches kernel's ruleset, this flush job is not created.
> 
> Let me rephrase this:
> 
> 1) process A fetches the ruleset, finds no chain C (no flush job then)
> 2) process B adds new chain C, flush job is present
> 3) process B adds the ruleset
> 4) process A appends rules to the existing chain C (because there is
>    no flush job)
> 
> Is this the scenario? If so, I wonder why the generation ID is not
> helping to refresh and retry.

Not quite, let me try to put this more clearly:

* Dump A:
  | *filter
  | :FOO - [0:0] # flush chain FOO
  | -X BAR       # remove chain BAR
  | COMMIT

* Dump B:
  | *filter
  | -A FOO -j BAR # reference BAR from a rule in FOO
  | COMMIT

* Kernel ruleset:
  | *filter
  | :BAR - [0:0]
  | COMMIT

* Process A:
  * read dump A
  * fetch cache

* Process B:
  * read dump B
  * fetch ruleset
  * commit to kernel

* Process A:
  * skip flush chain FOO job: not present
  * add delete chain BAR job: chain exists
  * commit fails (genid outdated)
  * refresh transaction:
    * delete chain BAR job remains active
    * genid updated
  * commit fails: can't remove chain BAR: EBUSY

I realize the test case is not quite effective, ruleset should be
emptied upon each iteration of concurrent restore job startup.

Cheers, Phil