Message ID | Pine.LNX.4.64.0902090853360.20576@wrl-59.cs.helsinki.fi |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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. > [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 <ilpo.jarvinen@helsinki.fi> > --- > 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; } } Thanks,
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: > > > > 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. Actually, I don't see how we can have tcp_urg_mode being true when his strace shows no OOB data at all (it only did write and writev). So I'm not sure if this bug is the one causing this. Cheers,
On Mon, 9 Feb 2009, Herbert Xu wrote: > 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. > > > [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 <ilpo.jarvinen@helsinki.fi> > > --- > > 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.
On Mon, Feb 9, 2009 at 3:13 PM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote: > 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; > } > -- > 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. -- 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
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; }