Hi,
To Hans and Patrick,
On Mon, 2012-08-27 at 14:02 +0200, Patrick McHardy wrote:
quoted
On Mon, 27 Aug 2012, Hans Schillstrom wrote:
quoted
quoted
quoted
quoted
On Mon, 20 Aug 2012, Jesper Dangaard Brouer wrote:
quoted
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?
Should we give it a try to put it in inet6_skb_parm
and minimize what we put there ?
I think it could be worth it.
To Patrick,
On Wed, 2012-08-29 at 11:47 +0200, Hans Schillstrom wrote:
quoted
On Mon, 2012-08-27 at 14:02 +0200, Patrick McHardy wrote:
quoted
On Mon, 27 Aug 2012, Hans Schillstrom wrote:
quoted
quoted
quoted
quoted
On Mon, 20 Aug 2012, Jesper Dangaard Brouer wrote:
quoted
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?
Should we give it a try to put it in inet6_skb_parm
and minimize what we put there ?
I think it could be worth it.
Okay, but then I do need some help and guidance, especially from
Patrick, think.
First of all, where in the netfilter code, should we update the new
fields in inet6_skb_parm?
On Wed, 29 Aug 2012, Jesper Dangaard Brouer wrote:
To Patrick,
On Wed, 2012-08-29 at 11:47 +0200, Hans Schillstrom wrote:
quoted
quoted
On Mon, 2012-08-27 at 14:02 +0200, Patrick McHardy wrote:
quoted
On Mon, 27 Aug 2012, Hans Schillstrom wrote:
quoted
quoted
quoted
quoted
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?
Should we give it a try to put it in inet6_skb_parm
and minimize what we put there ?
I think it could be worth it.
Okay, but then I do need some help and guidance, especially from
Patrick, think.
First of all, where in the netfilter code, should we update the new
fields in inet6_skb_parm?
Good question. I think we'd need at least three spots since every one
of these subsystems can be used indepedently from each other:
- conntrack/IPVS: PRE_ROUTING/LOCAL_OUT at lowest priority
- ip6tables: first time packet hits ip6t_do_table()?
Actually, looking at ipv6_rcv(), this might not work at all since it
sets skb->transport_header to the first header following the IPv6
header. This is used when processing extension headers by IPv6.
On Wed, 2012-08-29 at 14:34 +0200, Patrick McHardy wrote:
On Wed, 29 Aug 2012, Jesper Dangaard Brouer wrote:
quoted
On Wed, 2012-08-29 at 11:47 +0200, Hans Schillstrom wrote:
quoted
quoted
On Mon, 2012-08-27 at 14:02 +0200, Patrick McHardy wrote:
quoted
On Mon, 27 Aug 2012, Hans Schillstrom wrote:
quoted
quoted
quoted
quoted
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?
[...cut...]
quoted
quoted
quoted
quoted
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.
[...cut...]
quoted
quoted
Should we give it a try to put it in inet6_skb_parm
and minimize what we put there ?
I think it could be worth it.
Okay, but then I do need some help and guidance, especially from
Patrick, think.
First of all, where in the netfilter code, should we update the new
fields in inet6_skb_parm?
Good question. I think we'd need at least three spots since every one
of these subsystems can be used indepedently from each other:
- conntrack/IPVS: PRE_ROUTING/LOCAL_OUT at lowest priority
- ip6tables: first time packet hits ip6t_do_table()?
I've been looking at the code for ip6t_do_table() and it already calls
ipv6_find_hdr(). ip6t_do_table() calls ip6_packet_match()
And ip6_packet_match() already calls
ipv6_find_hdr(skb, protoff, -1, &_frag_off, NULL);
but only if((ip6info->flags & IP6T_F_PROTO))
ip6t_do_table() uses the data found by
ipv6_find_hdr()/ip6_packet_match() and updates 'struct xt_action_param
acpar' (which is passed on to all netfilter modules/functions as 'par')
protohdr = ipv6_find_hdr(skb, protoff, -1, &_frag_off, NULL)
*fragoff = _frag_off;
par->thoff = protoff /* thoff = Transport Header Offset */
par->fragoff = fragoff /* frag indicator and fragment offset */
The returned protocol (protohdr) is only used inside
ip6_packet_match(), thus the info on the protocol is lost.
(Side note) Saving the protocol could be useful for, the following
modules, as they call ipv6_find_hdr() once again to extract this:
net/netfilter/xt_TPROXY.c: function tproxy_tg6_v1()
net/netfilter/xt_socket.c: function socket_mt6_v1()
Thus, the netfilter framework already have this information available.
It just uses the 'struct xt_action_param par' to carry this
information, to its modules.
Mine and Hans's patch are basically introducing the same thing for IPVS,
only that this information is carried via 'struct ip_vs_iphdr'.
I don't know, if its worth to store this information in
inet6_skb_parm/IP6CB ?
I guess, to would make sense to store 'thoff' transport header offset,
especially for IPv6, given the extension headers.
But how many (code) users are there?
Is it only Netfilter and IPVS that want to look at the port numbers?
There also seems to a lot of users of "ipv6_skip_exthdr", which could
benefit? But I simply don't know the IPv6 code well enough...
--
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