tcp_sack problem Re: [Bug 11721] after upgrade to 2.6.27 i cannot navigate

Nice job Aldo!

I forward this message to netdev and Cc our best tcp expert. Any new replies should be rather "Reply All" (not bugzilla only).

Thanks,
Jarek P.

...

[RESEND] I forgot to add Aldo's email before - sorry!

...

...

...

Aldo, for curiosity, could you also try if this change only (instead of tcp_sack etc.) can't do similar effect?:

echo 0 > /proc/sys/net/ipv4/tcp_timestamps

Thanks,
Jarek P.

So this ended up into tcp domain after all (I took earlier a brief anyway and found out that there are not that many changes 2.6.26..2.6.27 -- net/ipv4/tcp*.c include/net/tcp.h)...

I compared your packet against a good one from elsewhere.. I couldn't compare your latest dumps fully because attachments 18366 and 18367 are with different TCP options (you forgot zeros to sysctls in them?)... Anyway, only thing that seemed to be different to that case from elsewhere were those extra bytes in the beginning (some below ip protocol that gets captured by tcpdump?) which are equal in both working and broken case of yours and the different ordering of the tcp options as noted by Jarek earlier. I tried to go through the fields one by one but nothing seemed to be wrong...

...Might be something crazy in the way that is too picky on tcp option ordering which wouldn't surprise me that much... :-) Please try if the patch below does any difference (on paperino is enough, the gw seems innocent here).

If that didn't help, can you please restore the sysctls to 1 and redo 2.6.26.6 dump (like in attachment 18366) so that I get a fully comparable sample.

 i.

index 990a584..850a4e9 100644
@@ -376,6 +376,12 @@ static void tcp_options_write(__be32 *ptr, struct tcp_sock *tp,
 		*md5_hash = NULL;
 	}
 
+	if (unlikely(opts->mss)) {
+		*ptr++ = htonl((TCPOPT_MSS << 24) |
+			       (TCPOLEN_MSS << 16) |
+			       opts->mss);
+	}
+
 	if (likely(OPTION_TS & opts->options)) {
 		if (unlikely(OPTION_SACK_ADVERTISE & opts->options)) {
 			*ptr++ = htonl((TCPOPT_SACK_PERM << 24) |

@@ -392,12 +398,6 @@ static void tcp_options_write(__be32 *ptr, struct tcp_sock *tp,
 		*ptr++ = htonl(opts->tsecr);
 	}
 
-	if (unlikely(opts->mss)) {
-		*ptr++ = htonl((TCPOPT_MSS << 24) |
-			       (TCPOLEN_MSS << 16) |
-			       opts->mss);
-	}
-
 	if (unlikely(OPTION_SACK_ADVERTISE & opts->options &&
 		     !(OPTION_TS & opts->options))) {
 		*ptr++ = htonl((TCPOPT_NOP << 24) |

On Mon, Oct 20, 2008 at 12:38:51PM +0300, Ilpo Järvinen wrote:
...

I've just thought about this too(!), and it seems that turning of tcp_timestamps should do similar reorder. But of course, I'll try not to disturb anymore...

Thanks,
Jarek P.

Yeah, you should be right. No need to patch+compile+boot hassle him, turning timestamps should be "enough" to move mss into first slot... :-)

If that works we probably need to play around a bit to figure out what is the actual cause (since it could involve timestamps or mss which the sysctl alone cannot prove).

 i.

...

Aldo, I forward this message to Ilpo and netdev, because "they" aren't on the Cc list of this report. (With next replies try to use the mail with their addresses.)

I think, you could test Ilpo's patch as well: this would make slightly different test/header. (Of course, all these /proc tcp values and mtus should be back to normal.)

Thanks,
Jarek P.

Jarek,
sorry i did not read your last msg before, so i have used the bug page for posting.

i repeat here what i wrote there:

Ilpo,
i'm sending herewith attached the output of
tcpdump -i eth0 -nXX -c3 'dst port 80 and tcp-syn != 0' on topolino when:
1) kernel 2.6.26.6 is running on paperino and tcp_window_scaling, tcp_timestamps, tcp_sack are set to "1"
2) kernel 2.6.26.6 is running on paperino and tcp_window_scaling, tcp_timestamps, tcp_sack are set to "0"
3) kernel 2.6.27-rc1-git1 is running on paperino and tcp_window_scaling, tcp_timestamps, tcp_sack are set to "1"
4) kernel 2.6.27-rc1-git1 is running on paperino and tcp_window_scaling, tcp_timestamps, tcp_sack are set to "0"

moreover, i read more carefully what you and Jarek wrote above, please let me know if you deem still necessary that i apply the patch you provided and compile or if you like i compile the latest kernel or whatever.

thanks
aldo

On Mon, Oct 20, 2008 at 09:48:18PM +0200, Aldo Maggi wrote:
...

Aldo, my advice, in case Ilpo is busy now:

Since it looks like the problem/bug isn't in the kernel, but rather somewhere in between (router, modem etc.) which is mislead by changed (compared to 2.6.26) order of tcp options, I doubt Ilpo needs now these dumps so much: it should be enough to say works or doesn't work.

You have checked it works in 2 cases (without tcp_sack, and without tcp_timestamp) when mss is the first option again. With Ilpo's patch these two options could be still present, and mss at the beginning. It should work because it's like 2.6.26 did this, but I think, it would be nice to check this anyway, if it's not a problem. Then trying the latest kernel (at least 2.6.27) with this patch should be most useful.

Thanks,
Jarek P.

Thanks, very unambiquously being told what each case is, helps a lot... :-)

Difference is: ip checksum, ipid, lsb of dstaddr, sport, seqno, tcp checksum, timestamp. Plus the option reordering as discussed before. Here are the options:

    28  0204  <
    29  05b4  <
    30  0402    0402
    31  080a    080a
    32  0019  | ffff
    33  db30  | 8169
    34  0000    0000
    35  0000    0000
    36        > 0204
    37        > 05b4
    38  0103    0103
    39  0306    0306

Could you try if tcp_window_scaling=1, tcp_sack=1, tcp_timestamps=0 also works, if you didn't already (it's not that obvious from the earlier acknowledgement you gave that these were the exact options you used or not)...

If that ws=1,s=1,ts=0 test succeeds, then testing my patch would be useful too. Anyway it seems that we're dealing with some violation of spec here (by either the peer or some middle node, 2.6.27-whatever is not doing something outside of spec), just trying to narrow down which of the options is the actual cause. We might need to try later some other ordering of options too to rule out possibilities by trial-and-error (ie., unless you have a better knowledge about the devices on the path, information which I sort of expect to be out of (y)our reach :-)).

 i.

Reply to all please... ...I restored them again for you... :-)

Np, thanks for the confirmation. I sort of assumed this already since the alternative interpretation of Jarek's mails didn't make too much sense after all (and I jumped into this thread in between, so I just scanned them through with haste on the first time concentrating more on your responses and which of the logs I should download :-)).

Ok, I'll check that tomorrow then as mine trimmed down 2.6.28-rc (ah, it's still -g something, no rc1 just yet) did complete already :-), just going to try to bombard google.it from here too and then it's the bedtime for me...

Ok, it's quite likely that both will work with the patch. Anyway, it's enough that you do the future tests with 2.6.27.y if we decide to try some other ordering (I'll try to think a bit of that tomorrow to see what would be to most sensible ordering).

 i.

Il giorno Tue, 21 Oct 2008 00:51:26 +0300 (EEST) "Ilpo Järvinen" <[email protected]> ha scritto:

sorry! i'll be more careful in the future!

[...]

thanks :-)
aldo

[...]

ok! the new kernel with ilpo's patch works!!
please find herewith attached the tcpdump output on eth0 on topolino while on paperino kernel 2.6.27-rc1-git1 with ilpo patch was running , the three tcp files all set to 1

[...]

as soon as i come back home i will start compilation of 2.6.27.2 with ilpo patch

thanks
aldo

In general, please use reply to all when dealing with linux developers (so that related people & lists end up being cc'ed), we just work that way :-). I did restore CCs for you this time.

-5 offset might well happen and patch was able to apply it right regardless of that (I might have had some unrelated modifications or so in my local dev tree which causes the line numbers to shift a bit).

I'm fine with the kernel and your procedure... :-) And also 2.6.27 won't make much difference to 2.6.27-rc-something I think, so you can ignore Jarek's suggestion to use 2.6.27+patch if you feel -rc is easier for you now that you've started with it. I'm fine with either kernel version+patch.

 i.