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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
