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

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

[Richard Weinberger]

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

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

[Richard Weinberger]

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

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

[Stephen Hemminger]

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

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

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

Am 03.01.2012 17:15, schrieb Stephen Hemminger:
> On Tue,  3 Jan 2012 14:26:04 +0100
> Richard Weinberger <richard@nod.at> 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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Bart De Schuymer]

Op 3/01/2012 18:42, Richard Weinberger schreef:
> Am 03.01.2012 17:15, schrieb Stephen Hemminger:
>> On Tue,  3 Jan 2012 14:26:04 +0100
>> Richard Weinberger<richard@nod.at>  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.
> 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

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

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Bart De Schuymer]

Op 3/01/2012 21:29, Richard Weinberger schreef:
> 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...

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

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

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]

Am 04.01.2012 18:55, schrieb Bart De Schuymer:
> Op 3/01/2012 21:29, Richard Weinberger schreef:
>> 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...
> 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Bart De Schuymer]

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

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

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 772 bytes --]

Am 05.01.2012 20:50, schrieb Bart De Schuymer:
> 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.
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]