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
