netfilter: disable parallel use of xtables and nftables nat

PATCH In review Florian Westphal 2 participants 9 messages Dec 8, 2017 – Dec 13, 2017

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Florian Westphal

author maintainer

2017-12-08 16:02 UTC #0 lore raw

cover netfilter: disable parallel use of xtables and nftables nat

The netfilter NAT core cannot deal with both nftables and ip(6)tables nat at the same time.
When both are active, one of the two will not work, depending on the order in which the modules were loaded.

NAT hooks install a NAT null binding in case the iptables/nftables nat chain did not specify a nat mapping.

Null bindings are needed to detect port collsisions between NAT-ed and non-NAT-ed connections.

This causes nftables NAT rules to not work when iptable_nat module is loaded, and vice versa because nat binding has already been attached when the second nat hook is consulted.

The alternative is to allow both at the same time.
This requirs new internal nat hooks to do the null binding, but doesn't handle the more fundamental issue of precedence.

Example:

iptables -t nat -A POSTROUTING -o ppp0 -j MASQUERADE
nft add rule ip nat postrouting oifname ppp0 ip protocol udp dport 53 snat to 10.1.1.1

should nft override iptables?
What if one is restored before the other (this seems to be the worst part as it could cause random behaviour with parallel init schemes ...)

So, better explicitly disallow this for now.

Florian Westphal

author maintainer

2017-12-08 16:02 UTC #1 lore raw

PATCH 1/3 netfilter: xtables: add and use xt_request_find_table_lock

currently we always return -ENOENT to userspace if we can't find a particular table, or if the table initialization fails.

Followup patch will make nat table init fail in case nftables already registered a nat hook so this change makes xt_find_table_lock return an ERR_PTR to return the errno value reported from the table init function.

Add xt_request_find_table_lock as try_then_request_module replacement and use it where needed.

Signed-off-by Florian Westphal <[email protected]>

---
 include/linux/netfilter/x_tables.h |  2 ++
 net/ipv4/netfilter/arp_tables.c    | 26 ++++++++++++--------------
 net/ipv4/netfilter/ip_tables.c     | 26 ++++++++++++--------------
 net/ipv6/netfilter/ip6_tables.c    | 26 ++++++++++++--------------
 net/netfilter/x_tables.c           | 37 +++++++++++++++++++++++++++----------
 5 files changed, 65 insertions(+), 52 deletions(-)

include/linux/netfilter/x_tables.h +2 -0

index 33f7530f96b9..1313b35c3ab7 100644
@@ -320,6 +320,8 @@ int xt_find_revision(u8 af, const char *name, u8 revision, int target,
 
 struct xt_table *xt_find_table_lock(struct net *net, u_int8_t af,
 				    const char *name);
+struct xt_table *xt_request_find_table_lock(struct net *net, u_int8_t af,
+					    const char *name);
 void xt_table_unlock(struct xt_table *t);
 
 int xt_proto_init(struct net *net, u_int8_t af);

net/ipv4/netfilter/arp_tables.c +12 ~~-14~~

index f88221aebc9d..8cfe3d37cbb1 100644
@@ -811,9 +811,8 @@ static int get_info(struct net *net, void __user *user,
 	if (compat)
 		xt_compat_lock(NFPROTO_ARP);
 #endif
-	t = try_then_request_module(xt_find_table_lock(net, NFPROTO_ARP, name),
-				    "arptable_%s", name);
-	if (t) {
+	t = xt_request_find_table_lock(net, NFPROTO_ARP, name);
+	if (!IS_ERR(t)) {
 		struct arpt_getinfo info;
 		const struct xt_table_info *private = t->private;
 #ifdef CONFIG_COMPAT

@@ -842,7 +841,7 @@ static int get_info(struct net *net, void __user *user,
 		xt_table_unlock(t);
 		module_put(t->me);
 	} else
-		ret = -ENOENT;
+		ret = PTR_ERR(t);
 #ifdef CONFIG_COMPAT
 	if (compat)
 		xt_compat_unlock(NFPROTO_ARP);

@@ -867,7 +866,7 @@ static int get_entries(struct net *net, struct arpt_get_entries __user *uptr,
 	get.name[sizeof(get.name) - 1] = '\0';
 
 	t = xt_find_table_lock(net, NFPROTO_ARP, get.name);
-	if (t) {
+	if (!IS_ERR(t)) {
 		const struct xt_table_info *private = t->private;
 
 		if (get.size == private->size)

@@ -879,7 +878,7 @@ static int get_entries(struct net *net, struct arpt_get_entries __user *uptr,
 		module_put(t->me);
 		xt_table_unlock(t);
 	} else
-		ret = -ENOENT;
+		ret = PTR_ERR(t);
 
 	return ret;
 }

@@ -904,10 +903,9 @@ static int __do_replace(struct net *net, const char *name,
 		goto out;
 	}
 
-	t = try_then_request_module(xt_find_table_lock(net, NFPROTO_ARP, name),
-				    "arptable_%s", name);
-	if (!t) {
-		ret = -ENOENT;
+	t = xt_request_find_table_lock(net, NFPROTO_ARP, name);
+	if (IS_ERR(t)) {
+		ret = ERR_PTR(t);
 		goto free_newinfo_counters_untrans;
 	}

@@ -1021,8 +1019,8 @@ static int do_add_counters(struct net *net, const void __user *user,
 		return PTR_ERR(paddc);
 
 	t = xt_find_table_lock(net, NFPROTO_ARP, tmp.name);
-	if (!t) {
-		ret = -ENOENT;
+	if (IS_ERR(t)) {
+		ret = ERR_PTR(t);
 		goto free;
 	}

@@ -1409,7 +1407,7 @@ static int compat_get_entries(struct net *net,
 
 	xt_compat_lock(NFPROTO_ARP);
 	t = xt_find_table_lock(net, NFPROTO_ARP, get.name);
-	if (t) {
+	if (!IS_ERR(t)) {
 		const struct xt_table_info *private = t->private;
 		struct xt_table_info info;

@@ -1424,7 +1422,7 @@ static int compat_get_entries(struct net *net,
 		module_put(t->me);
 		xt_table_unlock(t);
 	} else
-		ret = -ENOENT;
+		ret = PTR_ERR(t);
 
 	xt_compat_unlock(NFPROTO_ARP);
 	return ret;

net/ipv4/netfilter/ip_tables.c +12 ~~-14~~

index 4cbe5e80f3bf..38e3a19dbac3 100644
@@ -974,9 +974,8 @@ static int get_info(struct net *net, void __user *user,
 	if (compat)
 		xt_compat_lock(AF_INET);
 #endif
-	t = try_then_request_module(xt_find_table_lock(net, AF_INET, name),
-				    "iptable_%s", name);
-	if (t) {
+	t = xt_request_find_table_lock(net, AF_INET, name);
+	if (!IS_ERR(t)) {
 		struct ipt_getinfo info;
 		const struct xt_table_info *private = t->private;
 #ifdef CONFIG_COMPAT

@@ -1006,7 +1005,7 @@ static int get_info(struct net *net, void __user *user,
 		xt_table_unlock(t);
 		module_put(t->me);
 	} else
-		ret = -ENOENT;
+		ret = PTR_ERR(t);
 #ifdef CONFIG_COMPAT
 	if (compat)
 		xt_compat_unlock(AF_INET);

@@ -1031,7 +1030,7 @@ get_entries(struct net *net, struct ipt_get_entries __user *uptr,
 	get.name[sizeof(get.name) - 1] = '\0';
 
 	t = xt_find_table_lock(net, AF_INET, get.name);
-	if (t) {
+	if (!IS_ERR(t)) {
 		const struct xt_table_info *private = t->private;
 		if (get.size == private->size)
 			ret = copy_entries_to_user(private->size,

@@ -1042,7 +1041,7 @@ get_entries(struct net *net, struct ipt_get_entries __user *uptr,
 		module_put(t->me);
 		xt_table_unlock(t);
 	} else
-		ret = -ENOENT;
+		ret = PTR_ERR(t);
 
 	return ret;
 }

@@ -1065,10 +1064,9 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 		goto out;
 	}
 
-	t = try_then_request_module(xt_find_table_lock(net, AF_INET, name),
-				    "iptable_%s", name);
-	if (!t) {
-		ret = -ENOENT;
+	t = xt_request_find_table_lock(net, AF_INET, name);
+	if (IS_ERR(t)) {
+		ret = PTR_ERR(t);
 		goto free_newinfo_counters_untrans;
 	}

@@ -1182,8 +1180,8 @@ do_add_counters(struct net *net, const void __user *user,
 		return PTR_ERR(paddc);
 
 	t = xt_find_table_lock(net, AF_INET, tmp.name);
-	if (!t) {
-		ret = -ENOENT;
+	if (IS_ERR(t)) {
+		ret = PTR_ERR(t);
 		goto free;
 	}

@@ -1626,7 +1624,7 @@ compat_get_entries(struct net *net, struct compat_ipt_get_entries __user *uptr,
 
 	xt_compat_lock(AF_INET);
 	t = xt_find_table_lock(net, AF_INET, get.name);
-	if (t) {
+	if (!IS_ERR(t)) {
 		const struct xt_table_info *private = t->private;
 		struct xt_table_info info;
 		ret = compat_table_info(private, &info);

@@ -1640,7 +1638,7 @@ compat_get_entries(struct net *net, struct compat_ipt_get_entries __user *uptr,
 		module_put(t->me);
 		xt_table_unlock(t);
 	} else
-		ret = -ENOENT;
+		ret = PTR_ERR(t);
 
 	xt_compat_unlock(AF_INET);
 	return ret;

net/ipv6/netfilter/ip6_tables.c +12 ~~-14~~

index f06e25065a34..fb7bb55ab788 100644
@@ -992,9 +992,8 @@ static int get_info(struct net *net, void __user *user,
 	if (compat)
 		xt_compat_lock(AF_INET6);
 #endif
-	t = try_then_request_module(xt_find_table_lock(net, AF_INET6, name),
-				    "ip6table_%s", name);
-	if (t) {
+	t = xt_request_find_table_lock(net, AF_INET6, name);
+	if (!IS_ERR(t)) {
 		struct ip6t_getinfo info;
 		const struct xt_table_info *private = t->private;
 #ifdef CONFIG_COMPAT

@@ -1024,7 +1023,7 @@ static int get_info(struct net *net, void __user *user,
 		xt_table_unlock(t);
 		module_put(t->me);
 	} else
-		ret = -ENOENT;
+		ret = PTR_ERR(t);
 #ifdef CONFIG_COMPAT
 	if (compat)
 		xt_compat_unlock(AF_INET6);

@@ -1050,7 +1049,7 @@ get_entries(struct net *net, struct ip6t_get_entries __user *uptr,
 	get.name[sizeof(get.name) - 1] = '\0';
 
 	t = xt_find_table_lock(net, AF_INET6, get.name);
-	if (t) {
+	if (!IS_ERR(t)) {
 		struct xt_table_info *private = t->private;
 		if (get.size == private->size)
 			ret = copy_entries_to_user(private->size,

@@ -1061,7 +1060,7 @@ get_entries(struct net *net, struct ip6t_get_entries __user *uptr,
 		module_put(t->me);
 		xt_table_unlock(t);
 	} else
-		ret = -ENOENT;
+		ret = PTR_ERR(t);
 
 	return ret;
 }

@@ -1084,10 +1083,9 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 		goto out;
 	}
 
-	t = try_then_request_module(xt_find_table_lock(net, AF_INET6, name),
-				    "ip6table_%s", name);
-	if (!t) {
-		ret = -ENOENT;
+	t = xt_request_find_table_lock(net, AF_INET6, name);
+	if (IS_ERR(t)) {
+		ret = PTR_ERR(t);
 		goto free_newinfo_counters_untrans;
 	}

@@ -1200,8 +1198,8 @@ do_add_counters(struct net *net, const void __user *user, unsigned int len,
 	if (IS_ERR(paddc))
 		return PTR_ERR(paddc);
 	t = xt_find_table_lock(net, AF_INET6, tmp.name);
-	if (!t) {
-		ret = -ENOENT;
+	if (IS_ERR(t)) {
+		ret = PTR_ERR(t);
 		goto free;
 	}

@@ -1637,7 +1635,7 @@ compat_get_entries(struct net *net, struct compat_ip6t_get_entries __user *uptr,
 
 	xt_compat_lock(AF_INET6);
 	t = xt_find_table_lock(net, AF_INET6, get.name);
-	if (t) {
+	if (!IS_ERR(t)) {
 		const struct xt_table_info *private = t->private;
 		struct xt_table_info info;
 		ret = compat_table_info(private, &info);

@@ -1651,7 +1649,7 @@ compat_get_entries(struct net *net, struct compat_ip6t_get_entries __user *uptr,
 		module_put(t->me);
 		xt_table_unlock(t);
 	} else
-		ret = -ENOENT;
+		ret = PTR_ERR(t);
 
 	xt_compat_unlock(AF_INET6);
 	return ret;

net/netfilter/x_tables.c +27 ~~-10~~

index 7c1414e89f4d..40360bae7792 100644
@@ -1027,7 +1027,7 @@ void xt_free_table_info(struct xt_table_info *info)
 }
 EXPORT_SYMBOL(xt_free_table_info);
 
-/* Find table by name, grabs mutex & ref.  Returns NULL on error. */
+/* Find table by name, grabs mutex & ref.  Returns ERR_PTR on error. */
 struct xt_table *xt_find_table_lock(struct net *net, u_int8_t af,
 				    const char *name)
 {

@@ -1043,17 +1043,17 @@ struct xt_table *xt_find_table_lock(struct net *net, u_int8_t af,
 
 	/* Table doesn't exist in this netns, re-try init */
 	list_for_each_entry(t, &init_net.xt.tables[af], list) {
+		int err;
+
 		if (strcmp(t->name, name))
 			continue;
-		if (!try_module_get(t->me)) {
-			mutex_unlock(&xt[af].mutex);
-			return NULL;
-		}
-
+		if (!try_module_get(t->me))
+			goto out;
 		mutex_unlock(&xt[af].mutex);
-		if (t->table_init(net) != 0) {
+		err = t->table_init(net);
+		if (err < 0) {
 			module_put(t->me);
-			return NULL;
+			return ERR_PTR(err);
 		}
 
 		found = t;

@@ -1073,9 +1073,26 @@ struct xt_table *xt_find_table_lock(struct net *net, u_int8_t af,
 	module_put(found->me);
  out:
 	mutex_unlock(&xt[af].mutex);
-	return NULL;
+	return ERR_PTR(-ENOENT);
+}
+
+struct xt_table *xt_request_find_table_lock(struct net *net, u_int8_t af,
+					    const char *name)
+{
+	struct xt_table *t = xt_find_table_lock(net, af, name);
+
+#ifdef CONFIG_MODULE
+	if (IS_ERR(t)) {
+		int err = request_module("%stable_%s", xt_prefix[af], name);
+		if (err)
+			return ERR_PTR(err);
+		t = xt_find_table_lock(net, af, name);
+	}
+#endif
+
+	return t;
 }
-EXPORT_SYMBOL_GPL(xt_find_table_lock);
+EXPORT_SYMBOL_GPL(xt_request_find_table_lock);
 
 void xt_table_unlock(struct xt_table *table)
 {

Signature · 1 line

2.13.6

Pablo Neira Ayuso

maintainer

2017-12-09 15:23 UTC #2 lore raw

Re: [PATCH nf-next 1/3] netfilter: xtables: add and use xt_request_find_table_lock

Quoted 77 lines · Florian Westphal

currently we always return -ENOENT to userspace if we can't find a particular table, or if the table initialization fails.

Add xt_request_find_table_lock as try_then_request_module replacement and use it where needed.

Signed-off-by Florian Westphal <[email protected]>

---
 include/linux/netfilter/x_tables.h |  2 ++
 net/ipv4/netfilter/arp_tables.c    | 26 ++++++++++++--------------
 net/ipv4/netfilter/ip_tables.c     | 26 ++++++++++++--------------
 net/ipv6/netfilter/ip6_tables.c    | 26 ++++++++++++--------------
 net/netfilter/x_tables.c           | 37 +++++++++++++++++++++++++++----------
 5 files changed, 65 insertions(+), 52 deletions(-)

include/linux/netfilter/x_tables.h +2 -0

index 33f7530f96b9..1313b35c3ab7 100644
@@ -320,6 +320,8 @@ int xt_find_revision(u8 af, const char *name, u8 revision, int target,
 
 struct xt_table *xt_find_table_lock(struct net *net, u_int8_t af,
 				    const char *name);
+struct xt_table *xt_request_find_table_lock(struct net *net, u_int8_t af,
+					    const char *name);
 void xt_table_unlock(struct xt_table *t);
 
 int xt_proto_init(struct net *net, u_int8_t af);

net/ipv4/netfilter/arp_tables.c +5 -6

index f88221aebc9d..8cfe3d37cbb1 100644
@@ -811,9 +811,8 @@ static int get_info(struct net *net, void __user *user,
 	if (compat)
 		xt_compat_lock(NFPROTO_ARP);
 #endif
-	t = try_then_request_module(xt_find_table_lock(net, NFPROTO_ARP, name),
-				    "arptable_%s", name);
-	if (t) {
+	t = xt_request_find_table_lock(net, NFPROTO_ARP, name);
+	if (!IS_ERR(t)) {
 		struct arpt_getinfo info;
 		const struct xt_table_info *private = t->private;
 #ifdef CONFIG_COMPAT

@@ -842,7 +841,7 @@ static int get_info(struct net *net, void __user *user,
 		xt_table_unlock(t);
 		module_put(t->me);
 	} else
-		ret = -ENOENT;
+		ret = PTR_ERR(t);
 #ifdef CONFIG_COMPAT
 	if (compat)
 		xt_compat_unlock(NFPROTO_ARP);

@@ -867,7 +866,7 @@ static int get_entries(struct net *net, struct arpt_get_entries __user *uptr,
 	get.name[sizeof(get.name) - 1] = '\0';
 
 	t = xt_find_table_lock(net, NFPROTO_ARP, get.name);
-	if (t) {
+	if (!IS_ERR(t)) {
 		const struct xt_table_info *private = t->private;
 
 		if (get.size == private->size)

@@ -879,7 +878,7 @@ static int get_entries(struct net *net, struct arpt_get_entries __user *uptr,
 		module_put(t->me);
 		xt_table_unlock(t);
 	} else
-		ret = -ENOENT;
+		ret = PTR_ERR(t);
 
 	return ret;
 }

net/ipv4/netfilter/arp_tables.c static int __do_replace(struct net *net, const char *name, · line 908

Quoted 10 lines

net/ipv4/netfilter/arp_tables.c +3 -4

@@ -904,10 +903,9 @@ static int __do_replace(struct net *net, const char *name,
 		goto out;
 	}
 
-	t = try_then_request_module(xt_find_table_lock(net, NFPROTO_ARP, name),
-				    "arptable_%s", name);
-	if (!t) {
-		ret = -ENOENT;
+	t = xt_request_find_table_lock(net, NFPROTO_ARP, name);
+	if (IS_ERR(t)) {
+		ret = ERR_PTR(t);

net/ipv4/netfilter/arp_tables.c:908:17: warning: passing argument 1 of ‘ERR_PTR’ makes integer from pointer without a cast net/ipv4/netfilter/arp_tables.c:908:7: warning: assignment makes integer from pointer without a cast net/ipv4/netfilter/arp_tables.c:1023:17: warning: passing argument 1 of ‘ERR_PTR’ makes integer from pointer without a cast net/ipv4/netfilter/arp_tables.c:1023:7: warning: assignment makes integer from pointer without a cast

Will fix this here, I need to invert ERR_PTR() to PTR_ERR().

Florian Westphal

author maintainer

2017-12-08 16:02 UTC #3 lore raw

PATCH 2/3 netfilter: core: only allow one nat hook per hook point

In reply to Florian Westphal

The netfilter NAT core cannot deal with more than one NAT hook per hook location (prerouting, input ...), because the NAT hooks install a NAT null binding in case the iptables nat table (iptable_nat hooks) or the corresponding nftables chain (nft nat hooks) doesn't specify a nat transformation.

Null bindings are needed to detect port collsisions between NAT-ed and non-NAT-ed connections.

This causes nftables NAT rules to not work when iptable_nat module is loaded, and vice versa because nat binding has already been attached when the second nat hook is consulted.

The netfilter core is not really the correct location to handle this (hooks are just hooks, the core has no notion of what kinds of side
effects a hook implements), but its the only place where we can check
for conflicts between both iptables hooks and nftables hooks without adding dependencies.

So add nat annotation to hook_ops to describe those hooks that will add NAT bindings and then make core reject if such a hook already exists. The annotation fills a padding hole, in case further restrictions appar we might change this to a 'u8 type' instead of bool.

iptables error if nft nat hook active:
iptables -t nat -A POSTROUTING -j MASQUERADE
iptables v1.4.21: can't initialize iptables table `nat': File exists Perhaps iptables or your kernel needs to be upgraded.

nftables error if iptables nat table present:
nft -f /etc/nftables/ipv4-nat
/usr/etc/nftables/ipv4-nat:3:1-2: Error: Could not process rule: File exists table nat {
^^

Signed-off-by Florian Westphal <[email protected]>

---
 include/linux/netfilter.h         | 1 +
 net/ipv4/netfilter/iptable_nat.c  | 4 ++++
 net/ipv6/netfilter/ip6table_nat.c | 4 ++++
 net/netfilter/core.c              | 6 ++++++
 net/netfilter/nf_tables_api.c     | 2 ++
 5 files changed, 17 insertions(+)

include/linux/netfilter.h +1 -0

index 9dcbcdfa3b82..579b1ba86ee6 100644
@@ -67,6 +67,7 @@ struct nf_hook_ops {
 	struct net_device	*dev;
 	void			*priv;
 	u_int8_t		pf;
+	bool			nat_hook;
 	unsigned int		hooknum;
 	/* Hooks are ordered in ascending priority. */
 	int			priority;

net/ipv4/netfilter/iptable_nat.c +4 -0

index a1a07b338ccf..0f7255cc65ee 100644
@@ -72,6 +72,7 @@ static const struct nf_hook_ops nf_nat_ipv4_ops[] = {
 	{
 		.hook		= iptable_nat_ipv4_in,
 		.pf		= NFPROTO_IPV4,
+		.nat_hook	= true,
 		.hooknum	= NF_INET_PRE_ROUTING,
 		.priority	= NF_IP_PRI_NAT_DST,
 	},

@@ -79,6 +80,7 @@ static const struct nf_hook_ops nf_nat_ipv4_ops[] = {
 	{
 		.hook		= iptable_nat_ipv4_out,
 		.pf		= NFPROTO_IPV4,
+		.nat_hook	= true,
 		.hooknum	= NF_INET_POST_ROUTING,
 		.priority	= NF_IP_PRI_NAT_SRC,
 	},

@@ -86,6 +88,7 @@ static const struct nf_hook_ops nf_nat_ipv4_ops[] = {
 	{
 		.hook		= iptable_nat_ipv4_local_fn,
 		.pf		= NFPROTO_IPV4,
+		.nat_hook	= true,
 		.hooknum	= NF_INET_LOCAL_OUT,
 		.priority	= NF_IP_PRI_NAT_DST,
 	},

@@ -93,6 +96,7 @@ static const struct nf_hook_ops nf_nat_ipv4_ops[] = {
 	{
 		.hook		= iptable_nat_ipv4_fn,
 		.pf		= NFPROTO_IPV4,
+		.nat_hook	= true,
 		.hooknum	= NF_INET_LOCAL_IN,
 		.priority	= NF_IP_PRI_NAT_SRC,
 	},

net/ipv6/netfilter/ip6table_nat.c +4 -0

index 991512576c8c..47306e45a80a 100644
@@ -74,6 +74,7 @@ static const struct nf_hook_ops nf_nat_ipv6_ops[] = {
 	{
 		.hook		= ip6table_nat_in,
 		.pf		= NFPROTO_IPV6,
+		.nat_hook	= true,
 		.hooknum	= NF_INET_PRE_ROUTING,
 		.priority	= NF_IP6_PRI_NAT_DST,
 	},

@@ -81,6 +82,7 @@ static const struct nf_hook_ops nf_nat_ipv6_ops[] = {
 	{
 		.hook		= ip6table_nat_out,
 		.pf		= NFPROTO_IPV6,
+		.nat_hook	= true,
 		.hooknum	= NF_INET_POST_ROUTING,
 		.priority	= NF_IP6_PRI_NAT_SRC,
 	},

@@ -88,12 +90,14 @@ static const struct nf_hook_ops nf_nat_ipv6_ops[] = {
 	{
 		.hook		= ip6table_nat_local_fn,
 		.pf		= NFPROTO_IPV6,
+		.nat_hook	= true,
 		.hooknum	= NF_INET_LOCAL_OUT,
 		.priority	= NF_IP6_PRI_NAT_DST,
 	},
 	/* After packet filtering, change source */
 	{
 		.hook		= ip6table_nat_fn,
+		.nat_hook	= true,
 		.pf		= NFPROTO_IPV6,
 		.hooknum	= NF_INET_LOCAL_IN,
 		.priority	= NF_IP6_PRI_NAT_SRC,

net/netfilter/core.c +6 -0

index a6eaaf303be8..6c263efed2b6 100644
@@ -160,6 +160,12 @@ nf_hook_entries_grow(const struct nf_hook_entries *old,
 			++i;
 			continue;
 		}
+
+		if (reg->nat_hook && orig_ops[i]->nat_hook) {
+			kvfree(new);
+			return ERR_PTR(-EEXIST);
+		}
+
 		if (inserted || reg->priority > orig_ops[i]->priority) {
 			new_ops[nhooks] = (void *)orig_ops[i];
 			new->hooks[nhooks] = old->hooks[i];

net/netfilter/nf_tables_api.c +2 -0

index d8327b43e4dc..f000d4399c7a 100644
@@ -1400,6 +1400,8 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
 				ops->hook = hookfn;
 			if (afi->hook_ops_init)
 				afi->hook_ops_init(ops, i);
+			if (basechain->type->type == NFT_CHAIN_T_NAT)
+				ops->nat_hook = true;
 		}
 
 		chain->flags |= NFT_BASE_CHAIN;

Signature · 1 line

2.13.6

Pablo Neira Ayuso

maintainer

2017-12-08 21:28 UTC #4 lore raw

Re: [PATCH nf-next 2/3] netfilter: core: only allow one nat hook per hook point

Quoted 29 lines · Florian Westphal

The netfilter NAT core cannot deal with more than one NAT hook per hook location (prerouting, input ...), because the NAT hooks install a NAT null binding in case the iptables nat table (iptable_nat hooks) or the corresponding nftables chain (nft nat hooks) doesn't specify a nat transformation.
Null bindings are needed to detect port collsisions between NAT-ed and non-NAT-ed connections.
This causes nftables NAT rules to not work when iptable_nat module is loaded, and vice versa because nat binding has already been attached when the second nat hook is consulted.
The netfilter core is not really the correct location to handle this (hooks are just hooks, the core has no notion of what kinds of side
effects a hook implements), but its the only place where we can check
for conflicts between both iptables hooks and nftables hooks without adding dependencies.
So add nat annotation to hook_ops to describe those hooks that will add NAT bindings and then make core reject if such a hook already exists. The annotation fills a padding hole, in case further restrictions appar we might change this to a 'u8 type' instead of bool.
iptables error if nft nat hook active:
iptables -t nat -A POSTROUTING -j MASQUERADE
iptables v1.4.21: can't initialize iptables table `nat': File exists Perhaps iptables or your kernel needs to be upgraded.

Quoted 5 lines

nftables error if iptables nat table present:
nft -f /etc/nftables/ipv4-nat
/usr/etc/nftables/ipv4-nat:3:1-2: Error: Could not process rule: File exists table nat {
^^

This reminds me we have to fix error reporting on chains, it seems location is unset by the time, there's a bugreport on the wiki on this.

Quoted 26 lines

Signed-off-by Florian Westphal <[email protected]>

---
 include/linux/netfilter.h         | 1 +
 net/ipv4/netfilter/iptable_nat.c  | 4 ++++
 net/ipv6/netfilter/ip6table_nat.c | 4 ++++
 net/netfilter/core.c              | 6 ++++++
 net/netfilter/nf_tables_api.c     | 2 ++
 5 files changed, 17 insertions(+)

include/linux/netfilter.h +1 -0

index 9dcbcdfa3b82..579b1ba86ee6 100644
@@ -67,6 +67,7 @@ struct nf_hook_ops {
 	struct net_device	*dev;
 	void			*priv;
 	u_int8_t		pf;
+	bool			nat_hook;
 	unsigned int		hooknum;
 	/* Hooks are ordered in ascending priority. */
 	int			priority;

net/ipv4/netfilter/iptable_nat.c +0 -0

index a1a07b338ccf..0f7255cc65ee 100644

net/ipv4/netfilter/iptable_nat.c static const struct nf_hook_ops nf_nat_ipv4_ops[] = { · line 75

Quoted 4 lines

net/ipv4/netfilter/iptable_nat.c +1 -0

@@ -72,6 +72,7 @@ static const struct nf_hook_ops nf_nat_ipv4_ops[] = {
 	{
 		.hook		= iptable_nat_ipv4_in,
 		.pf		= NFPROTO_IPV4,
+		.nat_hook	= true,

Just a suggestion: This nat_hook basically means that we only allow this hook to be a singleton in this spot. So I would call it like this, ie. singleton, given we have no NAT semantics in the netfilter core.

Quoted 99 lines

net/ipv4/netfilter/iptable_nat.c +3 -0

 		.hooknum	= NF_INET_PRE_ROUTING,
 		.priority	= NF_IP_PRI_NAT_DST,
 	},

@@ -79,6 +80,7 @@ static const struct nf_hook_ops nf_nat_ipv4_ops[] = {
 	{
 		.hook		= iptable_nat_ipv4_out,
 		.pf		= NFPROTO_IPV4,
+		.nat_hook	= true,
 		.hooknum	= NF_INET_POST_ROUTING,
 		.priority	= NF_IP_PRI_NAT_SRC,
 	},

@@ -86,6 +88,7 @@ static const struct nf_hook_ops nf_nat_ipv4_ops[] = {
 	{
 		.hook		= iptable_nat_ipv4_local_fn,
 		.pf		= NFPROTO_IPV4,
+		.nat_hook	= true,
 		.hooknum	= NF_INET_LOCAL_OUT,
 		.priority	= NF_IP_PRI_NAT_DST,
 	},

@@ -93,6 +96,7 @@ static const struct nf_hook_ops nf_nat_ipv4_ops[] = {
 	{
 		.hook		= iptable_nat_ipv4_fn,
 		.pf		= NFPROTO_IPV4,
+		.nat_hook	= true,
 		.hooknum	= NF_INET_LOCAL_IN,
 		.priority	= NF_IP_PRI_NAT_SRC,
 	},

net/ipv6/netfilter/ip6table_nat.c +4 -0

index 991512576c8c..47306e45a80a 100644
@@ -74,6 +74,7 @@ static const struct nf_hook_ops nf_nat_ipv6_ops[] = {
 	{
 		.hook		= ip6table_nat_in,
 		.pf		= NFPROTO_IPV6,
+		.nat_hook	= true,
 		.hooknum	= NF_INET_PRE_ROUTING,
 		.priority	= NF_IP6_PRI_NAT_DST,
 	},

@@ -81,6 +82,7 @@ static const struct nf_hook_ops nf_nat_ipv6_ops[] = {
 	{
 		.hook		= ip6table_nat_out,
 		.pf		= NFPROTO_IPV6,
+		.nat_hook	= true,
 		.hooknum	= NF_INET_POST_ROUTING,
 		.priority	= NF_IP6_PRI_NAT_SRC,
 	},

@@ -88,12 +90,14 @@ static const struct nf_hook_ops nf_nat_ipv6_ops[] = {
 	{
 		.hook		= ip6table_nat_local_fn,
 		.pf		= NFPROTO_IPV6,
+		.nat_hook	= true,
 		.hooknum	= NF_INET_LOCAL_OUT,
 		.priority	= NF_IP6_PRI_NAT_DST,
 	},
 	/* After packet filtering, change source */
 	{
 		.hook		= ip6table_nat_fn,
+		.nat_hook	= true,
 		.pf		= NFPROTO_IPV6,
 		.hooknum	= NF_INET_LOCAL_IN,
 		.priority	= NF_IP6_PRI_NAT_SRC,

net/netfilter/core.c +6 -0

index a6eaaf303be8..6c263efed2b6 100644
@@ -160,6 +160,12 @@ nf_hook_entries_grow(const struct nf_hook_entries *old,
 			++i;
 			continue;
 		}
+
+		if (reg->nat_hook && orig_ops[i]->nat_hook) {
+			kvfree(new);
+			return ERR_PTR(-EEXIST);
+		}
+
 		if (inserted || reg->priority > orig_ops[i]->priority) {
 			new_ops[nhooks] = (void *)orig_ops[i];
 			new->hooks[nhooks] = old->hooks[i];

net/netfilter/nf_tables_api.c +2 -0

index d8327b43e4dc..f000d4399c7a 100644
@@ -1400,6 +1400,8 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
 				ops->hook = hookfn;
 			if (afi->hook_ops_init)
 				afi->hook_ops_init(ops, i);
+			if (basechain->type->type == NFT_CHAIN_T_NAT)
+				ops->nat_hook = true;
 		}
 
 		chain->flags |= NFT_BASE_CHAIN;

Signature · 6 lines

2.13.6

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

maintainer

2017-12-08 21:33 UTC #5 lore raw

Re: [PATCH nf-next 2/3] netfilter: core: only allow one nat hook per hook point

Quoted 1 line · Pablo Neira Ayuso

On Fri, Dec 08, 2017 at 05:01:54PM +0100, Florian Westphal wrote:

[...]

Quoted 6 lines

net/ipv4/netfilter/iptable_nat.c +0 -0

index a1a07b338ccf..0f7255cc65ee 100644
@@ -72,6 +72,7 @@ static const struct nf_hook_ops nf_nat_ipv4_ops[] = {
 	{

Quoted 8 lines

net/ipv4/netfilter/iptable_nat.c static const struct nf_hook_ops nf_nat_ipv4_ops[] = { · line 75
Quoted 3 lines
net/ipv4/netfilter/iptable_nat.c +1 -0
 		.hook		= iptable_nat_ipv4_in,
 		.pf		= NFPROTO_IPV4,
+		.nat_hook	= true,
Just a suggestion: This nat_hook basically means that we only allow this hook to be a singleton in this spot. So I would call it like this, ie. singleton, given we have no NAT semantics in the netfilter core.

Hm, never mind about names.

Florian Westphal

author maintainer

2017-12-08 16:02 UTC #6 lore raw

PATCH 3/3 nftables: reject nat hook registration if prio is before conntrack

In reply to Florian Westphal

No problem for iptables as priorities are fixed values defined in the nat modules, but in nftables the priority its coming from userspace.

Reject in case we see that such a hook would not work.

Signed-off-by Florian Westphal <[email protected]>

---
 net/netfilter/nf_tables_api.c | 5 +++++
 1 file changed, 5 insertions(+)

net/netfilter/nf_tables_api.c +5 -0

index f000d4399c7a..4ed66f1b40b5 100644
@@ -1303,6 +1303,11 @@ static int nft_chain_parse_hook(struct net *net,
 	}
 	if (!(type->hook_mask & (1 << hook->num)))
 		return -EOPNOTSUPP;
+
+	if (type->type == NFT_CHAIN_T_NAT &&
+	    hook->priority <= NF_IP_PRI_CONNTRACK)
+		return -EINVAL;
+
 	if (!try_module_get(type->owner))
 		return -ENOENT;

Signature · 1 line

2.13.6

Pablo Neira Ayuso

maintainer

2017-12-08 21:23 UTC #7 lore raw

Re: [PATCH nf-next 3/3] nftables: reject nat hook registration if prio is before conntrack

Quoted 15 lines · Florian Westphal

No problem for iptables as priorities are fixed values defined in the nat modules, but in nftables the priority its coming from userspace.
Reject in case we see that such a hook would not work.
Signed-off-by Florian Westphal <[email protected]>
---
 net/netfilter/nf_tables_api.c | 5 +++++
 1 file changed, 5 insertions(+)
net/netfilter/nf_tables_api.c +0 -0
index f000d4399c7a..4ed66f1b40b5 100644

net/netfilter/nf_tables_api.c static int nft_chain_parse_hook(struct net *net, · line 1309

Quoted 7 lines

net/netfilter/nf_tables_api.c +4 -0

@@ -1303,6 +1303,11 @@ static int nft_chain_parse_hook(struct net *net,
 	}
 	if (!(type->hook_mask & (1 << hook->num)))
 		return -EOPNOTSUPP;
+
+	if (type->type == NFT_CHAIN_T_NAT &&
+	    hook->priority <= NF_IP_PRI_CONNTRACK)
+		return -EINVAL;

EINVAL is usually for missing netlink attributes, so I'd go for EOPNOTSUPP instead. No need to resend I can mangle this here if you prefer.

Quoted 11 lines

net/netfilter/nf_tables_api.c +1 -0

+
 	if (!try_module_get(type->owner))
 		return -ENOENT;

Signature · 6 lines

2.13.6

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

maintainer

2017-12-13 16:28 UTC #8 lore raw

Re: [PATCH nf-next 3/3] nftables: reject nat hook registration if prio is before conntrack

Quoted 11 lines · Pablo Neira Ayuso

Quoted 10 lines · Florian Westphal
No problem for iptables as priorities are fixed values defined in the nat modules, but in nftables the priority its coming from userspace.
Reject in case we see that such a hook would not work.
Signed-off-by Florian Westphal <[email protected]>
---
 net/netfilter/nf_tables_api.c | 5 +++++
 1 file changed, 5 insertions(+)

Quoted 16 lines

net/netfilter/nf_tables_api.c static int nft_chain_parse_hook(struct net *net, · line 1309
Quoted 12 lines
net/netfilter/nf_tables_api.c +4 -0
index f000d4399c7a..4ed66f1b40b5 100644
@@ -1303,6 +1303,11 @@ static int nft_chain_parse_hook(struct net *net,
 	}
 	if (!(type->hook_mask & (1 << hook->num)))
 		return -EOPNOTSUPP;
+
+	if (type->type == NFT_CHAIN_T_NAT &&
+	    hook->priority <= NF_IP_PRI_CONNTRACK)
+		return -EINVAL;
EINVAL is usually for missing netlink attributes, so I'd go for EOPNOTSUPP instead. No need to resend I can mangle this here if you prefer.

This patch also needs this, otherwise
tests/shell/chains/0006masquerade_0 breaks.

net/netfilter/nf_tables_api.c +0 -0

index 1cc1faefed69..2a29a5a58913 100644
@@ -1259,7 +1259,7 @@ static void nf_tables_chain_destroy(struct

nft_chain *chain)

 struct nft_chain_hook {
        u32                             num;
-       u32                             priority;
+       s32                             priority;
        const struct nf_chain_type      *type;
        struct net_device               *dev;

};

I didn't push out this yet, so I'm going to collapse this to your original patch.

Thanks.