[nf-next PATCH 0/5] Dynamic hook interface binding

[nf-next PATCH 0/5] Dynamic hook interface binding

[Phil Sutter]

Currently, netdev-family chains and flowtables expect their interfaces
to exist at creation time. In practice, this bites users of virtual
interfaces if these happen to be created after the nftables service
starts up and loads the stored ruleset.

Vice-versa, if an interface disappears at run-time (via module unloading
or 'ip link del'), it also disappears from the ruleset, along with the
chain and its rules which binds to it. This is at least problematic for
setups which store the running ruleset during system shutdown.

This series attempts to solve these problems by effectively making
netdev hooks name-based: If no matching interface is found at hook
creation time, it will be inactive until a matching interface appears.
If a bound interface is renamed, a matching inactive hook is searched
for it.

Ruleset dumps will stabilize in that regard. To still provide
information about which existing interfaces a chain/flowtable currently
binds to, new netlink attributes *_ACT_DEVS are introduced which are
filled from the active hooks only.

This series is also prep work for a simple ildcard interface binding
similar to the wildcard interface matching in meta expression. It should
suffice to turn struct nft_hook::ops into an array of all matching
interfaces, but the respective code does not exist yet.

Phil Sutter (5):
  netfilter: nf_tables: Store user-defined hook ifname
  netfilter: nf_tables: Relax hook interface binding
  netfilter: nf_tables: Report active interfaces to user space
  netfilter: nf_tables: Dynamic hook interface binding
  netfilter: nf_tables: Correctly handle NETDEV_RENAME events

 include/net/netfilter/nf_tables.h        |   4 +-
 include/uapi/linux/netfilter/nf_tables.h |   6 +-
 net/netfilter/nf_tables_api.c            | 185 +++++++++++++++--------
 net/netfilter/nft_chain_filter.c         |  70 +++++----
 4 files changed, 172 insertions(+), 93 deletions(-)

-- 
2.43.0

[nf-next PATCH 1/5] netfilter: nf_tables: Store user-defined hook ifname

[Phil Sutter]

In order to support dynamic interface binding, the name must be stored
separately. Also store the attribute length, it may serve to implement
simple wildcards (eth* for instance).

Also use the stored name when filling hook's NFTA_DEVICE_NAME attribute.
This avoids at least inadvertent changes in stored rulesets if an
interface is renamed at run-time.

Compare hooks by this stored interface name instead of the 'ops.dev'
pointer. Also prerequisite work for dynamic interface binding.

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

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 3f1ed467f951..3dec239bdb22 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1183,6 +1183,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 84fa25305b4f..4f64dbac5abc 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1799,15 +1799,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);
@@ -2118,7 +2119,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;
 
@@ -2128,12 +2128,13 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
 		goto err_hook_alloc;
 	}
 
-	nla_strscpy(ifname, attr, IFNAMSIZ);
+	nla_strscpy(hook->ifname, attr, IFNAMSIZ);
+	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;
@@ -2154,7 +2155,8 @@ 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 (hook->ifnamelen == this->ifnamelen &&
+		    !strncmp(hook->ifname, this->ifname, hook->ifnamelen))
 			return hook;
 	}
 
@@ -8908,7 +8910,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.43.0

[nf-next PATCH 2/5] netfilter: nf_tables: Relax hook interface binding

[Phil Sutter]

When creating a new flowtable or netdev-family chain, accept that the
devices to bind to may not exist and proceed to create a stub hook.
Such inactive hooks are identified by 'ops.dev' pointer being NULL,
ignore them for practical purposes.

When reacting upon a vanishing interface, merely deactivate the hook
instead of removing it from the list. Also leave netdev chains in place
even if no active hooks remain. In combination with externally stored
interface names, this stabilizes ruleset dumps with regard to
disappearing interfaces.

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

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 3dec239bdb22..9cbef71f0462 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1220,8 +1220,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 4f64dbac5abc..35990fbed444 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -282,6 +282,9 @@ static int nft_netdev_register_hooks(struct net *net,
 
 	j = 0;
 	list_for_each_entry(hook, hook_list, list) {
+		if (!hook->ops.dev)
+			continue;
+
 		err = nf_register_net_hook(net, &hook->ops);
 		if (err < 0)
 			goto err_register;
@@ -292,6 +295,9 @@ static int nft_netdev_register_hooks(struct net *net,
 
 err_register:
 	list_for_each_entry(hook, hook_list, list) {
+		if (!hook->ops.dev)
+			continue;
+
 		if (j-- <= 0)
 			break;
 
@@ -307,7 +313,10 @@ static void nft_netdev_unregister_hooks(struct net *net,
 	struct nft_hook *hook, *next;
 
 	list_for_each_entry_safe(hook, next, hook_list, list) {
-		nf_unregister_net_hook(net, &hook->ops);
+		if (hook->ops.dev) {
+			nf_unregister_net_hook(net, &hook->ops);
+			hook->ops.dev = NULL;
+		}
 		if (release_netdev) {
 			list_del(&hook->list);
 			kfree_rcu(hook, rcu);
@@ -2118,7 +2127,6 @@ void nf_tables_chain_destroy(struct nft_ctx *ctx)
 static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
 					      const struct nlattr *attr)
 {
-	struct net_device *dev;
 	struct nft_hook *hook;
 	int err;
 
@@ -2134,17 +2142,10 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
 	 * indirectly serializing all the other holders of the commit_mutex with
 	 * the rtnl_mutex.
 	 */
-	dev = __dev_get_by_name(net, hook->ifname);
-	if (!dev) {
-		err = -ENOENT;
-		goto err_hook_dev;
-	}
-	hook->ops.dev = dev;
+	hook->ops.dev = __dev_get_by_name(net, hook->ifname);
 
 	return hook;
 
-err_hook_dev:
-	kfree(hook);
 err_hook_alloc:
 	return ERR_PTR(err);
 }
@@ -8452,6 +8453,9 @@ static void nft_unregister_flowtable_hook(struct net *net,
 					  struct nft_flowtable *flowtable,
 					  struct nft_hook *hook)
 {
+	if (!hook->ops.dev)
+		return;
+
 	nf_unregister_net_hook(net, &hook->ops);
 	flowtable->data.type->setup(&flowtable->data, hook->ops.dev,
 				    FLOW_BLOCK_UNBIND);
@@ -8464,7 +8468,8 @@ static void __nft_unregister_flowtable_net_hooks(struct net *net,
 	struct nft_hook *hook, *next;
 
 	list_for_each_entry_safe(hook, next, hook_list, list) {
-		nf_unregister_net_hook(net, &hook->ops);
+		if (hook->ops.dev)
+			nf_unregister_net_hook(net, &hook->ops);
 		if (release_netdev) {
 			list_del(&hook->list);
 			kfree_rcu(hook, rcu);
@@ -8488,6 +8493,9 @@ static int nft_register_flowtable_net_hooks(struct net *net,
 	int err, i = 0;
 
 	list_for_each_entry(hook, hook_list, list) {
+		if (!hook->ops.dev)
+			continue;
+
 		list_for_each_entry(ft, &table->flowtables, list) {
 			if (!nft_is_active_next(net, ft))
 				continue;
@@ -8522,6 +8530,9 @@ static int nft_register_flowtable_net_hooks(struct net *net,
 
 err_unregister_net_hooks:
 	list_for_each_entry_safe(hook, next, hook_list, list) {
+		if (!hook->ops.dev)
+			continue;
+
 		if (i-- <= 0)
 			break;
 
@@ -9117,8 +9128,10 @@ static void nf_tables_flowtable_destroy(struct nft_flowtable *flowtable)
 
 	flowtable->data.type->free(&flowtable->data);
 	list_for_each_entry_safe(hook, next, &flowtable->hook_list, list) {
-		flowtable->data.type->setup(&flowtable->data, hook->ops.dev,
-					    FLOW_BLOCK_UNBIND);
+		if (hook->ops.dev)
+			flowtable->data.type->setup(&flowtable->data,
+						    hook->ops.dev,
+						    FLOW_BLOCK_UNBIND);
 		list_del_rcu(&hook->list);
 		kfree(hook);
 	}
@@ -9164,8 +9177,7 @@ static void nft_flowtable_event(unsigned long event, struct net_device *dev,
 
 		/* flow_offload_netdev_event() cleans up entries for us. */
 		nft_unregister_flowtable_hook(dev_net(dev), flowtable, hook);
-		list_del_rcu(&hook->list);
-		kfree_rcu(hook, rcu);
+		hook->ops.dev = NULL;
 		break;
 	}
 }
@@ -11406,27 +11418,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);
-
-	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 274b6f7e6bb5..ddb438bc2afd 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -322,35 +322,18 @@ 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;
 
 	if (event != NETDEV_UNREGISTER)
 		return;
 
 	list_for_each_entry(hook, &basechain->hook_list, list) {
-		if (hook->ops.dev == dev)
-			found = hook;
-
-		n++;
-	}
-	if (!found)
-		return;
-
-	if (n > 1) {
-		nf_unregister_net_hook(ctx->net, &found->ops);
-		list_del_rcu(&found->list);
-		kfree_rcu(found, rcu);
-		return;
+		if (hook->ops.dev == dev) {
+			nf_unregister_net_hook(ctx->net, &hook->ops);
+			hook->ops.dev = NULL;
+			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.43.0

[nf-next PATCH 3/5] netfilter: nf_tables: Report active interfaces to user space

[Phil Sutter]

Since netdev family chains and flowtables now report the interfaces they
were created for irrespective of their existence, introduce new netlink
attributes holding the currently active set of interfaces.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/uapi/linux/netfilter/nf_tables.h |  6 +++++-
 net/netfilter/nf_tables_api.c            | 25 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index aa4094ca2444..adcac6ee619d 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -164,6 +164,7 @@ enum nft_list_attributes {
  * @NFTA_HOOK_PRIORITY: netfilter hook priority (NLA_U32)
  * @NFTA_HOOK_DEV: netdevice name (NLA_STRING)
  * @NFTA_HOOK_DEVS: list of netdevices (NLA_NESTED)
+ * @NFTA_HOOK_ACT_DEVS: list of active netdevices (NLA_NESTED)
  */
 enum nft_hook_attributes {
 	NFTA_HOOK_UNSPEC,
@@ -171,6 +172,7 @@ enum nft_hook_attributes {
 	NFTA_HOOK_PRIORITY,
 	NFTA_HOOK_DEV,
 	NFTA_HOOK_DEVS,
+	NFTA_HOOK_ACT_DEVS,
 	__NFTA_HOOK_MAX
 };
 #define NFTA_HOOK_MAX		(__NFTA_HOOK_MAX - 1)
@@ -1717,13 +1719,15 @@ enum nft_flowtable_attributes {
  *
  * @NFTA_FLOWTABLE_HOOK_NUM: netfilter hook number (NLA_U32)
  * @NFTA_FLOWTABLE_HOOK_PRIORITY: netfilter hook priority (NLA_U32)
- * @NFTA_FLOWTABLE_HOOK_DEVS: input devices this flow table is bound to (NLA_NESTED)
+ * @NFTA_FLOWTABLE_HOOK_DEVS: input devices this flow table is configured for (NLA_NESTED)
+ * @NFTA_FLOWTABLE_HOOK_ACT_DEVS: input devices this flow table is currently bound to (NLA_NESTED)
  */
 enum nft_flowtable_hook_attributes {
 	NFTA_FLOWTABLE_HOOK_UNSPEC,
 	NFTA_FLOWTABLE_HOOK_NUM,
 	NFTA_FLOWTABLE_HOOK_PRIORITY,
 	NFTA_FLOWTABLE_HOOK_DEVS,
+	NFTA_FLOWTABLE_HOOK_ACT_DEVS,
 	__NFTA_FLOWTABLE_HOOK_MAX
 };
 #define NFTA_FLOWTABLE_HOOK_MAX	(__NFTA_FLOWTABLE_HOOK_MAX - 1)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 35990fbed444..87576accc2b2 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1819,6 +1819,18 @@ static int nft_dump_basechain_hook(struct sk_buff *skb, int family,
 		    nla_put(skb, NFTA_HOOK_DEV,
 			    first->ifnamelen, first->ifname))
 			goto nla_put_failure;
+
+		nest_devs = nla_nest_start_noflag(skb, NFTA_HOOK_ACT_DEVS);
+		if (!nest_devs)
+			goto nla_put_failure;
+
+		list_for_each_entry(hook, hook_list, list) {
+			if (hook->ops.dev &&
+			    nla_put_string(skb, NFTA_DEVICE_NAME,
+					   hook->ops.dev->name))
+				goto nla_put_failure;
+		}
+		nla_nest_end(skb, nest_devs);
 	}
 	nla_nest_end(skb, nest);
 
@@ -8926,6 +8938,19 @@ static int nf_tables_fill_flowtable_info(struct sk_buff *skb, struct net *net,
 			goto nla_put_failure;
 	}
 	nla_nest_end(skb, nest_devs);
+
+	nest_devs = nla_nest_start_noflag(skb, NFTA_FLOWTABLE_HOOK_ACT_DEVS);
+	if (!nest_devs)
+		goto nla_put_failure;
+
+	list_for_each_entry_rcu(hook, hook_list, list) {
+		if (hook->ops.dev &&
+		    nla_put_string(skb, NFTA_DEVICE_NAME,
+				   hook->ops.dev->name))
+			goto nla_put_failure;
+	}
+	nla_nest_end(skb, nest_devs);
+
 	nla_nest_end(skb, nest);
 
 	nlmsg_end(skb, nlh);
-- 
2.43.0

[nf-next PATCH 4/5] netfilter: nf_tables: Dynamic hook interface binding

[Phil Sutter]

Upon NETDEV_REGISTER event, search existing flowtables and netdev-family
chains for a matching inactive hook and bind the device.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c    | 76 +++++++++++++++++++++++---------
 net/netfilter/nft_chain_filter.c | 40 +++++++++++++++--
 2 files changed, 91 insertions(+), 25 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 87576accc2b2..b19f40874c48 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -8460,6 +8460,27 @@ nft_flowtable_type_get(struct net *net, u8 family)
 	return ERR_PTR(-ENOENT);
 }
 
+static int nft_register_flowtable_hook(struct net *net,
+				       struct nft_flowtable *flowtable,
+				       struct nft_hook *hook)
+{
+	int err;
+
+	err = flowtable->data.type->setup(&flowtable->data,
+					  hook->ops.dev, FLOW_BLOCK_BIND);
+	if (err < 0)
+		return err;
+
+	err = nf_register_net_hook(net, &hook->ops);
+	if (err < 0) {
+		flowtable->data.type->setup(&flowtable->data,
+					    hook->ops.dev, FLOW_BLOCK_UNBIND);
+		return err;
+	}
+
+	return 0;
+}
+
 /* Only called from error and netdev event paths. */
 static void nft_unregister_flowtable_hook(struct net *net,
 					  struct nft_flowtable *flowtable,
@@ -8521,20 +8542,10 @@ static int nft_register_flowtable_net_hooks(struct net *net,
 			}
 		}
 
-		err = flowtable->data.type->setup(&flowtable->data,
-						  hook->ops.dev,
-						  FLOW_BLOCK_BIND);
+		err = nft_register_flowtable_hook(net, flowtable, hook);
 		if (err < 0)
 			goto err_unregister_net_hooks;
 
-		err = nf_register_net_hook(net, &hook->ops);
-		if (err < 0) {
-			flowtable->data.type->setup(&flowtable->data,
-						    hook->ops.dev,
-						    FLOW_BLOCK_UNBIND);
-			goto err_unregister_net_hooks;
-		}
-
 		i++;
 	}
 
@@ -9191,20 +9202,40 @@ static int nf_tables_fill_gen_info(struct sk_buff *skb, struct net *net,
 	return -EMSGSIZE;
 }
 
-static void nft_flowtable_event(unsigned long event, struct net_device *dev,
-				struct nft_flowtable *flowtable)
+static int nft_flowtable_event(unsigned long event, struct net_device *dev,
+			       struct nft_flowtable *flowtable)
 {
 	struct nft_hook *hook;
 
 	list_for_each_entry(hook, &flowtable->hook_list, list) {
-		if (hook->ops.dev != dev)
-			continue;
+		switch (event) {
+		case NETDEV_UNREGISTER:
+			if (hook->ops.dev != dev)
+				break;
 
-		/* flow_offload_netdev_event() cleans up entries for us. */
-		nft_unregister_flowtable_hook(dev_net(dev), flowtable, hook);
-		hook->ops.dev = NULL;
-		break;
+			/* flow_offload_netdev_event() cleans up entries for us. */
+			nft_unregister_flowtable_hook(dev_net(dev),
+						      flowtable, hook);
+			hook->ops.dev = NULL;
+			return 1;
+		case NETDEV_REGISTER:
+			if (hook->ops.dev ||
+			    strncmp(hook->ifname, dev->name, hook->ifnamelen))
+				break;
+
+			hook->ops.dev = dev;
+			if (!nft_register_flowtable_hook(dev_net(dev),
+							 flowtable, hook))
+				return 1;
+
+			printk(KERN_ERR
+			       "flowtable %s: Can't hook into device %s\n",
+			       flowtable->name, dev->name);
+			hook->ops.dev = NULL;
+			break;
+		}
 	}
+	return 0;
 }
 
 static int nf_tables_flowtable_event(struct notifier_block *this,
@@ -9216,7 +9247,8 @@ static int nf_tables_flowtable_event(struct notifier_block *this,
 	struct nft_table *table;
 	struct net *net;
 
-	if (event != NETDEV_UNREGISTER)
+	if (event != NETDEV_UNREGISTER &&
+	    event != NETDEV_REGISTER)
 		return 0;
 
 	net = dev_net(dev);
@@ -9224,9 +9256,11 @@ static int nf_tables_flowtable_event(struct notifier_block *this,
 	mutex_lock(&nft_net->commit_mutex);
 	list_for_each_entry(table, &nft_net->tables, list) {
 		list_for_each_entry(flowtable, &table->flowtables, list) {
-			nft_flowtable_event(event, dev, flowtable);
+			if (nft_flowtable_event(event, dev, flowtable))
+				goto out_unlock;
 		}
 	}
+out_unlock:
 	mutex_unlock(&nft_net->commit_mutex);
 
 	return NOTIFY_DONE;
diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
index ddb438bc2afd..b2147f8be60c 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -318,19 +318,50 @@ static const struct nft_chain_type nft_chain_filter_netdev = {
 	},
 };
 
+static int nft_netdev_hook_dev_update(struct nft_hook *hook,
+				      struct net_device *dev)
+{
+	int ret = 0;
+
+	if (hook->ops.dev)
+		nf_unregister_net_hook(dev_net(hook->ops.dev), &hook->ops);
+
+	hook->ops.dev = dev;
+
+	if (dev) {
+		ret = nf_register_net_hook(dev_net(dev), &hook->ops);
+		if (ret < 0)
+			hook->ops.dev = NULL;
+	}
+
+	return ret;
+}
+
 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;
 
-	if (event != NETDEV_UNREGISTER)
+	if (event != NETDEV_UNREGISTER &&
+	    event != NETDEV_REGISTER)
 		return;
 
 	list_for_each_entry(hook, &basechain->hook_list, list) {
-		if (hook->ops.dev == dev) {
-			nf_unregister_net_hook(ctx->net, &hook->ops);
-			hook->ops.dev = NULL;
+		switch (event) {
+		case NETDEV_UNREGISTER:
+			if (hook->ops.dev == dev)
+				nft_netdev_hook_dev_update(hook, NULL);
+			break;
+		case NETDEV_REGISTER:
+			if (hook->ops.dev ||
+			    strncmp(hook->ifname, dev->name, hook->ifnamelen))
+				break;
+			if (!nft_netdev_hook_dev_update(hook, dev))
+				return;
+
+			printk(KERN_ERR "chain %s: Can't hook into device %s\n",
+			       ctx->chain->name, dev->name);
 			break;
 		}
 	}
@@ -349,6 +380,7 @@ static int nf_tables_netdev_event(struct notifier_block *this,
 	};
 
 	if (event != NETDEV_UNREGISTER &&
+	    event != NETDEV_REGISTER &&
 	    event != NETDEV_CHANGENAME)
 		return NOTIFY_DONE;
 
-- 
2.43.0

[nf-next PATCH 5/5] netfilter: nf_tables: Correctly handle NETDEV_RENAME events

[Phil Sutter]

Treat a netdev rename like removal and recreation with a different name.
In theory, one could leave hooks in place which still cover the new
name, but this is both unlikely and needlessly complicates the
code.

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

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index b19f40874c48..b3a5a2878459 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -9247,9 +9247,13 @@ static int nf_tables_flowtable_event(struct notifier_block *this,
 	struct nft_table *table;
 	struct net *net;
 
-	if (event != NETDEV_UNREGISTER &&
-	    event != NETDEV_REGISTER)
-		return 0;
+	if (event == NETDEV_CHANGENAME) {
+		nf_tables_flowtable_event(this, NETDEV_UNREGISTER, ptr);
+		event = NETDEV_REGISTER;
+	} else if (event != NETDEV_UNREGISTER &&
+		   event != NETDEV_REGISTER) {
+		return NOTIFY_DONE;
+	}
 
 	net = dev_net(dev);
 	nft_net = nft_pernet(net);
diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
index b2147f8be60c..cc0cf47503f4 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -379,10 +379,13 @@ static int nf_tables_netdev_event(struct notifier_block *this,
 		.net	= dev_net(dev),
 	};
 
-	if (event != NETDEV_UNREGISTER &&
-	    event != NETDEV_REGISTER &&
-	    event != NETDEV_CHANGENAME)
+	if (event == NETDEV_CHANGENAME) {
+		nf_tables_netdev_event(this, NETDEV_UNREGISTER, ptr);
+		event = NETDEV_REGISTER;
+	} else if (event != NETDEV_UNREGISTER &&
+		   event != NETDEV_REGISTER) {
 		return NOTIFY_DONE;
+	}
 
 	nft_net = nft_pernet(ctx.net);
 	mutex_lock(&nft_net->commit_mutex);
-- 
2.43.0

Re: [nf-next PATCH 0/5] Dynamic hook interface binding

[Pablo Neira Ayuso]

Hi Phil,

On Fri, May 03, 2024 at 09:50:40PM +0200, Phil Sutter wrote:
> Currently, netdev-family chains and flowtables expect their interfaces
> to exist at creation time. In practice, this bites users of virtual
> interfaces if these happen to be created after the nftables service
> starts up and loads the stored ruleset.
> 
> Vice-versa, if an interface disappears at run-time (via module unloading
> or 'ip link del'), it also disappears from the ruleset, along with the
> chain and its rules which binds to it. This is at least problematic for
> setups which store the running ruleset during system shutdown.
> 
> This series attempts to solve these problems by effectively making
> netdev hooks name-based: If no matching interface is found at hook
> creation time, it will be inactive until a matching interface appears.
> If a bound interface is renamed, a matching inactive hook is searched
> for it.
> 
> Ruleset dumps will stabilize in that regard. To still provide
> information about which existing interfaces a chain/flowtable currently
> binds to, new netlink attributes *_ACT_DEVS are introduced which are
> filled from the active hooks only.
> 
> This series is also prep work for a simple ildcard interface binding
> similar to the wildcard interface matching in meta expression. It should
> suffice to turn struct nft_hook::ops into an array of all matching
> interfaces, but the respective code does not exist yet.

Before taking a closer look: Would it be possible to have a torture
test to exercise this path from userspace?

Thanks!

Re: [nf-next PATCH 0/5] Dynamic hook interface binding

[Phil Sutter]

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

Hi Pablo,

On Fri, May 10, 2024 at 02:13:11AM +0200, Pablo Neira Ayuso wrote:
> Before taking a closer look: Would it be possible to have a torture
> test to exercise this path from userspace?

Please kindly find my torture script attached. It does the following:

1) Create three netns connected by VETH pairs:
   client [cr0]<->[rc0] router [rs0]<->[sr0] server

2) In router ns, add an nftables ruleset with:
   - A netdev chain for each interface rcN and rsN (N e [0,9])
   - A flowtable for each interface pair (rcN, rsN) (N e [0,9])
   - A base chain in forward hook with ten rules adding traffic to
     the respective flowtable.

3) Run iperf3 between client and server ns for a minute

4) While iperf runs, rename rcN -> rc((N+1)%10) (same for rsN) in a busy
   loop.

I extended my series meanwhile by an extra patch adding notifications
for each hook update and had (a patched) 'nft monitor' running in
parallel.

WDYT, is something still missing I could add to the test? Also, I'm not
sure whether I should add it to netfilter selftests as it doesn't have a
defined failure outcome.

Cheers, Phil

[-- Attachment #2: nft_interface_stress.sh --]
[-- Type: application/x-sh, Size: 1846 bytes --]

Re: [nf-next PATCH 0/5] Dynamic hook interface binding

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> WDYT, is something still missing I could add to the test? Also, I'm not
> sure whether I should add it to netfilter selftests as it doesn't have a
> defined failure outcome.

Isn't the expected outcome "did not crash"?

You could just append a test for /proc/sys/kernel/tainted,
i.e. script ran and no splat was triggered.

As the selftests are run in regular intervals on the netdev
CI the only critical factor is total test run time, but so far
the netfilter tests are not too bad and for the much-slower-debug kernel
the script can detect this and spent fewer cycles.

Re: [nf-next PATCH 0/5] Dynamic hook interface binding

[Phil Sutter]

On Wed, May 15, 2024 at 03:24:10PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > WDYT, is something still missing I could add to the test? Also, I'm not
> > sure whether I should add it to netfilter selftests as it doesn't have a
> > defined failure outcome.
> 
> Isn't the expected outcome "did not crash"?
> 
> You could just append a test for /proc/sys/kernel/tainted,
> i.e. script ran and no splat was triggered.

Fair point, thanks!

> As the selftests are run in regular intervals on the netdev
> CI the only critical factor is total test run time, but so far
> the netfilter tests are not too bad and for the much-slower-debug kernel
> the script can detect this and spent fewer cycles.

My script is bound by the configured iperf3 test time, so in theory it
should take the same time irrespective of system performance.

Cheers, Phil