* [PATCH nf-next 0/3] netfilter: disable parallel use of xtables and nftables nat @ 2017-12-08 16:01 Florian Westphal 2017-12-08 16:01 ` [PATCH nf-next 1/3] netfilter: xtables: add and use xt_request_find_table_lock Florian Westphal ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Florian Westphal @ 2017-12-08 16:01 UTC (permalink / raw) To: netfilter-devel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH nf-next 1/3] netfilter: xtables: add and use xt_request_find_table_lock 2017-12-08 16:01 [PATCH nf-next 0/3] netfilter: disable parallel use of xtables and nftables nat Florian Westphal @ 2017-12-08 16:01 ` Florian Westphal 2017-12-09 15:23 ` Pablo Neira Ayuso 2017-12-08 16:01 ` [PATCH nf-next 2/3] netfilter: core: only allow one nat hook per hook point Florian Westphal 2017-12-08 16:01 ` [PATCH nf-next 3/3] nftables: reject nat hook registration if prio is before conntrack Florian Westphal 2 siblings, 1 reply; 9+ messages in thread From: Florian Westphal @ 2017-12-08 16:01 UTC (permalink / raw) To: netfilter-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH nf-next 1/3] netfilter: xtables: add and use xt_request_find_table_lock 2017-12-08 16:01 ` [PATCH nf-next 1/3] netfilter: xtables: add and use xt_request_find_table_lock Florian Westphal @ 2017-12-09 15:23 ` Pablo Neira Ayuso 0 siblings, 0 replies; 9+ messages in thread From: Pablo Neira Ayuso @ 2017-12-09 15:23 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel 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(). ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH nf-next 2/3] netfilter: core: only allow one nat hook per hook point 2017-12-08 16:01 [PATCH nf-next 0/3] netfilter: disable parallel use of xtables and nftables nat Florian Westphal 2017-12-08 16:01 ` [PATCH nf-next 1/3] netfilter: xtables: add and use xt_request_find_table_lock Florian Westphal @ 2017-12-08 16:01 ` Florian Westphal 2017-12-08 21:28 ` Pablo Neira Ayuso 2017-12-08 16:01 ` [PATCH nf-next 3/3] nftables: reject nat hook registration if prio is before conntrack Florian Westphal 2 siblings, 1 reply; 9+ messages in thread From: Florian Westphal @ 2017-12-08 16:01 UTC (permalink / raw) To: netfilter-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH nf-next 2/3] netfilter: core: only allow one nat hook per hook point 2017-12-08 16:01 ` [PATCH nf-next 2/3] netfilter: core: only allow one nat hook per hook point Florian Westphal @ 2017-12-08 21:28 ` Pablo Neira Ayuso 2017-12-08 21:33 ` Pablo Neira Ayuso 0 siblings, 1 reply; 9+ messages in thread From: Pablo Neira Ayuso @ 2017-12-08 21:28 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH nf-next 2/3] netfilter: core: only allow one nat hook per hook point 2017-12-08 21:28 ` Pablo Neira Ayuso @ 2017-12-08 21:33 ` Pablo Neira Ayuso 0 siblings, 0 replies; 9+ messages in thread From: Pablo Neira Ayuso @ 2017-12-08 21:33 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH nf-next 3/3] nftables: reject nat hook registration if prio is before conntrack 2017-12-08 16:01 [PATCH nf-next 0/3] netfilter: disable parallel use of xtables and nftables nat Florian Westphal 2017-12-08 16:01 ` [PATCH nf-next 1/3] netfilter: xtables: add and use xt_request_find_table_lock Florian Westphal 2017-12-08 16:01 ` [PATCH nf-next 2/3] netfilter: core: only allow one nat hook per hook point Florian Westphal @ 2017-12-08 16:01 ` Florian Westphal 2017-12-08 21:22 ` Pablo Neira Ayuso 2 siblings, 1 reply; 9+ messages in thread From: Florian Westphal @ 2017-12-08 16:01 UTC (permalink / raw) To: netfilter-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH nf-next 3/3] nftables: reject nat hook registration if prio is before conntrack 2017-12-08 16:01 ` [PATCH nf-next 3/3] nftables: reject nat hook registration if prio is before conntrack Florian Westphal @ 2017-12-08 21:22 ` Pablo Neira Ayuso 2017-12-13 16:28 ` Pablo Neira Ayuso 0 siblings, 1 reply; 9+ messages in thread From: Pablo Neira Ayuso @ 2017-12-08 21:22 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH nf-next 3/3] nftables: reject nat hook registration if prio is before conntrack 2017-12-08 21:22 ` Pablo Neira Ayuso @ 2017-12-13 16:28 ` Pablo Neira Ayuso 0 siblings, 0 replies; 9+ messages in thread From: Pablo Neira Ayuso @ 2017-12-13 16:28 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel 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. ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-12-13 16:28 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-08 16:01 [PATCH nf-next 0/3] netfilter: disable parallel use of xtables and nftables nat Florian Westphal 2017-12-08 16:01 ` [PATCH nf-next 1/3] netfilter: xtables: add and use xt_request_find_table_lock Florian Westphal 2017-12-09 15:23 ` Pablo Neira Ayuso 2017-12-08 16:01 ` [PATCH nf-next 2/3] netfilter: core: only allow one nat hook per hook point Florian Westphal 2017-12-08 21:28 ` Pablo Neira Ayuso 2017-12-08 21:33 ` Pablo Neira Ayuso 2017-12-08 16:01 ` [PATCH nf-next 3/3] nftables: reject nat hook registration if prio is before conntrack Florian Westphal 2017-12-08 21:22 ` Pablo Neira Ayuso 2017-12-13 16:28 ` Pablo Neira Ayuso
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.