Regression in pmtud lvs-nat with ipv6 since at least 3.10

Regression in pmtud lvs-nat with ipv6 since at least 3.10

[Art -kwaak- van Breemen]

Hi,
I just discovered a major regression in path-mtu-discovery with
lvs-nat-ipv6
It seems that at least since 3.10.27, verifiedn on 3.12.10, with
the latest known working at least 3.7.10.

The problem is this:
The ICMP6, packet too big gets "corrupted". In a sence the icmp
itself does not get corrupted, but the replacement of source and
destination address are plain wrong:

A correct dump (on 3.7.10):
16:16:55.351103 IP6 2a02:XXXX:0:XXX::2 > 2a02:310:0:2950::80:108: ICMP6, packet too big, mtu 1480, length 1240
        0x0000:  6000 0000 04d8 3a3d 2a02 XXXX 0000 XXXX  `.....:=*.......
        0x0010:  0000 0000 0000 0002 2a02 0310 0000 2950  ........*.....)P
        0x0020:  0000 0000 0080 0108 0200 6564 0000 05c8  ..........ed....
        0x0030:  6000 0000 05b4 063c 2a02 0310 0000 2950  `......<*.....)P
        0x0040:  0000 0000 0080 0108 2a02 XXXX XXXX XXXX  ........*....._.
        0x0050:  XXXX XXXX 996e 3404 01bb e128 a28b 89fd  eK0y.n4....(....
        0x0060:  45ac 7c39 8010 0078 6708 0000 0101 080a  E.|9...xg.......
        0x0070:  c8e9 759f 002b e18d a003 0201 0202 0302  ..u..+..........
        0x0080:  36d1 300d 0609 2a86 4886 f70d 0101 0505  6.0...*.H.......
        0x0090:  0030 4231 0b30 0906 0355 0406 1302 5553  .0B1.0...U....US
<snip>


A wrong dump (any since 3.10.27):
14:32:11.822345 IP6 2001:7b8:2ff:6f:2a02:310:0:2950 > ::80:104:0:0:80:104: ICMP6, packet too big, mtu 1472, length 1240
        0x0000:  6000 aa5f 04d8 3a3c 2001 07b8 02ff 006f  `.._..:<.......o
        0x0010:  2a02 0310 0000 2950 0000 0000 0080 0104  *.....)P........
        0x0020:  0000 0000 0080 0104 0200 0617 0000 05c0  ................
        0x0030:  6000 0000 05a8 063b 2a02 0310 0000 0100  `......;*.......
        0x0040:  0200 f8ff fe80 0004 2001 07b8 032d 0000  .............-..
        0x0050:  YYYY YYYY YYYY YYYY 0050 c723 7e6a 835c  .d....66.P.#~j.\
        0x0060:  ca00 19c5 8010 0078 f3b6 0000 0101 080a  .......x........
        0x0070:  c8df df1e 0945 23c2 4854 5450 2f31 2e31  .....E#.HTTP/1.1
<snip>

What we can clearly see in the wrong picture is this:
source higher 64 bits is correct (my home gw address)
source lower 64 bits is the higher 64 bits of the destination machine.
destination higher 64 bits is the lower 64 bits of the destination machine
destination lower 64 bits is the lower 64 bits of the destination machine.
That last part in question is this on the loadbalancer:
TCP  [2a02:310:0:100:200:f8ff:fe80:4]:http wrr
  -> [2a02:310:0:2950::80:104]:http Masq    4      0          0         
  -> [2a02:310:0:2950::80:204]:http Masq    4      0          0         

The addresses within the icmp6 packet itself are also not translated.


I've been browsing git but I cannot see any real differences in icmpv6
handling, so I think it might be that the core has some changes.

Regards,

Ard van Breemen

Re: Regression in pmtud lvs-nat with ipv6 since at least 3.10

[Art -kwaak- van Breemen]

Hi,

On Mon, Feb 17, 2014 at 04:32:06PM +0100, Art -kwaak- van Breemen wrote:
> A wrong dump (any since 3.10.27):
> 14:32:11.822345 IP6 2001:7b8:2ff:6f:2a02:310:0:2950 > ::80:104:0:0:80:104: ICMP6, packet too big, mtu 1472, length 1240
>         0x0000:  6000 aa5f 04d8 3a3c 2001 07b8 02ff 006f  `.._..:<.......o
>         0x0010:  2a02 0310 0000 2950 0000 0000 0080 0104  *.....)P........
>         0x0020:  0000 0000 0080 0104 0200 0617 0000 05c0  ................
>         0x0030:  6000 0000 05a8 063b 2a02 0310 0000 0100  `......;*.......
>         0x0040:  0200 f8ff fe80 0004 2001 07b8 032d 0000  .............-..
>         0x0050:  YYYY YYYY YYYY YYYY 0050 c723 7e6a 835c  .d....66.P.#~j.\
>         0x0060:  ca00 19c5 8010 0078 f3b6 0000 0101 080a  .......x........
>         0x0070:  c8df df1e 0945 23c2 4854 5450 2f31 2e31  .....E#.HTTP/1.1
> <snip>
> 
> What we can clearly see in the wrong picture is this:
> source higher 64 bits is correct (my home gw address)
> source lower 64 bits is the higher 64 bits of the destination machine.
> destination higher 64 bits is the lower 64 bits of the destination machine
> destination lower 64 bits is the lower 64 bits of the destination machine.
> That last part in question is this on the loadbalancer:
> TCP  [2a02:310:0:100:200:f8ff:fe80:4]:http wrr
>   -> [2a02:310:0:2950::80:104]:http Masq    4      0          0         
>   -> [2a02:310:0:2950::80:204]:http Masq    4      0          0         
> 
> The addresses within the icmp6 packet itself are also not translated.
What I think happened is that the ipv6 headers themselves are
correctly handled, but that the ipv6 address in the icmp part is
written to the wrong place.
(See: net/netfilter/ipvs/ip_vs_core.c: ip_vs_nat_icmp_v6() )

So what happens is this:
ip6:source address is written,
icmp6:destination address is written
ip6:destination address is rewritten to correct destination machine
icmp6:source address is actually written to the ip6 source
address+8 bytes, overwriting the last 64 bits of ip6:saddr and
the first 64 bits of ip6:daddr...

I think there might be an of by  wrong structsize problem here.

Re: Regression in pmtud lvs-nat with ipv6 since at least 3.10

[Julian Anastasov]

	Hello,

On Mon, 17 Feb 2014, Art -kwaak- van Breemen wrote:

> Hi,
> 
> On Mon, Feb 17, 2014 at 04:32:06PM +0100, Art -kwaak- van Breemen wrote:
> > A wrong dump (any since 3.10.27):
> > 14:32:11.822345 IP6 2001:7b8:2ff:6f:2a02:310:0:2950 > ::80:104:0:0:80:104: ICMP6, packet too big, mtu 1472, length 1240
> >         0x0000:  6000 aa5f 04d8 3a3c 2001 07b8 02ff 006f  `.._..:<.......o
> >         0x0010:  2a02 0310 0000 2950 0000 0000 0080 0104  *.....)P........
> >         0x0020:  0000 0000 0080 0104 0200 0617 0000 05c0  ................
> >         0x0030:  6000 0000 05a8 063b 2a02 0310 0000 0100  `......;*.......
> >         0x0040:  0200 f8ff fe80 0004 2001 07b8 032d 0000  .............-..
> >         0x0050:  YYYY YYYY YYYY YYYY 0050 c723 7e6a 835c  .d....66.P.#~j.\
> >         0x0060:  ca00 19c5 8010 0078 f3b6 0000 0101 080a  .......x........
> >         0x0070:  c8df df1e 0945 23c2 4854 5450 2f31 2e31  .....E#.HTTP/1.1
> > <snip>
> > 
> > What we can clearly see in the wrong picture is this:
> > source higher 64 bits is correct (my home gw address)
> > source lower 64 bits is the higher 64 bits of the destination machine.
> > destination higher 64 bits is the lower 64 bits of the destination machine
> > destination lower 64 bits is the lower 64 bits of the destination machine.
> > That last part in question is this on the loadbalancer:
> > TCP  [2a02:310:0:100:200:f8ff:fe80:4]:http wrr
> >   -> [2a02:310:0:2950::80:104]:http Masq    4      0          0         
> >   -> [2a02:310:0:2950::80:204]:http Masq    4      0          0         
> > 
> > The addresses within the icmp6 packet itself are also not translated.
> What I think happened is that the ipv6 headers themselves are
> correctly handled, but that the ipv6 address in the icmp part is
> written to the wrong place.
> (See: net/netfilter/ipvs/ip_vs_core.c: ip_vs_nat_icmp_v6() )
> 
> So what happens is this:
> ip6:source address is written,
> icmp6:destination address is written
> ip6:destination address is rewritten to correct destination machine
> icmp6:source address is actually written to the ip6 source
> address+8 bytes, overwriting the last 64 bits of ip6:saddr and
> the first 64 bits of ip6:daddr...
> 
> I think there might be an of by  wrong structsize problem here.

	Can you try to change the IPPROTO_ICMPV6 argument
of ipv6_find_hdr with -1, eg:

from:

ipv6_find_hdr(skb, &icmp_offset, IPPROTO_ICMPV6, &fragoffs, NULL);

to:

ipv6_find_hdr(skb, &icmp_offset, -1, &fragoffs, NULL);

	May be ipv6_find_hdr() can not be called with
exact protocol number, the offset is not updated,
it remains 0 and -ENOENT is returned. As result,
icmp_offset is not changed to 40.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Regression in pmtud lvs-nat with ipv6 since at least 3.10

[Art -kwaak- van Breemen]

Hi Julian,

On Mon, Feb 17, 2014 at 11:34:43PM +0200, Julian Anastasov wrote:
> 	May be ipv6_find_hdr() can not be called with
> exact protocol number, the offset is not updated,
> it remains 0 and -ENOENT is returned. As result,
> icmp_offset is not changed to 40.

I was thinking along that line too, since sizeof(struct icmp6hdr)
would exactly skip that part, hence pointing ciph at the
second half of the first ip address.
I try to make a test setup, since the real lvs servers are
actually production ;-).
But I can live with a little downtime on my desktop 8-D.

Regards,
Ard

Re: Regression in pmtud lvs-nat with ipv6 since at least 3.10

[Art -kwaak- van Breemen]

[-- Attachment #1: Type: text/plain, Size: 3764 bytes --]

Hi Julian,
> On Mon, Feb 17, 2014 at 11:34:43PM +0200, Julian Anastasov wrote:
> > 	May be ipv6_find_hdr() can not be called with
> > exact protocol number, the offset is not updated,
> > it remains 0 and -ENOENT is returned. As result,
> > icmp_offset is not changed to 40.
> 
> I was thinking along that line too, since sizeof(struct icmp6hdr)
> would exactly skip that part, hence pointing ciph at the
> second half of the first ip address.

I've made a test setup.
With attached patch (agains 3.13.3, but probably generic to >= 3.10) I get this:

Feb 18 14:24:41 c43236 kernel: [ 7103.570868] IPVS: Enter: ip_vs_out, net/netfilter/ipvs/ip_vs_core.c line 1119
Feb 18 14:24:41 c43236 kernel: [ 7103.570872] IPVS: Outgoing ICMPv6 (2,0) 2001:7b8:2ff:6f::1->2a02:310:0:1013::1005
Feb 18 14:24:41 c43236 kernel: [ 7103.570875] IPVS: lookup/out TCP [2001:7b8:32d:0:1864:b6ff:febf:3636]:38259->[2a02:310:0:1013::1005]:80 not hit
Feb 18 14:24:41 c43236 kernel: [ 7103.570877] IPVS: Incoming ICMPv6 (2,0) 2001:7b8:2ff:6f::1->2a02:310:0:1013::1005
Feb 18 14:24:41 c43236 kernel: [ 7103.570881] IPVS: lookup/in TCP [2001:7b8:32d:0:1864:b6ff:febf:3636]:38259->[2a02:310:0:1013::1005]:80 hit
Feb 18 14:24:41 c43236 kernel: [ 7103.570883] IPVS: Enter: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1186
Feb 18 14:24:41 c43236 kernel: [ 7103.570887] IPVS: Enter: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 738
Feb 18 14:24:41 c43236 kernel: [ 7103.570888] IPVS: icmp_offset=40,protocol=58
Feb 18 14:24:41 c43236 kernel: [ 7103.570889] IPVS: ip_vs_nat_icmp_v6() changed port 80 to 80
Feb 18 14:24:41 c43236 kernel: [ 7103.570891] IPVS: Leave: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 785
Feb 18 14:24:41 c43236 kernel: [ 7103.570896] IPVS: Leave: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1263

With a former version when the -1 was still IPPROTO_ICMPV6, it was:
Feb 18 13:31:30 c43236 kernel: [ 3912.655400] IPVS: Enter: ip_vs_out, net/netfilter/ipvs/ip_vs_core.c line 1119
Feb 18 13:31:30 c43236 kernel: [ 3912.655404] IPVS: Outgoing ICMPv6 (2,0) 2001:7b8:2ff:6f::1->2a02:310:0:1013::1005
Feb 18 13:31:30 c43236 kernel: [ 3912.655407] IPVS: lookup/out TCP [2001:7b8:32d:0:1864:b6ff:febf:3636]:38198->[2a02:310:0:1013::1005]:80 not hit
Feb 18 13:31:30 c43236 kernel: [ 3912.655408] IPVS: Incoming ICMPv6 (2,0) 2001:7b8:2ff:6f::1->2a02:310:0:1013::1005
Feb 18 13:31:30 c43236 kernel: [ 3912.655412] IPVS: lookup/in TCP [2001:7b8:32d:0:1864:b6ff:febf:3636]:38198->[2a02:310:0:1013::1005]:80 hit
Feb 18 13:31:30 c43236 kernel: [ 3912.655413] IPVS: Enter: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1186
Feb 18 13:31:30 c43236 kernel: [ 3912.655417] IPVS: Enter: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 738
Feb 18 13:31:30 c43236 kernel: [ 3912.655418] IPVS: icmp_offset=0
Feb 18 13:31:30 c43236 kernel: [ 3912.655419] IPv6 header not found
Feb 18 13:31:30 c43236 kernel: [ 3912.655422] IPVS: Leave: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 785
Feb 18 13:31:30 c43236 kernel: [ 3912.655426] IPVS: Leave: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1263
Feb 18 13:31:30 c43236 kernel: [ 3912.655430] IPVS: Enter: ip_vs_out, net/netfilter/ipvs/ip_vs_core.c line 1119

So the icmp_offset was indeed 0, which made the second find-hdr fail.

When reading net/ipv6/exthdrs_core.c it seems to me that outer protocols should
not be used. We already know it is procol 58 though :-).

I've added the Enterfunction debugs because of the:
"IPv6 header not found" . It actually means that something is probably wrong
with the code. Fixing the icmp_offset removed the error message, but it might
be a nice pointer when the icmp message itself is crippled.

Regards,
Ard van Breemen

[-- Attachment #2: lvs-icmp-bug.patch --]
[-- Type: text/x-diff, Size: 824 bytes --]

--- l-3.13.3/net/netfilter/ipvs/ip_vs_core.c.org	2014-01-22 14:46:53.222738221 +0100
+++ l-3.13.3/net/netfilter/ipvs/ip_vs_core.c	2014-02-18 14:17:56.516319899 +0100
@@ -735,7 +735,9 @@
 	struct ipv6hdr *ciph;
 	unsigned short fragoffs;
 
-	ipv6_find_hdr(skb, &icmp_offset, IPPROTO_ICMPV6, &fragoffs, NULL);
+	EnterFunction(10);
+	protocol=ipv6_find_hdr(skb, &icmp_offset, -1, &fragoffs, NULL);
+	IP_VS_DBG(15,"icmp_offset=%d,protocol=%d\n",icmp_offset,protocol);
 	icmph = (struct icmp6hdr *)(skb_network_header(skb) + icmp_offset);
 	offs = icmp_offset + sizeof(struct icmp6hdr);
 	ciph = (struct ipv6hdr *)(skb_network_header(skb) + offs);
@@ -780,6 +782,7 @@
 		IP_VS_DBG_PKT(11, AF_INET6, pp, skb,
 			      (void *)ciph - (void *)iph,
 			      "Forwarding altered incoming ICMPv6");
+	LeaveFunction(10);
 }
 #endif
 

[PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Art -kwaak- van Breemen]

[-- Attachment #1: Type: text/plain, Size: 646 bytes --]

On Tue, Feb 18, 2014 at 02:37:25PM +0100, Art -kwaak- van Breemen wrote:
> With attached patch (agains 3.13.3, but probably generic to >= 3.10) I get this:

should have been diff -up, instead of diff.

A cleanup of the icmpv6 handling for natted lvs services resulted
in the icmp packet being corrupted.
The ipv6_find_hdr seems to want to have -1 as a target for outer
level headers instead of a target >=0. The result is that packet
mangling was writing to the wrong offset, corrupting the packet,
and so disabling path-mtu-discovery.
- add extra debugging only output
- change target to -1

Signed-off-by: Ard van Breemen <ard@telegraafnet.nl>

[-- Attachment #2: lvs-icmp-bug.patch --]
[-- Type: text/x-diff, Size: 906 bytes --]

--- l-3.13.3/net/netfilter/ipvs/ip_vs_core.c.org	2014-01-22 14:46:53.222738221 +0100
+++ l-3.13.3/net/netfilter/ipvs/ip_vs_core.c	2014-02-18 14:17:56.516319899 +0100
@@ -735,7 +735,9 @@ void ip_vs_nat_icmp_v6(struct sk_buff *s
 	struct ipv6hdr *ciph;
 	unsigned short fragoffs;
 
-	ipv6_find_hdr(skb, &icmp_offset, IPPROTO_ICMPV6, &fragoffs, NULL);
+	EnterFunction(10);
+	protocol=ipv6_find_hdr(skb, &icmp_offset, -1, &fragoffs, NULL);
+	IP_VS_DBG(15,"icmp_offset=%d,protocol=%d\n",icmp_offset,protocol);
 	icmph = (struct icmp6hdr *)(skb_network_header(skb) + icmp_offset);
 	offs = icmp_offset + sizeof(struct icmp6hdr);
 	ciph = (struct ipv6hdr *)(skb_network_header(skb) + offs);
@@ -780,6 +782,7 @@ void ip_vs_nat_icmp_v6(struct sk_buff *s
 		IP_VS_DBG_PKT(11, AF_INET6, pp, skb,
 			      (void *)ciph - (void *)iph,
 			      "Forwarding altered incoming ICMPv6");
+	LeaveFunction(10);
 }
 #endif
 

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Art -kwaak- van Breemen]

Hi,

The patch applies cleanly to 3.13, 3.12 and with an offset to
l-3.10. The fix should be applied to all these versions since
it's a grave regression.

Regards,
Ard

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Julian Anastasov]

	Hello,

On Tue, 18 Feb 2014, Art -kwaak- van Breemen wrote:

> On Tue, Feb 18, 2014 at 02:37:25PM +0100, Art -kwaak- van Breemen wrote:
> > With attached patch (agains 3.13.3, but probably generic to >= 3.10) I get this:
> 
> should have been diff -up, instead of diff.
> 
> A cleanup of the icmpv6 handling for natted lvs services resulted
> in the icmp packet being corrupted.
> The ipv6_find_hdr seems to want to have -1 as a target for outer
> level headers instead of a target >=0. The result is that packet
> mangling was writing to the wrong offset, corrupting the packet,
> and so disabling path-mtu-discovery.
> - add extra debugging only output
> - change target to -1
> 
> Signed-off-by: Ard van Breemen <ard@telegraafnet.nl>

	Thanks for testing! This patch needs some tuning,
refer to Documentation/CodingStyle for the rules.
checkpatch.pl reports for the problems:

====
# scripts/checkpatch.pl /tmp/lvs-icmp-bug.patch 
ERROR: spaces required around that '=' (ctx:VxV)
#9: FILE: net/netfilter/ipvs/ip_vs_core.c:739:
+       protocol=ipv6_find_hdr(skb, &icmp_offset, -1, &fragoffs, NULL);
                ^

ERROR: space required after that ',' (ctx:VxV)
#10: FILE: net/netfilter/ipvs/ip_vs_core.c:740:
+       IP_VS_DBG(15,"icmp_offset=%d,protocol=%d\n",icmp_offset,protocol);
                    ^

ERROR: space required after that ',' (ctx:VxV)
#10: FILE: net/netfilter/ipvs/ip_vs_core.c:740:
+       IP_VS_DBG(15,"icmp_offset=%d,protocol=%d\n",icmp_offset,protocol);
                                                   ^

ERROR: space required after that ',' (ctx:VxV)
#10: FILE: net/netfilter/ipvs/ip_vs_core.c:740:
+       IP_VS_DBG(15,"icmp_offset=%d,protocol=%d\n",icmp_offset,protocol);
                                                               ^

ERROR: Missing Signed-off-by: line(s)

total: 5 errors, 0 warnings, 17 lines checked

/tmp/lvs-icmp-bug.patch has style problems, please review.
====

	I agree for the comment but not sure if it is
appropriate for bugfixes that go to stable kernels.
Also, the format should be icmp_offset=%u, not %d.

	Also, we should mention the problematic commit
and to CC the authors. You can tune/borrow from the
following example:

====
[PATCH] ipvs: fix wrong icmp_offset in ip_vs_nat_icmp_v6

Fix regression introduced in 3.8 with commit 63dca2c0b0e7a9
("ipvs: Fix faulty IPv6 extension header handling in IPVS").
Calling ipv6_find_hdr with protocol (IPPROTO_ICMPV6) is not
supported, use -1 instead. Solves problem of damaged IPv6
headers in NAT-ed ICMP packets.

Signed-off-by: Ard van Breemen <ard@telegraafnet.nl>
CC: Jesper Dangaard Brouer <brouer@redhat.com>
CC: Hans Schillstrom <hans.schillstrom@ericsson.com>
====

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Hans Schillstrom]

[-- Attachment #1: Type: text/plain, Size: 780 bytes --]

Hello Ard

Sorry for not looking into this but I've been on vacation for a couple
of weeks and came back right now.

My first impression is that there is something fishy here...
can you give me a day tho check this.

Regards
Hans Schillstrom

btw
I don't have the ericsoon.com email any longer.



On Tue, 2014-02-18 at 15:04 +0100, Art -kwaak- van Breemen wrote:
> Hi,
> 
> The patch applies cleanly to 3.13, 3.12 and with an offset to
> l-3.10. The fix should be applied to all these versions since
> it's a grave regression.
> 
> Regards,
> Ard
> --
> To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5717 bytes --]

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Art -kwaak- van Breemen]

Hi Julian,
On Tue, Feb 18, 2014 at 11:02:13PM +0200, Julian Anastasov wrote:
> 	Thanks for testing! This patch needs some tuning,
> refer to Documentation/CodingStyle for the rules.
> checkpatch.pl reports for the problems:

Thanks. I was a reading it, but something with impatience and
lazy, and worst of all, I still don't know an easy way to blame
without cloning the repository :-(

<snipped proof of lazy incompetence>                                                                ^

> 	I agree for the comment but not sure if it is
> appropriate for bugfixes that go to stable kernels.
> Also, the format should be icmp_offset=%u, not %d.

Thanks for yet another oversight ;-). And it would definitely not
be needed here. I do wonder about the EnterFunction though.
I could not clearly see what really generatd the "invalid header"
error message in the log.

> 	Also, we should mention the problematic commit
> and to CC the authors. You can tune/borrow from the
> following example:

Thanks!

> ====
> [PATCH] ipvs: fix wrong icmp_offset in ip_vs_nat_icmp_v6
> 
> Fix regression introduced in 3.8 with commit 63dca2c0b0e7a9
> ("ipvs: Fix faulty IPv6 extension header handling in IPVS").
> Calling ipv6_find_hdr with protocol (IPPROTO_ICMPV6) is not
> supported, use -1 instead. Solves problem of damaged IPv6
> headers in NAT-ed ICMP packets.

Jesper and Hans, I think that ipv6_find_hdr should have a target
of -1 to find the protocol header, and any other next-header
target will be valid after that.

Anyway, I will read the committing patches again and reduce the
patch to just a change -1.

If I see any "invalid headers" again on the firewall, I will add
the debugging.

The current patch is live using 3.12.11
( http://www.speurders.nl/ and http://www.spitsnieuws.nl/ f.i. )

Regards,

Ard

[PATCH] ipvs: fix wrong icmp_offset in ip_vs_nat_icmp_v6

[Art -kwaak- van Breemen]

Fix regression introduced in 3.8 with commit 63dca2c0b0e7a9
("ipvs: Fix faulty IPv6 extension header handling in IPVS").
Calling ipv6_find_hdr with protocol (IPPROTO_ICMPV6) is not
supported, use -1 instead. Solves (pmtud) problems caused by
damaged IPv6 headers in NAT-ed ICMP packets.

Signed-off-by: Ard van Breemen <ard@telegraafnet.nl>
CC: Jesper Dangaard Brouer <brouer@redhat.com>
CC: Hans Schillstrom <hans@schillstrom.com>

---

--- l-3.13.3/net/netfilter/ipvs/ip_vs_core.c.org	2014-01-22 14:46:53.222738221 +0100
+++ l-3.13.3/net/netfilter/ipvs/ip_vs_core.c	2014-02-19 12:22:25.641612230 +0100
@@ -735,7 +735,7 @@ void ip_vs_nat_icmp_v6(struct sk_buff *s
 	struct ipv6hdr *ciph;
 	unsigned short fragoffs;
 
-	ipv6_find_hdr(skb, &icmp_offset, IPPROTO_ICMPV6, &fragoffs, NULL);
+	ipv6_find_hdr(skb, &icmp_offset, -1, &fragoffs, NULL);
 	icmph = (struct icmp6hdr *)(skb_network_header(skb) + icmp_offset);
 	offs = icmp_offset + sizeof(struct icmp6hdr);
 	ciph = (struct ipv6hdr *)(skb_network_header(skb) + offs);

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Hans Schillstrom]

[-- Attachment #1: Type: text/plain, Size: 3478 bytes --]

Hi Ard and Ansis

On Wed, 2014-02-19 at 11:27 +0100, Art -kwaak- van Breemen wrote:
> Hi Julian,
> On Tue, Feb 18, 2014 at 11:02:13PM +0200, Julian Anastasov wrote:
> > 	Thanks for testing! This patch needs some tuning,
> > refer to Documentation/CodingStyle for the rules.
> > checkpatch.pl reports for the problems:
> 
> Thanks. I was a reading it, but something with impatience and
> lazy, and worst of all, I still don't know an easy way to blame
> without cloning the repository :-(
> 
> <snipped proof of lazy incompetence>                                                                ^
> 
> > 	I agree for the comment but not sure if it is
> > appropriate for bugfixes that go to stable kernels.
> > Also, the format should be icmp_offset=%u, not %d.
> 
> Thanks for yet another oversight ;-). And it would definitely not
> be needed here. I do wonder about the EnterFunction though.
> I could not clearly see what really generatd the "invalid header"
> error message in the log.
> 
> > 	Also, we should mention the problematic commit
> > and to CC the authors. You can tune/borrow from the
> > following example:
> 
> Thanks!
> 
> > ====
> > [PATCH] ipvs: fix wrong icmp_offset in ip_vs_nat_icmp_v6
> > 
> > Fix regression introduced in 3.8 with commit 63dca2c0b0e7a9
> > ("ipvs: Fix faulty IPv6 extension header handling in IPVS").
> > Calling ipv6_find_hdr with protocol (IPPROTO_ICMPV6) is not
> > supported, use -1 instead. Solves problem of damaged IPv6
> > headers in NAT-ed ICMP packets.
> 
> Jesper and Hans, I think that ipv6_find_hdr should have a target
> of -1 to find the protocol header, and any other next-header
> target will be valid after that.

The problem is if icmp6 is not the first header it will not work...
i.e. it can be other headers before icmp and if you have -1 you will not
always get the icmp header.


The patch that broke it was:
commit 9195bb8e381d81d5a315f911904cdf0cfcc919b8
Author: Ansis Atteka <aatteka@nicira.com>

Before there was a 
while (nexthdr != target) {
..
}

now it's  

do {
..
} while (!found)

which doesn't work for ipvs, when target is != -1

If you specify a target and it's the first header you should break.

I need to look deeper into the other users also to see that it doesn't
break anything.

Ansis, I don't think it will break your patch or ?


--- a/net/ipv6/exthdrs_core.c     2014-02-19 16:36:22.031686037 +0100
+++ b/net/ipv6/exthdrs_core.c     2014-02-19 16:37:28.838082168 +0100
@@ -211,6 +211,9 @@ int ipv6_find_hdr(const struct sk_buff *
                unsigned int hdrlen;
                found = (nexthdr == target);
 
+               if (found && (target > 0))
+                       break;
+
                if ((!ipv6_ext_hdr(nexthdr)) || nexthdr == NEXTHDR_NONE)
{
                        if (target < 0)
                                break;

Regards
Hans

> 
> Anyway, I will read the committing patches again and reduce the
> patch to just a change -1.
> 
> If I see any "invalid headers" again on the firewall, I will add
> the debugging.
> 
> The current patch is live using 3.12.11
> ( http://www.speurders.nl/ and http://www.spitsnieuws.nl/ f.i. )
> 
> Regards,
> 
> Ard
> --
> To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5717 bytes --]

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Art -kwaak- van Breemen]

Hi Hans,

On Wed, Feb 19, 2014 at 05:04:17PM +0100, Hans Schillstrom wrote:
> The problem is if icmp6 is not the first header it will not work...
> i.e. it can be other headers before icmp and if you have -1 you will not
> always get the icmp header.

Ah bah, I thought they move every extra header after the payload.

> The patch that broke it was:
> commit 9195bb8e381d81d5a315f911904cdf0cfcc919b8
> Author: Ansis Atteka <aatteka@nicira.com>

I will take a hard look into that one :-).

> --- a/net/ipv6/exthdrs_core.c     2014-02-19 16:36:22.031686037 +0100
> +++ b/net/ipv6/exthdrs_core.c     2014-02-19 16:37:28.838082168 +0100

If I patch my kernel:
ard@freeze8dev:/mnt/source/kernels/build-hp-ws/l-3.13.3$ diff -u net/netfilter/ipvs/ip_vs_core.c{.org,} ;diff -u net/ipv6/exthdrs_core.c{.org,}
--- net/netfilter/ipvs/ip_vs_core.c.org 2014-01-22 14:46:53.222738221 +0100
+++ net/netfilter/ipvs/ip_vs_core.c     2014-02-19 17:48:09.306379357 +0100
@@ -735,7 +735,10 @@
        struct ipv6hdr *ciph;
        unsigned short fragoffs;
 
-       ipv6_find_hdr(skb, &icmp_offset, IPPROTO_ICMPV6, &fragoffs, NULL);
+       EnterFunction(10);
+       protocol=ipv6_find_hdr(skb, &icmp_offset, IPPROTO_ICMPV6, &fragoffs, NULL);
+       //ipv6_find_hdr(skb, &icmp_offset, -1, &fragoffs, NULL);
+       IP_VS_DBG(15,"icmp_offset=%d,protocol=%d\n",icmp_offset,protocol);
        icmph = (struct icmp6hdr *)(skb_network_header(skb) + icmp_offset);
        offs = icmp_offset + sizeof(struct icmp6hdr);
        ciph = (struct ipv6hdr *)(skb_network_header(skb) + offs);
@@ -780,6 +783,7 @@
                IP_VS_DBG_PKT(11, AF_INET6, pp, skb,
                              (void *)ciph - (void *)iph,
                              "Forwarding altered incoming ICMPv6");
+       LeaveFunction(10);
 }
 #endif
 
--- net/ipv6/exthdrs_core.c.org 2013-11-06 13:32:34.653688901 +0100
+++ net/ipv6/exthdrs_core.c     2014-02-19 17:49:38.771351902 +0100
@@ -211,6 +211,8 @@
                unsigned int hdrlen;
                found = (nexthdr == target);
 
+               if (found && (target > 0))
+                       break;
                if ((!ipv6_ext_hdr(nexthdr)) || nexthdr == NEXTHDR_NONE) {
                        if (target < 0)
                                break;

I get this again:
Feb 19 18:02:36 c43236 kernel: [106578.432947] IPVS: Enter: ip_vs_out, net/netfilter/ipvs/ip_vs_core.c line 1120
Feb 19 18:02:36 c43236 kernel: [106578.432950] IPVS: Outgoing ICMPv6 (2,0) 2001:7b8:2ff:6f::1->2a02:310:0:1013::1005
Feb 19 18:02:36 c43236 kernel: [106578.432954] IPVS: lookup/out TCP [2001:7b8:32d:0:1864:b6ff:febf:3636]:39993->[2a02:310:0:1013::1005]:80 not hit
Feb 19 18:02:36 c43236 kernel: [106578.432956] IPVS: Incoming ICMPv6 (2,0) 2001:7b8:2ff:6f::1->2a02:310:0:1013::1005
Feb 19 18:02:36 c43236 kernel: [106578.432960] IPVS: lookup/in TCP [2001:7b8:32d:0:1864:b6ff:febf:3636]:39993->[2a02:310:0:1013::1005]:80 hit
Feb 19 18:02:36 c43236 kernel: [106578.432962] IPVS: Enter: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1186
Feb 19 18:02:36 c43236 kernel: [106578.432964] IPVS: Enter: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 738
Feb 19 18:02:36 c43236 kernel: [106578.432965] IPVS: icmp_offset=0,protocol=-2
Feb 19 18:02:36 c43236 kernel: [106578.432966] IPv6 header not found
Feb 19 18:02:36 c43236 kernel: [106578.432969] IPVS: Leave: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 786
Feb 19 18:02:36 c43236 kernel: [106578.432974] IPVS: Leave: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1263

I will take a closer look...

Regards,
Ard

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Art -kwaak- van Breemen]

On Wed, Feb 19, 2014 at 05:04:17PM +0100, Hans Schillstrom wrote:
> The problem is if icmp6 is not the first header it will not work...
> i.e. it can be other headers before icmp and if you have -1 you will not
> always get the icmp header.

If I look at the ipv6_find_hdr code, it will actually loop until
all headers have been visited, because no header will equal -1.

Eventually it will stop because there are no next headers
anymore. So -1 will point at the last header before the payload.

I think -1 is still the right target, as we already know it's
about ICMP.
Still the question remains is why do we get an ENOENT when
looking for NEXTHDR_ICMP ?

Regards,
Ard

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Art -kwaak- van Breemen]

Hi,

On Wed, Feb 19, 2014 at 06:53:35PM +0100, Art -kwaak- van Breemen wrote:
> I think -1 is still the right target, as we already know it's
> about ICMP.
> Still the question remains is why do we get an ENOENT when
> looking for NEXTHDR_ICMP ?
Found it:

if ((!ipv6_ext_hdr(nexthdr)) || nexthdr == NEXTHDR_NONE)
So, it keeps traversing until no more ext headers are found (i.e.
the protocol header), or no headers at all.
Then it tests if you wanted that last header (<0 -> ok leave).

So target may only be an ext_hdr, or -1.

Regards,
Ard van Breemen

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Art -kwaak- van Breemen]

Hans,
I want to keep the patch as is, but change the description:

====
[PATCH] ipvs: fix wrong icmp_offset in ip_vs_nat_icmp_v6
From: Ard van Breemen <ard@telegraafnet.nl>


Fix regression introduced in 3.8 with commit 9195bb8e381d81
("ipv6: improve ipv6_find_hdr() to skip empty routing headers")
which broke commit 63dca2c0b0e7a9
("ipvs: Fix faulty IPv6 extension header handling in IPVS").
by a small change in ipv6_find_hdr: finding specific protocols is not
supported anymore, use -1 instead. Solves (pmtud) problems caused by
damaged IPv6 headers in NAT-ed ICMP packets.

Signed-off-by: Ard van Breemen <ard@telegraafnet.nl>
CC: Jesper Dangaard Brouer <brouer@redhat.com>
CC: Hans Schillstrom <hans@schillstrom.com>

---

Do you and Ansis agree with me?

Regards,

Ard van Breemen

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Hans Schillstrom]

[-- Attachment #1: Type: text/plain, Size: 4256 bytes --]

Hello

On Wed, 2014-02-19 at 18:05 +0100, Art -kwaak- van Breemen wrote:
> Hi Hans,
> 
> On Wed, Feb 19, 2014 at 05:04:17PM +0100, Hans Schillstrom wrote:
> > The problem is if icmp6 is not the first header it will not work...
> > i.e. it can be other headers before icmp and if you have -1 you will not
> > always get the icmp header.
> 
> Ah bah, I thought they move every extra header after the payload.
> 
> > The patch that broke it was:
> > commit 9195bb8e381d81d5a315f911904cdf0cfcc919b8
> > Author: Ansis Atteka <aatteka@nicira.com>
> 
> I will take a hard look into that one :-).
> 
> > --- a/net/ipv6/exthdrs_core.c     2014-02-19 16:36:22.031686037 +0100
> > +++ b/net/ipv6/exthdrs_core.c     2014-02-19 16:37:28.838082168 +0100
> 
> If I patch my kernel:
> ard@freeze8dev:/mnt/source/kernels/build-hp-ws/l-3.13.3$ diff -u net/netfilter/ipvs/ip_vs_core.c{.org,} ;diff -u net/ipv6/exthdrs_core.c{.org,}
> --- net/netfilter/ipvs/ip_vs_core.c.org 2014-01-22 14:46:53.222738221 +0100
> +++ net/netfilter/ipvs/ip_vs_core.c     2014-02-19 17:48:09.306379357 +0100
> @@ -735,7 +735,10 @@
>         struct ipv6hdr *ciph;
>         unsigned short fragoffs;
>  
> -       ipv6_find_hdr(skb, &icmp_offset, IPPROTO_ICMPV6, &fragoffs, NULL);
> +       EnterFunction(10);
> +       protocol=ipv6_find_hdr(skb, &icmp_offset, IPPROTO_ICMPV6, &fragoffs, NULL);
> +       //ipv6_find_hdr(skb, &icmp_offset, -1, &fragoffs, NULL);
> +       IP_VS_DBG(15,"icmp_offset=%d,protocol=%d\n",icmp_offset,protocol);
>         icmph = (struct icmp6hdr *)(skb_network_header(skb) + icmp_offset);
>         offs = icmp_offset + sizeof(struct icmp6hdr);
>         ciph = (struct ipv6hdr *)(skb_network_header(skb) + offs);
> @@ -780,6 +783,7 @@
>                 IP_VS_DBG_PKT(11, AF_INET6, pp, skb,
>                               (void *)ciph - (void *)iph,
>                               "Forwarding altered incoming ICMPv6");
> +       LeaveFunction(10);
>  }
>  #endif
>  
> --- net/ipv6/exthdrs_core.c.org 2013-11-06 13:32:34.653688901 +0100
> +++ net/ipv6/exthdrs_core.c     2014-02-19 17:49:38.771351902 +0100
> @@ -211,6 +211,8 @@
>                 unsigned int hdrlen;
>                 found = (nexthdr == target);
>  
> +               if (found && (target > 0))
> +                       break;
>                 if ((!ipv6_ext_hdr(nexthdr)) || nexthdr == NEXTHDR_NONE) {
>                         if (target < 0)
>                                 break;
> 
> I get this again:
> Feb 19 18:02:36 c43236 kernel: [106578.432947] IPVS: Enter: ip_vs_out, net/netfilter/ipvs/ip_vs_core.c line 1120
> Feb 19 18:02:36 c43236 kernel: [106578.432950] IPVS: Outgoing ICMPv6 (2,0) 2001:7b8:2ff:6f::1->2a02:310:0:1013::1005
> Feb 19 18:02:36 c43236 kernel: [106578.432954] IPVS: lookup/out TCP [2001:7b8:32d:0:1864:b6ff:febf:3636]:39993->[2a02:310:0:1013::1005]:80 not hit
> Feb 19 18:02:36 c43236 kernel: [106578.432956] IPVS: Incoming ICMPv6 (2,0) 2001:7b8:2ff:6f::1->2a02:310:0:1013::1005
> Feb 19 18:02:36 c43236 kernel: [106578.432960] IPVS: lookup/in TCP [2001:7b8:32d:0:1864:b6ff:febf:3636]:39993->[2a02:310:0:1013::1005]:80 hit
> Feb 19 18:02:36 c43236 kernel: [106578.432962] IPVS: Enter: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1186
> Feb 19 18:02:36 c43236 kernel: [106578.432964] IPVS: Enter: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 738
> Feb 19 18:02:36 c43236 kernel: [106578.432965] IPVS: icmp_offset=0,protocol=-2

This one bothers me,
Why does it returns -ENOENT ???

When enter ipv6_find_hdr() the initial nexthdr should be 0x3a ICMPv6 
  u8 nexthdr = ipv6_hdr(skb)->nexthdr;
and target also 0x3a i.e. found is true and target > 0
 then break and return 3a

That didn't happen  why ?
 - Just a check did you rebuild ipv6 or if not a module the kernel ? 


> Feb 19 18:02:36 c43236 kernel: [106578.432966] IPv6 header not found
> Feb 19 18:02:36 c43236 kernel: [106578.432969] IPVS: Leave: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 786
> Feb 19 18:02:36 c43236 kernel: [106578.432974] IPVS: Leave: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1263
> 
> I will take a closer look...
> 
> Regards,
> Ard
> 
> 


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5717 bytes --]

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Ansis Atteka]

On Wed, Feb 19, 2014 at 10:21 AM, Art -kwaak- van Breemen
<ard@telegraafnet.nl> wrote:
>
> Hans,
> I want to keep the patch as is, but change the description:
>
> ====
> [PATCH] ipvs: fix wrong icmp_offset in ip_vs_nat_icmp_v6
> From: Ard van Breemen <ard@telegraafnet.nl>
>
>
> Fix regression introduced in 3.8 with commit 9195bb8e381d81
> ("ipv6: improve ipv6_find_hdr() to skip empty routing headers")
> which broke commit 63dca2c0b0e7a9
> ("ipvs: Fix faulty IPv6 extension header handling in IPVS").
> by a small change in ipv6_find_hdr: finding specific protocols is not
> supported anymore, use -1 instead. Solves (pmtud) problems caused by
> damaged IPv6 headers in NAT-ed ICMP packets.
>
> Signed-off-by: Ard van Breemen <ard@telegraafnet.nl>
> CC: Jesper Dangaard Brouer <brouer@redhat.com>
> CC: Hans Schillstrom <hans@schillstrom.com>
>
> ---
>
> Do you and Ansis agree with me?
My changes to this function were necessary for the Open vSwitch
set_ipv6() action implementation so that checksums would be correctly
recalculated.

I introduced IP6_FH_F_SKIP_RH flag that skips all Routing Headers,
where segments_left==0. This flag allows Open vSwitch kernel module to
figure out whether it needs to recalculate checksum after changing
destination IP address in IPv6 header. In ipv6 the checkum is
calculated over final destination IP address that could also be in
Routing Header intead of ipv6 header (see rfc2460 section 8.1 for more
details).

I believe your patch would break meaning of IP6_FH_F_SKIP_RH flag,
because it would exit early when it saw Routing Header where segments
left == 0.

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Julian Anastasov]

	Hello,

On Wed, 19 Feb 2014, Art -kwaak- van Breemen wrote:

> I could not clearly see what really generatd the "invalid header"
> error message in the log.

	You mean "IPv6 header not found" ?
I guess it happens for the second
protocol = ipv6_find_hdr(skb, &offs, -1, &fragoffs, NULL);
when icmp_offset was 0. Message is from ipv6_find_hdr.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Julian Anastasov]

	Hello,

On Wed, 19 Feb 2014, Hans Schillstrom wrote:

> The problem is if icmp6 is not the first header it will not work...
> i.e. it can be other headers before icmp and if you have -1 you will not
> always get the icmp header.
> 
> 
> The patch that broke it was:
> commit 9195bb8e381d81d5a315f911904cdf0cfcc919b8
> Author: Ansis Atteka <aatteka@nicira.com>
> 
> Before there was a 
> while (nexthdr != target) {
> ..
> }
> 
> now it's  
> 
> do {
> ..
> } while (!found)
> 
> which doesn't work for ipvs, when target is != -1
> 
> If you specify a target and it's the first header you should break.
> 
> I need to look deeper into the other users also to see that it doesn't
> break anything.
> 
> Ansis, I don't think it will break your patch or ?
> 
> 
> --- a/net/ipv6/exthdrs_core.c     2014-02-19 16:36:22.031686037 +0100
> +++ b/net/ipv6/exthdrs_core.c     2014-02-19 16:37:28.838082168 +0100
> @@ -211,6 +211,9 @@ int ipv6_find_hdr(const struct sk_buff *
>                 unsigned int hdrlen;
>                 found = (nexthdr == target);
>  
> +               if (found && (target > 0))
> +                       break;
> +

	This is against the goal of the above commit.

>                 if ((!ipv6_ext_hdr(nexthdr)) || nexthdr == NEXTHDR_NONE)
> {
>                         if (target < 0)
>                                 break;

	May be above check should be:

	if (target < 0 || found)
		break;

	We have to check some callers with -1, may be some
need check for NEXTHDR_NONE, for example, tproxy_tg6_v1(),
also the second call in hmark_pkt_set_htuple_ipv6(). Not sure
about nft_set_pktinfo_ipv6 and its callers.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Hans Schillstrom]

[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]

Hi Ansis & Patrick

On Wed, 2014-02-19 at 12:32 -0800, Ansis Atteka wrote:
> On Wed, Feb 19, 2014 at 10:21 AM, Art -kwaak- van Breemen
> <ard@telegraafnet.nl> wrote:
> >
> > Hans,
> > I want to keep the patch as is, but change the description:
> >
> > ====
> > [PATCH] ipvs: fix wrong icmp_offset in ip_vs_nat_icmp_v6
> > From: Ard van Breemen <ard@telegraafnet.nl>
> >
> >
> > Fix regression introduced in 3.8 with commit 9195bb8e381d81
> > ("ipv6: improve ipv6_find_hdr() to skip empty routing headers")
> > which broke commit 63dca2c0b0e7a9
> > ("ipvs: Fix faulty IPv6 extension header handling in IPVS").
> > by a small change in ipv6_find_hdr: finding specific protocols is not
> > supported anymore, use -1 instead. Solves (pmtud) problems caused by
> > damaged IPv6 headers in NAT-ed ICMP packets.
> >
> > Signed-off-by: Ard van Breemen <ard@telegraafnet.nl>
> > CC: Jesper Dangaard Brouer <brouer@redhat.com>
> > CC: Hans Schillstrom <hans@schillstrom.com>
> >
> > ---
> >
> > Do you and Ansis agree with me?
> My changes to this function were necessary for the Open vSwitch
> set_ipv6() action implementation so that checksums would be correctly
> recalculated.
> 
> I introduced IP6_FH_F_SKIP_RH flag that skips all Routing Headers,
> where segments_left==0. This flag allows Open vSwitch kernel module to
> figure out whether it needs to recalculate checksum after changing
> destination IP address in IPv6 header. In ipv6 the checkum is
> calculated over final destination IP address that could also be in
> Routing Header intead of ipv6 header (see rfc2460 section 8.1 for more
> details).
> 
> I believe your patch would break meaning of IP6_FH_F_SKIP_RH flag,
> because it would exit early when it saw Routing Header where segments
> left == 0.

I saw that too in openvswitch/actions.c, i.e it will break your patch

But if you want to find a specific header ex. NEXTHDR_GRE,
that is not in ipv6_ext_hdr() ipv6_find_hdr() will fail to do that
it will return -ENOENT

I still think ipv6_find_hdr is broken for nft_exthdr_eval() after
commit 9195bb8e381d81d5a315f911904cdf0cfcc919b8

Patrick,
I guess the intention with nft_exthdr_eval() is to be able to find any
extension header or ?

I might be wrong here ...

Regards
Hans


> --
> To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5717 bytes --]

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Hans Schillstrom]

[-- Attachment #1: Type: text/plain, Size: 2134 bytes --]

Hi Julian

As usual I'm to quick to send the mail...

On Wed, 2014-02-19 at 23:34 +0200, Julian Anastasov wrote:
> 	Hello,
> 
> On Wed, 19 Feb 2014, Hans Schillstrom wrote:
> 
> > The problem is if icmp6 is not the first header it will not work...
> > i.e. it can be other headers before icmp and if you have -1 you will not
> > always get the icmp header.
> > 
> > 
> > The patch that broke it was:
> > commit 9195bb8e381d81d5a315f911904cdf0cfcc919b8
> > Author: Ansis Atteka <aatteka@nicira.com>
> > 
> > Before there was a 
> > while (nexthdr != target) {
> > ..
> > }
> > 
> > now it's  
> > 
> > do {
> > ..
> > } while (!found)
> > 
> > which doesn't work for ipvs, when target is != -1
> > 
> > If you specify a target and it's the first header you should break.
> > 
> > I need to look deeper into the other users also to see that it doesn't
> > break anything.
> > 
> > Ansis, I don't think it will break your patch or ?
> > 
> > 
> > --- a/net/ipv6/exthdrs_core.c     2014-02-19 16:36:22.031686037 +0100
> > +++ b/net/ipv6/exthdrs_core.c     2014-02-19 16:37:28.838082168 +0100
> > @@ -211,6 +211,9 @@ int ipv6_find_hdr(const struct sk_buff *
> >                 unsigned int hdrlen;
> >                 found = (nexthdr == target);
> >  
> > +               if (found && (target > 0))
> > +                       break;
> > +
> 
> 	This is against the goal of the above commit.
> 
> >                 if ((!ipv6_ext_hdr(nexthdr)) || nexthdr == NEXTHDR_NONE)
> > {
> >                         if (target < 0)
> >                                 break;
> 
> 	May be above check should be:
> 
> 	if (target < 0 || found)
> 		break;

It will work for hmark and it looks like it will work for others,
with -1 

Maybe Patrick have another opinion...

> We have to check some callers with -1, may be some
> need check for NEXTHDR_NONE, for example, tproxy_tg6_v1(),
> also the second call in hmark_pkt_set_htuple_ipv6(). 


> Not sure
> about nft_set_pktinfo_ipv6 and its callers.

> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>


Regards
Hans


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5717 bytes --]

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Art -kwaak- van Breemen]

Hi,

On Wed, Feb 19, 2014 at 10:44:02PM +0200, Julian Anastasov wrote:
> On Wed, 19 Feb 2014, Art -kwaak- van Breemen wrote:
> 
> > I could not clearly see what really generatd the "invalid header"
> > error message in the log.
> 
> 	You mean "IPv6 header not found" ?
> I guess it happens for the second
> protocol = ipv6_find_hdr(skb, &offs, -1, &fragoffs, NULL);
> when icmp_offset was 0. Message is from ipv6_find_hdr.

Yes :-). I figured that after I added debugging to pinpoint it
;-).

I always saw that message in the log but I thought that's some
new warning about other devices bugs.
The message as it is right now, states something is wrong, and
please go find out yourself :-).
It would be handy if it could be either quiet "Yes, I know it can
fail but it's usually right", or stack trace it once every...
The use of ipv6_find_hdr is not that widespread yet. And in the
current case the IPPROTO_ICMPV6 is the only test in the kernel
that searches for a non ext hdr.
To be clear: if I look at my 1200+ client wireless network the
ipv6 takes up 30% of the traffic. That means ipv6 gets more and
more important (== more code) and a clear understanding of
ipv6_find_hdr is important.

Thanks for all the work guys!

Regards,

Ard van Breemen

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Art -kwaak- van Breemen]

Hi,

On Wed, Feb 19, 2014 at 09:28:38PM +0100, Hans Schillstrom wrote:
> That didn't happen  why ?
>  - Just a check did you rebuild ipv6 or if not a module the kernel ? 

Ah, f**k me :-(. I was so caught up in reloading ipvs that I
forgot to reload wherever that function is in, but I guess that's
static :-(. So tests like that will take me some longer to
perform.

Sorry about that noise, good catch!

Regards,
Ard van Breemen

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Hans Schillstrom]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]



On Thu, 2014-02-20 at 09:51 +0100, Art -kwaak- van Breemen wrote:
> Hi,
> 
> On Wed, Feb 19, 2014 at 09:28:38PM +0100, Hans Schillstrom wrote:
> > That didn't happen  why ?
> >  - Just a check did you rebuild ipv6 or if not a module the kernel ? 
> 
> Ah, f**k me :-(. I was so caught up in reloading ipvs that I
> forgot to reload wherever that function is in, but I guess that's
> static :-(. So tests like that will take me some longer to
> perform.
> 
Can you try Julians patch insted ?
mine was not so good :-)

i.e. 
-        if (target < 0)
+        if (target < 0 || found)
                break;


> Sorry about that noise, good catch!
> 
> Regards,
> Ard van Breemen
> 
> 

Regards
Hans

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5717 bytes --]

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Art -kwaak- van Breemen]

Hi,

On Wed, Feb 19, 2014 at 11:08:43PM +0100, Hans Schillstrom wrote:
> > 	May be above check should be:
> > 
> > 	if (target < 0 || found)
> > 		break;
> 
> It will work for hmark and it looks like it will work for others,
> with -1 
> 
> Maybe Patrick have another opinion...

It will be on my next desktop reboot test round :-).
I think I will test 2 hours before EOD, that's around 18:00+0100

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Art -kwaak- van Breemen]

Hi,

On Thu, Feb 20, 2014 at 02:10:44PM +0100, Art -kwaak- van Breemen wrote:
> On Wed, Feb 19, 2014 at 11:08:43PM +0100, Hans Schillstrom wrote:
> > > 	May be above check should be:
> > > 
> > > 	if (target < 0 || found)
> > > 		break;
> > 
> > It will work for hmark and it looks like it will work for others,
> > with -1 
> > 
> > Maybe Patrick have another opinion...

I ack the working of that change for my specific case: passing
pmtud's correctly:
Feb 20 18:58:59 c43236 kernel: [  721.473388] IPVS: Enter: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1186
Feb 20 18:58:59 c43236 kernel: [  721.473389] IPVS: Enter: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 738
Feb 20 18:58:59 c43236 kernel: [  721.473390] IPVS: icmp_offset=40,protocol=58
Feb 20 18:58:59 c43236 kernel: [  721.473391] IPVS: ip_vs_nat_icmp_v6() changed port 80 to 80
Feb 20 18:58:59 c43236 kernel: [  721.473393] IPVS: Leave: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 786
Feb 20 18:58:59 c43236 kernel: [  721.473396] IPVS: Leave: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1263

and:
18:58:59.067282 00:23:24:26:b4:5c > 00:23:24:26:b4:34, ethertype IPv6 (0x86dd), length 1294: 2001:7b8:2ff:6f::1 > 2a02:310:0:1013::1003: ICMP6, packet too big, mtu 1472, length 1240

So the "|| found" sounds sane.
But now I'm going to be an ass by saying that maybe both patches must be
applied because we only get into ip_vs_nat_icmp_v6 by ip_vs_fill_iph_skb in
include/net/ip_vs.h, which determines the protocol and the start of the
protocol header by using -1 as target in ipv6_find_hdr.
Actually, before we reach that nat function we have traversed several constructs of:
protocol,offset = ipv6_find_hdr (target = -1 )
if ( protocol != IPPROTO_ICMPV6)
  bail out (return NF_ACCEPT actually) .
(and maybe use offset)

So I think it's clearer for the total code if we follow the exact same construct:
find a protocol, bail out if protocol wrong.
It should never happen at that point, but there are more things that never
should happen :-).

Anyway: whatever you guys decide, I owe you all beer. I think we are one of the
few companies that assume a working pmtud for ipv6. Most of the top companies
just use an mtu of 1280 because the "hardware" loadbalancer cannot handle it
(yet) or just want to prevent the hassle. Thanks!

Regards,

Ard van Breemen

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Julian Anastasov]

	Hello,

On Thu, 20 Feb 2014, Art -kwaak- van Breemen wrote:

> I ack the working of that change for my specific case: passing
> pmtud's correctly:
> Feb 20 18:58:59 c43236 kernel: [  721.473388] IPVS: Enter: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1186
> Feb 20 18:58:59 c43236 kernel: [  721.473389] IPVS: Enter: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 738
> Feb 20 18:58:59 c43236 kernel: [  721.473390] IPVS: icmp_offset=40,protocol=58
> Feb 20 18:58:59 c43236 kernel: [  721.473391] IPVS: ip_vs_nat_icmp_v6() changed port 80 to 80
> Feb 20 18:58:59 c43236 kernel: [  721.473393] IPVS: Leave: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 786
> Feb 20 18:58:59 c43236 kernel: [  721.473396] IPVS: Leave: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1263
> 
> and:
> 18:58:59.067282 00:23:24:26:b4:5c > 00:23:24:26:b4:34, ethertype IPv6 (0x86dd), length 1294: 2001:7b8:2ff:6f::1 > 2a02:310:0:1013::1003: ICMP6, packet too big, mtu 1472, length 1240
> 
> So the "|| found" sounds sane.

	Thanks for the confirmation. Then may be Hans
can post a fix for this problem after checking the callers
of ipv6_find_hdr.

> But now I'm going to be an ass by saying that maybe both patches must be
> applied because we only get into ip_vs_nat_icmp_v6 by ip_vs_fill_iph_skb in
> include/net/ip_vs.h, which determines the protocol and the start of the
> protocol header by using -1 as target in ipv6_find_hdr.
> Actually, before we reach that nat function we have traversed several constructs of:
> protocol,offset = ipv6_find_hdr (target = -1 )
> if ( protocol != IPPROTO_ICMPV6)
>   bail out (return NF_ACCEPT actually) .

	Yes, it is not expected, the protocol was
already validated. May be we will save some cycles
without such check...

> (and maybe use offset)
> 
> So I think it's clearer for the total code if we follow the exact same construct:
> find a protocol, bail out if protocol wrong.
> It should never happen at that point, but there are more things that never
> should happen :-).
> 
> Anyway: whatever you guys decide, I owe you all beer. I think we are one of the
> few companies that assume a working pmtud for ipv6. Most of the top companies
> just use an mtu of 1280 because the "hardware" loadbalancer cannot handle it
> (yet) or just want to prevent the hassle. Thanks!
> 
> Regards,
> 
> Ard van Breemen

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Hans Schillstrom]

[-- Attachment #1: Type: text/plain, Size: 2860 bytes --]

Hello

On Fri, 2014-02-21 at 00:17 +0200, Julian Anastasov wrote:
> 	Hello,
> 
> On Thu, 20 Feb 2014, Art -kwaak- van Breemen wrote:
> 
> > I ack the working of that change for my specific case: passing
> > pmtud's correctly:
> > Feb 20 18:58:59 c43236 kernel: [  721.473388] IPVS: Enter: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1186
> > Feb 20 18:58:59 c43236 kernel: [  721.473389] IPVS: Enter: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 738
> > Feb 20 18:58:59 c43236 kernel: [  721.473390] IPVS: icmp_offset=40,protocol=58
> > Feb 20 18:58:59 c43236 kernel: [  721.473391] IPVS: ip_vs_nat_icmp_v6() changed port 80 to 80
> > Feb 20 18:58:59 c43236 kernel: [  721.473393] IPVS: Leave: ip_vs_nat_icmp_v6, net/netfilter/ipvs/ip_vs_core.c line 786
> > Feb 20 18:58:59 c43236 kernel: [  721.473396] IPVS: Leave: ip_vs_icmp_xmit_v6, net/netfilter/ipvs/ip_vs_xmit.c line 1263
> > 
> > and:
> > 18:58:59.067282 00:23:24:26:b4:5c > 00:23:24:26:b4:34, ethertype IPv6 (0x86dd), length 1294: 2001:7b8:2ff:6f::1 > 2a02:310:0:1013::1003: ICMP6, packet too big, mtu 1472, length 1240
> > 
> > So the "|| found" sounds sane.
> 
> 	Thanks for the confirmation. Then may be Hans
> can post a fix for this problem after checking the callers
> of ipv6_find_hdr.

I'll do that.

> 
> > But now I'm going to be an ass by saying that maybe both patches must be
> > applied because we only get into ip_vs_nat_icmp_v6 by ip_vs_fill_iph_skb in
> > include/net/ip_vs.h, which determines the protocol and the start of the
> > protocol header by using -1 as target in ipv6_find_hdr.
> > Actually, before we reach that nat function we have traversed several constructs of:
> > protocol,offset = ipv6_find_hdr (target = -1 )
> > if ( protocol != IPPROTO_ICMPV6)
> >   bail out (return NF_ACCEPT actually) .
> 
> 	Yes, it is not expected, the protocol was
> already validated. May be we will save some cycles
> without such check...

We will save some cycles here, very few actually..
I'm not sure about the mobility header if it can break this.
Have not read the RFC :-)

The -1 is OK for me right now

> > (and maybe use offset)
> > 
> > So I think it's clearer for the total code if we follow the exact same construct:
> > find a protocol, bail out if protocol wrong.
> > It should never happen at that point, but there are more things that never
> > should happen :-).
> > 
> > Anyway: whatever you guys decide, I owe you all beer. I think we are one of the
> > few companies that assume a working pmtud for ipv6. Most of the top companies
> > just use an mtu of 1280 because the "hardware" loadbalancer cannot handle it
> > (yet) or just want to prevent the hassle. Thanks!
> > 
> > Regards,
> > 
> > Ard van Breemen
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>

Regards
Hans

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5717 bytes --]

Re: [PATCH] ipvs: fix ipv6 icmp forwarding in natted services

[Julian Anastasov]

	Hello,

On Fri, 21 Feb 2014, Hans Schillstrom wrote:

> We will save some cycles here, very few actually..
> I'm not sure about the mobility header if it can break this.
> Have not read the RFC :-)
> 
> The -1 is OK for me right now

	I don't see any difference what we use here because
we used -1 in ip_vs_fill_iph_skb. If not validated once, with
-1 we can get a NEXTHDR_NONE result when ipv6_find_hdr
returns without error.

	I see that IPVS is hooked before ip6_input_finish()
where all protocol headers without INET6_PROTO_FINAL bit
are pulled from the head.

	I guess such headers are pulled one by one and
at a final step after multiple ip6_input calls IPVS can
see the protocols it supports.

	So, IPVS catches known protocols if they are
first. IMHO, we can continue to use IPPROTO_ICMPV6
because ipv6_find_hdr always stops at the first final
header.

Regards

--
Julian Anastasov <ja@ssi.bg>