[PATCH nf-next 0/3] netfilter: speed up netns cleanup

[PATCH nf-next 0/3] netfilter: speed up netns cleanup

[Florian Westphal]

low hanging fruits to speed up netns cleanup in netfilter.
We're way too happy to issue expensive synchronize_rcu() all
over the place.

On my test vm 8 processes doing 32 unshare each finish in ~3 minutes,
with these patches it gets down to 40 seconds.

We could probably further improve this.

 include/net/netfilter/nf_queue.h |    3 --
 net/netfilter/core.c             |   55 ++++++++++++++++++++++++++++++++-------
 net/netfilter/nf_internals.h     |    2 -
 net/netfilter/nf_log.c           |    1 
 net/netfilter/nf_queue.c         |    7 +++-
 net/netfilter/nfnetlink_queue.c  |   18 +++++-------
 6 files changed, 61 insertions(+), 25 deletions(-)

[PATCH nf-next 1/3] netfilter: batch synchronize_net calls during hook unregister

[Florian Westphal]

synchronize_net is expensive and slows down netns cleanup a lot.

We have two APIs to unregister a hook:
nf_unregister_net_hook (which calls synchronize_net())
and
nf_unregister_net_hooks (calls nf_unregister_net_hook in a loop)

Make nf_unregister_net_hook a wapper around new helper
__nf_unregister_net_hook, which unlinks the hook but does not free it.

Then, we can call that helper in nf_unregister_net_hooks and then
call synchronize_net() only once.

Andrey Konovalov reports this change improves syzkaller fuzzing speed at
least twice.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/core.c | 46 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index a87a6f8a74d8..b5d908851cc8 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -126,14 +126,15 @@ int nf_register_net_hook(struct net *net, const struct nf_hook_ops *reg)
 }
 EXPORT_SYMBOL(nf_register_net_hook);
 
-void nf_unregister_net_hook(struct net *net, const struct nf_hook_ops *reg)
+static struct nf_hook_entry *
+__nf_unregister_net_hook(struct net *net, const struct nf_hook_ops *reg)
 {
 	struct nf_hook_entry __rcu **pp;
 	struct nf_hook_entry *p;
 
 	pp = nf_hook_entry_head(net, reg);
 	if (WARN_ON_ONCE(!pp))
-		return;
+		return NULL;
 
 	mutex_lock(&nf_hook_mutex);
 	for (; (p = nf_entry_dereference(*pp)) != NULL; pp = &p->next) {
@@ -145,7 +146,7 @@ void nf_unregister_net_hook(struct net *net, const struct nf_hook_ops *reg)
 	mutex_unlock(&nf_hook_mutex);
 	if (!p) {
 		WARN(1, "nf_unregister_net_hook: hook not found!\n");
-		return;
+		return NULL;
 	}
 #ifdef CONFIG_NETFILTER_INGRESS
 	if (reg->pf == NFPROTO_NETDEV && reg->hooknum == NF_NETDEV_INGRESS)
@@ -154,6 +155,17 @@ void nf_unregister_net_hook(struct net *net, const struct nf_hook_ops *reg)
 #ifdef HAVE_JUMP_LABEL
 	static_key_slow_dec(&nf_hooks_needed[reg->pf][reg->hooknum]);
 #endif
+
+	return p;
+}
+
+void nf_unregister_net_hook(struct net *net, const struct nf_hook_ops *reg)
+{
+	struct nf_hook_entry *p = __nf_unregister_net_hook(net, reg);
+
+	if (!p)
+		return;
+
 	synchronize_net();
 	nf_queue_nf_hook_drop(net, p);
 	/* other cpu might still process nfqueue verdict that used reg */
@@ -183,10 +195,32 @@ int nf_register_net_hooks(struct net *net, const struct nf_hook_ops *reg,
 EXPORT_SYMBOL(nf_register_net_hooks);
 
 void nf_unregister_net_hooks(struct net *net, const struct nf_hook_ops *reg,
-			     unsigned int n)
+			     unsigned int hookcount)
 {
-	while (n-- > 0)
-		nf_unregister_net_hook(net, &reg[n]);
+	struct nf_hook_entry *to_free[16];
+	unsigned int i, n;
+
+	do {
+		n = min_t(unsigned int, hookcount, ARRAY_SIZE(to_free));
+
+		for (i = 0; i < n; i++)
+			to_free[i] = __nf_unregister_net_hook(net, &reg[i]);
+
+		synchronize_net();
+
+		for (i = 0; i < n; i++) {
+			if (to_free[i])
+				nf_queue_nf_hook_drop(net, to_free[i]);
+		}
+
+		synchronize_net();
+
+		for (i = 0; i < n; i++)
+			kfree(to_free[i]);
+
+		reg += n;
+		hookcount -= n;
+	} while (hookcount > 0);
 }
 EXPORT_SYMBOL(nf_unregister_net_hooks);
 
-- 
2.10.2

[PATCH nf-next 2/3] netfilter: nf_log: don't call synchronize_rcu in nf_log_unset

[Florian Westphal]

nf_log_unregister() (which is what gets called in the logger backends
module exit paths) does a (required, module is removed) synchronize_rcu().

But nf_log_unset() is only called from pernet exit handlers. It doesn't
free any memory so there appears to be no need to call synchronize_rcu.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_log.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index cc32727e3f32..8bb152a7cca4 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -71,7 +71,6 @@ void nf_log_unset(struct net *net, const struct nf_logger *logger)
 			RCU_INIT_POINTER(net->nf.nf_loggers[i], NULL);
 	}
 	mutex_unlock(&nf_log_mutex);
-	synchronize_rcu();
 }
 EXPORT_SYMBOL(nf_log_unset);
 
-- 
2.10.2

Re: [PATCH nf-next 2/3] netfilter: nf_log: don't call synchronize_rcu in nf_log_unset

[Liping Zhang]

Hi Florian,

2017-04-24 21:37 GMT+08:00 Florian Westphal <fw@strlen.de>:
> nf_log_unregister() (which is what gets called in the logger backends
> module exit paths) does a (required, module is removed) synchronize_rcu().
>
> But nf_log_unset() is only called from pernet exit handlers. It doesn't
> free any memory so there appears to be no need to call synchronize_rcu.

If I did not miss something, maybe there's an exception here, in
the nfnetlink_log.ko's module exit routine, nf_log_unregister
is invoked before unregister_pernet_subsys, i.e. _log_unregister
will be done before _log_unset.

So removing the synchronize_rcu in _log_unset will become unsafe
when uninstalling the nfnetlink_log.ko:

static void __exit nfnetlink_log_fini(void)
{
    nf_log_unregister(&nfulnl_logger);  --> synchronize_rcu here.
    nfnetlink_subsys_unregister(&nfulnl_subsys);
    netlink_unregister_notifier(&nfulnl_rtnl_notifier);
    unregister_pernet_subsys(&nfnl_log_net_ops); --> _log_unset
without synchronize_rcu.
}

So I think after dropping the synchronize_rcu in _log_unset, we
should also put the "nf_log_unregister(&nfulnl_logger);" to the
end, just like other nf_log_X modules do.

[PATCH nf-next 3/3] netfilter: nf_queue: only call synchronize_net twice if nf_queue is active

[Florian Westphal]

nf_unregister_net_hook(s) can avoid a second call to synchronize_net,
provided there is no nfqueue active in that net namespace (which is
the common case).

This also gets rid of the extra arg to nf_queue_nf_hook_drop(), normally
this gets called during netns cleanup so no packets should be queued.

For the rare case of base chain being unregistered or module removal
while nfqueue is in use the extra hiccup due to the packet drops isn't
a big deal.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_queue.h |  3 +--
 net/netfilter/core.c             | 21 ++++++++++++---------
 net/netfilter/nf_internals.h     |  2 +-
 net/netfilter/nf_queue.c         |  7 +++++--
 net/netfilter/nfnetlink_queue.c  | 18 ++++++++----------
 5 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h
index 09948d10e38e..4454719ff849 100644
--- a/include/net/netfilter/nf_queue.h
+++ b/include/net/netfilter/nf_queue.h
@@ -24,8 +24,7 @@ struct nf_queue_entry {
 struct nf_queue_handler {
 	int		(*outfn)(struct nf_queue_entry *entry,
 				 unsigned int queuenum);
-	void		(*nf_hook_drop)(struct net *net,
-					const struct nf_hook_entry *hooks);
+	unsigned int	(*nf_hook_drop)(struct net *net);
 };
 
 void nf_register_queue_handler(struct net *net, const struct nf_queue_handler *qh);
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index b5d908851cc8..552d606e57ca 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -162,14 +162,17 @@ __nf_unregister_net_hook(struct net *net, const struct nf_hook_ops *reg)
 void nf_unregister_net_hook(struct net *net, const struct nf_hook_ops *reg)
 {
 	struct nf_hook_entry *p = __nf_unregister_net_hook(net, reg);
+	unsigned int nfq;
 
 	if (!p)
 		return;
 
 	synchronize_net();
-	nf_queue_nf_hook_drop(net, p);
+
 	/* other cpu might still process nfqueue verdict that used reg */
-	synchronize_net();
+	nfq = nf_queue_nf_hook_drop(net);
+	if (nfq)
+		synchronize_net();
 	kfree(p);
 }
 EXPORT_SYMBOL(nf_unregister_net_hook);
@@ -198,7 +201,7 @@ void nf_unregister_net_hooks(struct net *net, const struct nf_hook_ops *reg,
 			     unsigned int hookcount)
 {
 	struct nf_hook_entry *to_free[16];
-	unsigned int i, n;
+	unsigned int i, n, nfq;
 
 	do {
 		n = min_t(unsigned int, hookcount, ARRAY_SIZE(to_free));
@@ -208,12 +211,12 @@ void nf_unregister_net_hooks(struct net *net, const struct nf_hook_ops *reg,
 
 		synchronize_net();
 
-		for (i = 0; i < n; i++) {
-			if (to_free[i])
-				nf_queue_nf_hook_drop(net, to_free[i]);
-		}
-
-		synchronize_net();
+		/* need 2nd synchronize_net() if nfqueue is used, skb
+		 * can get reinjected right before nf_queue_hook_drop()
+		 */
+		nfq = nf_queue_nf_hook_drop(net);
+		if (nfq)
+			synchronize_net();
 
 		for (i = 0; i < n; i++)
 			kfree(to_free[i]);
diff --git a/net/netfilter/nf_internals.h b/net/netfilter/nf_internals.h
index c46d214d5323..bfa742da83af 100644
--- a/net/netfilter/nf_internals.h
+++ b/net/netfilter/nf_internals.h
@@ -14,7 +14,7 @@
 /* nf_queue.c */
 int nf_queue(struct sk_buff *skb, struct nf_hook_state *state,
 	     struct nf_hook_entry **entryp, unsigned int verdict);
-void nf_queue_nf_hook_drop(struct net *net, const struct nf_hook_entry *entry);
+unsigned int nf_queue_nf_hook_drop(struct net *net);
 int __init netfilter_queue_init(void);
 
 /* nf_log.c */
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 4a7662486f44..043850c9d154 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -96,15 +96,18 @@ void nf_queue_entry_get_refs(struct nf_queue_entry *entry)
 }
 EXPORT_SYMBOL_GPL(nf_queue_entry_get_refs);
 
-void nf_queue_nf_hook_drop(struct net *net, const struct nf_hook_entry *entry)
+unsigned int nf_queue_nf_hook_drop(struct net *net)
 {
 	const struct nf_queue_handler *qh;
+	unsigned int count = 0;
 
 	rcu_read_lock();
 	qh = rcu_dereference(net->nf.queue_handler);
 	if (qh)
-		qh->nf_hook_drop(net, entry);
+		count = qh->nf_hook_drop(net);
 	rcu_read_unlock();
+
+	return count;
 }
 
 static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index d09ab49e102a..dd8ec5b0fcd9 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -922,16 +922,10 @@ static struct notifier_block nfqnl_dev_notifier = {
 	.notifier_call	= nfqnl_rcv_dev_event,
 };
 
-static int nf_hook_cmp(struct nf_queue_entry *entry, unsigned long entry_ptr)
-{
-	return rcu_access_pointer(entry->hook) ==
-		(struct nf_hook_entry *)entry_ptr;
-}
-
-static void nfqnl_nf_hook_drop(struct net *net,
-			       const struct nf_hook_entry *hook)
+static unsigned int nfqnl_nf_hook_drop(struct net *net)
 {
 	struct nfnl_queue_net *q = nfnl_queue_pernet(net);
+	unsigned int instances = 0;
 	int i;
 
 	rcu_read_lock();
@@ -939,10 +933,14 @@ static void nfqnl_nf_hook_drop(struct net *net,
 		struct nfqnl_instance *inst;
 		struct hlist_head *head = &q->instance_table[i];
 
-		hlist_for_each_entry_rcu(inst, head, hlist)
-			nfqnl_flush(inst, nf_hook_cmp, (unsigned long)hook);
+		hlist_for_each_entry_rcu(inst, head, hlist) {
+			nfqnl_flush(inst, NULL, 0);
+			instances++;
+		}
 	}
 	rcu_read_unlock();
+
+	return instances;
 }
 
 static int
-- 
2.10.2

[PATCHo nf-next v2] netfilter: nf_log: don't call synchronize_rcu in nf_log_unset

[Florian Westphal]

nf_log_unregister() (which is what gets called in the logger backends
module exit paths) does a (required, module is removed) synchronize_rcu().

But nf_log_unset() is only called from pernet exit handlers. It doesn't
free any memory so there appears to be no need to call synchronize_rcu.

v2: Liping Zhang points out that nf_log_unregister() needs to be called
after pernet unregister, else rmmod would become unsafe.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_log.c        | 1 -
 net/netfilter/nfnetlink_log.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index cc32727e3f32..8bb152a7cca4 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -71,7 +71,6 @@ void nf_log_unset(struct net *net, const struct nf_logger *logger)
 			RCU_INIT_POINTER(net->nf.nf_loggers[i], NULL);
 	}
 	mutex_unlock(&nf_log_mutex);
-	synchronize_rcu();
 }
 EXPORT_SYMBOL(nf_log_unset);
 
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 896741206a50..da9704971a83 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -1140,10 +1140,10 @@ static int __init nfnetlink_log_init(void)
 
 static void __exit nfnetlink_log_fini(void)
 {
-	nf_log_unregister(&nfulnl_logger);
 	nfnetlink_subsys_unregister(&nfulnl_subsys);
 	netlink_unregister_notifier(&nfulnl_rtnl_notifier);
 	unregister_pernet_subsys(&nfnl_log_net_ops);
+	nf_log_unregister(&nfulnl_logger);
 }
 
 MODULE_DESCRIPTION("netfilter userspace logging");
-- 
2.10.2

Re: [PATCH nf-next 0/3] netfilter: speed up netns cleanup

[Pablo Neira Ayuso]

On Mon, Apr 24, 2017 at 03:37:38PM +0200, Florian Westphal wrote:
> low hanging fruits to speed up netns cleanup in netfilter.
> We're way too happy to issue expensive synchronize_rcu() all
> over the place.
> 
> On my test vm 8 processes doing 32 unshare each finish in ~3 minutes,
> with these patches it gets down to 40 seconds.

If this is a script, can we place this somewhere to keep an eye on
this?

> We could probably further improve this.

Series applied. I have taken v2 of 2/3.

Re: [PATCH nf-next 0/3] netfilter: speed up netns cleanup

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Apr 24, 2017 at 03:37:38PM +0200, Florian Westphal wrote:
> > low hanging fruits to speed up netns cleanup in netfilter.
> > We're way too happy to issue expensive synchronize_rcu() all
> > over the place.
> > 
> > On my test vm 8 processes doing 32 unshare each finish in ~3 minutes,
> > with these patches it gets down to 40 seconds.
> 
> If this is a script, can we place this somewhere to keep an eye on
> this?

It was based on Andreys sample program but this is enough actually
to show the difference:

time for x in $(seq 1 100); do unshare -n /bin/true;done