Re: commit 64ff3b938ec6782e6585a83d5459b98b0c3f6eb8 breaks rlogin

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(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 557fe16..c808d73 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c

@@ -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;
-		} else if (after(tcb->seq + 0xFFFF, tp->snd_nxt)) {
+		} else if (after(tcb->seq + 0xFFFF, tp->snd_nxt) &&
+			   before(tcb->seq, tcp->snd_up)) {
 			th->urg_ptr = 0xFFFF;
 			th->urg = 1;
 		}

We could rewrite this as

	if (unlikely(tcp_urg_mode(tp) && before(tcb->seq, tcp->snd_up))) {
		if (before(tp->sndup, tcb->seq + 0x10000)) {
			th->urg_ptr = htons(tp->snd_up - tcb->seq);
			th->urg = 1;
		} else if (after(tcb->seq + 0xFFFF, tp->snd_nxt)) {
			th->urg_ptr = 0xFFFF;
			th->urg = 1;
		}
	}

Yes, that should work too except the gcc catchable typo.


-- 
 i.