From: Jeff Chua <hidden> Date: 2009-02-06 05:39:37
On Fri, Feb 6, 2009 at 7:29 AM, David Miller [off-list ref] wrote:
[ netdev CC:'d, please always do this for networking stuff ]
Will do.
Oh well, what is running on each side of the rlogin attempt?
Nothing at all. Just a pure "rlogin remotehost" trying to get a login
prompt. Nothing inside .profile.
On Fri, Feb 6, 2009 at 11:10 AM, Herbert Xu [off-list ref] wrote:
Could you please try a strace on both sides, with and without
this patch applied?
Attached are two strace. t1.good is a good rlogin run. t4.bad is bad
rlogin run where nothing is displayed but typing "~-." gets back to
the host.
With strace, it takes a bit more try to get it to hang. In fact, this
bug was quite to trigger and I've been having this problem on-and-off
since December, and but it was intermittent and didn't think it was a
kernel problem until serious chasing. Some network switches "mask" out
the problem, and some direct connect always has this problem. Some
servers occurred very frequent, some seldom.
On Fri, Feb 6, 2009 at 7:32 AM, Linus Torvalds
[off-list ref] wrote:
- even just a whiff of "breaks legacy app" just says "revert it".
I agree reverting is the right thing here.
Thanks,
Jeff.
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2009-02-06 05:46:14
On Fri, Feb 06, 2009 at 01:39:19PM +0800, Jeff Chua wrote:
Attached are two strace. t1.good is a good rlogin run. t4.bad is bad
rlogin run where nothing is displayed but typing "~-." gets back to
the host.
You traced the side receiving the urgent pointer. This has
nothing to do with the patch since it only changes sending
behaviour.
Did you change the kernel on the other side during these tests?
If reverting the patch on the other side does fix the problem
for you, please do the two straces there.
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: Jeff Chua <hidden> Date: 2009-02-06 11:36:05
On Fri, Feb 6, 2009 at 1:45 PM, Herbert Xu [off-list ref] wrote:
On Fri, Feb 06, 2009 at 01:39:19PM +0800, Jeff Chua wrote:
quoted
Attached are two strace. t1.good is a good rlogin run. t4.bad is bad
rlogin run where nothing is displayed but typing "~-." gets back to
the host.
You traced the side receiving the urgent pointer. This has
nothing to do with the patch since it only changes sending
behaviour.
The problem is the "remote" side that's frozen. The "local" side can
be on any kernel version and it won't have any effect. The runs that I
did was for one good run with "strace -o t1.good rlogin ju" that has
the bad kernel and a second one with "strace -o t4.bad rlogin ju" that
just hang.
Did you change the kernel on the other side during these tests?
No. Was hoping that the traces would show where it hang for the good
and bad trace.
If reverting the patch on the other side does fix the problem
for you, please do the two straces there.
Sorry, forgot to send the good trace run for the patched remote side.
Here's attached. t5.good.
Thanks,
Jeff.
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2009-02-09 05:59:22
On Fri, Feb 06, 2009 at 07:35:50PM +0800, Jeff Chua wrote:
quoted
Did you change the kernel on the other side during these tests?
No. Was hoping that the traces would show where it hang for the good
and bad trace.
Then the problem isn't my patch at all. My patch only affects
the side sending the urgent data, according to your straces, rlogin
is only receiving urgent data, not sending it. So the patch cannot
have an affect.
quoted
If reverting the patch on the other side does fix the problem
for you, please do the two straces there.
Sorry, forgot to send the good trace run for the patched remote side.
Here's attached. t5.good.
From: David Miller <davem@davemloft.net> Date: 2009-02-09 06:04:20
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 9 Feb 2009 16:58:48 +1100
On Fri, Feb 06, 2009 at 07:35:50PM +0800, Jeff Chua wrote:
quoted
quoted
Did you change the kernel on the other side during these tests?
No. Was hoping that the traces would show where it hang for the good
and bad trace.
Then the problem isn't my patch at all. My patch only affects
the side sending the urgent data, according to your straces, rlogin
is only receiving urgent data, not sending it. So the patch cannot
have an affect.
Hmmm... let's figure out exactly what's going on here.
I would expect to see the URG packets from rlogin not rlogind, from
things like hitting Ctrl-C etc.
On the other hand, I can't imagine what the server, rlogind, would be
sending URGs out for.
So I would say that it is more likely to be rlogin sending the
URG packets.
kill(6433, SIGURG) = 0
FD 3 is the TCP socket. As you can see, the only operations done
were write/writev/read/setsockopt. None of these can enter urgent
mode for sending, so that's how I concluded that my patch should
have zero effect on this side.
Note also that we got an SIGURG, which indicates that we received
urgent data.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
kill(6433, SIGURG) = 0
FD 3 is the TCP socket. As you can see, the only operations done
were write/writev/read/setsockopt. None of these can enter urgent
mode for sending, so that's how I concluded that my patch should
have zero effect on this side.
Note also that we got an SIGURG, which indicates that we received
urgent data.
Hmm, what happens if a opposite dir ACK gets sent (it's zero sized seqno
range will be above urgent sequence when urg is not yet acknowledged)? So
we'll take the later branch after your change and send bogus 0xffff urg?
I think you're change might have actually broken bidir tcp completely (and
probably window probing as well under some conditions). Congraz, two flies
with a single stroke :-). You would have needed an additional check that
the tcb->seq is below urg...
...Below is a patch but Jeff might need to revert the revert first if
he tests with the latest Linus' tree.
--
i.
[PATCH] tcp: check that we're still < urg instead of sending a bogus one
Opposite dir ACK with zero sized seqno range can go into the
later condition once urg is in the outstanding window not
yet acknowledge (we're still in urg mode but seqno we sent in
the ack won't be below snd_up). Need to check that we never
advertize bogus urg for a segment which is after the snd_up.
Signed-off-by: Ilpo Järvinen <redacted>
---
net/ipv4/tcp_output.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2009-02-09 07:28:38
On Mon, Feb 09, 2009 at 09:13:00AM +0200, Ilpo Järvinen wrote:
Hmm, what happens if a opposite dir ACK gets sent (it's zero sized seqno
range will be above urgent sequence when urg is not yet acknowledged)? So
we'll take the later branch after your change and send bogus 0xffff urg?
I think you're change might have actually broken bidir tcp completely (and
probably window probing as well under some conditions). Congraz, two flies
with a single stroke :-). You would have needed an additional check that
the tcb->seq is below urg...
...Below is a patch but Jeff might need to revert the revert first if
he tests with the latest Linus' tree.
Good point! That would definitely explain this.
quoted hunk
[PATCH] tcp: check that we're still < urg instead of sending a bogus one
Opposite dir ACK with zero sized seqno range can go into the
later condition once urg is in the outstanding window not
yet acknowledge (we're still in urg mode but seqno we sent in
the ack won't be below snd_up). Need to check that we never
advertize bogus urg for a segment which is after the snd_up.
Signed-off-by: Ilpo Järvinen <redacted>
---
net/ipv4/tcp_output.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2009-02-09 07:34:21
On Mon, Feb 09, 2009 at 06:28:06PM +1100, Herbert Xu wrote:
On Mon, Feb 09, 2009 at 09:13:00AM +0200, Ilpo Järvinen wrote:
quoted
Hmm, what happens if a opposite dir ACK gets sent (it's zero sized seqno
range will be above urgent sequence when urg is not yet acknowledged)? So
we'll take the later branch after your change and send bogus 0xffff urg?
I think you're change might have actually broken bidir tcp completely (and
probably window probing as well under some conditions). Congraz, two flies
with a single stroke :-). You would have needed an additional check that
the tcb->seq is below urg...
...Below is a patch but Jeff might need to revert the revert first if
he tests with the latest Linus' tree.
From: Ilpo Järvinen <hidden> Date: 2009-02-09 13:28:24
On Mon, 9 Feb 2009, Herbert Xu wrote:
On Mon, Feb 09, 2009 at 09:13:00AM +0200, Ilpo Järvinen wrote:
quoted
Hmm, what happens if a opposite dir ACK gets sent (it's zero sized seqno
range will be above urgent sequence when urg is not yet acknowledged)? So
we'll take the later branch after your change and send bogus 0xffff urg?
I think you're change might have actually broken bidir tcp completely (and
probably window probing as well under some conditions). Congraz, two flies
with a single stroke :-). You would have needed an additional check that
the tcb->seq is below urg...
...Below is a patch but Jeff might need to revert the revert first if
he tests with the latest Linus' tree.
Good point! That would definitely explain this.
quoted
[PATCH] tcp: check that we're still < urg instead of sending a bogus one
Opposite dir ACK with zero sized seqno range can go into the
later condition once urg is in the outstanding window not
yet acknowledge (we're still in urg mode but seqno we sent in
the ack won't be below snd_up). Need to check that we never
advertize bogus urg for a segment which is after the snd_up.
Signed-off-by: Ilpo Järvinen <redacted>
---
net/ipv4/tcp_output.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
@@ -667,7 +667,8 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,if(between(tp->snd_up,tcb->seq+1,tcb->seq+0xFFFF)){th->urg_ptr=htons(tp->snd_up-tcb->seq);th->urg=1;-}elseif(after(tcb->seq+0xFFFF,tp->snd_nxt)){+}elseif(after(tcb->seq+0xFFFF,tp->snd_nxt)&&+before(tcb->seq,tcp->snd_up)){th->urg_ptr=0xFFFF;th->urg=1;}--
1.5.6.5
Ilpo,
Got this error ...
CC net/ipv4/tcp_output.o
net/ipv4/tcp_output.c: In function 'tcp_transmit_skb':
net/ipv4/tcp_output.c:671: error: 'tcp' undeclared (first use in this function)
net/ipv4/tcp_output.c:671: error: (Each undeclared identifier is
reported only once
net/ipv4/tcp_output.c:671: error: for each function it appears in.)
+ before(tcb->seq, tcp->snd_up)) {
That should be "tp->snd_up" instead of "tcp->snd_up". Other than that,
the good news is I've tested your patch on top of Herbert's patch, and
it works! No more rlogin hangs!
Thanks,
Jeff.
Then the problem isn't my patch at all. My patch only affects
the side sending the urgent data, according to your straces, rlogin
is only receiving urgent data, not sending it. So the patch cannot
have an affect.
The trace is incolclusive about that - just look at it. It's not following
threads/children etc, so it's missing all the relevant pieces.
Which you should have realized, Herbert, since you had the knowledge that
your patch _DOES_ make a difference, enough to bisect to it, and enough
to see that reverting it changes behaviour.
Why are you arguing against facts?
From the trace:
clone(child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0xb7d81918) = 6433
rt_sigaction(SIGURG, {0x80491d0, [URG], SA_RESTART}, {SIG_DFL}, 8) = 0
rt_sigaction(SIGUSR1, {0x8048de0, [USR1], SA_RESTART}, {SIG_DFL}, 8) = 0
rt_sigprocmask(SIG_SETMASK, [], [USR1 URG], 8) = 0
--- SIGURG (Urgent I/O condition) @ 0 (0) ---
kill(6433, SIGURG) = 0
IOW, the trace isn't saying anything at all, because all the real work
probably got done in that child process.
No you're still stracing rlogin, which in this case is the receiver
of the urgent data. Please strace the sending side, which would be
presumably be rlogind in this case.
Herbert, how sure are you that there is no URG data on that side?
You clearly spent _zero_ time actually looking at the trace, so...
Linus
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2009-02-09 21:20:54
On Mon, Feb 09, 2009 at 07:44:56AM -0800, Linus Torvalds wrote:
The trace is incolclusive about that - just look at it. It's not following
threads/children etc, so it's missing all the relevant pieces.
Which you should have realized, Herbert, since you had the knowledge that
your patch _DOES_ make a difference, enough to bisect to it, and enough
to see that reverting it changes behaviour.
Why are you arguing against facts?
quoted
From the trace:
clone(child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0xb7d81918) = 6433
rt_sigaction(SIGURG, {0x80491d0, [URG], SA_RESTART}, {SIG_DFL}, 8) = 0
rt_sigaction(SIGUSR1, {0x8048de0, [USR1], SA_RESTART}, {SIG_DFL}, 8) = 0
rt_sigprocmask(SIG_SETMASK, [], [USR1 URG], 8) = 0
--- SIGURG (Urgent I/O condition) @ 0 (0) ---
kill(6433, SIGURG) = 0
IOW, the trace isn't saying anything at all, because all the real work
probably got done in that child process.
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2009-02-21 13:25:00
On Mon, Feb 09, 2009 at 03:28:09PM +0200, Ilpo Järvinen wrote:
Yes, that should work too except the gcc catchable typo.
Thanks, so let's try again for net-next:
tcp: Always set urgent pointer if it's beyond snd_nxt
Our TCP stack does not set the urgent flag if the urgent pointer
does not fit in 16 bits, i.e., if it is more than 64K from the
sequence number of a packet.
This behaviour is different from the BSDs, and clearly contradicts
the purpose of urgent mode, which is to send the notification
(though not necessarily the associated data) as soon as possible.
Our current behaviour may in fact delay the urgent notification
indefinitely if the receiver window does not open up.
Simply matching BSD however may break legacy applications which
incorrectly rely on the out-of-band delivery of urgent data, and
conversely the in-band delivery of non-urgent data.
Alexey Kuznetsov suggested a safe solution of following BSD only
if the urgent pointer itself has not yet been transmitted. This
way we guarantee that when the remote end sees the packet with
non-urgent data marked as urgent due to wrap-around we would have
advanced the urgent pointer beyond, either to the actual urgent
data or to an as-yet untransmitted packet.
The only potential downside is that applications on the remote
end may see multiple SIGURG notifications. However, this would
occur anyway with other TCP stacks. More importantly, the outcome
of such a duplicate notification is likely to be harmless since
the signal itself does not carry any information other than the
fact that we're in urgent mode.
Thanks to Ilpo Järvinen for fixing a critical bug in this and
Jeff Chua for reporting that bug.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
@@ -663,10 +663,14 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,th->urg_ptr=0;/* The urg_mode check is necessary during a below snd_una win probe */-if(unlikely(tcp_urg_mode(tp)&&-between(tp->snd_up,tcb->seq+1,tcb->seq+0xFFFF))){-th->urg_ptr=htons(tp->snd_up-tcb->seq);-th->urg=1;+if(unlikely(tcp_urg_mode(tp)&&before(tcb->seq,tp->snd_up))){+if(before(tp->snd_up,tcb->seq+0x10000)){+th->urg_ptr=htons(tp->snd_up-tcb->seq);+th->urg=1;+}elseif(after(tcb->seq+0xFFFF,tp->snd_nxt)){+th->urg_ptr=0xFFFF;+th->urg=1;+}}tcp_options_write((__be32*)(th+1),tp,&opts,&md5_hash_location);
From: Ilpo Järvinen <hidden> Date: 2009-02-21 18:54:25
On Sat, 21 Feb 2009, Herbert Xu wrote:
quoted hunk
On Mon, Feb 09, 2009 at 03:28:09PM +0200, Ilpo Järvinen wrote:
quoted
Yes, that should work too except the gcc catchable typo.
Thanks, so let's try again for net-next:
tcp: Always set urgent pointer if it's beyond snd_nxt
Our TCP stack does not set the urgent flag if the urgent pointer
does not fit in 16 bits, i.e., if it is more than 64K from the
sequence number of a packet.
This behaviour is different from the BSDs, and clearly contradicts
the purpose of urgent mode, which is to send the notification
(though not necessarily the associated data) as soon as possible.
Our current behaviour may in fact delay the urgent notification
indefinitely if the receiver window does not open up.
Simply matching BSD however may break legacy applications which
incorrectly rely on the out-of-band delivery of urgent data, and
conversely the in-band delivery of non-urgent data.
Alexey Kuznetsov suggested a safe solution of following BSD only
if the urgent pointer itself has not yet been transmitted. This
way we guarantee that when the remote end sees the packet with
non-urgent data marked as urgent due to wrap-around we would have
advanced the urgent pointer beyond, either to the actual urgent
data or to an as-yet untransmitted packet.
The only potential downside is that applications on the remote
end may see multiple SIGURG notifications. However, this would
occur anyway with other TCP stacks. More importantly, the outcome
of such a duplicate notification is likely to be harmless since
the signal itself does not carry any information other than the
fact that we're in urgent mode.
Thanks to Ilpo Järvinen for fixing a critical bug in this and
Jeff Chua for reporting that bug.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
@@ -663,10 +663,14 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,th->urg_ptr=0;/* The urg_mode check is necessary during a below snd_una win probe */-if(unlikely(tcp_urg_mode(tp)&&-between(tp->snd_up,tcb->seq+1,tcb->seq+0xFFFF))){-th->urg_ptr=htons(tp->snd_up-tcb->seq);-th->urg=1;+if(unlikely(tcp_urg_mode(tp)&&before(tcb->seq,tp->snd_up))){+if(before(tp->snd_up,tcb->seq+0x10000)){+th->urg_ptr=htons(tp->snd_up-tcb->seq);+th->urg=1;+}elseif(after(tcb->seq+0xFFFF,tp->snd_nxt)){+th->urg_ptr=0xFFFF;+th->urg=1;+}}tcp_options_write((__be32*)(th+1),tp,&opts,&md5_hash_location);
I thought this through already at the last time... so I think it really
should work in all cases I could think of (including the below window
probes whose existance is not very obvious before they bite back :-)):
Acked-by: Ilpo Järvinen <redacted>
But I mostly agree with Linus that URG is legacy that would be good
to just deprecate instead of trying to "fix" it.
--
i.
On Mon, Feb 09, 2009 at 03:28:09PM +0200, Ilpo Järvinen wrote:
quoted
Yes, that should work too except the gcc catchable typo.
Thanks, so let's try again for net-next:
tcp: Always set urgent pointer if it's beyond snd_nxt
...
I thought this through already at the last time... so I think it really
should work in all cases I could think of (including the below window
probes whose existance is not very obvious before they bite back :-)):
Acked-by: Ilpo Järvinen <redacted>
But I mostly agree with Linus that URG is legacy that would be good
to just deprecate instead of trying to "fix" it.
Sure, but I'll give Herbert one more chance :-)
Applied to net-next-2.6
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2009-02-24 06:45:04
On Tue, Feb 24, 2009 at 02:40:30PM +0800, Jeff Chua wrote:
On Sun, Feb 22, 2009 at 3:53 PM, David Miller [off-list ref] wrote:
quoted
Sure, but I'll give Herbert one more chance :-)
Applied to net-next-2.6
I see that this is already in linux-2.6.29-rc6. Tested and working fine.
Are you sure? It's not in my copy of rc6 :)
net-next-2.6 is for the release after the current one so the
earliest you're going to see this is in 2.6.30.
For now you'll either need to pull net-next-2.6 directly, use
linux-next, or apply the patch by hand.
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: Jeff Chua <hidden> Date: 2009-02-24 08:01:54
On Tue, Feb 24, 2009 at 2:46 PM, David Miller [off-list ref] wrote:
Actually, it isn't.
Herbert's change is in net-next-2.6 which will show up in linux-next
rather than Linus's 2.6.29-rcX tree.
Uh, I got it wrong. Didn't look into details. Just noticed that
tcp_output.c was changed today, so I just assume that was the one ...
which turned out to a one-liner comment removed.
Anyway, I just tested the Herbert's patch and it looks good. rlogin not hanging.
Sorry for the earlier fault report.
Thanks
Jeff.