On Wed, May 19, 2021 at 11:01:50AM +0200, Pablo Neira Ayuso wrote:
> On Wed, May 19, 2021 at 10:30:28AM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > > On Wed, May 19, 2021 at 12:56:19AM +0200, Florian Westphal wrote:
> > > > Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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.
Actually, the sequence this handle is:
add table x
add chain x y { type filter hook input priority 0; }
add table x { flags dormant; }
add table x { ; }
add table x { flags dormant; }
which is similar to:
add table x
delete table x
add table x
> A robot could generate such sequence above.
