Re: [PATCH nf] netfilter: nftables: accept all dummy chain when table is dormant

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, May 19, 2021 at 10:30:28AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, May 19, 2021 at 12:56:19AM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > The dormant flag need to be updated from the preparation phase,
> > > > otherwise, two consecutive requests to dorm a table in the same batch
> > > > might try to remove the same hooks twice, resulting in the following
> > > > warning:
> > > > 
> > > >  hook not found, pf 3 num 0
> > > >  WARNING: CPU: 0 PID: 334 at net/netfilter/core.c:480 __nf_unregister_net_hook+0x1eb/0x610 net/netfilter/core.c:480
> > > >  Modules linked in:
> > > >  CPU: 0 PID: 334 Comm: kworker/u4:5 Not tainted 5.12.0-syzkaller #0
> > > >  Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > > >  Workqueue: netns cleanup_net
> > > >  RIP: 0010:__nf_unregister_net_hook+0x1eb/0x610 net/netfilter/core.c:480
> > > 
> > > Would it be possible to reject such a batch instead of having to add
> > > rely on dummy hooking instead?
> > 
> > That's a simple way to fix it, yes, ie. hit EBUSY.
> > 
> > > I don't think we should try to be clever with nonsensical yes-no-yes-yes-no
> > > type commits.
> > 
> > Note that no such EBUSY limitation exists so far in the transaction
> > semantics that I know [*]. We already discussed that robots might do
> > non-sensical stuff when creating a batches, and reporting EBUSY for
> > this add-del-add case might just break them.
> 
> I don't think this breaks existing users, noone except syzbot
> reported such WARN splat so far.
> 
> > This also removes the conditional hook registration, so hooks are
> > registered once at chain creation. This simplifies interaction with
> > the netfilter core at the cost of adding complexity to
> > nf_tables_commit_chain_prepare() path.
> 
> It also adds side effect (hook registration) during preparation phase.

Chain hook registration always happened from preparation phase before
this patch.

> I think its similar to
> 
> add table foo
> delete table foo
> delete table foo
> 
> ... and that gives -ENOENT.

This is the preparation phase that is rejecting it with -ENOENT.

The sequence that this patch handles is similar to:

add table foo
delete table foo
add table foo

which does _not_ hit EBUSY.

The existing transaction semantics handles similar sequences for the
existing objects.

This patch ensures that:

add table x
add chain x y { type filter hook input priority 0; }
add table x { flags dormant; }
add table x { ; }

in a batch file works fine.

A robot could generate such sequence above.