Bridge: do not defragment packets unless connection tracking is enabled

RFC PATCH Superseded Vasily Averin 4 participants 20 messages May 2, 2014 – May 7, 2014 2 messages not in the archive

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Vasily Averin

author

2014-05-02 15:42 UTC #1 lore raw

RFC PATCH 1/1 Bridge: do not defragment packets unless connection tracking is enabled

In reply to a message not in the archive

Dear Pablo, Florian,

could you please take look at patch below?
I've added per network namespace flag for manage of ipv4 defragmentation in bridge. In OpenVZ we can have enabled conntracks in some container and disabled in another ones. Seems this functionality was not merged into mainline yet, however I hope it will be done sooner or later.

I'm not sure about name of flag variable and about its location. I believe you can have better ideas about this.

Also I have one more question -- about defrag user check.

In my patch I use
if ((user >= IP_DEFRAG_CONNTRACK_BRIDGE_IN) &&

    (user <= __IP_DEFRAG_CONNTRACK_BRIDGE_IN))

because nf_ct_defrag_user can add zone.

I've found defrag user check in ip_expire() -- but it does not take account of zone.
Is it a bug in ip_expire() or I missed something?

---[patch rfc]---
Currently bridge can silently drop ipv4 fragments.
If node have loaded nf_defrag_ipv4 module but have no nf_conntrack_ipv4, br_nf_pre_routing defragments incoming ipv4 fragments, but skb->nfct check in br_nf_dev_queue_xmit does not allow to re-fragment combined packet back, and therefore it is dropped in br_dev_queue_push_xmit without incrementing of any failcounters.

According to Patrick McHardy, bridge should not defragment and fragment packets unless conntrack is enabled.

This patch adds per network namespace flag to manage ipv4 defragmentation in bridge.

Signed-off-by Vasily Averin <[email protected]>

---
 include/net/netns/conntrack.h                  |    1 +
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |    2 +
 net/ipv4/netfilter/nf_defrag_ipv4.c            |   39 ++++++++++++++++++++++-
 3 files changed, 41 insertions(+), 1 deletions(-)

include/net/netns/conntrack.h +1 -0

index 773cce3..7589937 100644
@@ -25,6 +25,7 @@ struct nf_proto_net {
 struct nf_generic_net {
 	struct nf_proto_net pn;
 	unsigned int timeout;
+	bool br_ipv4_defrag_disabled;
 };
 
 struct nf_tcp_net {

net/ipv4/netfilter/nf_defrag_ipv4.c +38 -1

index d807822..5f773d4 100644
@@ -87,6 +87,20 @@ static unsigned int ipv4_conntrack_defrag(const struct nf_hook_ops *ops,
 		enum ip_defrag_users user =
 			nf_ct_defrag_user(ops->hooknum, skb);
 
+#if IS_ENABLED(CONFIG_NF_CONNTRACK) && defined (CONFIG_BRIDGE_NETFILTER)
+		if ((user >= IP_DEFRAG_CONNTRACK_BRIDGE_IN) &&
+		    (user <= __IP_DEFRAG_CONNTRACK_BRIDGE_IN)) {
+#ifdef CONFIG_NET_NS
+			struct net *net = skb->sk->sk_net;
+#else
+			struct net *net = &init_net;
+#endif
+			/* A bridge should not defragment and fragment packets. 
+			   We only do it if connection tracking is enabled. */
+			if (net->ct.nf_ct_proto.generic.br_ipv4_defrag_disabled)
+				return NF_ACCEPT;
+		}
+#endif
 		if (nf_ct_ipv4_gather_frags(skb, user))
 			return NF_STOLEN;
 	}

@@ -110,14 +124,37 @@ static struct nf_hook_ops ipv4_defrag_ops[] = {
 	},
 };
 
+static int nf_defrag_ipv4_net_init(struct net *net)
+{
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
+	net->ct.nf_ct_proto.generic.br_ipv4_defrag_disabled = true;
+#endif
+	return 0;
+}
+
+static struct pernet_operations nf_defrag_ipv4_net_ops = {
+	.init = nf_defrag_ipv4_net_init,
+};
+
 static int __init nf_defrag_init(void)
 {
-	return nf_register_hooks(ipv4_defrag_ops, ARRAY_SIZE(ipv4_defrag_ops));
+	int ret = 0;
+
+	ret = register_pernet_subsys(&nf_defrag_ipv4_net_ops);
+	if (ret)
+		goto out;
+
+	ret = nf_register_hooks(ipv4_defrag_ops, ARRAY_SIZE(ipv4_defrag_ops));
+	if (ret)
+		unregister_pernet_subsys(&nf_defrag_ipv4_net_ops);
+out:
+	return ret;
 }
 
 static void __exit nf_defrag_fini(void)
 {
 	nf_unregister_hooks(ipv4_defrag_ops, ARRAY_SIZE(ipv4_defrag_ops));
+	unregister_pernet_subsys(&nf_defrag_ipv4_net_ops);
 }
 
 void nf_defrag_ipv4_enable(void)

Signature · 1 line

1.7.5.4

Florian Westphal

maintainer

2014-05-02 22:55 UTC #2 lore raw

Quoted 10 lines · Vasily Averin

Also I have one more question -- about defrag user check.
In my patch I use
if ((user >= IP_DEFRAG_CONNTRACK_BRIDGE_IN) &&
    (user <= __IP_DEFRAG_CONNTRACK_BRIDGE_IN))
because nf_ct_defrag_user can add zone.
I've found defrag user check in ip_expire() -- but it does not take account of zone.
Is it a bug in ip_expire() or I missed something?

Looks like a bug to me.

Quoted 8 lines

---[patch rfc]---
Currently bridge can silently drop ipv4 fragments.
If node have loaded nf_defrag_ipv4 module but have no nf_conntrack_ipv4, br_nf_pre_routing defragments incoming ipv4 fragments, but skb->nfct check in br_nf_dev_queue_xmit does not allow to re-fragment combined packet back, and therefore it is dropped in br_dev_queue_push_xmit without incrementing of any failcounters.

Quoted 7 lines

According to Patrick McHardy, bridge should not defragment and fragment packets unless conntrack is enabled.
This patch adds per network namespace flag to manage ipv4 defragmentation in bridge.
Signed-off-by Vasily Averin <[email protected]>

Are we sure this is required rather than just removing the skb->nfct test in br_nf_dev_queue_xmit() and be done with it?

Because that seems a lot saner to me, I fail to see how

if (skb->protocol == htons(ETH_P_IP) &&

           skb->len + nf_bridge_mtu_reduction(skb) >
		    skb->dev->mtu && !skb_is_gso(skb)) {

Would evaluate as 'true' without nf_defrag_ipv4 module loaded.

[ its from br_nf_dev_queue_xmit function ]

Quoted 19 lines

struct nf_generic_net {

include/net/netns/conntrack.h +1 -0

 	struct nf_proto_net pn;
 	unsigned int timeout;
+	bool br_ipv4_defrag_disabled;
 };
 
 struct nf_tcp_net {

net/ipv4/netfilter/nf_defrag_ipv4.c +4 -0

index d807822..5f773d4 100644
@@ -87,6 +87,20 @@ static unsigned int ipv4_conntrack_defrag(const struct nf_hook_ops *ops,
 		enum ip_defrag_users user =
 			nf_ct_defrag_user(ops->hooknum, skb);
 
+#if IS_ENABLED(CONFIG_NF_CONNTRACK) && defined (CONFIG_BRIDGE_NETFILTER)
+		if ((user >= IP_DEFRAG_CONNTRACK_BRIDGE_IN) &&
+		    (user <= __IP_DEFRAG_CONNTRACK_BRIDGE_IN)) {
+#ifdef CONFIG_NET_NS

net/ipv4/netfilter/nf_defrag_ipv4.c static unsigned int ipv4_conntrack_defrag(const struct nf_hook_ops *ops, · line 103

Quoted 10 lines

net/ipv4/netfilter/nf_defrag_ipv4.c +10 -0

+			struct net *net = skb->sk->sk_net;
+#else
+			struct net *net = &init_net;
+#endif
+			/* A bridge should not defragment and fragment packets. 
+			   We only do it if connection tracking is enabled. */
+			if (net->ct.nf_ct_proto.generic.br_ipv4_defrag_disabled)
+				return NF_ACCEPT;
+		}
+#endif

I wonder who would be responsible to set br_ipv4_defrag_disabled to false to enable conntracking on a bridge again?

Did i miss something in the patch?

Thanks.

Vasily Averin

author

2014-05-03 07:17 UTC #3 lore raw

Quoted 6 lines · Florian Westphal

Quoted 3 lines · Vasily Averin
I've found defrag user check in ip_expire() -- but it does not take account of zone.
Is it a bug in ip_expire() or I missed something?
Looks like a bug to me.

Thanks, I've submitted separated patch to fix it.

Quoted 8 lines

Quoted 3 lines
---[patch rfc]---
This patch adds per network namespace flag to manage ipv4 defragmentation in bridge.
Are we sure this is required rather than just removing the skb->nfct test in br_nf_dev_queue_xmit() and be done with it? Because that seems a lot saner to me, I fail to see how

Quoted 7 lines

if (skb->protocol == htons(ETH_P_IP) &&
           skb->len + nf_bridge_mtu_reduction(skb) >
		    skb->dev->mtu && !skb_is_gso(skb)) {
Would evaluate as 'true' without nf_defrag_ipv4 module loaded.
[ its from br_nf_dev_queue_xmit function ]

I think you are right, seems skb->nfct check will be extra anyway.

However my patch fixes wrong processing packets in bridge in case disabled conntracks. Probably Patrick can elaborate in more details why this is bad.

I have noticed only that currently bridge without conntracks can silently merge 2 small packets if their common size < mtu. Probably it can be unexpected for processing on destination side. However if we enable connection tracking -- we'll get the same situation again.

Quoted 4 lines

I wonder who would be responsible to set br_ipv4_defrag_disabled to false to enable conntracking on a bridge again?
Did i miss something in the patch?

I'm sorry, you are right, changes of nf_conntrack_ipv4 pernet_operations was lost I'll resend resend updated patch soon.

Vasily Averin

author

2014-05-03 07:20 UTC #4 lore raw

RFC PATCH v2 1/1 Bridge: do not defragment packets unless connection tracking is enabled

In reply to Florian Westphal

Currently bridge can silently drop ipv4 fragments.
If node have loaded nf_defrag_ipv4 module but have no nf_conntrack_ipv4, br_nf_pre_routing defragments incoming ipv4 fragments, but skb->nfct check in br_nf_dev_queue_xmit does not allow to re-fragment combined packet back, and therefore it is dropped in br_dev_queue_push_xmit without incrementing of any failcounters.

According to Patrick McHardy, bridge should not defragment and fragment packets unless conntrack is enabled.

This patch adds per network namespace flag to manage ipv4 defragmentation in bridge.

v2: added missed hooks: nf_conntrack pernet_operations hooks changes flag state

Signed-off-by Vasily Averin <[email protected]>

---
 include/net/netns/conntrack.h                  |    1 +
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |    2 +
 net/ipv4/netfilter/nf_defrag_ipv4.c            |   39 +++++++++++++++++++++++-
 3 files changed, 41 insertions(+), 1 deletions(-)

include/net/netns/conntrack.h +1 -0

index 773cce3..7589937 100644
@@ -25,6 +25,7 @@ struct nf_proto_net {
 struct nf_generic_net {
 	struct nf_proto_net pn;
 	unsigned int timeout;
+	bool br_ipv4_defrag_disabled;
 };
 
 struct nf_tcp_net {

net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c +2 -0

index 8127dc8..9ca9333 100644
@@ -432,6 +432,7 @@ static int ipv4_net_init(struct net *net)
 		pr_err("nf_conntrack_ipv4: pernet registration failed\n");
 		goto out_ipv4;
 	}
+	net->ct.nf_ct_proto.generic.br_ipv4_defrag_disabled = false;
 	return 0;
 out_ipv4:
 	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_icmp);

@@ -445,6 +446,7 @@ out_tcp:
 
 static void ipv4_net_exit(struct net *net)
 {
+	net->ct.nf_ct_proto.generic.br_ipv4_defrag_disabled = true;
 	nf_ct_l3proto_pernet_unregister(net, &nf_conntrack_l3proto_ipv4);
 	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_icmp);
 	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_udp4);

net/ipv4/netfilter/nf_defrag_ipv4.c +38 -1

index 12e13bd..0bd499b 100644
@@ -86,6 +86,20 @@ static unsigned int ipv4_conntrack_defrag(const struct nf_hook_ops *ops,
 		enum ip_defrag_users user =
 			nf_ct_defrag_user(ops->hooknum, skb);
 
+#if IS_ENABLED(CONFIG_NF_CONNTRACK) && defined (CONFIG_BRIDGE_NETFILTER)
+		if ((user >= IP_DEFRAG_CONNTRACK_BRIDGE_IN) &&
+		    (user <= __IP_DEFRAG_CONNTRACK_BRIDGE_IN)) {
+#ifdef CONFIG_NET_NS
+			struct net *net = skb->sk->sk_net;
+#else
+			struct net *net = &init_net;
+#endif
+			/* A bridge should not defragment and fragment packets. 
+			   We only do it if connection tracking is enabled. */
+			if (net->ct.nf_ct_proto.generic.br_ipv4_defrag_disabled)
+				return NF_ACCEPT;
+		}
+#endif
 		if (nf_ct_ipv4_gather_frags(skb, user))
 			return NF_STOLEN;
 	}

@@ -109,14 +123,37 @@ static struct nf_hook_ops ipv4_defrag_ops[] = {
 	},
 };
 
+static int nf_defrag_ipv4_net_init(struct net *net)
+{
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
+	net->ct.nf_ct_proto.generic.br_ipv4_defrag_disabled = true;
+#endif
+	return 0;
+}
+
+static struct pernet_operations nf_defrag_ipv4_net_ops = {
+	.init = nf_defrag_ipv4_net_init,
+};
+
 static int __init nf_defrag_init(void)
 {
-	return nf_register_hooks(ipv4_defrag_ops, ARRAY_SIZE(ipv4_defrag_ops));
+	int ret = 0;
+
+	ret = register_pernet_subsys(&nf_defrag_ipv4_net_ops);
+	if (ret)
+		goto out;
+
+	ret = nf_register_hooks(ipv4_defrag_ops, ARRAY_SIZE(ipv4_defrag_ops));
+	if (ret)
+		unregister_pernet_subsys(&nf_defrag_ipv4_net_ops);
+out:
+	return ret;
 }
 
 static void __exit nf_defrag_fini(void)
 {
 	nf_unregister_hooks(ipv4_defrag_ops, ARRAY_SIZE(ipv4_defrag_ops));
+	unregister_pernet_subsys(&nf_defrag_ipv4_net_ops);
 }
 
 void nf_defrag_ipv4_enable(void)

Signature · 1 line

1.7.5.4

Pablo Neira Ayuso

maintainer

2014-05-03 23:39 UTC #5 lore raw

In reply to Florian Westphal

On Sat, May 03, 2014 at 12:55:22AM +0200, Florian Westphal wrote:
[...]

Quoted 21 lines

Quoted 8 lines
---[patch rfc]---
Currently bridge can silently drop ipv4 fragments.
If node have loaded nf_defrag_ipv4 module but have no nf_conntrack_ipv4, br_nf_pre_routing defragments incoming ipv4 fragments, but skb->nfct check in br_nf_dev_queue_xmit does not allow to re-fragment combined packet back, and therefore it is dropped in br_dev_queue_push_xmit without incrementing of any failcounters.
Quoted 7 lines
According to Patrick McHardy, bridge should not defragment and fragment packets unless conntrack is enabled.
This patch adds per network namespace flag to manage ipv4 defragmentation in bridge.
Signed-off-by Vasily Averin <[email protected]>
Are we sure this is required rather than just removing the skb->nfct test in br_nf_dev_queue_xmit() and be done with it?
Because that seems a lot saner to me, I fail to see how

Quoted 7 lines

if (skb->protocol == htons(ETH_P_IP) &&
           skb->len + nf_bridge_mtu_reduction(skb) >
		    skb->dev->mtu && !skb_is_gso(skb)) {
Would evaluate as 'true' without nf_defrag_ipv4 module loaded.
[ its from br_nf_dev_queue_xmit function ]

I think we still may see IP packets larger than the mtu in that path. It would be a rare case since we need that the bridge has different (smaller) mtu than the sender, but still possible. The is_skb_forwardable() check in the current tree snapshot comes just a bit later, so if we remove that skb->nfct, the bridge will fragment large packets.

In general, I believe bridges should silently drop packets that are larger than the mtu and they should perform no fragmentation handling, no gathering and no [re]fragmentation. They are transparent devices that operate at layer 2.

The conntrack case is a special case that forces us to enable fragmentation handling since we get sort of a bridge that inspects layer 3 and 4 packet information. So we have sort of, let's call it, a mutant bridge.

We also have the tproxy target and the socket match, they seem to require defragmentation as well, I'm afraid the skb->nfct check will not help for those cases. I think that we need some counter to know how many clients we have that require the gathering + fragmentation code, so if we have at least one, we have to enable it.

Perhaps we can also display a message to inform the user that netfilter fragmentation handling is enabled.

Florian Westphal

maintainer

2014-05-04 00:23 UTC #6 lore raw

Quoted 27 lines · Pablo Neira Ayuso

Quoted 27 lines
Quoted 8 lines
---[patch rfc]---
Currently bridge can silently drop ipv4 fragments.
If node have loaded nf_defrag_ipv4 module but have no nf_conntrack_ipv4, br_nf_pre_routing defragments incoming ipv4 fragments, but skb->nfct check in br_nf_dev_queue_xmit does not allow to re-fragment combined packet back, and therefore it is dropped in br_dev_queue_push_xmit without incrementing of any failcounters.
Quoted 7 lines
According to Patrick McHardy, bridge should not defragment and fragment packets unless conntrack is enabled.
This patch adds per network namespace flag to manage ipv4 defragmentation in bridge.
Signed-off-by Vasily Averin <[email protected]>
Are we sure this is required rather than just removing the skb->nfct test in br_nf_dev_queue_xmit() and be done with it?
Because that seems a lot saner to me, I fail to see how
if (skb->protocol == htons(ETH_P_IP) &&
           skb->len + nf_bridge_mtu_reduction(skb) >
		    skb->dev->mtu && !skb_is_gso(skb)) {
Would evaluate as 'true' without nf_defrag_ipv4 module loaded.

Quoted 7 lines

Quoted 1 line
[ its from br_nf_dev_queue_xmit function ]
I think we still may see IP packets larger than the mtu in that path. It would be a rare case since we need that the bridge has different (smaller) mtu than the sender, but still possible. The is_skb_forwardable() check in the current tree snapshot comes just a bit later, so if we remove that skb->nfct, the bridge will fragment large packets.

I have to confess that I never tried that out; I assumed the nic would toss the over-mtu frame.

Quoted 4 lines

In general, I believe bridges should silently drop packets that are larger than the mtu and they should perform no fragmentation handling, no gathering and no [re]fragmentation. They are transparent devices that operate at layer 2.

Not sure I agree. Silent drops are bad (or perhaps I misunderstand you, if we do 'silent drop' in br_nf_dev_queue_xmit there should at least be a mib counter of some sort).

The last part stands of course, I agree that bridges should be transparent and not do frag handling etc.

Quoted 4 lines

The conntrack case is a special case that forces us to enable fragmentation handling since we get sort of a bridge that inspects layer 3 and 4 packet information. So we have sort of, let's call it, a mutant bridge.

Yes 8-/

Quoted 5 lines

We also have the tproxy target and the socket match, they seem to require defragmentation as well, I'm afraid the skb->nfct check will not help for those cases. I think that we need some counter to know how many clients we have that require the gathering + fragmentation code, so if we have at least one, we have to enable it.

Last time I tried TPROXY on top of bridge it was a pain in the neck.

Essentially one has to build a 'brouter' and force packets upwards the stack (DROP via ebtables in broute table).

Such packets will not be seen by the bridge since they're routed normally via the ip stack for local delivery.

(-j TPROXY needs policy routing for the redirect to work).

It is also rather fragile in my experience (due to ebtables just seeing ethernet frames doing 'broute DROP only for tcp port 80' doesn't work universally since we don't see netfilter-defragmented packets at that stage).

All things considered I think that just doing the re-fragmentation (aka just remove skb->nfct test) is really the least-sucky one of the options we have.

If you do IP NAT/TPROXY/conntrack on bridges you're already asking for varying degrees of layering violations, so I think it would at least be preferable to have one that "works" :-)

Pablo Neira Ayuso

maintainer

2014-05-04 11:15 UTC #7 lore raw

On Sun, May 04, 2014 at 02:23:17AM +0200, Florian Westphal wrote:
[...]

Quoted 11 lines

Quoted 5 lines
We also have the tproxy target and the socket match, they seem to require defragmentation as well, I'm afraid the skb->nfct check will not help for those cases. I think that we need some counter to know how many clients we have that require the gathering + fragmentation code, so if we have at least one, we have to enable it.
Last time I tried TPROXY on top of bridge it was a pain in the neck.
Essentially one has to build a 'brouter' and force packets upwards the stack (DROP via ebtables in broute table).

Quoted 8 lines

Such packets will not be seen by the bridge since they're routed normally via the ip stack for local delivery.
(-j TPROXY needs policy routing for the redirect to work).
It is also rather fragile in my experience (due to ebtables just seeing ethernet frames doing 'broute DROP only for tcp port 80' doesn't work universally since we don't see netfilter-defragmented packets at that stage).

All those bridge-nf-call-* were quite a hack IMO, I don't think this is the only extension with problems.

Quoted 3 lines

All things considered I think that just doing the re-fragmentation (aka just remove skb->nfct test) is really the least-sucky one of the options we have.

I see, and I think it's reasonable to assume that if nf_defrag_* is loaded, the user expects that its bridge may fragment traffic. OK, let's remove the skb->nfct check there.

Quoted 3 lines

If you do IP NAT/TPROXY/conntrack on bridges you're already asking for varying degrees of layering violations, so I think it would at least be preferable to have one that "works" :-)

Perhaps it would be good to restrict extensions that we know that don't work/have severe limitations to iptables/ip6tables, at least those that we really know that don't work or need some more bits to get them working.

Bart De Schuymer

2014-05-04 20:06 UTC #8 lore raw

In reply to Pablo Neira Ayuso

Pablo Neira Ayuso schreef op 4/05/2014 1:39:

Quoted 10 lines

I think we still may see IP packets larger than the mtu in that path. It would be a rare case since we need that the bridge has different (smaller) mtu than the sender, but still possible. The is_skb_forwardable() check in the current tree snapshot comes just a bit later, so if we remove that skb->nfct, the bridge will fragment large packets.
In general, I believe bridges should silently drop packets that are larger than the mtu and they should perform no fragmentation handling, no gathering and no [re]fragmentation. They are transparent devices that operate at layer 2.

I agree. I don't think it's a good idea to commit code that would do fragmentation of IP packets that weren't defragmented first.

Quoted 10 lines

The conntrack case is a special case that forces us to enable fragmentation handling since we get sort of a bridge that inspects layer 3 and 4 packet information. So we have sort of, let's call it, a mutant bridge.
We also have the tproxy target and the socket match, they seem to require defragmentation as well, I'm afraid the skb->nfct check will not help for those cases. I think that we need some counter to know how many clients we have that require the gathering + fragmentation code, so if we have at least one, we have to enable it.

If I understood Vasily correctly, in his setup ip_defrag is being called from code that isn't connection tracking. Glancing at the code, at least IP virtual server and the code that handles the router attention IP option also call ip_defrag.

Isn't there an easy way to see that the skb contains a defragmented IP packet? If there were, then it seems replacing the "skb->nfct != NULL" by "is_defragmented(skb)" would suffice, no? I see no reason to artificially restrict defrag/refrag to connection tracking.

cheers,

Bart

Pablo Neira Ayuso

maintainer

2014-05-04 23:01 UTC #9 lore raw

Quoted 8 lines · Bart De Schuymer

If I understood Vasily correctly, in his setup ip_defrag is being called from code that isn't connection tracking. Glancing at the code, at least IP virtual server and the code that handles the router attention IP option also call ip_defrag.
Isn't there an easy way to see that the skb contains a defragmented IP packet? If there were, then it seems replacing the "skb->nfct != NULL" by "is_defragmented(skb)" would suffice, no?

We didn't find any way that a packet larger than the mtu can hit that code. The (re-)fragmentation only applies to a skb that fulfills skb_has_frag_list(), so no need to restrict it.

Quoted 2 lines

I see no reason to artificially restrict defrag/refrag to connection tracking.

After Vasily's patch, fragmentation/defragmentation on a bridge will basically depend on if nf_defrag_ipv4 is loaded or not.

Vasily Averin

author

2014-05-05 12:57 UTC #10 lore raw

cover users counter to manage ipv4 defragmentation on bridge

In reply to Pablo Neira Ayuso

Pablo,

I've implemented your wishes, could you please review my new patch set?

Quoted 10 lines

In general, I believe bridges should silently drop packets that are larger than the mtu and they should perform no fragmentation handling, no gathering and no [re]fragmentation. They are transparent devices that operate at layer 2.
The conntrack case is a special case that forces us to enable fragmentation handling since we get sort of a bridge that inspects layer 3 and 4 packet information. So we have sort of, let's call it, a mutant bridge.

Quoted 8 lines

We also have the tproxy target and the socket match, they seem to require defragmentation as well, I'm afraid the skb->nfct check will not help for those cases. I think that we need some counter to know how many clients we have that require the gathering + fragmentation code, so if we have at least one, we have to enable it.
Perhaps we can also display a message to inform the user that netfilter fragmentation handling is enabled.

For nf_conntrack_ipv4 I increment counter once only,
For TPROXY target and socket match I increment counter on checkentry and decrement on destroy hook. So if these modules are just loaded but are not used in net namespace, they will not affect ipv4 defragmentation.

Please let me know if you have some better ideas.

Vasily Averin (7):
nf: added per net namespace ipv4 defragmentation users counter
nf: initialization of ipv4 defragmentation users counter
nf: increment and decrement for ipv4 defragmentation users counter
nf: ipv4 defragmentation users counter changes in nf_conntrack_ipv4
nf: ipv4 defragmentation users counter changes in TPROXY target
nf: ipv4 defragmentation users counter changes in xt_socket match
nf: use counter to manage ipv4 defragmentation on bridge

 include/net/net_namespace.h                    |    3 ++
 include/net/netfilter/ipv4/nf_defrag_ipv4.h    |   13 +++++++
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |    2 +
 net/ipv4/netfilter/nf_defrag_ipv4.c            |   46 +++++++++++++++++++++++-
 net/netfilter/xt_TPROXY.c                      |   11 +++++-
 net/netfilter/xt_socket.c                      |   19 ++++++++++
 6 files changed, 92 insertions(+), 2 deletions(-)

Signature · 1 line

1.7.5.4

Vasily Averin

author

2014-05-05 12:57 UTC #15 lore raw

PATCH 1/7 nf: added per net namespace ipv4 defragmentation users counter

In reply to a message not in the archive

Signed-off-by Vasily Averin <[email protected]>

---
 include/net/net_namespace.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

include/net/net_namespace.h +3 -0

index 5f9eb26..d82e4b2 100644
@@ -113,6 +113,9 @@ struct net {
 #if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
 	struct netns_nf_frag	nf_frag;
 #endif
+#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV4)
+	atomic_t		br_defrag_ipv4_users_count;
+#endif
 	struct sock		*nfnl;
 	struct sock		*nfnl_stash;
 #endif

Signature · 1 line

1.7.5.4

Vasily Averin

author

2014-05-05 12:57 UTC #16 lore raw

PATCH 2/7 nf: initialization of ipv4 defragmentation users counter

In reply to a message not in the archive

Signed-off-by Vasily Averin <[email protected]>

---
 net/ipv4/netfilter/nf_defrag_ipv4.c |   32 +++++++++++++++++++++++++++++++-
 1 files changed, 31 insertions(+), 1 deletions(-)

net/ipv4/netfilter/nf_defrag_ipv4.c +31 -1

index f40f321..f82685c 100644
@@ -110,14 +110,44 @@ static struct nf_hook_ops ipv4_defrag_ops[] = {
 	},
 };
 
+static int nf_defrag_ipv4_net_init(struct net *net)
+{
+	atomic_set(&net->br_defrag_ipv4_users_count, 0);
+	return 0;
+}
+
+static void nf_defrag_ipv4_net_exit(struct net *net)
+{
+	if (unlikely(atomic_read(&net->br_defrag_ipv4_users_count) != 0)) {
+		pr_warn("net %p cleanup: br_defrag_users_count = %d\n",
+			net, atomic_read(&net->br_defrag_ipv4_users_count));
+	}
+}
+
+static struct pernet_operations nf_defrag_ipv4_net_ops = {
+	.init = nf_defrag_ipv4_net_init,
+	.exit = nf_defrag_ipv4_net_exit,
+};
+
 static int __init nf_defrag_init(void)
 {
-	return nf_register_hooks(ipv4_defrag_ops, ARRAY_SIZE(ipv4_defrag_ops));
+	int ret = 0;
+
+	ret = register_pernet_subsys(&nf_defrag_ipv4_net_ops);
+	if (ret)
+		goto out;
+
+	ret = nf_register_hooks(ipv4_defrag_ops, ARRAY_SIZE(ipv4_defrag_ops));
+	if (ret)
+		unregister_pernet_subsys(&nf_defrag_ipv4_net_ops);
+out:
+	return ret;
 }
 
 static void __exit nf_defrag_fini(void)
 {
 	nf_unregister_hooks(ipv4_defrag_ops, ARRAY_SIZE(ipv4_defrag_ops));
+	unregister_pernet_subsys(&nf_defrag_ipv4_net_ops);
 }
 
 void nf_defrag_ipv4_enable(void)

Signature · 1 line

1.7.5.4

Vasily Averin

author

2014-05-05 12:57 UTC #17 lore raw

PATCH 3/7 nf: increment and decrement functions for ipv4 defragmentation users counter

In reply to a message not in the archive

Signed-off-by Vasily Averin <[email protected]>

---
 include/net/netfilter/ipv4/nf_defrag_ipv4.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/net/netfilter/ipv4/nf_defrag_ipv4.h +13 -0

index f01ef20..2a433ba 100644
@@ -3,4 +3,17 @@
 
 void nf_defrag_ipv4_enable(void);
 
+static inline void inc_br_defrag_ipv4_users_count(struct net *net)
+{
+	if (atomic_inc_return(&net->br_defrag_ipv4_users_count) == 1)
+		pr_info("net %p: bridge ipv4 defragmentation is enabled\n",
+			net);
+}
+
+static inline void dec_br_defrag_ipv4_users_count(struct net *net)
+{
+	if (atomic_dec_and_test(&net->br_defrag_ipv4_users_count))
+		pr_info("net %p: bridge ipv4 defragmentation is disabled\n",
+			net);
+}
 #endif /* _NF_DEFRAG_IPV4_H */

Signature · 1 line

1.7.5.4

Vasily Averin

author

2014-05-05 12:57 UTC #18 lore raw

PATCH 4/7 nf: ipv4 defragmentation users counter changes in nf_conntrack_ipv4 module

In reply to a message not in the archive

Signed-off-by Vasily Averin <[email protected]>

---
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c +2 -0

index 8127dc8..1376055 100644
@@ -432,6 +432,7 @@ static int ipv4_net_init(struct net *net)
 		pr_err("nf_conntrack_ipv4: pernet registration failed\n");
 		goto out_ipv4;
 	}
+	inc_br_defrag_ipv4_users_count(net);
 	return 0;
 out_ipv4:
 	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_icmp);

@@ -445,6 +446,7 @@ out_tcp:
 
 static void ipv4_net_exit(struct net *net)
 {
+	dec_br_defrag_ipv4_users_count(net);
 	nf_ct_l3proto_pernet_unregister(net, &nf_conntrack_l3proto_ipv4);
 	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_icmp);
 	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_udp4);

Signature · 1 line

1.7.5.4

Vasily Averin

author

2014-05-05 12:57 UTC #19 lore raw

PATCH 5/7 nf: ipv4 defragmentation users counter changes in TPROXY target

In reply to a message not in the archive

Signed-off-by Vasily Averin <[email protected]>

---
 net/netfilter/xt_TPROXY.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

net/netfilter/xt_TPROXY.c +10 -1

index ef8a926..ce99ad4 100644
@@ -528,14 +528,21 @@ static int tproxy_tg4_check(const struct xt_tgchk_param *par)
 	const struct ipt_ip *i = par->entryinfo;
 
 	if ((i->proto == IPPROTO_TCP || i->proto == IPPROTO_UDP)
-	    && !(i->invflags & IPT_INV_PROTO))
+	    && !(i->invflags & IPT_INV_PROTO)) {
+		inc_br_defrag_ipv4_users_count(par->net);
 		return 0;
+	}
 
 	pr_info("Can be used only in combination with "
 		"either -p tcp or -p udp\n");
 	return -EINVAL;
 }
 
+void tproxy_tg4_destroy(const struct xt_tgdtor_param *par)
+{
+	dec_br_defrag_ipv4_users_count(par->net);
+}
+
 static struct xt_target tproxy_tg_reg[] __read_mostly = {
 	{
 		.name		= "TPROXY",

@@ -545,6 +552,7 @@ static struct xt_target tproxy_tg_reg[] __read_mostly = {
 		.revision	= 0,
 		.targetsize	= sizeof(struct xt_tproxy_target_info),
 		.checkentry	= tproxy_tg4_check,
+		.destroy	= tproxy_tg4_destroy,
 		.hooks		= 1 << NF_INET_PRE_ROUTING,
 		.me		= THIS_MODULE,
 	},

@@ -556,6 +564,7 @@ static struct xt_target tproxy_tg_reg[] __read_mostly = {
 		.revision	= 1,
 		.targetsize	= sizeof(struct xt_tproxy_target_info_v1),
 		.checkentry	= tproxy_tg4_check,
+		.destroy	= tproxy_tg4_destroy,
 		.hooks		= 1 << NF_INET_PRE_ROUTING,
 		.me		= THIS_MODULE,
 	},

Signature · 1 line

1.7.5.4

Vasily Averin

author

2014-05-05 12:58 UTC #20 lore raw

PATCH 6/7 nf: ipv4 defragmentation users counter changes in xt_socket match

In reply to a message not in the archive

Signed-off-by Vasily Averin <[email protected]>

---
 net/netfilter/xt_socket.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

net/netfilter/xt_socket.c +19 -0

index 1ba6793..982bd58 100644
@@ -388,6 +388,12 @@ socket_mt6_v1_v2(const struct sk_buff *skb, struct xt_action_param *par)
 }
 #endif
 
+static int socket_mt4_v0_check(const struct xt_mtchk_param *par)
+{
+	inc_br_defrag_ipv4_users_count(par->net);
+	return 0;
+}
+
 static int socket_mt_v1_check(const struct xt_mtchk_param *par)
 {
 	const struct xt_socket_mtinfo1 *info = (struct xt_socket_mtinfo1 *) par->matchinfo;

@@ -396,6 +402,8 @@ static int socket_mt_v1_check(const struct xt_mtchk_param *par)
 		pr_info("unknown flags 0x%x\n", info->flags & ~XT_SOCKET_FLAGS_V1);
 		return -EINVAL;
 	}
+	if (par->family == NFPROTO_IPV4)
+		inc_br_defrag_ipv4_users_count(par->net);
 	return 0;
 }

@@ -407,15 +415,24 @@ static int socket_mt_v2_check(const struct xt_mtchk_param *par)
 		pr_info("unknown flags 0x%x\n", info->flags & ~XT_SOCKET_FLAGS_V2);
 		return -EINVAL;
 	}
+	if (par->family == NFPROTO_IPV4)
+		inc_br_defrag_ipv4_users_count(par->net);
 	return 0;
 }
 
+static void socket_mt4_destroy(const struct xt_mtdtor_param *par)
+{
+	dec_br_defrag_ipv4_users_count(par->net);
+}
+
 static struct xt_match socket_mt_reg[] __read_mostly = {
 	{
 		.name		= "socket",
 		.revision	= 0,
 		.family		= NFPROTO_IPV4,
 		.match		= socket_mt4_v0,
+		.checkentry	= socket_mt4_v0_check,
+		.destroy	= socket_mt4_destroy,
 		.hooks		= (1 << NF_INET_PRE_ROUTING) |
 				  (1 << NF_INET_LOCAL_IN),
 		.me		= THIS_MODULE,

@@ -426,6 +443,7 @@ static struct xt_match socket_mt_reg[] __read_mostly = {
 		.family		= NFPROTO_IPV4,
 		.match		= socket_mt4_v1_v2,
 		.checkentry	= socket_mt_v1_check,
+		.destroy	= socket_mt4_destroy,
 		.matchsize	= sizeof(struct xt_socket_mtinfo1),
 		.hooks		= (1 << NF_INET_PRE_ROUTING) |
 				  (1 << NF_INET_LOCAL_IN),

@@ -450,6 +468,7 @@ static struct xt_match socket_mt_reg[] __read_mostly = {
 		.family		= NFPROTO_IPV4,
 		.match		= socket_mt4_v1_v2,
 		.checkentry	= socket_mt_v2_check,
+		.destroy	= socket_mt4_destroy,
 		.matchsize	= sizeof(struct xt_socket_mtinfo1),
 		.hooks		= (1 << NF_INET_PRE_ROUTING) |
 				  (1 << NF_INET_LOCAL_IN),

Signature · 1 line

1.7.5.4

Vasily Averin

author

2014-05-05 12:58 UTC #21 lore raw

PATCH 7/7 nf: use counter to manage ipv4 defragmentation on bridge

In reply to a message not in the archive

Signed-off-by Vasily Averin <[email protected]>

---
 net/ipv4/netfilter/nf_defrag_ipv4.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

net/ipv4/netfilter/nf_defrag_ipv4.c +14 -0

index f82685c..40cbd05 100644
@@ -87,6 +87,20 @@ static unsigned int ipv4_conntrack_defrag(const struct nf_hook_ops *ops,
 		enum ip_defrag_users user =
 			nf_ct_defrag_user(ops->hooknum, skb);
 
+#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV4) && defined (CONFIG_BRIDGE_NETFILTER)
+		if ((user >= IP_DEFRAG_CONNTRACK_BRIDGE_IN) &&
+		    (user <= __IP_DEFRAG_CONNTRACK_BRIDGE_IN)) {
+			struct net *net = sock_net(skb->sk);
+
+			/* A bridge should not defragment and fragment packets. 
+			 * However if connection tracking is enabled or
+			 * if some target (TPROXY) or matches (socket) are used 
+			 * we enable ipv4 defragmentation on bridge
+			 */
+			if (atomic_read(&net->br_defrag_ipv4_users_count) == 0)
+				return NF_ACCEPT;
+		}
+#endif
 		if (nf_ct_ipv4_gather_frags(skb, user))
 			return NF_STOLEN;
 	}

Signature · 1 line

1.7.5.4

Florian Westphal

maintainer

2014-05-05 20:58 UTC #11 lore raw

Re: [PATCH RFC 0/7] users counter to manage ipv4 defragmentation on bridge

In reply to Vasily Averin

Quoted 5 lines · Vasily Averin

For nf_conntrack_ipv4 I increment counter once only,
For TPROXY target and socket match I increment counter on checkentry and decrement on destroy hook. So if these modules are just loaded but are not used in net namespace, they will not affect ipv4 defragmentation. Please let me know if you have some better ideas.

bridges defrag packets (if the nf_defrag_ipv4 is loaded) because brnf_call_iptables sysctl is set to 1 by default.

What about making this sysctl per-netns?

That way a bridge running inside a netns could disable iptables processing, it seems to be global switch at this time.

This way you could not enable iptables processing on a bridge without defrag enabled (again, if the module is loaded), OTOH I don't see why one would want iptables on a bridge without conntrack (might as well just use ebtables).

Vasily Averin

author

2014-05-07 13:29 UTC #12 lore raw

Re: [PATCH RFC 0/7] users counter to manage ipv4 defragmentation on bridge

Quoted 11 lines · Florian Westphal

Quoted 5 lines · Vasily Averin
For nf_conntrack_ipv4 I increment counter once only,
For TPROXY target and socket match I increment counter on checkentry and decrement on destroy hook. So if these modules are just loaded but are not used in net namespace, they will not affect ipv4 defragmentation. Please let me know if you have some better ideas.
bridges defrag packets (if the nf_defrag_ipv4 is loaded) because brnf_call_iptables sysctl is set to 1 by default.
What about making this sysctl per-netns?

I think it is great idea,
I'm agree it's much better than my patch set.

However, could anybody explain,
if nobody likes bridge-netfilters, why according sysctls are enabled in kernel by default? I've found in RHEL6 tries to disable them via /etc/sysctl.conf
however it doesn't work when bridge module is loaded after applying settings saved in sysctl.conf

Thank you,
Vasily Averin

Bart De Schuymer

2014-05-07 18:50 UTC #13 lore raw

Re: [PATCH RFC 0/7] users counter to manage ipv4 defragmentation on bridge

Vasily Averin schreef op 7/05/2014 15:27:

Quoted 8 lines · Florian Westphal

Quoted 7 lines
Quoted 5 lines · Vasily Averin
For nf_conntrack_ipv4 I increment counter once only,
For TPROXY target and socket match I increment counter on checkentry and decrement on destroy hook. So if these modules are just loaded but are not used in net namespace, they will not affect ipv4 defragmentation. Please let me know if you have some better ideas.

Quoted 7 lines

Quoted 4 lines
bridges defrag packets (if the nf_defrag_ipv4 is loaded) because brnf_call_iptables sysctl is set to 1 by default.
What about making this sysctl per-netns?
I think it is great idea,
I'm agree it's much better than my patch set.

No objections from me.

Quoted 2 lines

However, could anybody explain,
if nobody likes bridge-netfilters, why according sysctls are enabled in kernel by default?

If nobody likes it, it's quite simple: don't enable bridge-nf when configuring the kernel. Here are the reasons why the defaults are set to 1:
- Backwards compatibility (the sysctl options were added later)
- Default behaviour should be independent of sysctl being enabled or not
- If someone compiles bridge-nf into the kernel, one might expect that it's intended to be used.