Skip to contentSkip to conversation

ipvs: Fix faulty IPv6 extension header handling in IPVS

PATCH In review Hans Schillstrom 4 participants 8 messages Aug 27, 2012 – Sep 4, 2012

Subsystems (4)
Networking Network drivers netfilter IPVS
lore
Share & tools
lore

Hans Schillstrom

author
#0 lore raw
Quoted 22 lines
Quoted 22 lines
Quoted 21 lines · Jesper Dangaard Brouer
Quoted 7 lines

Based on patch from: Hans Schillstrom

IPv6 headers must be processed in order of appearance, neither can it be assumed that Upper layer headers is first. If anything else than L4 is the first header IPVS will throw it.

Quoted 8 lines

IPVS will write SNAT & DNAT modifications at a fixed pos which will corrupt the message. Proper header position must be found before writing modifying packet.

This patch contains a lot of API changes. This is done, to avoid the costly scan of finding the IPv6 headers, via ipv6_find_hdr(). Finding the IPv6 headers is done as early as possible, and passed on as a pointer "struct ip_vs_iphdr *" to the affected functions.

How about we change netfilter to set up the skb's transport header at an early time so we can avoid all (most of) these header scans in netfilter?

Quoted 5 lines
Quoted 1 line

I think that would be great, maybe it should be global i.e. not only a netfilter issue.

I think in most other cases the headers are supposed to be processed sequentially. One problem though - to be useful for netfilter/IPVS we'd also need to store the transport layer protocol somewhere.

I guess that's the problem, adding it to the skb will not be popular .... Right now I don't have a good solution, maybe a more generic netfilter ptr in the skb ...

#1 lore raw
Quoted 22 lines · Hans Schillstrom
Quoted 22 lines
Quoted 22 lines
Quoted 21 lines · Jesper Dangaard Brouer
Quoted 7 lines

Based on patch from: Hans Schillstrom

IPv6 headers must be processed in order of appearance, neither can it be assumed that Upper layer headers is first. If anything else than L4 is the first header IPVS will throw it.

Quoted 8 lines

IPVS will write SNAT & DNAT modifications at a fixed pos which will corrupt the message. Proper header position must be found before writing modifying packet.

This patch contains a lot of API changes. This is done, to avoid the costly scan of finding the IPv6 headers, via ipv6_find_hdr(). Finding the IPv6 headers is done as early as possible, and passed on as a pointer "struct ip_vs_iphdr *" to the affected functions.

How about we change netfilter to set up the skb's transport header at an early time so we can avoid all (most of) these header scans in netfilter?

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

I think that would be great, maybe it should be global i.e. not only a netfilter issue.

I think in most other cases the headers are supposed to be processed sequentially. One problem though - to be useful for netfilter/IPVS we'd also need to store the transport layer protocol somewhere.

I guess that's the problem, adding it to the skb will not be popular .... Right now I don't have a good solution, maybe a more generic netfilter ptr in the skb ...

I guess inet6_skb_parm will be at least slightly more popular than adding it to the skb itself. The netfilter pointers are all used for optional things, so we can't really add it to any of those.

#2 lore raw

To Hans and Patrick,

Quoted 30 lines · Patrick McHardy
Quoted 28 lines · Hans Schillstrom
Quoted 27 lines
Quoted 18 lines
Quoted 18 lines · Jesper Dangaard Brouer
Quoted 15 lines

Based on patch from: Hans Schillstrom

IPv6 headers must be processed in order of appearance, neither can it be assumed that Upper layer headers is first. If anything else than L4 is the first header IPVS will throw it.

IPVS will write SNAT & DNAT modifications at a fixed pos which will corrupt the message. Proper header position must be found before writing modifying packet.

This patch contains a lot of API changes. This is done, to avoid the costly scan of finding the IPv6 headers, via ipv6_find_hdr(). Finding the IPv6 headers is done as early as possible, and passed on as a pointer "struct ip_vs_iphdr *" to the affected functions.

Quoted 5 lines
Quoted 3 lines

How about we change netfilter to set up the skb's transport header at an early time so we can avoid all (most of) these header scans in netfilter?

I think that would be great, maybe it should be global i.e. not only a netfilter issue.

I think in most other cases the headers are supposed to be processed sequentially. One problem though - to be useful for netfilter/IPVS we'd also need to store the transport layer protocol somewhere.

Quoted 6 lines
Quoted 2 lines

I guess that's the problem, adding it to the skb will not be popular .... Right now I don't have a good solution, maybe a more generic netfilter ptr in the skb ...

I guess inet6_skb_parm will be at least slightly more popular than adding it to the skb itself. The netfilter pointers are all used for optional things, so we can't really add it to any of those.

Okay, but how do we go from here?

Hans, should this hold back the patch ("ipvs: Fix faulty IPv6 extension header handling in IPVS"). Or should we pursue our patch, and circle back later once e.g. Patrick have found a generic solution for IPv6 transport header handling?

Signature · 5 lines
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer
#3 lore raw
Quoted 40 lines · Jesper Dangaard Brouer

To Hans and Patrick,

Quoted 30 lines · Patrick McHardy
Quoted 28 lines · Hans Schillstrom
Quoted 27 lines
Quoted 18 lines
Quoted 18 lines · Jesper Dangaard Brouer
Quoted 15 lines

Based on patch from: Hans Schillstrom

IPv6 headers must be processed in order of appearance, neither can it be assumed that Upper layer headers is first. If anything else than L4 is the first header IPVS will throw it.

IPVS will write SNAT & DNAT modifications at a fixed pos which will corrupt the message. Proper header position must be found before writing modifying packet.

This patch contains a lot of API changes. This is done, to avoid the costly scan of finding the IPv6 headers, via ipv6_find_hdr(). Finding the IPv6 headers is done as early as possible, and passed on as a pointer "struct ip_vs_iphdr *" to the affected functions.

Quoted 5 lines
Quoted 3 lines

How about we change netfilter to set up the skb's transport header at an early time so we can avoid all (most of) these header scans in netfilter?

I think that would be great, maybe it should be global i.e. not only a netfilter issue.

I think in most other cases the headers are supposed to be processed sequentially. One problem though - to be useful for netfilter/IPVS we'd also need to store the transport layer protocol somewhere.

Quoted 6 lines
Quoted 2 lines

I guess that's the problem, adding it to the skb will not be popular .... Right now I don't have a good solution, maybe a more generic netfilter ptr in the skb ...

I guess inet6_skb_parm will be at least slightly more popular than adding it to the skb itself. The netfilter pointers are all used for optional things, so we can't really add it to any of those.

Quoted 6 lines

Okay, but how do we go from here?

Hans, should this hold back the patch ("ipvs: Fix faulty IPv6 extension header handling in IPVS"). Or should we pursue our patch, and circle back later once e.g. Patrick have found a generic solution for IPv6 transport header handling?

I don't think we can do much better than using inet6_skb_parm. I think
the main question is whether it is really worth it, the iteration shouldn't be that expensive in most cases.

#4 lore raw

cover RFC: Caching IPv6 exthdr in skb->cb[]

Hi Patrick and Hans,

This is my followup to:
[PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS

Where you proposed improving the overall architecture of IPv6 extension header parsing not only for IPVS but for other related Netfilter subsystems as well.

We discussed using/extending inet6_skb_parm/IP6CB. There was not enough room for extending inet6_skb_parm directly, so I have introduced a struct inet6_skb_exthdr_cache/IP6CB_EXTHDR, which extend IP6CB.

The question is if this approach will work. Can netfilter be allowed to modify data after inet6_skb_parm/IP6CB, given all the different HOOKs ?

If we find this is a valid approach, then I'll update the IPVS patches to also use this.

This patchset is based upon:
Patrick's tree: git://github.com/kaber/nf-next-ipv6-nat.git

---

Jesper Dangaard Brouer (2):
netfilter: More users of ipv6_find_hdr_cb()
net: Cache IPv6 extension header "skip" parsing

 include/linux/ipv6.h                      |   15 +++++++++++
 include/linux/netfilter_ipv6/ip6_tables.h |   40 +++++++++++++++++++++++++++++
 net/ipv6/netfilter/ip6_tables.c           |    2 +
 net/netfilter/xt_TPROXY.c                 |    2 +
 net/netfilter/xt_socket.c                 |    2 +
 5 files changed, 58 insertions(+), 3 deletions(-)

--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer

#5 lore raw

RFC PATCH 1/2 net: Cache IPv6 extension header "skip" parsing

Store IPv6 exthdr data from ipv6_find_hdr() in skb->cb[].

Introducing struct inet6_skb_exthdr_cache / IP6CB_EXTHDR, as its not possible to extend inet6_skb_parm directly, or else DCCP dccp_skb_cb CB size gets too big.

Adding:
__u16 thoff; /* L4 Transport Header offset */
__s16 protocol;/* L4 protocol */
__u16 fragoff; /* packet is a fragment, with offset */

Optimize ip6t_do_table() by using the new ipv6_find_hdr_cb() in ip6_packet_match(). Before ipv6_find_hdr() were called for each iptables rule, with a "-p" protocol option (including -p all), which is very commonly used. The function ipv6_find_hdr_cb() is inlined to optimize this case and avoid a function call.

The inet6_skb_exthdr_cache / IP6CB_EXTHDR is added to linux/ipv6.h in the hope that ipv6_skip_exthdr() might also take advantage of this, in the future.

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---
Can someone tell me if netfilter (with all of its different HOOKs) can be allowed to modify data after inet6_skb_parm/IP6CB ???

 include/linux/ipv6.h                      |   15 +++++++++++
 include/linux/netfilter_ipv6/ip6_tables.h |   40 +++++++++++++++++++++++++++++
 net/ipv6/netfilter/ip6_tables.c           |    2 +
 3 files changed, 56 insertions(+), 1 deletions(-)
include/linux/ipv6.h +15 -0
index 0b94e91..21d8cc3 100644
@@ -266,6 +266,21 @@ struct inet6_skb_parm {
 #define IP6CB(skb)	((struct inet6_skb_parm*)((skb)->cb))
 #define IP6CBMTU(skb)	((struct ip6_mtuinfo *)((skb)->cb))
 
+/* Caching of exthdr info
+ * - This is used by netfilter and IPVS.
+ * (cannot extend inet6_skb_parm directly due to DCCP CB size gets too big)
+ */
+struct inet6_skb_exthdr_cache {
+	union {
+		struct inet6_skb_parm	h6;
+	} header;
+	/* Caching IPv6 exthdr "skip" info */
+	__u16	thoff;   /* L4 Transport Header offset */
+	__s16	protocol;/* L4 protocol */
+	__u16	fragoff; /* packet is a fragment, with offset */
+};
+#define IP6CB_EXTHDR(skb) ((struct inet6_skb_exthdr_cache*)((skb)->cb))
+
 static inline int inet6_iif(const struct sk_buff *skb)
 {
 	return IP6CB(skb)->iif;
include/linux/netfilter_ipv6/ip6_tables.h +40 -0
index 08c2cbb..5209aff 100644
@@ -303,6 +303,46 @@ enum {
 	IP6T_FH_F_AUTH	= (1 << 1),
 };
 
+/* Function caches IPv6 exthdr data from ipv6_find_hdr() in skb->cb[]
+ * - uses inet6_skb_exthdr_cache / IP6CB_EXTHDR
+ */
+static inline int
+ipv6_find_hdr_cb(const struct sk_buff *skb, unsigned int *offset,
+		 int target, unsigned short *fragoff, int *flags)
+{
+	struct inet6_skb_exthdr_cache *exthdr_cache = IP6CB_EXTHDR(skb);
+	__s16 protocol;
+
+	/* Only cache the "skip" exthdr usage */
+	if (target > 0) /* looking for a specific nexthdr target */
+		goto no_cache;
+	if (*offset) /* start at a specific offset */
+		goto no_cache;
+	if (flags) /* not caching flags at the moment */
+		goto no_cache;
+
+	/* Use thoff (transport header) as cache avail indicator */
+	if (exthdr_cache->thoff > 0) {
+		*offset  = exthdr_cache->thoff;
+		protocol = exthdr_cache->protocol;
+		if (fragoff)
+			*fragoff = exthdr_cache->fragoff;
+		return protocol;
+	}
+
+	protocol = ipv6_find_hdr(skb, offset, target, fragoff, flags);
+	if (protocol > 0) {
+                /* save a copy in the CB */
+		exthdr_cache->thoff = *offset;
+		exthdr_cache->protocol = protocol;
+		if (fragoff)
+			exthdr_cache->fragoff = *fragoff;
+	}
+	return protocol;
+no_cache:
+	return ipv6_find_hdr(skb, offset, target, fragoff, flags);
+}
+
 /* find specified header and get offset to it */
 extern int ipv6_find_hdr(const struct sk_buff *skb, unsigned int *offset,
 			 int target, unsigned short *fragoff, int *fragflg);
net/ipv6/netfilter/ip6_tables.c +1 -1
index d7cb045..79367f2 100644
@@ -133,7 +133,7 @@ ip6_packet_match(const struct sk_buff *skb,
 		int protohdr;
 		unsigned short _frag_off;
 
-		protohdr = ipv6_find_hdr(skb, protoff, -1, &_frag_off, NULL);
+		protohdr = ipv6_find_hdr_cb(skb, protoff, -1, &_frag_off, NULL);
 		if (protohdr < 0) {
 			if (_frag_off == 0)
 				*hotdrop = true;
#6 lore raw

RFC PATCH 2/2 netfilter: More users of ipv6_find_hdr_cb()

In reply to Jesper Dangaard Brouer

Functions tproxy_tg6_v1() and socket_mt6_v1 could use the CB cached version of ipv6_find_hdr().

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---
Could someone with tproxy or socket experience tell me, if the SKB CB data can survive this far?

 net/netfilter/xt_TPROXY.c |    2 +-
 net/netfilter/xt_socket.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
net/netfilter/xt_TPROXY.c +1 -1
index d7f1953..c73b713 100644
@@ -285,7 +285,7 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par)
 	int thoff = 0;
 	int tproto;
 
-	tproto = ipv6_find_hdr(skb, &thoff, -1, NULL, NULL);
+	tproto = ipv6_find_hdr_cb(skb, &thoff, -1, NULL, NULL);
 	if (tproto < 0) {
 		pr_debug("unable to find transport header in IPv6 packet, dropping\n");
 		return NF_DROP;
net/netfilter/xt_socket.c +1 -1
index 9ea482d..4feca50 100644
@@ -266,7 +266,7 @@ socket_mt6_v1(const struct sk_buff *skb, struct xt_action_param *par)
 	int thoff = 0, tproto;
 	const struct xt_socket_mtinfo1 *info = (struct xt_socket_mtinfo1 *) par->matchinfo;
 
-	tproto = ipv6_find_hdr(skb, &thoff, -1, NULL, NULL);
+	tproto = ipv6_find_hdr_cb(skb, &thoff, -1, NULL, NULL);
 	if (tproto < 0) {
 		pr_debug("unable to find transport header in IPv6 packet, dropping\n");
 		return NF_DROP;
maintainer
#7 lore raw

Re: [RFC PATCH 0/2] RFC: Caching IPv6 exthdr in skb->cb[]

In reply to Jesper Dangaard Brouer

Hello,

Quoted 9 lines · Jesper Dangaard Brouer

Hi Patrick and Hans,

This is my followup to:
[PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS

Where you proposed improving the overall architecture of IPv6 extension header parsing not only for IPVS but for other related Netfilter subsystems as well.

Quoted 8 lines

We discussed using/extending inet6_skb_parm/IP6CB. There was not enough room for extending inet6_skb_parm directly, so I have introduced a struct inet6_skb_exthdr_cache/IP6CB_EXTHDR, which extend IP6CB.

The question is if this approach will work. Can netfilter be allowed to modify data after inet6_skb_parm/IP6CB, given all the different HOOKs ?

Yes, as long as we do not reach the protocol
receiving handlers, eg. tcp_v4_rcv. But the problem is that many modules can use this CB part but nobody knows if it is valid - it is not reset.

If data is part of IP6CB it is reset in ipv6_rcv
with memset(IP6CB(skb), 0, sizeof(struct inet6_skb_parm));

One option is to extend this memset to cover this
new IP6CB_EXTHDR part if CONFIG_NF_DEFRAG_IPV6 is defined. I.e. we can create new struct inet6_skb_parm_exthdr, DCCP, TCP will continue to use/extend inet6_skb_parm but ipv6_find_hdr_cb will know that their part is reset. Then above memset can become:

memset(IP6CB_EXTHDR(skb), 0, sizeof(struct inet6_skb_parm_exthdr));

struct inet6_skb_parm_exthdr {
	struct inet6_skb_parm	h6;
#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
	__u16	thoff; ...
	...

#endif
};

Not sure if it should depend only on
CONFIG_NF_DEFRAG_IPV6.

Quoted 2 lines

If we find this is a valid approach, then I'll update the IPVS patches to also use this.

Regards

--
Julian Anastasov <[email protected]>