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

[Julian Anastasov <ja@xxxxxx>]

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

        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@xxxxxxxxxxxxxxx>
CC: Jesper Dangaard Brouer <brouer@xxxxxxxxxx>
CC: Hans Schillstrom <hans.schillstrom@xxxxxxxxxxxx>
====

Regards

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