Re[3]: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS

Re[3]: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS

[Hans Schillstrom]

>>>
>>> On Mon, 20 Aug 2012, Jesper Dangaard Brouer wrote:
>>>
>>>> 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.
>>>
>>> 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.

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

Re[3]: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS

[Patrick McHardy]

On Mon, 27 Aug 2012, Hans Schillstrom wrote:

>>>>
>>>> On Mon, 20 Aug 2012, Jesper Dangaard Brouer wrote:
>>>>
>>>>> 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.
>>>>
>>>> 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.
>
> 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.

Re: Re[3]: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS

[Jesper Dangaard Brouer]

To Hans and Patrick,

On Mon, 2012-08-27 at 14:02 +0200, Patrick McHardy wrote:
> On Mon, 27 Aug 2012, Hans Schillstrom wrote:
> 
> >>>>
> >>>> On Mon, 20 Aug 2012, Jesper Dangaard Brouer wrote:
> >>>>
> >>>>> 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.
> >>>>
> >>>> 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.
> >
> > 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?

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

Re: Re[3]: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS

[Patrick McHardy]

On Wed, 29 Aug 2012, Jesper Dangaard Brouer wrote:

> To Hans and Patrick,
>
> On Mon, 2012-08-27 at 14:02 +0200, Patrick McHardy wrote:
>> On Mon, 27 Aug 2012, Hans Schillstrom wrote:
>>
>>>>>>
>>>>>> On Mon, 20 Aug 2012, Jesper Dangaard Brouer wrote:
>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> 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.
>>>
>>> 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?

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.

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

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

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

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

[Jesper Dangaard Brouer]

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 <brouer@redhat.com>

---
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(-)

diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index 0b94e91..21d8cc3 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -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;
diff --git a/include/linux/netfilter_ipv6/ip6_tables.h b/include/linux/netfilter_ipv6/ip6_tables.h
index 08c2cbb..5209aff 100644
--- a/include/linux/netfilter_ipv6/ip6_tables.h
+++ b/include/linux/netfilter_ipv6/ip6_tables.h
@@ -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);
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index d7cb045..79367f2 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -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;

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

[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 <brouer@redhat.com>

---
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(-)

diff --git a/net/netfilter/xt_TPROXY.c b/net/netfilter/xt_TPROXY.c
index d7f1953..c73b713 100644
--- a/net/netfilter/xt_TPROXY.c
+++ b/net/netfilter/xt_TPROXY.c
@@ -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;
diff --git a/net/netfilter/xt_socket.c b/net/netfilter/xt_socket.c
index 9ea482d..4feca50 100644
--- a/net/netfilter/xt_socket.c
+++ b/net/netfilter/xt_socket.c
@@ -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;

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

[Julian Anastasov]

	Hello,

On Mon, 3 Sep 2012, Jesper Dangaard Brouer wrote:

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

	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.

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

Regards

--
Julian Anastasov <ja@ssi.bg>