Re: [nf-next PATCH v6 0/7] Dynamic hook interface binding part 1

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Fri, Nov 15, 2024 at 12:57:09PM +0100, Pablo Neira Ayuso wrote:
> Sorry for slowness.

No worries!

> On Wed, Oct 23, 2024 at 04:57:23PM +0200, Phil Sutter wrote:
> > Changes since v5:
> > - Extract the initial set of patches making netdev hooks name-based as
> >   suggested by Florian.
> > - Drop Fixes: tag from patch 1: It is not correct (the pointless check
> >   existed before that commit already) and it is rather an optimization
> >   than fixing a bug.
> > 
> > This series makes netdev hooks store the interface name spec they were
> > created for and establishes this stored name as the key identifier. The
> > previous one which is the hook's 'ops.dev' pointer is thereby freed to
> > vanish, so a vanishing netdev no longer has to drag the hook along with
> > it. (Patches 2-4)
> > 
> > Furthermore, it aligns behaviour of netdev-family chains with that of
> > flowtables in situations of vanishing interfaces. When previously a
> > chain losing its last interface was torn down and deleted, it may now
> > remain in place (albeit with no remaining interfaces). (Patch 5)
> > 
> > Patch 6 is a cleanup following patch 5, patches 1 and 7 are independent
> > code simplifications.
> 
> Patch 1-4 can be integrated, they are relatively small.
> 
> Patches 5-6 will need a rebase due to my fix in that path.
> 
> Patch 7 is probably uncovering an issue with flowtable hardware
> offload support, because I suspect _UNBIND is not called from that
> path, I need to have a look.

Checking callers of nft_unregister_flowtable_net_hooks():

nf_tables_commit() calls it for DELFLOWTABLE, code-paths differ for
flowtable updates or complete deletions: With the latter,
nft_commit_release() calls nf_tables_flowtable_destroy() which does the
UNBIND. So if deleting individual interfaces from an offloaded flowtable
is supported, we may miss the UNBIND there.

__nf_tables_abort() calls it for NEWFLOWTABLE. The hooks should have
been bound by nf_tables_newflowtable() (or nft_flowtable_update(),
respectively) so this seems like missing UNBIND there.

Now about __nft_release_hook, I see:

nf_tables_pre_exit_net
-> __nft_release_hooks
  -> __nft_release_hook

Do we have to UNBIND at netns exit?

There is also:

nft_rcv_nl_event
-> __nft_release_hook

I don't see where hooks of flowtables in owner flag tables are unbound.

> I am inclined to postpone this batch to the next development cycle.

FWIW, the bugs are older than my trivial function elimination. But
indeed, the above needs more attention than the new feature.

Cheers, Phil