Patchwork two other cases Re: [Bug 11721] after upgrade to 2.6.27 i cannot navigate

login
register
mail settings
Submitter Ilpo Järvinen
Date Oct. 22, 2008, 10 a.m.
Message ID <Pine.LNX.4.64.0810221244390.7072@wrl-59.cs.helsinki.fi>
Download mbox | patch
Permalink /patch/5329/
State Accepted
Delegated to: David Miller
Headers show

Comments

Ilpo Järvinen - Oct. 22, 2008, 10 a.m.
On Tue, 21 Oct 2008, David Miller wrote:

> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Tue, 21 Oct 2008 20:45:39 +0200
> 
> > On Tue, Oct 21, 2008 at 08:18:57PM +0200, Aldo Maggi wrote:
> > ...
> > > i've compiled 2.6.27.2 source after having patched it with your today's
> > > patch.
> > > 
> > > it works! i.e. i can navigate (w3m kernel.org) and update (apt-get
> > > update)
> > 
> > Ilpo, I should say you're incredible! ...But, since I've promised to
> > myself not to disturb you anymore (again), I can't do this, sorry :-(
> 
> Indeed, excellent work Ilpo.
> 
> Ilpo, now that we know this fixes things for sure, could you submit
> this formally with a proper signoff?
> 
> I'll queue it up for -stable too.
> 
> Thanks!

Sure, here below is one with a warning (it's the first patch + comment).

Olon, can you please check this as well if it affect to your case too 
(though the symptoms were not that clear in your case).

It would be nice for Aldo to check what the result will be with my second 
patch (only) using sack=1,ts=0,wscale=0. I guess it does but it's a bit 
unclear if nop's in front help or not (having the patch below should 
anyway help also in that case as the mss option gets moved before it 
anyway).

--
[PATCH] tcp: Restore ordering of TCP options for the sake of inter-operability

This is not our bug! Sadly some devices cannot cope with the change
of TCP option ordering which was a result of the recent rewrite of
the option code (not that there was some particular reason steming
from the rewrite for the reordering) though any ordering of TCP
options is perfectly legal. Thus we restore the original ordering
to allow interoperability with/through such broken devices and add
some warning about this trap. Since the reordering just happened
without any particular reason, this change shouldn't cost us
anything.

There are already couple of known failure reports (within close
proximity of the last release), so the problem might be more
wide-spread than a single device. And other reports which may
be due to the same problem though the symptoms were less obvious.
Analysis of one of the case revealed (with very high probability)
that sack capability cannot be negotiated as the first option
(SYN never got a response).

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
Reported-by: Aldo Maggi <sentiniate@tiscali.it>
Tested-by: Aldo Maggi <sentiniate@tiscali.it>
---
 net/ipv4/tcp_output.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)
Aldo Maggi - Oct. 22, 2008, 11:49 a.m.
Il giorno Wed, 22 Oct 2008 13:00:01 +0300 (EEST)
"Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> ha scritto:

[...]

> 
> It would be nice for Aldo to check what the result will be with my
> second patch (only) using sack=1,ts=0,wscale=0. I guess it does but
> it's a bit unclear if nop's in front help or not (having the patch
> below should anyway help also in that case as the mss option gets
> moved before it anyway).

in order to avoid misunderstandings, i list herebelow my actions:

i've used on paperino the kernel 2.6.27.2 patched with ilpo's
21.10.2008 patch (see please comment 47 in
http://bugzilla.kernel.org/show_bug.cgi?id=11721, and NOT with the patch
contained in comment 58, right?)
i've modified the following files:
echo 0 > /proc/sys/net/ipv4/tcp_window_scaling
echo 0 > /proc/sys/net/ipv4/tcp_timestamps
echo 1 > /proc/sys/net/ipv4/tcp_sack

the results are that i CAN navigate (w3m kernel.org) and update my
system (apt-get update in debian).

should it be of any help to ilpo, i attach the usual tcpdump on the wan
eth of topolino (my home server/gw)

ciao!
aldo



[...]
Aldo Maggi - Oct. 22, 2008, 1:19 p.m.
Il giorno Wed, 22 Oct 2008 15:09:40 +0300 (EEST)
"Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> ha scritto:

[...]

> So only thing that was problem in your case is that sackOK option (or 
> SACK_PERM in code notation) couldn't begin TCP options. With any
> other option as the first one, including nop, it does work. Thanks
> for all the testing you made and reporting it in the first place.

i thank you for your efforts in solving this problem!
it has been a real pleasere for me to cooperate with you!

all the best to you all

[...]
 
> Thanks again.
> 

i thank you! :-)

aldo

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 23, 2008, 9:07 p.m.
From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Wed, 22 Oct 2008 13:00:01 +0300 (EEST)

> [PATCH] tcp: Restore ordering of TCP options for the sake of inter-operability
> 
> This is not our bug! Sadly some devices cannot cope with the change
> of TCP option ordering which was a result of the recent rewrite of
> the option code (not that there was some particular reason steming
> from the rewrite for the reordering) though any ordering of TCP
> options is perfectly legal. Thus we restore the original ordering
> to allow interoperability with/through such broken devices and add
> some warning about this trap. Since the reordering just happened
> without any particular reason, this change shouldn't cost us
> anything.
> 
> There are already couple of known failure reports (within close
> proximity of the last release), so the problem might be more
> wide-spread than a single device. And other reports which may
> be due to the same problem though the symptoms were less obvious.
> Analysis of one of the case revealed (with very high probability)
> that sack capability cannot be negotiated as the first option
> (SYN never got a response).
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> Reported-by: Aldo Maggi <sentiniate@tiscali.it>
> Tested-by: Aldo Maggi <sentiniate@tiscali.it>

Applied and I'll queue up for -stable, thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski - Oct. 27, 2008, 6:51 a.m.
On Sat, Oct 25, 2008 at 10:15:15PM +0200, Aldo Maggi wrote:
> Il giorno Tue, 21 Oct 2008 07:49:54 +0000
> Jarek Poplawski <jarkao2@gmail.com> ha scritto:
> 
> > On Tue, Oct 21, 2008 at 09:27:21AM +0200, Aldo Maggi wrote:
> > ...
> > > as soon as i've time i'll replace the modem and run some tests.
> 
> just to let you know!
> i have changed my modem with a new one.
> the bug has disappeared with no modification of the tcp files.
> this shows that the problem was due to my old zyxel.
> 
> ciao!
> aldo
> 

Aldo, I think it's a very useful information, so I forward this to the
people.

Thanks again,
Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index de54f02..e4c5ac9 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -362,6 +362,17 @@  struct tcp_out_options {
 	__u32 tsval, tsecr;	/* need to include OPTION_TS */
 };
 
+/* Beware: Something in the Internet is very sensitive to the ordering of
+ * TCP options, we learned this through the hard way, so be careful here.
+ * Luckily we can at least blame others for their non-compliance but from
+ * inter-operatibility perspective it seems that we're somewhat stuck with
+ * the ordering which we have been using if we want to keep working with
+ * those broken things (not that it currently hurts anybody as there isn't
+ * particular reason why the ordering would need to be changed).
+ *
+ * At least SACK_PERM as the first option is known to lead to a disaster
+ * (but it may well be that other scenarios fail similarly).
+ */
 static void tcp_options_write(__be32 *ptr, struct tcp_sock *tp,
 			      const struct tcp_out_options *opts,
 			      __u8 **md5_hash) {
@@ -376,6 +387,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 +409,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) |