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

[Phil Sutter <phil@nwl.cc>]

Hi,

On Tue, Nov 19, 2024 at 05:09:17PM +0100, Phil Sutter wrote:
[...]
> 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.

So I validated these findings by adding printks to BIND and UNBIND calls
and performing these actions:

- Delete an interface from a flowtable with multiple interfaces

- Add a (device to a) flowtable with --check flag

- Delete a netns containing a flowtable

- In an interactive nft session, create a table with owner flag and
  flowtable inside, then quit

All these cases cause imbalance between BIND and UNBIND calls. Looking
at possible fixes, I wonder how things are supposed to be: When deleting
a flowtable, nf_tables_commit will unregister hooks (via
nf_unregister_net_hook), but not unlink/free them. Then, in
nft_commit_release, the UNBIND happens along with unlink/free. Is this
the correct process? Namely unregister and wait for RCU grace period
before performing UNBIND? Or is this arbitrary and combining unregister
with UBIND is OK in all cases?

Cheers, Phil