: [NETFILTER]: Defer fragmentation in ip_output when connection tracking is used

PATCH In review Patrick McHardy 3 participants 7 messages Nov 11, 2005 – Nov 22, 2005

Subsystems (4)

Networking Network drivers netfilter IPv4/IPv6

lore

Share & tools

lore

Patrick McHardy

author

2005-11-11 03:19 UTC #0 lore raw

PATCH 2/10 : [NETFILTER]: Defer fragmentation in ip_output when connection tracking is used

[NETFILTER]: Defer fragmentation in ip_output when connection tracking is used

This allows to get rid of the okfn use in ip_refrag and save the useless fragmentation/defragmentation step when NAT is used.

Signed-off-by Patrick McHardy <[email protected]>

---
commit dc924f62fd0611bb349e398c80ce3ffa5c3a9025
tree 7eb988fef33fa008d1a64fceaee0b310bba30162
parent e718fcad3d24ada7b56fdca8dd234c9ba7459219
author Patrick McHardy <[email protected]> Fri, 11 Nov 2005 02:10:19 +0100 committer Patrick McHardy <[email protected]> Fri, 11 Nov 2005 02:10:19 +0100

 include/linux/netfilter.h                    |    6 ++++++
 include/net/ip.h                             |    1 -
 net/ipv4/ip_output.c                         |   18 ++++++++++++++---
 net/ipv4/netfilter/ip_conntrack_standalone.c |   27 +-------------------------
 net/ipv4/netfilter/ip_nat_standalone.c       |   19 ++----------------
 5 files changed, 24 insertions(+), 47 deletions(-)

include/linux/netfilter.h +6 -0

index be365e7..4c89c2c 100644
@@ -261,6 +261,11 @@ struct nf_queue_rerouter {
 extern int nf_register_queue_rerouter(int pf, struct nf_queue_rerouter *rer);
 extern int nf_unregister_queue_rerouter(int pf);
 
+static inline int nf_defer_fragment(struct sk_buff *skb)
+{
+	return skb->nfct != NULL;
+}
+
 #ifdef CONFIG_PROC_FS
 #include <linux/proc_fs.h>
 extern struct proc_dir_entry *proc_net_netfilter;

@@ -269,6 +274,7 @@ extern struct proc_dir_entry *proc_net_n
 #else /* !CONFIG_NETFILTER */
 #define NF_HOOK(pf, hook, skb, indev, outdev, okfn) (okfn)(skb)
 static inline void nf_ct_attach(struct sk_buff *new, struct sk_buff *skb) {}
+static inline int nf_defer_fragment(struct sk_buff *skb) { return 0; }
 #endif /*CONFIG_NETFILTER*/
 
 #endif /*__KERNEL__*/

include/net/ip.h +0 -1

index e4563bb..9f09882 100644
@@ -310,7 +310,6 @@ enum ip_defrag_users
 	IP_DEFRAG_CALL_RA_CHAIN,
 	IP_DEFRAG_CONNTRACK_IN,
 	IP_DEFRAG_CONNTRACK_OUT,
-	IP_DEFRAG_NAT_OUT,
 	IP_DEFRAG_VS_IN,
 	IP_DEFRAG_VS_OUT,
 	IP_DEFRAG_VS_FWD

net/ipv4/ip_output.c +15 -3

index 1775823..b0487a2 100644
@@ -160,7 +160,7 @@ int ip_build_and_send_pkt(struct sk_buff
 
 EXPORT_SYMBOL_GPL(ip_build_and_send_pkt);
 
-static inline int ip_finish_output2(struct sk_buff *skb)
+static inline int ip_finish_output3(struct sk_buff *skb)
 {
 	struct dst_entry *dst = skb->dst;
 	struct hh_cache *hh = dst->hh;

@@ -200,6 +200,17 @@ static inline int ip_finish_output2(stru
 	return -EINVAL;
 }
 
+static inline int ip_finish_output2(struct sk_buff *skb)
+{
+#ifdef CONFIG_NETFILTER
+	/* defered fragmentation when connection tracking is used */
+	if (skb->len > dst_mtu(skb->dst) &&
+	    !(skb_shinfo(skb)->ufo_size || skb_shinfo(skb)->tso_size))
+		return ip_fragment(skb, ip_finish_output3);
+#endif
+	return ip_finish_output3(skb);
+}
+
 static inline int ip_finish_output(struct sk_buff *skb)
 {
 	struct net_device *dev = skb->dst->dev;

@@ -265,7 +276,7 @@ int ip_mc_output(struct sk_buff *skb)
 				newskb->dev, ip_dev_loopback_xmit);
 	}
 
-	if (skb->len > dst_mtu(&rt->u.dst))
+	if (skb->len > dst_mtu(&rt->u.dst) && !nf_defer_fragment(skb))
 		return ip_fragment(skb, ip_finish_output);
 	else
 		return ip_finish_output(skb);

@@ -276,7 +287,8 @@ int ip_output(struct sk_buff *skb)
 	IP_INC_STATS(IPSTATS_MIB_OUTREQUESTS);
 
 	if (skb->len > dst_mtu(skb->dst) &&
-		!(skb_shinfo(skb)->ufo_size || skb_shinfo(skb)->tso_size))
+	    !(skb_shinfo(skb)->ufo_size || skb_shinfo(skb)->tso_size) &&
+	    !nf_defer_fragment(skb))
 		return ip_fragment(skb, ip_finish_output);
 	else
 		return ip_finish_output(skb);

net/ipv4/netfilter/ip_conntrack_standalone.c +1 ~~-26~~

index dd476b1..381650d 100644
@@ -450,30 +450,6 @@ static unsigned int ip_conntrack_defrag(
 	return NF_ACCEPT;
 }
 
-static unsigned int ip_refrag(unsigned int hooknum,
-			      struct sk_buff **pskb,
-			      const struct net_device *in,
-			      const struct net_device *out,
-			      int (*okfn)(struct sk_buff *))
-{
-	struct rtable *rt = (struct rtable *)(*pskb)->dst;
-
-	/* We've seen it coming out the other side: confirm */
-	if (ip_confirm(hooknum, pskb, in, out, okfn) != NF_ACCEPT)
-		return NF_DROP;
-
-	/* Local packets are never produced too large for their
-	   interface.  We degfragment them at LOCAL_OUT, however,
-	   so we have to refragment them here. */
-	if ((*pskb)->len > dst_mtu(&rt->u.dst) &&
-	    !skb_shinfo(*pskb)->tso_size) {
-		/* No hook can be after us, so this should be OK. */
-		ip_fragment(*pskb, okfn);
-		return NF_STOLEN;
-	}
-	return NF_ACCEPT;
-}
-
 static unsigned int ip_conntrack_local(unsigned int hooknum,
 				       struct sk_buff **pskb,
 				       const struct net_device *in,

@@ -541,9 +517,8 @@ static struct nf_hook_ops ip_conntrack_h
 	.priority	= NF_IP_PRI_CONNTRACK_HELPER,
 };
 
-/* Refragmenter; last chance. */
 static struct nf_hook_ops ip_conntrack_out_ops = {
-	.hook		= ip_refrag,
+	.hook		= ip_confirm,
 	.owner		= THIS_MODULE,
 	.pf		= PF_INET,
 	.hooknum	= NF_IP_POST_ROUTING,

net/ipv4/netfilter/ip_nat_standalone.c +2 ~~-17~~

index 30cd4e1..f0fff02 100644
@@ -69,7 +69,7 @@ ip_nat_fn(unsigned int hooknum,
 	enum ip_nat_manip_type maniptype = HOOK2MANIP(hooknum);
 
 	/* We never see fragments: conntrack defrags on pre-routing
-	   and local-out, and ip_nat_out protects post-routing. */
+	   and local-out. */
 	IP_NF_ASSERT(!((*pskb)->nh.iph->frag_off
 		       & htons(IP_MF|IP_OFFSET)));

@@ -190,22 +190,7 @@ ip_nat_out(unsigned int hooknum,
 	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr))
 		return NF_ACCEPT;
 
-	/* We can hit fragment here; forwarded packets get
-	   defragmented by connection tracking coming in, then
-	   fragmented (grr) by the forward code.
-
-	   In future: If we have nfct != NULL, AND we have NAT
-	   initialized, AND there is no helper, then we can do full
-	   NAPT on the head, and IP-address-only NAT on the rest.
-
-	   I'm starting to have nightmares about fragments.  */
-
-	if ((*pskb)->nh.iph->frag_off & htons(IP_MF|IP_OFFSET)) {
-		*pskb = ip_ct_gather_frags(*pskb, IP_DEFRAG_NAT_OUT);
-
-		if (!*pskb)
-			return NF_STOLEN;
-	}
+	WARN_ON((*pskb)->nh.iph->frag_off & htons(IP_MF|IP_OFFSET));
 
 	return ip_nat_fn(hooknum, pskb, in, out, okfn);
 }

Herbert Xu

2005-11-15 10:44 UTC #1 lore raw

Quoted 4 lines · Patrick McHardy

[NETFILTER]: Defer fragmentation in ip_output when connection tracking is used
This allows to get rid of the okfn use in ip_refrag and save the useless fragmentation/defragmentation step when NAT is used.

I'm slightly uneasy about this change because for POST_ROUTING, the defragmentation occurs in the middle of the hook, NF_IP_PRI_NAT_SRC.

This means that things like the mangle table currently sees the fragments as opposed to the whole packet. This patch will change that.

Now I'm not saying that this is necessarily a bad thing. In fact, for all I know it might make more sense to do this. But we should consider the possible implications before embarking on it.

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Patrick McHardy

author

2005-11-17 02:28 UTC #2 lore raw

Quoted 11 lines · Herbert Xu

Quoted 4 lines · Patrick McHardy
[NETFILTER]: Defer fragmentation in ip_output when connection tracking is used
This allows to get rid of the okfn use in ip_refrag and save the useless fragmentation/defragmentation step when NAT is used.
I'm slightly uneasy about this change because for POST_ROUTING, the defragmentation occurs in the middle of the hook, NF_IP_PRI_NAT_SRC.

Quoted 7 lines

This means that things like the mangle table currently sees the fragments as opposed to the whole packet. This patch will change that.
Now I'm not saying that this is necessarily a bad thing. In fact, for all I know it might make more sense to do this. But we should consider the possible implications before embarking on it.

Good point. I would also prefer to have fragmentation occur after POST_ROUTING in all cases. Looking at the in-tree targets, it means loosing the ability to do a couple of things:

- CLASSIFY fragments differently
- MARK fragments differently
- DSCP/ECN/TOS mark fragments differently
- Change TTLs of fragments to differently values

None of them seems very important. The DSCP and ECN targets were broken until not long ago without anyone noticing, the TTL target is relatively new. So it comes down to loosing the ability to classify fragments of one packet differently using iptables, which doesn't make much sense too me. In fact I think it would make classification easier if mangle would see the whole packet.

I've CCed Harald for his opinion in case I missed something.

Regards
Patrick

Patrick McHardy

author

2005-11-19 07:02 UTC #3 lore raw

Quoted 27 lines · Patrick McHardy

Quoted 7 lines · Herbert Xu
Quoted 2 lines · Patrick McHardy
[NETFILTER]: Defer fragmentation in ip_output when connection tracking is used
I'm slightly uneasy about this change because for POST_ROUTING, the defragmentation occurs in the middle of the hook, NF_IP_PRI_NAT_SRC.
Quoted 7 lines
This means that things like the mangle table currently sees the fragments as opposed to the whole packet. This patch will change that.
Now I'm not saying that this is necessarily a bad thing. In fact, for all I know it might make more sense to do this. But we should consider the possible implications before embarking on it.
Good point. I would also prefer to have fragmentation occur after POST_ROUTING in all cases. Looking at the in-tree targets, it means loosing the ability to do a couple of things:
- CLASSIFY fragments differently
- MARK fragments differently
- DSCP/ECN/TOS mark fragments differently
- Change TTLs of fragments to differently values

Quoted 6 lines

None of them seems very important. The DSCP and ECN targets were broken until not long ago without anyone noticing, the TTL target is relatively new. So it comes down to loosing the ability to classify fragments of one packet differently using iptables, which doesn't make much sense too me. In fact I think it would make classification easier if mangle would see the whole packet.

How about this patch that moves the POST_ROUTING hook before fragmentation instead of defering it? Can anyone think of a reason why mangle/POSTROUTING should see fragments?

[NETFILTER]: Call POST_ROUTING hook before fragmentation

Call POST_ROUTING hook before fragmentation to get rid of the okfn use in ip_refrag and save the useless fragmentation/defragmentation step when NAT is used.

The patch introduces one user-visible change, the POSTROUTING chain in the mangle table gets entire packets, not fragments, which should simplify use of the MARK and CLASSIFY targets for queueing as a nice side-effect.

Signed-off-by Patrick McHardy <[email protected]>

---
commit 1bead31b7e969e00bc5c043d34f20f9edb12a961
tree 16b82a0bedbb29b6a709140a7047ce2d52bb776f
parent 6dafdbcbf8b78409e4767ae30766826f21300a9f
author Patrick McHardy <[email protected]> Sat, 19 Nov 2005 07:58:46 +0100 committer Patrick McHardy <[email protected]> Sat, 19 Nov 2005 07:58:46 +0100

 include/net/ip.h                               |    1 -
 net/ipv4/ip_output.c                           |   30 +++++++++++-------------
 net/ipv4/netfilter/ip_conntrack_standalone.c   |   26 +--------------------
 net/ipv4/netfilter/ip_nat_standalone.c         |   17 --------------
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |   26 +--------------------
 5 files changed, 16 insertions(+), 84 deletions(-)

include/net/ip.h +0 -1

index e4563bb..9f09882 100644
@@ -310,7 +310,6 @@ enum ip_defrag_users
 	IP_DEFRAG_CALL_RA_CHAIN,
 	IP_DEFRAG_CONNTRACK_IN,
 	IP_DEFRAG_CONNTRACK_OUT,
-	IP_DEFRAG_NAT_OUT,
 	IP_DEFRAG_VS_IN,
 	IP_DEFRAG_VS_OUT,
 	IP_DEFRAG_VS_FWD

net/ipv4/ip_output.c +14 ~~-16~~

index 11c2f68..946e812 100644
@@ -202,13 +202,11 @@ static inline int ip_finish_output2(stru
 
 static inline int ip_finish_output(struct sk_buff *skb)
 {
-	struct net_device *dev = skb->dst->dev;
-
-	skb->dev = dev;
-	skb->protocol = htons(ETH_P_IP);
-
-	return NF_HOOK(PF_INET, NF_IP_POST_ROUTING, skb, NULL, dev,
-		       ip_finish_output2);
+	if (skb->len > dst_mtu(skb->dst) &&
+	    !(skb_shinfo(skb)->ufo_size || skb_shinfo(skb)->tso_size))
+		return ip_fragment(skb, ip_finish_output2);
+	else
+		return ip_finish_output2(skb);
 }
 
 int ip_mc_output(struct sk_buff *skb)

@@ -265,21 +263,21 @@ int ip_mc_output(struct sk_buff *skb)
 				newskb->dev, ip_dev_loopback_xmit);
 	}
 
-	if (skb->len > dst_mtu(&rt->u.dst))
-		return ip_fragment(skb, ip_finish_output);
-	else
-		return ip_finish_output(skb);
+	return NF_HOOK(PF_INET, NF_IP_POST_ROUTING, skb, NULL, skb->dev,
+		       ip_finish_output);
 }
 
 int ip_output(struct sk_buff *skb)
 {
+	struct net_device *dev = skb->dst->dev;
+
 	IP_INC_STATS(IPSTATS_MIB_OUTREQUESTS);
 
-	if (skb->len > dst_mtu(skb->dst) &&
-		!(skb_shinfo(skb)->ufo_size || skb_shinfo(skb)->tso_size))
-		return ip_fragment(skb, ip_finish_output);
-	else
-		return ip_finish_output(skb);
+	skb->dev = dev;
+	skb->protocol = htons(ETH_P_IP);
+
+	return NF_HOOK(PF_INET, NF_IP_POST_ROUTING, skb, NULL, dev,
+		       ip_finish_output);
 }
 
 int ip_queue_xmit(struct sk_buff *skb, int ipfragok)

net/ipv4/netfilter/ip_conntrack_standalone.c +1 ~~-25~~

index dd476b1..13ed18a 100644
@@ -450,30 +450,6 @@ static unsigned int ip_conntrack_defrag(
 	return NF_ACCEPT;
 }
 
-static unsigned int ip_refrag(unsigned int hooknum,
-			      struct sk_buff **pskb,
-			      const struct net_device *in,
-			      const struct net_device *out,
-			      int (*okfn)(struct sk_buff *))
-{
-	struct rtable *rt = (struct rtable *)(*pskb)->dst;
-
-	/* We've seen it coming out the other side: confirm */
-	if (ip_confirm(hooknum, pskb, in, out, okfn) != NF_ACCEPT)
-		return NF_DROP;
-
-	/* Local packets are never produced too large for their
-	   interface.  We degfragment them at LOCAL_OUT, however,
-	   so we have to refragment them here. */
-	if ((*pskb)->len > dst_mtu(&rt->u.dst) &&
-	    !skb_shinfo(*pskb)->tso_size) {
-		/* No hook can be after us, so this should be OK. */
-		ip_fragment(*pskb, okfn);
-		return NF_STOLEN;
-	}
-	return NF_ACCEPT;
-}
-
 static unsigned int ip_conntrack_local(unsigned int hooknum,
 				       struct sk_buff **pskb,
 				       const struct net_device *in,

@@ -543,7 +519,7 @@ static struct nf_hook_ops ip_conntrack_h
 
 /* Refragmenter; last chance. */
 static struct nf_hook_ops ip_conntrack_out_ops = {
-	.hook		= ip_refrag,
+	.hook		= ip_confirm,
 	.owner		= THIS_MODULE,
 	.pf		= PF_INET,
 	.hooknum	= NF_IP_POST_ROUTING,

net/ipv4/netfilter/ip_nat_standalone.c +0 ~~-17~~

index 30cd4e1..f04111f 100644
@@ -190,23 +190,6 @@ ip_nat_out(unsigned int hooknum,
 	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr))
 		return NF_ACCEPT;
 
-	/* We can hit fragment here; forwarded packets get
-	   defragmented by connection tracking coming in, then
-	   fragmented (grr) by the forward code.
-
-	   In future: If we have nfct != NULL, AND we have NAT
-	   initialized, AND there is no helper, then we can do full
-	   NAPT on the head, and IP-address-only NAT on the rest.
-
-	   I'm starting to have nightmares about fragments.  */
-
-	if ((*pskb)->nh.iph->frag_off & htons(IP_MF|IP_OFFSET)) {
-		*pskb = ip_ct_gather_frags(*pskb, IP_DEFRAG_NAT_OUT);
-
-		if (!*pskb)
-			return NF_STOLEN;
-	}
-
 	return ip_nat_fn(hooknum, pskb, in, out, okfn);
 }

net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c +1 ~~-25~~

index 8202c1c..818ba72 100644
@@ -180,30 +180,6 @@ static unsigned int ipv4_conntrack_defra
 	return NF_ACCEPT;
 }
 
-static unsigned int ipv4_refrag(unsigned int hooknum,
-				struct sk_buff **pskb,
-				const struct net_device *in,
-				const struct net_device *out,
-				int (*okfn)(struct sk_buff *))
-{
-	struct rtable *rt = (struct rtable *)(*pskb)->dst;
-
-	/* We've seen it coming out the other side: confirm */
-	if (ipv4_confirm(hooknum, pskb, in, out, okfn) != NF_ACCEPT)
-		return NF_DROP;
-
-	/* Local packets are never produced too large for their
-	   interface.  We degfragment them at LOCAL_OUT, however,
-	   so we have to refragment them here. */
-	if ((*pskb)->len > dst_mtu(&rt->u.dst) &&
-	    !skb_shinfo(*pskb)->tso_size) {
-		/* No hook can be after us, so this should be OK. */
-		ip_fragment(*pskb, okfn);
-		return NF_STOLEN;
-	}
-	return NF_ACCEPT;
-}
-
 static unsigned int ipv4_conntrack_in(unsigned int hooknum,
 				      struct sk_buff **pskb,
 				      const struct net_device *in,

@@ -283,7 +259,7 @@ static struct nf_hook_ops ipv4_conntrack
 
 /* Refragmenter; last chance. */
 static struct nf_hook_ops ipv4_conntrack_out_ops = {
-	.hook		= ipv4_refrag,
+	.hook		= ipv4_confirm,
 	.owner		= THIS_MODULE,
 	.pf		= PF_INET,
 	.hooknum	= NF_IP_POST_ROUTING,

Harald Welte

2005-11-22 07:59 UTC #4 lore raw

In reply to Patrick McHardy

Quoted 4 lines · Patrick McHardy

- CLASSIFY fragments differently
- MARK fragments differently
- DSCP/ECN/TOS mark fragments differently
- Change TTLs of fragments to differently values

[...]

Quoted 1 line

I've CCed Harald for his opinion in case I missed something.

I don't think you've missed something. I agree that the features you pointed out seem to be useful and/or important.

Signature · 5 lines

- Harald Welte <[email protected]>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

Patrick McHardy

author

2005-11-22 08:17 UTC #5 lore raw

Quoted 8 lines · Harald Welte

Quoted 4 lines · Patrick McHardy
- CLASSIFY fragments differently
- MARK fragments differently
- DSCP/ECN/TOS mark fragments differently
- Change TTLs of fragments to differently values

Quoted 7 lines

[...]
Quoted 1 line
I've CCed Harald for his opinion in case I missed something.
I don't think you've missed something. I agree that the features you pointed out seem to be useful and/or important.

I hope you meant to write "don't seem to be"? :)

Harald Welte

2005-11-22 14:19 UTC #6 lore raw

Quoted 5 lines · Patrick McHardy

Quoted 3 lines
Quoted 1 line
I've CCed Harald for his opinion in case I missed something.
I don't think you've missed something. I agree that the features you pointed out seem to be useful and/or important.
I hope you meant to write "don't seem to be"? :)

yes. I shouldn't be answering emails while holding a tutorial, sorry.

Signature · 5 lines

- Harald Welte <[email protected]>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie