| Message ID | 20241023145730.16896-1-phil@nwl.cc |
|---|---|
| Headers | show |
| Series | Dynamic hook interface binding part 1 | expand |
Hi Phil, Sorry for slowness. 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. I am inclined to postpone this batch to the next development cycle.
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
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
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.
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