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

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

[Phil Sutter]

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.

Phil Sutter (7):
  netfilter: nf_tables: Flowtable hook's pf value never varies
  netfilter: nf_tables: Store user-defined hook ifname
  netfilter: nf_tables: Use stored ifname in netdev hook dumps
  netfilter: nf_tables: Compare netdev hooks based on stored name
  netfilter: nf_tables: Tolerate chains with no remaining hooks
  netfilter: nf_tables: Simplify chain netdev notifier
  netfilter: nf_tables: Drop __nft_unregister_flowtable_net_hooks()

 include/net/netfilter/nf_tables.h |  4 +-
 net/netfilter/nf_tables_api.c     | 74 +++++++++----------------------
 net/netfilter/nft_chain_filter.c  | 48 ++++++--------------
 3 files changed, 35 insertions(+), 91 deletions(-)

-- 
2.47.0

[nf-next PATCH v6 1/7] netfilter: nf_tables: Flowtable hook's pf value never varies

[Phil Sutter]

When checking for duplicate hooks in nft_register_flowtable_net_hooks(),
comparing ops.pf value is pointless as it is always NFPROTO_NETDEV with
flowtable hooks.

Dropping the check leaves the search identical to the one in
nft_hook_list_find() so call that function instead of open coding.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 30331688301e..d4563313d5e0 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -8569,7 +8569,7 @@ static int nft_register_flowtable_net_hooks(struct net *net,
 					    struct list_head *hook_list,
 					    struct nft_flowtable *flowtable)
 {
-	struct nft_hook *hook, *hook2, *next;
+	struct nft_hook *hook, *next;
 	struct nft_flowtable *ft;
 	int err, i = 0;
 
@@ -8578,12 +8578,9 @@ static int nft_register_flowtable_net_hooks(struct net *net,
 			if (!nft_is_active_next(net, ft))
 				continue;
 
-			list_for_each_entry(hook2, &ft->hook_list, list) {
-				if (hook->ops.dev == hook2->ops.dev &&
-				    hook->ops.pf == hook2->ops.pf) {
-					err = -EEXIST;
-					goto err_unregister_net_hooks;
-				}
+			if (nft_hook_list_find(&ft->hook_list, hook)) {
+				err = -EEXIST;
+				goto err_unregister_net_hooks;
 			}
 		}
 
-- 
2.47.0

[nf-next PATCH v6 2/7] netfilter: nf_tables: Store user-defined hook ifname

[Phil Sutter]

Prepare for hooks with NULL ops.dev pointer (due to non-existent device)
and store the interface name and length as specified by the user upon
creation. No functional change intended.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/net/netfilter/nf_tables.h |  2 ++
 net/netfilter/nf_tables_api.c     | 10 +++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 91ae20cb7648..b06d88af9ee3 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1192,6 +1192,8 @@ struct nft_hook {
 	struct list_head	list;
 	struct nf_hook_ops	ops;
 	struct rcu_head		rcu;
+	char			ifname[IFNAMSIZ];
+	u8			ifnamelen;
 };
 
 /**
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index d4563313d5e0..088c0f901092 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2173,7 +2173,6 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
 					      const struct nlattr *attr)
 {
 	struct net_device *dev;
-	char ifname[IFNAMSIZ];
 	struct nft_hook *hook;
 	int err;
 
@@ -2183,12 +2182,17 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
 		goto err_hook_alloc;
 	}
 
-	nla_strscpy(ifname, attr, IFNAMSIZ);
+	err = nla_strscpy(hook->ifname, attr, IFNAMSIZ);
+	if (err < 0)
+		goto err_hook_dev;
+
+	hook->ifnamelen = nla_len(attr);
+
 	/* nf_tables_netdev_event() is called under rtnl_mutex, this is
 	 * indirectly serializing all the other holders of the commit_mutex with
 	 * the rtnl_mutex.
 	 */
-	dev = __dev_get_by_name(net, ifname);
+	dev = __dev_get_by_name(net, hook->ifname);
 	if (!dev) {
 		err = -ENOENT;
 		goto err_hook_dev;
-- 
2.47.0

[nf-next PATCH v6 3/7] netfilter: nf_tables: Use stored ifname in netdev hook dumps

[Phil Sutter]

The stored ifname and ops.dev->name may deviate after creation due to
interface name changes. Prefer the more deterministic stored name in
dumps which also helps avoiding inadvertent changes to stored ruleset
dumps.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 088c0f901092..ac25a7094093 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1853,15 +1853,16 @@ static int nft_dump_basechain_hook(struct sk_buff *skb, int family,
 			if (!first)
 				first = hook;
 
-			if (nla_put_string(skb, NFTA_DEVICE_NAME,
-					   hook->ops.dev->name))
+			if (nla_put(skb, NFTA_DEVICE_NAME,
+				    hook->ifnamelen, hook->ifname))
 				goto nla_put_failure;
 			n++;
 		}
 		nla_nest_end(skb, nest_devs);
 
 		if (n == 1 &&
-		    nla_put_string(skb, NFTA_HOOK_DEV, first->ops.dev->name))
+		    nla_put(skb, NFTA_HOOK_DEV,
+			    first->ifnamelen, first->ifname))
 			goto nla_put_failure;
 	}
 	nla_nest_end(skb, nest);
@@ -8997,7 +8998,8 @@ static int nf_tables_fill_flowtable_info(struct sk_buff *skb, struct net *net,
 		hook_list = &flowtable->hook_list;
 
 	list_for_each_entry_rcu(hook, hook_list, list) {
-		if (nla_put_string(skb, NFTA_DEVICE_NAME, hook->ops.dev->name))
+		if (nla_put(skb, NFTA_DEVICE_NAME,
+			    hook->ifnamelen, hook->ifname))
 			goto nla_put_failure;
 	}
 	nla_nest_end(skb, nest_devs);
-- 
2.47.0

[nf-next PATCH v6 4/7] netfilter: nf_tables: Compare netdev hooks based on stored name

[Phil Sutter]

The 1:1 relationship between nft_hook and nf_hook_ops is about to break,
so choose the stored ifname to uniquely identify hooks.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index ac25a7094093..edea65cc5e97 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2214,7 +2214,7 @@ static struct nft_hook *nft_hook_list_find(struct list_head *hook_list,
 	struct nft_hook *hook;
 
 	list_for_each_entry(hook, hook_list, list) {
-		if (this->ops.dev == hook->ops.dev)
+		if (!strcmp(hook->ifname, this->ifname))
 			return hook;
 	}
 
-- 
2.47.0

[nf-next PATCH v6 5/7] netfilter: nf_tables: Tolerate chains with no remaining hooks

[Phil Sutter]

Do not drop a netdev-family chain if the last interface it is registered
for vanishes. Users dumping and storing the ruleset upon shutdown for
restore upon next boot may otherwise lose the chain and all contained
rules. They will still lose the list of devices, a later patch will fix
that. For now, this aligns the event handler's behaviour with that for
flowtables.
The controversal situation at netns exit should be no problem here:
event handler will unregister the hooks, core nftables cleanup code will
drop the chain itself.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/net/netfilter/nf_tables.h |  2 --
 net/netfilter/nf_tables_api.c     | 21 ---------------------
 net/netfilter/nft_chain_filter.c  | 29 +++++++----------------------
 3 files changed, 7 insertions(+), 45 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index b06d88af9ee3..9d409c02ab6a 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1229,8 +1229,6 @@ static inline bool nft_is_base_chain(const struct nft_chain *chain)
 	return chain->flags & NFT_CHAIN_BASE;
 }
 
-int __nft_release_basechain(struct nft_ctx *ctx);
-
 unsigned int nft_do_chain(struct nft_pktinfo *pkt, void *priv);
 
 static inline bool nft_use_inc(u32 *use)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index edea65cc5e97..4c2a0caa145d 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -11431,27 +11431,6 @@ int nft_data_dump(struct sk_buff *skb, int attr, const struct nft_data *data,
 }
 EXPORT_SYMBOL_GPL(nft_data_dump);
 
-int __nft_release_basechain(struct nft_ctx *ctx)
-{
-	struct nft_rule *rule, *nr;
-
-	if (WARN_ON(!nft_is_base_chain(ctx->chain)))
-		return 0;
-
-	nf_tables_unregister_hook(ctx->net, ctx->chain->table, ctx->chain);
-	list_for_each_entry_safe(rule, nr, &ctx->chain->rules, list) {
-		list_del(&rule->list);
-		nft_use_dec(&ctx->chain->use);
-		nf_tables_rule_release(ctx, rule);
-	}
-	nft_chain_del(ctx->chain);
-	nft_use_dec(&ctx->table->use);
-	nf_tables_chain_destroy(ctx->chain);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(__nft_release_basechain);
-
 static void __nft_release_hook(struct net *net, struct nft_table *table)
 {
 	struct nft_flowtable *flowtable;
diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
index 7010541fcca6..543f258b7c6b 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -322,34 +322,19 @@ static void nft_netdev_event(unsigned long event, struct net_device *dev,
 			     struct nft_ctx *ctx)
 {
 	struct nft_base_chain *basechain = nft_base_chain(ctx->chain);
-	struct nft_hook *hook, *found = NULL;
-	int n = 0;
+	struct nft_hook *hook;
 
 	list_for_each_entry(hook, &basechain->hook_list, list) {
-		if (hook->ops.dev == dev)
-			found = hook;
-
-		n++;
-	}
-	if (!found)
-		return;
+		if (hook->ops.dev != dev)
+			continue;
 
-	if (n > 1) {
 		if (!(ctx->chain->table->flags & NFT_TABLE_F_DORMANT))
-			nf_unregister_net_hook(ctx->net, &found->ops);
+			nf_unregister_net_hook(ctx->net, &hook->ops);
 
-		list_del_rcu(&found->list);
-		kfree_rcu(found, rcu);
-		return;
+		list_del_rcu(&hook->list);
+		kfree_rcu(hook, rcu);
+		break;
 	}
-
-	/* UNREGISTER events are also happening on netns exit.
-	 *
-	 * Although nf_tables core releases all tables/chains, only this event
-	 * handler provides guarantee that hook->ops.dev is still accessible,
-	 * so we cannot skip exiting net namespaces.
-	 */
-	__nft_release_basechain(ctx);
 }
 
 static int nf_tables_netdev_event(struct notifier_block *this,
-- 
2.47.0

[nf-next PATCH v6 6/7] netfilter: nf_tables: Simplify chain netdev notifier

[Phil Sutter]

With conditional chain deletion gone, callback code simplifies: Instead
of filling an nft_ctx object, just pass basechain to the per-chain
function. Also plain list_for_each_entry() is safe now.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nft_chain_filter.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
index 543f258b7c6b..19a553550c76 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -319,17 +319,16 @@ static const struct nft_chain_type nft_chain_filter_netdev = {
 };
 
 static void nft_netdev_event(unsigned long event, struct net_device *dev,
-			     struct nft_ctx *ctx)
+			     struct nft_base_chain *basechain)
 {
-	struct nft_base_chain *basechain = nft_base_chain(ctx->chain);
 	struct nft_hook *hook;
 
 	list_for_each_entry(hook, &basechain->hook_list, list) {
 		if (hook->ops.dev != dev)
 			continue;
 
-		if (!(ctx->chain->table->flags & NFT_TABLE_F_DORMANT))
-			nf_unregister_net_hook(ctx->net, &hook->ops);
+		if (!(basechain->chain.table->flags & NFT_TABLE_F_DORMANT))
+			nf_unregister_net_hook(dev_net(dev), &hook->ops);
 
 		list_del_rcu(&hook->list);
 		kfree_rcu(hook, rcu);
@@ -343,25 +342,20 @@ static int nf_tables_netdev_event(struct notifier_block *this,
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct nft_base_chain *basechain;
 	struct nftables_pernet *nft_net;
-	struct nft_chain *chain, *nr;
+	struct nft_chain *chain;
 	struct nft_table *table;
-	struct nft_ctx ctx = {
-		.net	= dev_net(dev),
-	};
 
 	if (event != NETDEV_UNREGISTER)
 		return NOTIFY_DONE;
 
-	nft_net = nft_pernet(ctx.net);
+	nft_net = nft_pernet(dev_net(dev));
 	mutex_lock(&nft_net->commit_mutex);
 	list_for_each_entry(table, &nft_net->tables, list) {
 		if (table->family != NFPROTO_NETDEV &&
 		    table->family != NFPROTO_INET)
 			continue;
 
-		ctx.family = table->family;
-		ctx.table = table;
-		list_for_each_entry_safe(chain, nr, &table->chains, list) {
+		list_for_each_entry(chain, &table->chains, list) {
 			if (!nft_is_base_chain(chain))
 				continue;
 
@@ -370,8 +364,7 @@ static int nf_tables_netdev_event(struct notifier_block *this,
 			    basechain->ops.hooknum != NF_INET_INGRESS)
 				continue;
 
-			ctx.chain = chain;
-			nft_netdev_event(event, dev, &ctx);
+			nft_netdev_event(event, dev, basechain);
 		}
 	}
 	mutex_unlock(&nft_net->commit_mutex);
-- 
2.47.0

[nf-next PATCH v6 7/7] netfilter: nf_tables: Drop __nft_unregister_flowtable_net_hooks()

[Phil Sutter]

The function is a 1:1 copy of nft_netdev_unregister_hooks(), use the
latter in its place.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 4c2a0caa145d..e6c8314817e0 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -8548,25 +8548,10 @@ static void nft_unregister_flowtable_hook(struct net *net,
 				    FLOW_BLOCK_UNBIND);
 }
 
-static void __nft_unregister_flowtable_net_hooks(struct net *net,
-						 struct list_head *hook_list,
-					         bool release_netdev)
-{
-	struct nft_hook *hook, *next;
-
-	list_for_each_entry_safe(hook, next, hook_list, list) {
-		nf_unregister_net_hook(net, &hook->ops);
-		if (release_netdev) {
-			list_del(&hook->list);
-			kfree_rcu(hook, rcu);
-		}
-	}
-}
-
 static void nft_unregister_flowtable_net_hooks(struct net *net,
 					       struct list_head *hook_list)
 {
-	__nft_unregister_flowtable_net_hooks(net, hook_list, false);
+	nft_netdev_unregister_hooks(net, hook_list, false);
 }
 
 static int nft_register_flowtable_net_hooks(struct net *net,
@@ -11439,8 +11424,7 @@ static void __nft_release_hook(struct net *net, struct nft_table *table)
 	list_for_each_entry(chain, &table->chains, list)
 		__nf_tables_unregister_hook(net, table, chain, true);
 	list_for_each_entry(flowtable, &table->flowtables, list)
-		__nft_unregister_flowtable_net_hooks(net, &flowtable->hook_list,
-						     true);
+		nft_netdev_unregister_hooks(net, &flowtable->hook_list, true);
 }
 
 static void __nft_release_hooks(struct net *net)
-- 
2.47.0

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

[Pablo Neira Ayuso]

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.

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

[Phil Sutter]

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

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

[Phil Sutter]

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

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

[Pablo Neira Ayuso]

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.

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

[Phil Sutter]

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