Skip to contentSkip to conversation

Netfilter/IPVS fixes for net

PATCH In review Pablo Neira Ayuso 2 participants 11 messages Sep 8, 2017

Subsystems (4)
lore
Share & tools
lore

Pablo Neira Ayuso

author maintainer
#0 lore raw

cover Netfilter/IPVS fixes for net

Hi David,

The following patchset contains Netfilter/IPVS fixes for your net tree, they are:

1) Fix SCTP connection setup when IPVS module is loaded and any scheduler is registered, from Xin Long.

2) Don't create a SCTP connection from SCTP ABORT packets, also from Xin Long.

3) WARN_ON() and drop packet, instead of BUG_ON() races when calling nf_nat_setup_info(). This is specifically a longstanding problem when br_netfilter with conntrack support is in place, patch from Florian Westphal.

4) Avoid softlock splats via iptables-restore, also from Florian.

5) Revert NAT hashtable conversion to rhashtable, semantics of rhlist are different from our simple NAT hashtable, this has been causing problems in the recent Linux kernel releases. From Florian.

6) Add per-bucket spinlock for NAT hashtable, so at least we restore one of the benefits we got from the previous rhashtable conversion.

7) Fix incorrect hashtable size in memory allocation in xt_hashlimit, from Zhizhou Tian.

8) Fix build/link problems with hashlimit and 32-bit arches, to address recent fallout from a new hashlimit mode, from Vishwanath Pai.

You can pull these changes from:

git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git

Thanks!

----------------------------------------------------------------

The following changes since commit 8e0deed92406d93ae0365cb8a6134db5721e7aca:

tipc: remove unnecessary call to dev_net() (2017-09-06 21:25:52 -0700)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git HEAD

for you to fetch changes up to 90c4ae4e2c1da9f1eaf846136861af43d4c1ff34:

netfilter: xt_hashlimit: fix build error caused by 64bit division (2017-09-08 18:55:53 +0200)

----------------------------------------------------------------
Florian Westphal (5):
netfilter: nf_nat: don't bug when mapping already exists
netfilter: xtables: add scheduling opportunity in get_counters
netfilter: nat: Revert "netfilter: nat: convert nat bysrc hash to rhashtable"
netfilter: nat: use keyed locks
netfilter: core: remove erroneous warn_on

Vishwanath Pai (1):
netfilter: xt_hashlimit: fix build error caused by 64bit division

Xin Long (2):
netfilter: ipvs: fix the issue that sctp_conn_schedule drops non-INIT packet
netfilter: ipvs: do not create conn for ABORT packet in sctp_conn_schedule

Zhizhou Tian (1):
netfilter: xt_hashlimit: alloc hashtable with right size

 include/net/netfilter/nf_conntrack.h  |   3 +-
 include/net/netfilter/nf_nat.h        |   1 -
 net/ipv4/netfilter/arp_tables.c       |   1 +
 net/ipv4/netfilter/ip_tables.c        |   1 +
 net/ipv6/netfilter/ip6_tables.c       |   1 +
 net/netfilter/core.c                  |   2 +-
 net/netfilter/ipvs/ip_vs_proto_sctp.c |   8 +-
 net/netfilter/nf_nat_core.c           | 146 ++++++++++++++++------------------
 net/netfilter/xt_hashlimit.c          |  16 ++--
 9 files changed, 88 insertions(+), 91 deletions(-)
author maintainer
#1 lore raw

PATCH 1/9 netfilter: ipvs: fix the issue that sctp_conn_schedule drops non-INIT packet

From: Xin Long <[email protected]>

Commit 5e26b1b3abce ("ipvs: support scheduling inverse and icmp SCTP packets") changed to check packet type early. It introduced a side
effect: if it's not a INIT packet, ports will be set as NULL, and the packet will be dropped later.

It caused that sctp couldn't create connection when ipvs module is loaded and any scheduler is registered on server.

Li Shuang reproduced it by running the cmds on sctp server:

  # ipvsadm -A -t 1.1.1.1:80 -s rr
  # ipvsadm -D -t 1.1.1.1:80
then the server could't work any more.

This patch is to return 1 when it's not an INIT packet. It means ipvs will accept it without creating a conn for it, just like what it does for tcp.

Fixes 5e26b1b3abce ("ipvs: support scheduling inverse and icmp SCTP packets")
---
 net/netfilter/ipvs/ip_vs_proto_sctp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
net/netfilter/ipvs/ip_vs_proto_sctp.c +5 -2
index e1efa446b305..81f08198b125 100644
@@ -24,9 +24,12 @@ sctp_conn_schedule(struct netns_ipvs *ipvs, int af, struct sk_buff *skb,
 		if (sh) {
 			sch = skb_header_pointer(skb, iph->len + sizeof(_sctph),
 						 sizeof(_schunkh), &_schunkh);
-			if (sch && (sch->type == SCTP_CID_INIT ||
-				    sysctl_sloppy_sctp(ipvs)))
+			if (sch) {
+				if (!(sysctl_sloppy_sctp(ipvs) ||
+				      sch->type == SCTP_CID_INIT))
+					return 1;
 				ports = &sh->source;
+			}
 		}
 	} else {
 		ports = skb_header_pointer(
Signature · 1 line
2.1.4
author maintainer
#2 lore raw

PATCH 2/9 netfilter: ipvs: do not create conn for ABORT packet in sctp_conn_schedule

In reply to Pablo Neira Ayuso

From: Xin Long <[email protected]>

There's no reason for ipvs to create a conn for an ABORT packet even if sysctl_sloppy_sctp is set.

This patch is to accept it without creating a conn, just as ipvs does for tcp's RST packet.

---
 net/netfilter/ipvs/ip_vs_proto_sctp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/netfilter/ipvs/ip_vs_proto_sctp.c +2 -1
index 81f08198b125..57c8ee66491e 100644
@@ -25,7 +25,8 @@ sctp_conn_schedule(struct netns_ipvs *ipvs, int af, struct sk_buff *skb,
 			sch = skb_header_pointer(skb, iph->len + sizeof(_sctph),
 						 sizeof(_schunkh), &_schunkh);
 			if (sch) {
-				if (!(sysctl_sloppy_sctp(ipvs) ||
+				if (sch->type == SCTP_CID_ABORT ||
+				    !(sysctl_sloppy_sctp(ipvs) ||
 				      sch->type == SCTP_CID_INIT))
 					return 1;
 				ports = &sh->source;
Signature · 1 line
2.1.4
author maintainer
#3 lore raw

PATCH 3/9 netfilter: nf_nat: don't bug when mapping already exists

In reply to Pablo Neira Ayuso

From: Florian Westphal <[email protected]>

It seems preferrable to limp along if we have a conflicting mapping, its certainly better than a BUG().

---
 net/netfilter/nf_nat_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
net/netfilter/nf_nat_core.c +3 -1
index 40573aa6c133..dc3519cc7209 100644
@@ -416,7 +416,9 @@ nf_nat_setup_info(struct nf_conn *ct,
 
 	WARN_ON(maniptype != NF_NAT_MANIP_SRC &&
 		maniptype != NF_NAT_MANIP_DST);
-	BUG_ON(nf_nat_initialized(ct, maniptype));
+
+	if (WARN_ON(nf_nat_initialized(ct, maniptype)))
+		return NF_DROP;
 
 	/* What we've got will look like inverse of reply. Normally
 	 * this is what is in the conntrack, except for prior
Signature · 1 line
2.1.4
author maintainer
#4 lore raw

PATCH 4/9 netfilter: xtables: add scheduling opportunity in get_counters

In reply to Pablo Neira Ayuso

From: Florian Westphal <[email protected]>

There are reports about spurious softlockups during iptables-restore, a backtrace i saw points at get_counters -- it uses a sequence lock and also has unbounded restart loop.

---
 net/ipv4/netfilter/arp_tables.c | 1 +
 net/ipv4/netfilter/ip_tables.c  | 1 +
 net/ipv6/netfilter/ip6_tables.c | 1 +
 3 files changed, 3 insertions(+)
net/ipv4/netfilter/arp_tables.c +1 -0
index e04457198f93..9e2770fd00be 100644
@@ -629,6 +629,7 @@ static void get_counters(const struct xt_table_info *t,
 
 			ADD_COUNTER(counters[i], bcnt, pcnt);
 			++i;
+			cond_resched();
 		}
 	}
 }
net/ipv4/netfilter/ip_tables.c +1 -0
index 576cba2b57e9..39286e543ee6 100644
@@ -776,6 +776,7 @@ get_counters(const struct xt_table_info *t,
 
 			ADD_COUNTER(counters[i], bcnt, pcnt);
 			++i; /* macro does multi eval of i */
+			cond_resched();
 		}
 	}
 }
net/ipv6/netfilter/ip6_tables.c +1 -0
index 54b1e75eded1..01bd3ee5ebc6 100644
@@ -795,6 +795,7 @@ get_counters(const struct xt_table_info *t,
 
 			ADD_COUNTER(counters[i], bcnt, pcnt);
 			++i;
+			cond_resched();
 		}
 	}
 }
Signature · 1 line
2.1.4
author maintainer
#5 lore raw

PATCH 5/9 netfilter: nat: Revert "netfilter: nat: convert nat bysrc hash to rhashtable"

In reply to Pablo Neira Ayuso

From: Florian Westphal <[email protected]>

This reverts commit 870190a9ec9075205c0fa795a09fa931694a3ff1.

It was not a good idea. The custom hash table was a much better fit for this purpose.

A fast lookup is not essential, in fact for most cases there is no lookup at all because original tuple is not taken and can be used as-is. What needs to be fast is insertion and deletion.

rhlist removal however requires a rhlist walk.
We can have thousands of entries in such a list if source port/addresses are reused for multiple flows, if this happens removal requests are so expensive that deletions of a few thousand flows can take several seconds(!).

The advantages that we got from rhashtable are:
1) table auto-sizing
2) multiple locks

1) would be nice to have, but it is not essential as we have at most one lookup per new flow, so even a million flows in the bysource table are not a problem compared to current deletion cost.
2) is easy to add to custom hash table.

I tried to add hlist_node to rhlist to speed up rhltable_remove but this isn't doable without changing semantics. rhltable_remove_fast will check that the to-be-deleted object is part of the table and that requires a list walk that we want to avoid.

Furthermore, using hlist_node increases size of struct rhlist_head, which in turn increases nf_conn size.

---
 include/net/netfilter/nf_conntrack.h |   3 +-
 include/net/netfilter/nf_nat.h       |   1 -
 net/netfilter/nf_nat_core.c          | 130 ++++++++++++++---------------------
 3 files changed, 54 insertions(+), 80 deletions(-)
include/net/netfilter/nf_conntrack.h +1 -2
index fdc9c64a1c94..8f3bd30511de 100644
@@ -17,7 +17,6 @@
 #include <linux/bitops.h>
 #include <linux/compiler.h>
 #include <linux/atomic.h>
-#include <linux/rhashtable.h>
 
 #include <linux/netfilter/nf_conntrack_tcp.h>
 #include <linux/netfilter/nf_conntrack_dccp.h>
@@ -77,7 +76,7 @@ struct nf_conn {
 	possible_net_t ct_net;
 
 #if IS_ENABLED(CONFIG_NF_NAT)
-	struct rhlist_head nat_bysource;
+	struct hlist_node	nat_bysource;
 #endif
 	/* all members below initialized via memset */
 	u8 __nfct_init_offset[0];
include/net/netfilter/nf_nat.h +0 -1
index 05c82a1a4267..b71701302e61 100644
@@ -1,6 +1,5 @@
 #ifndef _NF_NAT_H
 #define _NF_NAT_H
-#include <linux/rhashtable.h>
 #include <linux/netfilter_ipv4.h>
 #include <linux/netfilter/nf_nat.h>
 #include <net/netfilter/nf_conntrack_tuple.h>
net/netfilter/nf_nat_core.c +53 -77

206 lines not on this page · raw message

Signature · 1 line
2.1.4
author maintainer
#6 lore raw

PATCH 6/9 netfilter: nat: use keyed locks

In reply to Pablo Neira Ayuso

From: Florian Westphal <[email protected]>

no need to serialize on a single lock, we can partition the table and add/delete in parallel to different slots.
This restores one of the advantages that got lost with the rhlist revert.

---
 net/netfilter/nf_nat_core.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)
net/netfilter/nf_nat_core.c +24 -12
index f090419f5f97..f393a7086025 100644
@@ -30,7 +30,7 @@
 #include <net/netfilter/nf_conntrack_zones.h>
 #include <linux/netfilter/nf_nat.h>
 
-static DEFINE_SPINLOCK(nf_nat_lock);
+static spinlock_t nf_nat_locks[CONNTRACK_LOCKS];
 
 static DEFINE_MUTEX(nf_nat_proto_mutex);
 static const struct nf_nat_l3proto __rcu *nf_nat_l3protos[NFPROTO_NUMPROTO]
@@ -425,13 +425,15 @@ nf_nat_setup_info(struct nf_conn *ct,
 
 	if (maniptype == NF_NAT_MANIP_SRC) {
 		unsigned int srchash;
+		spinlock_t *lock;
 
 		srchash = hash_by_src(net,
 				      &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
-		spin_lock_bh(&nf_nat_lock);
+		lock = &nf_nat_locks[srchash % ARRAY_SIZE(nf_nat_locks)];
+		spin_lock_bh(lock);
 		hlist_add_head_rcu(&ct->nat_bysource,
 				   &nf_nat_bysource[srchash]);
-		spin_unlock_bh(&nf_nat_lock);
+		spin_unlock_bh(lock);
 	}
 
 	/* It's done. */
@@ -525,6 +527,16 @@ static int nf_nat_proto_remove(struct nf_conn *i, void *data)
 	return i->status & IPS_NAT_MASK ? 1 : 0;
 }
 
+static void __nf_nat_cleanup_conntrack(struct nf_conn *ct)
+{
+	unsigned int h;
+
+	h = hash_by_src(nf_ct_net(ct), &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
+	spin_lock_bh(&nf_nat_locks[h % ARRAY_SIZE(nf_nat_locks)]);
+	hlist_del_rcu(&ct->nat_bysource);
+	spin_unlock_bh(&nf_nat_locks[h % ARRAY_SIZE(nf_nat_locks)]);
+}
+
 static int nf_nat_proto_clean(struct nf_conn *ct, void *data)
 {
 	if (nf_nat_proto_remove(ct, data))
@@ -540,9 +552,7 @@ static int nf_nat_proto_clean(struct nf_conn *ct, void *data)
 	 * will delete entry from already-freed table.
 	 */
 	clear_bit(IPS_SRC_NAT_DONE_BIT, &ct->status);
-	spin_lock_bh(&nf_nat_lock);
-	hlist_del_rcu(&ct->nat_bysource);
-	spin_unlock_bh(&nf_nat_lock);
+	__nf_nat_cleanup_conntrack(ct);
 
 	/* don't delete conntrack.  Although that would make things a lot
 	 * simpler, we'd end up flushing all conntracks on nat rmmod.
@@ -670,11 +680,8 @@ EXPORT_SYMBOL_GPL(nf_nat_l3proto_unregister);
 /* No one using conntrack by the time this called. */
 static void nf_nat_cleanup_conntrack(struct nf_conn *ct)
 {
-	if (ct->status & IPS_SRC_NAT_DONE) {
-		spin_lock_bh(&nf_nat_lock);
-		hlist_del_rcu(&ct->nat_bysource);
-		spin_unlock_bh(&nf_nat_lock);
-	}
+	if (ct->status & IPS_SRC_NAT_DONE)
+		__nf_nat_cleanup_conntrack(ct);
 }
 
 static struct nf_ct_ext_type nat_extend __read_mostly = {
@@ -796,10 +803,12 @@ static struct nf_ct_helper_expectfn follow_master_nat = {
 
 static int __init nf_nat_init(void)
 {
-	int ret;
+	int ret, i;
 
 	/* Leave them the same for the moment. */
 	nf_nat_htable_size = nf_conntrack_htable_size;
+	if (nf_nat_htable_size < ARRAY_SIZE(nf_nat_locks))
+		nf_nat_htable_size = ARRAY_SIZE(nf_nat_locks);
 
 	nf_nat_bysource = nf_ct_alloc_hashtable(&nf_nat_htable_size, 0);
 	if (!nf_nat_bysource)
@@ -812,6 +821,9 @@ static int __init nf_nat_init(void)
 		return ret;
 	}
 
+	for (i = 0; i < ARRAY_SIZE(nf_nat_locks); i++)
+		spin_lock_init(&nf_nat_locks[i]);
+
 	nf_ct_helper_expectfn_register(&follow_master_nat);
 
 	BUG_ON(nfnetlink_parse_nat_setup_hook != NULL);
Signature · 1 line
2.1.4
author maintainer
#7 lore raw

PATCH 7/9 netfilter: core: remove erroneous warn_on

In reply to Pablo Neira Ayuso

From: Florian Westphal <[email protected]>

kernel test robot reported:

WARNING: CPU: 0 PID: 1244 at net/netfilter/core.c:218 __nf_hook_entries_try_shrink+0x49/0xcd

[..]

After allowing batching in nf_unregister_net_hooks its possible that an earlier call to __nf_hook_entries_try_shrink already compacted the list. If this happens we don't need to do anything.

Fixes d3ad2c17b4047 ("netfilter: core: batch nf_unregister_net_hooks synchronize_net calls")
---
 net/netfilter/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/netfilter/core.c +1 -1
index 04fe25abc5f6..52cd2901a097 100644
@@ -215,7 +215,7 @@ static void *__nf_hook_entries_try_shrink(struct nf_hook_entries __rcu **pp)
 	if (skip == hook_entries)
 		goto out_assign;
 
-	if (WARN_ON(skip == 0))
+	if (skip == 0)
 		return NULL;
 
 	hook_entries -= skip;
Signature · 1 line
2.1.4
author maintainer
#8 lore raw

PATCH 8/9 netfilter: xt_hashlimit: alloc hashtable with right size

In reply to Pablo Neira Ayuso

From: Zhizhou Tian <[email protected]>

struct xt_byteslimit_htable used hlist_head, but memory allocation is done through sizeof(struct list_head).

---
 net/netfilter/xt_hashlimit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/netfilter/xt_hashlimit.c +2 -2
index 10d48234f5f4..962ea4a63d9f 100644
@@ -279,7 +279,7 @@ static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
 		size = cfg->size;
 	} else {
 		size = (totalram_pages << PAGE_SHIFT) / 16384 /
-		       sizeof(struct list_head);
+		       sizeof(struct hlist_head);
 		if (totalram_pages > 1024 * 1024 * 1024 / PAGE_SIZE)
 			size = 8192;
 		if (size < 16)
@@ -287,7 +287,7 @@ static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
 	}
 	/* FIXME: don't use vmalloc() here or anywhere else -HW */
 	hinfo = vmalloc(sizeof(struct xt_hashlimit_htable) +
-	                sizeof(struct list_head) * size);
+	                sizeof(struct hlist_head) * size);
 	if (hinfo == NULL)
 		return -ENOMEM;
 	*out_hinfo = hinfo;
Signature · 1 line
2.1.4
author maintainer
#9 lore raw

PATCH 9/9 netfilter: xt_hashlimit: fix build error caused by 64bit division

In reply to Pablo Neira Ayuso

From: Vishwanath Pai <[email protected]>

64bit division causes build/link errors on 32bit architectures. It prints out error messages like:

ERROR: "__aeabi_uldivmod" [net/netfilter/xt_hashlimit.ko] undefined!

The value of avg passed through by userspace in BYTE mode cannot exceed U32_MAX. Which means 64bit division in user2rate_bytes is unnecessary. To fix this I have changed the type of param 'user' to u32.

Since anything greater than U32_MAX is an invalid input we error out in hashlimit_mt_check_common() when this is the case.

Changes in v2:
Making return type as u32 would cause an overflow for small values of 'user' (for example 2, 3 etc). To avoid this I bumped up 'r' to u64 again as well as the return type. This is OK since the variable that stores the result is u64. We still avoid 64bit division here since 'user' is u32.

Fixes bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
---
 net/netfilter/xt_hashlimit.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
net/netfilter/xt_hashlimit.c +7 -5
index 962ea4a63d9f..5da8746f7b88 100644
@@ -35,6 +35,7 @@
 #include <linux/netfilter_ipv6/ip6_tables.h>
 #include <linux/netfilter/xt_hashlimit.h>
 #include <linux/mutex.h>
+#include <linux/kernel.h>
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Harald Welte <[email protected]>");
@@ -527,12 +528,12 @@ static u64 user2rate(u64 user)
 	}
 }
 
-static u64 user2rate_bytes(u64 user)
+static u64 user2rate_bytes(u32 user)
 {
 	u64 r;
 
-	r = user ? 0xFFFFFFFFULL / user : 0xFFFFFFFFULL;
-	r = (r - 1) << 4;
+	r = user ? U32_MAX / user : U32_MAX;
+	r = (r - 1) << XT_HASHLIMIT_BYTE_SHIFT;
 	return r;
 }
 
@@ -588,7 +589,8 @@ static void rateinfo_init(struct dsthash_ent *dh,
 		dh->rateinfo.prev_window = 0;
 		dh->rateinfo.current_rate = 0;
 		if (hinfo->cfg.mode & XT_HASHLIMIT_BYTES) {
-			dh->rateinfo.rate = user2rate_bytes(hinfo->cfg.avg);
+			dh->rateinfo.rate =
+				user2rate_bytes((u32)hinfo->cfg.avg);
 			if (hinfo->cfg.burst)
 				dh->rateinfo.burst =
 					hinfo->cfg.burst * dh->rateinfo.rate;
@@ -870,7 +872,7 @@ static int hashlimit_mt_check_common(const struct xt_mtchk_param *par,
 
 	/* Check for overflow. */
 	if (revision >= 3 && cfg->mode & XT_HASHLIMIT_RATE_MATCH) {
-		if (cfg->avg == 0) {
+		if (cfg->avg == 0 || cfg->avg > U32_MAX) {
 			pr_info("hashlimit invalid rate\n");
 			return -ERANGE;
 		}
Signature · 1 line
2.1.4
maintainer
#10 lore raw
In reply to Pablo Neira Ayuso

From: Pablo Neira Ayuso <[email protected]>
Date: Fri, 8 Sep 2017 19:45:39 +0200

Quoted 20 lines

The following patchset contains Netfilter/IPVS fixes for your net tree, they are:

1) Fix SCTP connection setup when IPVS module is loaded and any scheduler is registered, from Xin Long.

2) Don't create a SCTP connection from SCTP ABORT packets, also from Xin Long.

3) WARN_ON() and drop packet, instead of BUG_ON() races when calling nf_nat_setup_info(). This is specifically a longstanding problem when br_netfilter with conntrack support is in place, patch from Florian Westphal.

4) Avoid softlock splats via iptables-restore, also from Florian.

5) Revert NAT hashtable conversion to rhashtable, semantics of rhlist are different from our simple NAT hashtable, this has been causing problems in the recent Linux kernel releases. From Florian.

Quoted 8 lines

6) Add per-bucket spinlock for NAT hashtable, so at least we restore one of the benefits we got from the previous rhashtable conversion.

7) Fix incorrect hashtable size in memory allocation in xt_hashlimit, from Zhizhou Tian.

8) Fix build/link problems with hashlimit and 32-bit arches, to address recent fallout from a new hashlimit mode, from Vishwanath Pai.

Pulled, thanks Pablo.