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

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

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 <richard@nod.at>
---
 net/bridge/br_netfilter.c |   31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index fa8b8f7..f38a8e4 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c

@@ -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);

-- 
1.7.7.3

On Tue,  3 Jan 2012 14:26:04 +0100
Richard Weinberger [off-list ref] 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 <richard@nod.at>

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.

Am 03.01.2012 17:15, schrieb Stephen Hemminger:

On Tue,  3 Jan 2012 14:26:04 +0100
Richard Weinberger [off-list ref] wrote:

quoted

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 <richard@nod.at>

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...

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

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

Am 03.01.2012 17:15, schrieb Stephen Hemminger:

quoted

On Tue,  3 Jan 2012 14:26:04 +0100
Richard Weinberger[off-list ref]  wrote:

quoted

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<richard@nod.at>

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...

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


-- 
Bart De Schuymer
www.artinalgorithms.be

Am 03.01.2012 21:15, schrieb Bart De Schuymer:

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

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

Am 03.01.2012 21:15, schrieb Bart De Schuymer:

quoted

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...

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


-- 
Bart De Schuymer
www.artinalgorithms.be

Am 04.01.2012 18:55, schrieb Bart De Schuymer:

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

quoted

Am 03.01.2012 21:15, schrieb Bart De Schuymer:

quoted

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...

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...

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

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

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

-- 
Bart De Schuymer
www.artinalgorithms.be

Am 05.01.2012 20:50, schrieb Bart De Schuymer:

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

quoted

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.

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