[PATCH 1/2 v2 nf-next] net: bridge: don't register netfilter call_iptables hooks by default

Jesper reports that kernels with CONFIG_BRIDGE_NETFILTER=n show significantly
better performance vs. CONFIG_BRIDGE_NETFILTER=y, even with
bridge-nf-call-iptables=0.

This is because bridge registers some bridge netfilter hooks at
module load time, so the static key to bypass nf rule evaluation
via NF_HOOK() is false.

The hooks serve no purpose, unless iptables filtering for bridges is
desired (i.e., bridge-nf-call-*=1 and active iptables rules present).

The proper solution would be to just change the bridge-nf-call-iptables sysctl
default value to 0 and then register the hooks when user enables call-iptables
sysctl.  We cannot do that though since it breaks existing setups.

The next best solution is to delay registering of the hooks until
we know that

a) call-iptables sysctl is enabled (this is the default)
AND
b) ip(6)tables rules are loaded.

This adds br_nf_check_calliptables() helper in bridge input before
bridge 'prerouting' (sic) hooks to perform this check.

IOW, if user does not turn off call-iptables sysctl on the bridge, hook
registering is only done if NFPROTO_IPV4/IPV6 hooks are registered as
well once the first packet arrives on a bridge port.

Doing this check for every packet is still faster than registering
the hooks unconditionally.  To not add overhead for setups where
the call-iptables hooks are required, a static key shortcut is provided.

As its not possible to register hooks from bh context (grabs mutex)
its scheduled via workqueue.

Note that, to not make this overly complicated, the hooks are not
unregistered again when the sysctl is disabled later on.

If user toggles call-iptables sysctl to 0 before adding any ports
to the bridge, those hooks are not registered even if iptables rules
are loaded.

Daniel reports following results for super_netperf/200/TCP_RR:
CONFIG_BRIDGE_NETFILTER=n: ~988k TPS
CONFIG_BRIDGE_NETFILTER=y: ~971k TPS
CONFIG_BRIDGE_NETFILTER=y /w patch: ~988k TPS

This change can be removed once the default sysctl values
are changed to 0.

Reported-by: Jesper Dangaard Brouer <redacted>
Tested-by: Daniel Borkmann <redacted>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Changes since v1:
  - don't use mutex for synchronizing vs. work queue (Nik Alexandrov),
  instead use cmpxchg() to ensure only once cpu can register hook.
  - use bool return type since the -ERR values were not used

 net/bridge/br_input.c     | 39 +++++++++++++++++++++++++++++
 net/bridge/br_netfilter.c | 62 +++++++++++++++++++++++++++++++++++++----------
 net/bridge/br_private.h   | 15 ++++++++++++
 3 files changed, 103 insertions(+), 13 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 366c436..874d023 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c

@@ -18,6 +18,11 @@
 #include <linux/netfilter_bridge.h>
 #include <linux/export.h>
 #include <linux/rculist.h>
+
+#ifdef CONFIG_BRIDGE_NETFILTER
+#include <linux/jump_label.h>
+#endif
+
 #include "br_private.h"
 
 /* Hook for brouter */

@@ -153,6 +158,37 @@ static int br_handle_local_finish(struct sk_buff *skb)
 	return 0;	 /* process further */
 }
 
+static inline bool br_nf_check_calliptables(const struct net_bridge *br)
+{
+#ifdef CONFIG_BRIDGE_NETFILTER
+	bool ip, ip6, arp;
+	int i;
+
+	if (static_key_true(&brnf_hooks_active))
+		return true; /* ok, hooks registered */
+
+	ip  = brnf_call_iptables  || br->nf_call_iptables;
+	ip6 = brnf_call_ip6tables || br->nf_call_ip6tables;
+	arp = brnf_call_arptables || br->nf_call_arptables;
+
+	for (i=0; i < NF_MAX_HOOKS; i++) {
+		if (nf_hooks_active(NFPROTO_IPV4, i) && ip)
+			break;
+		if (nf_hooks_active(NFPROTO_IPV6, i) && ip6)
+			break;
+		if (nf_hooks_active(NFPROTO_ARP, i) && arp)
+			break;
+	}
+
+	/* rules active and nf_call_iptables is set, need to register
+	 * the required hooks.
+	 */
+	if (i < NF_MAX_HOOKS)
+		return br_netfiler_hooks_init();
+#endif
+	return true; /* also ok: hooks are not needed */
+}
+
 /*
  * Return NULL if skb is handled
  * note: already called with rcu_read_lock

@@ -176,6 +212,9 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
 
 	p = br_port_get_rcu(skb->dev);
 
+	if (!br_nf_check_calliptables(p->br))
+		goto drop;
+
 	if (unlikely(is_link_local_ether_addr(dest))) {
 		u16 fwd_mask = p->br->group_fwd_mask_required;
 

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index a615264..52c410f 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c

@@ -31,6 +31,8 @@
 #include <linux/netfilter_arp.h>
 #include <linux/in_route.h>
 #include <linux/inetdevice.h>
+#include <linux/workqueue.h>
+#include <linux/jump_label.h>
 
 #include <net/ip.h>
 #include <net/ipv6.h>

@@ -47,18 +49,19 @@
 #define store_orig_dstaddr(skb)	 (skb_origaddr(skb) = ip_hdr(skb)->daddr)
 #define dnat_took_place(skb)	 (skb_origaddr(skb) != ip_hdr(skb)->daddr)
 
+struct static_key brnf_hooks_active __read_mostly;
+static u32 brnf_hooks_loading __read_mostly;
+
 #ifdef CONFIG_SYSCTL
 static struct ctl_table_header *brnf_sysctl_header;
-static int brnf_call_iptables __read_mostly = 1;
-static int brnf_call_ip6tables __read_mostly = 1;
-static int brnf_call_arptables __read_mostly = 1;
 static int brnf_filter_vlan_tagged __read_mostly = 0;
 static int brnf_filter_pppoe_tagged __read_mostly = 0;
 static int brnf_pass_vlan_indev __read_mostly = 0;
+
+int brnf_call_iptables __read_mostly = 1;
+int brnf_call_ip6tables __read_mostly = 1;
+int brnf_call_arptables __read_mostly = 1;
 #else
-#define brnf_call_iptables 1
-#define brnf_call_ip6tables 1
-#define brnf_call_arptables 1
 #define brnf_filter_vlan_tagged 0
 #define brnf_filter_pppoe_tagged 0
 #define brnf_pass_vlan_indev 0

@@ -1059,6 +1062,39 @@ static struct ctl_table brnf_table[] = {
 };
 #endif
 
+static void __br_register_hooks_init(struct work_struct *w)
+{
+	int ret = nf_register_hooks(br_nf_ops, ARRAY_SIZE(br_nf_ops));
+
+	kfree(w);
+
+	if (ret) {
+		WARN_ONCE(ret, "could not register bridge netfilter hook (error %d)",
+										ret);
+		brnf_hooks_loading = 0;
+		return;
+	}
+	static_key_slow_inc(&brnf_hooks_active);
+}
+
+bool br_netfiler_hooks_init(void)
+{
+	struct work_struct *w;
+
+	if (cmpxchg(&brnf_hooks_loading, 0, 1) != 0)
+		return false; /* wq not finished */
+
+	w = kzalloc(sizeof(*w), GFP_ATOMIC);
+	if (!w) {
+		brnf_hooks_loading = 0;
+		return false;
+	}
+
+	INIT_WORK(w, __br_register_hooks_init);
+	schedule_work(w);
+	return false;
+}
+
 int __init br_netfilter_init(void)
 {
 	int ret;

@@ -1067,17 +1103,11 @@ int __init br_netfilter_init(void)
 	if (ret < 0)
 		return ret;
 
-	ret = nf_register_hooks(br_nf_ops, ARRAY_SIZE(br_nf_ops));
-	if (ret < 0) {
-		dst_entries_destroy(&fake_dst_ops);
-		return ret;
-	}
 #ifdef CONFIG_SYSCTL
 	brnf_sysctl_header = register_net_sysctl(&init_net, "net/bridge", brnf_table);
 	if (brnf_sysctl_header == NULL) {
 		printk(KERN_WARNING
 		       "br_netfilter: can't register to sysctl.\n");
-		nf_unregister_hooks(br_nf_ops, ARRAY_SIZE(br_nf_ops));
 		dst_entries_destroy(&fake_dst_ops);
 		return -ENOMEM;
 	}

@@ -1088,7 +1118,13 @@ int __init br_netfilter_init(void)
 
 void br_netfilter_fini(void)
 {
-	nf_unregister_hooks(br_nf_ops, ARRAY_SIZE(br_nf_ops));
+	while (brnf_hooks_loading && static_key_false(&brnf_hooks_active)) {
+		pr_info("waiting for hook register wq to finish");
+		schedule();
+	}
+	if (brnf_hooks_loading)
+		nf_unregister_hooks(br_nf_ops, ARRAY_SIZE(br_nf_ops));
+
 #ifdef CONFIG_SYSCTL
 	unregister_net_sysctl_table(brnf_sysctl_header);
 #endif

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 62a7fa2..7b39f0d 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h

@@ -426,6 +426,10 @@ void br_port_flags_change(struct net_bridge_port *port, unsigned long mask);
 void br_manage_promisc(struct net_bridge *br);
 
 /* br_input.c */
+#ifdef CONFIG_BRIDGE_NETFILTER
+extern struct static_key brnf_hooks_active __read_mostly;
+#endif
+
 int br_handle_frame_finish(struct sk_buff *skb);
 rx_handler_result_t br_handle_frame(struct sk_buff **pskb);
 

@@ -755,6 +759,17 @@ static inline int br_vlan_enabled(struct net_bridge *br)
 int br_netfilter_init(void);
 void br_netfilter_fini(void);
 void br_netfilter_rtable_init(struct net_bridge *);
+bool br_netfiler_hooks_init(void);
+
+#ifdef CONFIG_SYSCTL
+extern int brnf_call_iptables;
+extern int brnf_call_ip6tables;
+extern int brnf_call_arptables;
+#else
+#define brnf_call_iptables 1
+#define brnf_call_ip6tables 1
+#define brnf_call_arptables 1
+#endif
 #else
 #define br_netfilter_init()	(0)
 #define br_netfilter_fini()	do { } while (0)

-- 
1.8.1.5

add a warning and tell users to set call_iptables to 0 or 1
depending on desired behaviour before adding ports to the bridge.

In distant future, this might allow to disable call_iptables by default.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 no changes since v1.

 net/bridge/br_netfilter.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 52c410f..e0b581d 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c

@@ -58,9 +58,9 @@ static int brnf_filter_vlan_tagged __read_mostly = 0;
 static int brnf_filter_pppoe_tagged __read_mostly = 0;
 static int brnf_pass_vlan_indev __read_mostly = 0;
 
-int brnf_call_iptables __read_mostly = 1;
-int brnf_call_ip6tables __read_mostly = 1;
-int brnf_call_arptables __read_mostly = 1;
+int brnf_call_iptables __read_mostly = 2;
+int brnf_call_ip6tables __read_mostly = 2;
+int brnf_call_arptables __read_mostly = 2;
 #else
 #define brnf_filter_vlan_tagged 0
 #define brnf_filter_pppoe_tagged 0

@@ -1075,6 +1075,12 @@ static void __br_register_hooks_init(struct work_struct *w)
 		return;
 	}
 	static_key_slow_inc(&brnf_hooks_active);
+
+	if ((brnf_call_iptables|brnf_call_ip6tables|brnf_call_arptables) == 2)
+		pr_info("bridge: automatic filtering via arp/ip/ip6tables "
+			"is deprecated and it will be disabled soon. Set "
+			"bridge-nf-call-{ip,ip6,arp}tables sysctls to 1 or 0 "
+			"before adding bridge ports if you need this.\n");
 }
 
 bool br_netfiler_hooks_init(void)

-- 
1.8.1.5

Hi Florian,

On Wed, Sep 17, 2014 at 05:52:16PM +0200, Florian Westphal wrote:

Jesper reports that kernels with CONFIG_BRIDGE_NETFILTER=n show significantly
better performance vs. CONFIG_BRIDGE_NETFILTER=y, even with
bridge-nf-call-iptables=0.

This is because bridge registers some bridge netfilter hooks at
module load time, so the static key to bypass nf rule evaluation
via NF_HOOK() is false.

I'm attaching a patch to decouple bridge netfilter from the bridge
core.  The idea is to encapsulate the hook registration and most of
its code in a separated module: br_netfilter. The bridge core will
still request this module to be loaded from the initialization path,
I think this will retain backward compatibility for users.

I also guess most distributors will use CONFIG_BRIDGE_NETFILTER=m.
Then, users will get a warning message to let them know that they will
have to modprobe br_netfilter in the future if they need it, so we can
remove the deferred request_module from the br init path.

The patch is slightly larger than yours, and I can probably split it
in two patches (one to move several skb-nf-bridge related functions as
static inline to prepare the one that decouples br_netfilter from
bridge).

Unless I'm missing anything, I think br_netfilter should have been a
separated module since the beginning. The hook registration in other
netfilter modules is also ruled by rmmod/modprobe, so better if we
recover that path in this code.

I still have to check later again for typical kbuild robot reports on
Kconfig (bridge=y, br_netfilter=m) and so on, btw. I also may include
part of your original patch description if you're OK with it.

Please, let me know if I overlook anything. Thanks.

Pablo Neira Ayuso [off-list ref] wrote:

On Wed, Sep 17, 2014 at 05:52:16PM +0200, Florian Westphal wrote:

quoted

Jesper reports that kernels with CONFIG_BRIDGE_NETFILTER=n show significantly
better performance vs. CONFIG_BRIDGE_NETFILTER=y, even with
bridge-nf-call-iptables=0.

This is because bridge registers some bridge netfilter hooks at
module load time, so the static key to bypass nf rule evaluation
via NF_HOOK() is false.

I'm attaching a patch to decouple bridge netfilter from the bridge
core.  The idea is to encapsulate the hook registration and most of
its code in a separated module: br_netfilter.

Right, thats better.

I did not do this since it means modprobe bridge.ko will no longer
register the needed sysctls, and I deemed autoloading pointless
since that means the hooks are registered on bridge.ko load.

Also, it seems to be that we will have to wait forever to eventually
get rid of the autoload...

I also guess most distributors will use CONFIG_BRIDGE_NETFILTER=m.
Then, users will get a warning message to let them know that they will
have to modprobe br_netfilter in the future if they need it, so we can
remove the deferred request_module from the br init path.

Hmm, not sure if its safe to do this, e.g. with bridge=y,
br_netfilter=m, module might not yet be present in such cases?

Also, what about this:
iptables-restore < rules.txt
modprobe bridge
brctl addbr ...
brctl addif ...

at this point, any packet forwarded by bridge is filtered
via iptables.

After your patch, this might no longer be the case, if the modprobe
call is delayed (maybe this is far-fetched and not an issue in practice)?

2nd hypothetical issue:
modprobe bridge
sysctl net.bridge.bridge-nf-call-iptables=0

The sysctl could fail when br_nf_core is not yet present.

Unless I'm missing anything, I think br_netfilter should have been a
separated module since the beginning. The hook registration in other
netfilter modules is also ruled by rmmod/modprobe, so better if we
recover that path in this code.

Agreed.

Kconfig (bridge=y, br_netfilter=m) and so on, btw. I also may include
part of your original patch description if you're OK with it.

Sure, go ahead.

Please, let me know if I overlook anything. Thanks.

This patch modularizes br_netfilter so it can be
rmmod'ed, thus, the hooks can be unregistered.

I see.  Yes, that makes sense.

Another alternative would be to merge both approaches.
I.e, use my patch, but move all the hook registration to
your proposed br_nf_core module.

The sysctls would still be registered from bridge.ko.

Then, if call_iptables is set and iptables rules are active,
do the module autoload and print the warning from your patch, telling
people to modprobe br_nf_core if they want the filtering.

Unfortunately, we'd have to export (from bridge.ko) some hook
to allow br_nf_core to signal that the hooks have been registered.

Then, in two years or so, we would remove the autoload hook
in the packet procesing path.

The only other disadvantage that I see is that we'd still have
the bridge-nf-* sysctls in bridge.ko rather than the new glue module.

On Thu, Sep 18, 2014 at 04:31:08PM +0200, Florian Westphal wrote:

quoted

I also guess most distributors will use CONFIG_BRIDGE_NETFILTER=m.
Then, users will get a warning message to let them know that they will
have to modprobe br_netfilter in the future if they need it, so we can
remove the deferred request_module from the br init path.

Hmm, not sure if its safe to do this, e.g. with bridge=y,
br_netfilter=m, module might not yet be present in such cases?

Also, what about this:
iptables-restore < rules.txt
modprobe bridge
brctl addbr ...
brctl addif ...

at this point, any packet forwarded by bridge is filtered
via iptables.

After your patch, this might no longer be the case, if the modprobe
call is delayed (maybe this is far-fetched and not an issue in practice)?

Agreed, I think we can use your static key approach to drop traffic
from br_handle_frame after the work has called request_module.

2nd hypothetical issue:
modprobe bridge
sysctl net.bridge.bridge-nf-call-iptables=0

The sysctl could fail when br_nf_core is not yet present.

Indeed.

We can keep those sysctls there in bridge.ko and deprecated them in
favour of br_netlink. I think we can add some IFLA_BRPORT_NF for the
setlink command to replace the existing global proc nf-call-thing.
We'll have to enhance the 'bridge' tool in iproute2 to support this.
I can see this is packaged in testing already by debian at least.

Then, in two years or so, we would remove the autoload hook
in the packet procesing path.

Agreed. By that time, we can kill the bridge.ko <-> br_netfilter.ko
dependency and the /proc call-nf-bridge stuff in favour of br_netlink.

Let me know if you still have any concern. Thanks.

On 18. September 2014 15:35:52 MESZ, Pablo Neira Ayuso [off-list ref] wrote:

Unless I'm missing anything, I think br_netfilter should have been a
separated module since the beginning. 

Yeah absolutely. Basic rule: don't impose the costs of your cool new feature on all the people who will never need it.

Great someone finally fixes this up. Long term we still need a sane design for this though, some direct way of bridging to interact with conntrack, nftables provides for the rest.

On 18. September 2014 16:31:08 MESZ, Florian Westphal [off-list ref] wrote:

Pablo Neira Ayuso [off-list ref] wrote:

quoted

This patch modularizes br_netfilter so it can be
rmmod'ed, thus, the hooks can be unregistered.

I see.  Yes, that makes sense.

Another alternative would be to merge both approaches.
I.e, use my patch, but move all the hook registration to
your proposed br_nf_core module.

The sysctls would still be registered from bridge.ko.

Then, if call_iptables is set and iptables rules are active,
do the module autoload and print the warning from your patch, telling
people to modprobe br_nf_core if they want the filtering.

Unfortunately, we'd have to export (from bridge.ko) some hook
to allow br_nf_core to signal that the hooks have been registered.

Then, in two years or so, we would remove the autoload hook
in the packet procesing path.

I believe basically any transition which relies on people reading feature-removal-schedule or kernel messages and a flag day is just kidding ourselves and things will break for everyone except maybe some distributions at flag day.

I think taking compatibility seriously means we can't really change the default behaviour, but can only allow people to manually undo the damage and make sure they won't rely on this behaviour for at least nftables.

Or we can change it. But then why wait for two years, it won't change anything. 

Is there a way to make the userspace tools detect that the compat behaviour needs to be activated? 

Pablo Neira Ayuso [off-list ref] wrote:

We can keep those sysctls there in bridge.ko and deprecated them in
favour of br_netlink. I think we can add some IFLA_BRPORT_NF for the
setlink command to replace the existing global proc nf-call-thing.
We'll have to enhance the 'bridge' tool in iproute2 to support this.

Not sure this is needed.  Patrick added support for a per-bridge
sysfs flag a while back.

quoted

Then, in two years or so, we would remove the autoload hook
in the packet procesing path.

Agreed. By that time, we can kill the bridge.ko <-> br_netfilter.ko
dependency and the /proc call-nf-bridge stuff in favour of br_netlink.

Let me know if you still have any concern. Thanks.

Yes, well... I am afraid Patrick is right, we cannot remove it without
breaking some setups.

So, what to do?

I think the first step would be to go ahead and split the hook glue to
a separate module, and then add the autoprobing from the packet processing
path (only loading br_nf module when needed).

We could still add a deprecation warning later on (aka 'please modprobe
br_nf_core' instead).  But yes, Patricks probably right, waiting two
years doesn't make it more 'safe' than waiting 6 months...

What _might_ help is if there was an easy (and fast) way to tell wheter
iptables rules are loaded (right now it checks for active netfilter hooks).

Then, we could restrict the autoload to arp/ip(6)tables and not load
the br_nf glue module when nft is used instead.

That will allow removing autload with iptables removal.  Yes, I know
this is in a very distant future...