[PATCH nf-next 0/3] netfilter: disable parallel use of xtables and nftables nat

[PATCH nf-next 0/3] netfilter: disable parallel use of xtables and nftables nat

[Florian Westphal]

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.

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

[Florian Westphal]

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 <fw@strlen.de>
---
 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(-)

diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index 33f7530f96b9..1313b35c3ab7 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -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);
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index f88221aebc9d..8cfe3d37cbb1 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -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;
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 4cbe5e80f3bf..38e3a19dbac3 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -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;
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index f06e25065a34..fb7bb55ab788 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -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;
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 7c1414e89f4d..40360bae7792 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -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)
 {
-- 
2.13.6

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

[Pablo Neira Ayuso]

On Fri, Dec 08, 2017 at 05:01:53PM +0100, Florian Westphal wrote:
> 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 <fw@strlen.de>
> ---
>  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(-)
> 
> diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
> index 33f7530f96b9..1313b35c3ab7 100644
> --- a/include/linux/netfilter/x_tables.h
> +++ b/include/linux/netfilter/x_tables.h
> @@ -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);
> diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> index f88221aebc9d..8cfe3d37cbb1 100644
> --- a/net/ipv4/netfilter/arp_tables.c
> +++ b/net/ipv4/netfilter/arp_tables.c
> @@ -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);

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().

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

[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 <fw@strlen.de>
---
 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(+)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 9dcbcdfa3b82..579b1ba86ee6 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -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;
diff --git a/net/ipv4/netfilter/iptable_nat.c b/net/ipv4/netfilter/iptable_nat.c
index a1a07b338ccf..0f7255cc65ee 100644
--- a/net/ipv4/netfilter/iptable_nat.c
+++ b/net/ipv4/netfilter/iptable_nat.c
@@ -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,
 	},
diff --git a/net/ipv6/netfilter/ip6table_nat.c b/net/ipv6/netfilter/ip6table_nat.c
index 991512576c8c..47306e45a80a 100644
--- a/net/ipv6/netfilter/ip6table_nat.c
+++ b/net/ipv6/netfilter/ip6table_nat.c
@@ -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,
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index a6eaaf303be8..6c263efed2b6 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -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];
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index d8327b43e4dc..f000d4399c7a 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -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;
-- 
2.13.6

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

[Pablo Neira Ayuso]

On Fri, Dec 08, 2017 at 05:01:54PM +0100, Florian Westphal wrote:
> 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 {
> ^^

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.

> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  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(+)
> 
> diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
> index 9dcbcdfa3b82..579b1ba86ee6 100644
> --- a/include/linux/netfilter.h
> +++ b/include/linux/netfilter.h
> @@ -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;
> diff --git a/net/ipv4/netfilter/iptable_nat.c b/net/ipv4/netfilter/iptable_nat.c
> index a1a07b338ccf..0f7255cc65ee 100644
> --- a/net/ipv4/netfilter/iptable_nat.c
> +++ b/net/ipv4/netfilter/iptable_nat.c
> @@ -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.

>  		.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,
>  	},
> diff --git a/net/ipv6/netfilter/ip6table_nat.c b/net/ipv6/netfilter/ip6table_nat.c
> index 991512576c8c..47306e45a80a 100644
> --- a/net/ipv6/netfilter/ip6table_nat.c
> +++ b/net/ipv6/netfilter/ip6table_nat.c
> @@ -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,
> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> index a6eaaf303be8..6c263efed2b6 100644
> --- a/net/netfilter/core.c
> +++ b/net/netfilter/core.c
> @@ -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];
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index d8327b43e4dc..f000d4399c7a 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -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;
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pablo Neira Ayuso]

On Fri, Dec 08, 2017 at 10:28:28PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Dec 08, 2017 at 05:01:54PM +0100, Florian Westphal wrote:
[...]
> > diff --git a/net/ipv4/netfilter/iptable_nat.c b/net/ipv4/netfilter/iptable_nat.c
> > index a1a07b338ccf..0f7255cc65ee 100644
> > --- a/net/ipv4/netfilter/iptable_nat.c
> > +++ b/net/ipv4/netfilter/iptable_nat.c
> > @@ -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.

Hm, never mind about names.

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

[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 <fw@strlen.de>
---
 net/netfilter/nf_tables_api.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index f000d4399c7a..4ed66f1b40b5 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -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;
 
-- 
2.13.6

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

[Pablo Neira Ayuso]

On Fri, Dec 08, 2017 at 05:01:55PM +0100, Florian Westphal wrote:
> 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 <fw@strlen.de>
> ---
>  net/netfilter/nf_tables_api.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index f000d4399c7a..4ed66f1b40b5 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -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.

> +
>  	if (!try_module_get(type->owner))
>  		return -ENOENT;
>  
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pablo Neira Ayuso]

On Fri, Dec 08, 2017 at 10:22:57PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Dec 08, 2017 at 05:01:55PM +0100, Florian Westphal wrote:
> > 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 <fw@strlen.de>
> > ---
> >  net/netfilter/nf_tables_api.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index f000d4399c7a..4ed66f1b40b5 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -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.

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 1cc1faefed69..2a29a5a58913 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -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.