Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> On Wed, May 19, 2021 at 02:15:33PM +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
> > >
> > > This patch is a partial revert of 0ce7cf4127f1 ("netfilter: nftables:
> > > update table flags from the commit phase") to restore the previous
> > > behaviour, which updates the dormant flag from the preparation phase
> > > to address this issue.
> > >
> > > However, there is still another problem: A batch containing a series of
> > > dorm-wakeup-dorm table and vice-versa also trigger the warning above
> > > since hook unregistration happens from the preparation phase, while hook
> > > registration occurs from the commit phase.
> >
> > You could add nf_unregister_net_hook_try() or somesuch that elides
> > the WARN().
>
> Let me elaborate a bit more the problem here.
Thanks, that makes it much clearer.
> The hook registration happens from the preparation phase (because it
> might fail for several reasons) and hook unregistration happens from
> the commit phase (because registration from the abort phase is not
> possible, since registration might fail). Hence, nf_register_net_hook()
> cannot be called from the commit and abort phases, only from the
> preparation phase.
>
> I'm assuming that looking at dormant flags from commit/abort phase
> to decide what to do.
>
> Let's have a look a several possible scenarios:
>
> Scenario A) batch containing two commands: dorm + wake up
>
> From preparation phase.
>
> - dorm: preparation phase sets the dormant flag (hooks are
> still registered).
> - wake up: unset the dormant flag. This needs to skip hook
> registration, because they are already registered.
> (it needs a way to check that hooks are registered).
>
> From commit phase.
>
> - dorm: dormant flag is unset, no-op.
> - wake-up: dormant flag is unset, no-op.
>
> From abort phase (reversed), it undoes preparation phase.
>
> - wake-up: set the dormant flag, unregister hooks.
> - dorm: unset the dormant flag, register hooks (not possible)
>
> Problems: Needs a function to check if hooks are present.
> abort phase needs to register hooks.
I agree that abort and/or commit phases cannot register hooks.
> Scenario B) batch containing two commands: wake up + dorm
>
> From preparation phase.
>
> - wake up: unset the dormant flag. This needs to register hooks.
> - dorm: preparation phase sets the dormant flag (hooks are
> still registered).
>
> From commit phase.
>
> - wake-up: dormant flag is set, unregister hooks.
> - dorm: dormant flag is set, unregister hooks (again).
>
> From abort phase (reversed), it undoes preparation phase.
>
> - dorm: unset the dormant flag, register hooks (not possible)
> - wake-up: set dormant flag, unregister hooks.
>
> Problems: commit phase needs try_unregister hook function.
> abort phase needs to unregister hooks.
... but that is doable in the sense that unregister can't fail.
> I also tried looking at the transaction state instead of the dormant
> flags, similar problems.
>
> I also tried adding more internal flags to annotate context. I looked
> at adding fields to nft_table to count for the number of pending hook
> registration / unregistration. It's all tricky.
Ok, too bad.
> The patch I posted is addressing the issue by skipping hook
> registration / unregistration for dormant flags updates.
>
> What's your concern with the approach I'm proposing?
No concern, I did not understand the problem with hook register in
abort/commit.
I also dislike that dormat tables now retrain the hook overhead, but
I guess dormat tables are an exception and not the norm, so maybe
unfounded concern.
