xt_physdev has no effect if net.bridge.bridge-nf-call-iptables=0

PATCH In review Richard Weinberger 3 participants 11 messages Jan 3, 2012 – Jan 5, 2012

Subsystems (4)

Networking Network drivers netfilter Ethernet bridge

lore

Share & tools

lore

Richard Weinberger

author

2012-01-03 01:29 UTC #0 lore raw

Hi!

If net.bridge.bridge-nf-call-iptables is set to zero (which is the default setting in Fedroa and RHEL6) xt_physdev has no effect.

A rule like this one will never match:
iptables -t nat -A PREROUTING -i bridge0 -m physdev --physdev-in eth0 -p tcp --dport 80 -j DNAT --to-destination :8080

IMHO the cause of the problem is in net/bridge/br_netfilter.c, br_nf_pre_routing() returns NF_ACCEPT before skb->nf_bridge is allocated and skb->nf_bridge->physindev set to skb->dev.

Thanks,
//richard

Richard Weinberger

author

2012-01-03 13:26 UTC #1 lore raw

Hi!

Here is a fix for the problem I've reported yesterday.
http://marc.info/?l=netfilter-devel&m=132555432331663&w=2

Please review the patch carefully, I'm not a br_netfilter ninja. 8-)

Thanks,
//richard

Richard Weinberger

author

2012-01-03 13:26 UTC #2 lore raw

PATCH 1/1 netfilter: Fix br_nf_pre_routing() in conjunction with bridge-nf-call-ip(6)tables=0

If net.bridge.bridge-nf-call-iptables or net.bridge.bridge-nf-call-ip6tables are set to zero xt_physdev has no effect because skb->nf_bridge has not been set up.

Signed-off-by Richard Weinberger <[email protected]>

---
 net/bridge/br_netfilter.c |   31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)

net/bridge/br_netfilter.c +23 -8

index fa8b8f7..f38a8e4 100644
@@ -576,10 +576,12 @@ static unsigned int br_nf_pre_routing_ipv6(unsigned int hook,
 					   struct sk_buff *skb,
 					   const struct net_device *in,
 					   const struct net_device *out,
-					   int (*okfn)(struct sk_buff *))
+					   int (*okfn)(struct sk_buff *),
+					   struct net_bridge *br)
 {
 	const struct ipv6hdr *hdr;
 	u32 pkt_len;
+	struct nf_bridge_info *nf_bridge;
 
 	if (skb->len < sizeof(struct ipv6hdr))
 		return NF_DROP;

@@ -606,6 +608,15 @@ static unsigned int br_nf_pre_routing_ipv6(unsigned int hook,
 	nf_bridge_put(skb->nf_bridge);
 	if (!nf_bridge_alloc(skb))
 		return NF_DROP;
+
+	if (!brnf_call_ip6tables && !br->nf_call_ip6tables) {
+		nf_bridge = skb->nf_bridge;
+		nf_bridge->mask |= BRNF_NF_BRIDGE_PREROUTING;
+		nf_bridge->physindev = skb->dev;
+
+		return NF_ACCEPT;
+	}
+
 	if (!setup_pre_routing(skb))
 		return NF_DROP;

@@ -629,6 +640,7 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,
 {
 	struct net_bridge_port *p;
 	struct net_bridge *br;
+	struct nf_bridge_info *nf_bridge;
 	__u32 len = nf_bridge_encap_header_len(skb);
 
 	if (unlikely(!pskb_may_pull(skb, len)))

@@ -641,16 +653,10 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,
 
 	if (skb->protocol == htons(ETH_P_IPV6) || IS_VLAN_IPV6(skb) ||
 	    IS_PPPOE_IPV6(skb)) {
-		if (!brnf_call_ip6tables && !br->nf_call_ip6tables)
-			return NF_ACCEPT;
-
 		nf_bridge_pull_encap_header_rcsum(skb);
-		return br_nf_pre_routing_ipv6(hook, skb, in, out, okfn);
+		return br_nf_pre_routing_ipv6(hook, skb, in, out, okfn, br);
 	}
 
-	if (!brnf_call_iptables && !br->nf_call_iptables)
-		return NF_ACCEPT;
-
 	if (skb->protocol != htons(ETH_P_IP) && !IS_VLAN_IP(skb) &&
 	    !IS_PPPOE_IP(skb))
 		return NF_ACCEPT;

@@ -663,6 +669,15 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,
 	nf_bridge_put(skb->nf_bridge);
 	if (!nf_bridge_alloc(skb))
 		return NF_DROP;
+
+	if (!brnf_call_iptables && !br->nf_call_iptables) {
+		nf_bridge = skb->nf_bridge;
+		nf_bridge->mask |= BRNF_NF_BRIDGE_PREROUTING;
+		nf_bridge->physindev = skb->dev;
+
+		return NF_ACCEPT;
+	}
+
 	if (!setup_pre_routing(skb))
 		return NF_DROP;
 	store_orig_dstaddr(skb);

Signature · 1 line

1.7.7.3

Stephen Hemminger

2012-01-03 16:15 UTC #3 lore raw

Re: [PATCH] netfilter: Fix br_nf_pre_routing() in conjunction with bridge-nf-call-ip(6)tables=0

Quoted 4 lines · On Tue, 3 Jan 2012 14:26:04 +0100 Richard Weinberger <[email protected]> wrote:

If net.bridge.bridge-nf-call-iptables or net.bridge.bridge-nf-call-ip6tables are set to zero xt_physdev has no effect because skb->nf_bridge has not been set up.
Signed-off-by Richard Weinberger <[email protected]>

I am not sure if this is a valid configuration. The setting of sysctl is saying "don't do iptables on bridge (since I won't be using it)" and then you are later doing iptables and expecting the settings as if the iptables setup was being done.

Instead, you should just enable the net.bridge.bridge-nf-call-iptables sysctl. If a distro chooses to disable it then you may have to do it explicitly.

Richard Weinberger

author

2012-01-03 17:42 UTC #4 lore raw

Re: [PATCH] netfilter: Fix br_nf_pre_routing() in conjunction with bridge-nf-call-ip(6)tables=0

Am 03.01.2012 17:15, schrieb Stephen Hemminger:

Quoted 6 lines

Quoted 3 lines · On Tue, 3 Jan 2012 14:26:04 +0100 Richard Weinberger <[email protected]> wrote:
If net.bridge.bridge-nf-call-iptables or net.bridge.bridge-nf-call-ip6tables are set to zero xt_physdev has no effect because skb->nf_bridge has not been set up.

Quoted 6 lines

Quoted 1 line
Signed-off-by Richard Weinberger <[email protected]>
I am not sure if this is a valid configuration. The setting of sysctl is saying "don't do iptables on bridge (since I won't be using it)" and then you are later doing iptables and expecting the settings as if the iptables setup was being done.

I don't think so.

Also rules like this one are broken:
iptables -A INPUT -i bridge0 -m physdev --physdev-in eth0 -j ...

No firewalling is done on the bridge, xt_physdev is only using some meta information.

At least a big fat warning would be nice that xt_physdev does not work if bridge-nf-call-iptables=0.
It took me some time to figure out why my firewall rule set gone nuts on RHEL6...

Quoted 2 lines

Instead, you should just enable the net.bridge.bridge-nf-call-iptables sysctl. If a distro chooses to disable it then you may have to do it explicitly.

Fedora and RHEL have net.bridge.bridge-nf-call-iptables=0 per default due to KVM network performance issues.
I'm sure I'm not the only user of xt_physdev on RHEL and friends.

Thanks,
//richard

Bart De Schuymer

2012-01-03 20:15 UTC #5 lore raw

Re: [PATCH] netfilter: Fix br_nf_pre_routing() in conjunction with bridge-nf-call-ip(6)tables=0

Op 3/01/2012 18:42, Richard Weinberger schreef:

Quoted 17 lines

Am 03.01.2012 17:15, schrieb Stephen Hemminger:
Quoted 11 lines
Quoted 4 lines · On Tue, 3 Jan 2012 14:26:04 +0100 Richard Weinberger<[email protected]> wrote:
If net.bridge.bridge-nf-call-iptables or net.bridge.bridge-nf-call-ip6tables are set to zero xt_physdev has no effect because skb->nf_bridge has not been set up.
Signed-off-by Richard Weinberger<[email protected]>
I am not sure if this is a valid configuration. The setting of sysctl is saying "don't do iptables on bridge (since I won't be using it)" and then you are later doing iptables and expecting the settings as if the iptables setup was being done.
I don't think so.
Also rules like this one are broken:
iptables -A INPUT -i bridge0 -m physdev --physdev-in eth0 -j ...

Quoted 7 lines

No firewalling is done on the bridge, xt_physdev is only using some meta information.
At least a big fat warning would be nice that xt_physdev does not work if bridge-nf-call-iptables=0.
It took me some time to figure out why my firewall rule set gone nuts on RHEL6...

The documentation is probably not explicit enough, but I would keep the behavior as it is now. Setting bridge-nf-call-iptables to 0 makes iptables behave as if bridge-netfilter was not enabled at compilation.
Anyway, your patch is almost certainly flawed since the fact that skb->nf_bridge can be NULL is used as part of the logic in br_netfilter.c: it indicates that bridge-nf-call-iptables was 0 when the packet was first processed by bridge-netfilter and should therefore not be given to iptables in any other netfilter hook.

cheers,
Bart

Signature · 2 lines

Bart De Schuymer
www.artinalgorithms.be

Richard Weinberger

author

2012-01-03 20:29 UTC #6 lore raw

Re: [PATCH] netfilter: Fix br_nf_pre_routing() in conjunction with bridge-nf-call-ip(6)tables=0

Am 03.01.2012 21:15, schrieb Bart De Schuymer:

Quoted 8 lines

The documentation is probably not explicit enough, but I would keep the behavior as it is now. Setting bridge-nf-call-iptables to 0 makes iptables behave as if bridge-netfilter was not enabled at compilation. Anyway, your patch is almost certainly flawed since the fact that skb->nf_bridge can be NULL is used as part of the logic in br_netfilter.c: it indicates that bridge-nf-call-iptables was 0 when the packet was first processed by bridge-netfilter and should therefore not be given to iptables in any other netfilter hook.

Thanks for the explanation!

Wouldn't it make sense to check for bridge-nf-call-iptables in xt_physdev? So that the user gets warned that his iptables rule will never match...

Thanks,
//richard

Bart De Schuymer

2012-01-04 17:55 UTC #7 lore raw

Re: [PATCH] netfilter: Fix br_nf_pre_routing() in conjunction with bridge-nf-call-ip(6)tables=0

Op 3/01/2012 21:29, Richard Weinberger schreef:

Quoted 9 lines

Am 03.01.2012 21:15, schrieb Bart De Schuymer:
Quoted 8 lines
The documentation is probably not explicit enough, but I would keep the behavior as it is now. Setting bridge-nf-call-iptables to 0 makes iptables behave as if bridge-netfilter was not enabled at compilation. Anyway, your patch is almost certainly flawed since the fact that skb->nf_bridge can be NULL is used as part of the logic in br_netfilter.c: it indicates that bridge-nf-call-iptables was 0 when the packet was first processed by bridge-netfilter and should therefore not be given to iptables in any other netfilter hook.

Quoted 4 lines

Thanks for the explanation!
Wouldn't it make sense to check for bridge-nf-call-iptables in xt_physdev? So that the user gets warned that his iptables rule will never match...

We don't want to introduce module dependencies between the bridge module and the iptables physdev match. We could add a message to the syslog whenever these proc settings are changed (in br_netfilter.c::brnf_sysctl_call_tables()).

cheers,
Bart

Signature · 2 lines

Bart De Schuymer
www.artinalgorithms.be

Richard Weinberger

author

2012-01-04 23:13 UTC #8 lore raw

Re: [PATCH] netfilter: Fix br_nf_pre_routing() in conjunction with bridge-nf-call-ip(6)tables=0

Am 04.01.2012 18:55, schrieb Bart De Schuymer:

Quoted 10 lines

Op 3/01/2012 21:29, Richard Weinberger schreef:
Quoted 9 lines
Am 03.01.2012 21:15, schrieb Bart De Schuymer:
Quoted 8 lines
The documentation is probably not explicit enough, but I would keep the behavior as it is now. Setting bridge-nf-call-iptables to 0 makes iptables behave as if bridge-netfilter was not enabled at compilation. Anyway, your patch is almost certainly flawed since the fact that skb->nf_bridge can be NULL is used as part of the logic in br_netfilter.c: it indicates that bridge-nf-call-iptables was 0 when the packet was first processed by bridge-netfilter and should therefore not be given to iptables in any other netfilter hook.

Quoted 8 lines

Quoted 5 lines
Thanks for the explanation!
Wouldn't it make sense to check for bridge-nf-call-iptables in xt_physdev?
So that the user gets warned that his iptables rule will never match...
We don't want to introduce module dependencies between the bridge module and the iptables physdev match.

CONFIG_NETFILTER_XT_MATCH_PHYSDEV depends anyway on CONFIG_BRIDGE_NETFILTER...

Quoted 3 lines

We could add a message to the syslog whenever these proc settings are changed (in br_netfilter.c::brnf_sysctl_call_tables()).

Let's export brnf_call_iptables and brnf_call_ip6tables, such that physdev_mt_check() can notify the user that his iptables rule will have no effect.

Thanks,
//richard

Bart De Schuymer

2012-01-05 19:50 UTC #9 lore raw

Re: [PATCH] netfilter: Fix br_nf_pre_routing() in conjunction with bridge-nf-call-ip(6)tables=0

Op 5/01/2012 0:13, Richard Weinberger schreef:

Quoted 5 lines

Let's export brnf_call_iptables and brnf_call_ip6tables, such that physdev_mt_check() can notify the user that his iptables rule will have no effect.

I don't want to introduce a runtime dependency between the iptables physdev module and the bridge module. This should keep working:
#modprobe bridge
#modprobe xt_physdev
#rmmod bridge
It will stop working if you use exported symbols of the bridge module in the physdev module.

Bart

Signature · 2 lines

Bart De Schuymer
www.artinalgorithms.be

Richard Weinberger

author

2012-01-05 19:54 UTC #10 lore raw

Re: [PATCH] netfilter: Fix br_nf_pre_routing() in conjunction with bridge-nf-call-ip(6)tables=0

Am 05.01.2012 20:50, schrieb Bart De Schuymer:

Quoted 7 lines

Op 5/01/2012 0:13, Richard Weinberger schreef:
Quoted 5 lines
Let's export brnf_call_iptables and brnf_call_ip6tables, such that physdev_mt_check() can notify the user that his iptables rule will have no effect.

Quoted 9 lines

I don't want to introduce a runtime dependency between the iptables physdev module and the bridge module.
This should keep working:
#modprobe bridge
#modprobe xt_physdev
#rmmod bridge
It will stop working if you use exported symbols of the bridge module in the physdev module.

IMHO this behavior would be useful. 8-)

Removing bridge while xt_physdev is loaded will make some netfilter rules void.
Which is not fun on a production firewall.

Thanks,
//richard