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

[Phil Sutter <phil@nwl.cc>]

On Fri, Nov 22, 2024 at 02:39:31PM +0100, Pablo Neira Ayuso wrote:
> 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.

Oh, that's interesting news! Is it sufficient to reject flowtable
updates if nf_flowtable::flags has NF_FLOWTABLE_HW_OFFLOAD bit set?

> 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).

Those two should be unproblematic though, both netns exit and owner exit
cause full flowtable deletion - basically same mechanism as for regular
'nft delete flowtable' should suffice.

> 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.

Sure, thanks!

Cheers, Phil