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

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

On Thu, Nov 21, 2024 at 06:04:55PM +0100, Phil Sutter wrote:
> 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?

Thanks for the detailed report.

Basically, add/delete interface to an existing flowtable is not
supported by hardware offload at this stage, one option is to reject
this by now.

Then, netns integration was never considered, because it was not clear
to me how hardware offload mix with containers at this stage. This
needs to be fixed. Same applies interactive nft session (owner flag).

This is my mess, let me post a fix so we can soonish clean the way for
you to follow up on your effort to allow for dynamic interface
bindings in the next merge window once this fix gets to net.git.

Thanks.