| Message ID | 20090221132424.GA28861@gondor.apana.org.au |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, 21 Feb 2009, Herbert Xu wrote: > 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> > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index dda42f0..f5263c8 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -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; > + } else if (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 <ilpo.jarvinen@helsinki.fi> But I mostly agree with Linus that URG is legacy that would be good to just deprecate instead of trying to "fix" it.
From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> Date: Sat, 21 Feb 2009 20:54:11 +0200 (EET) > On Sat, 21 Feb 2009, Herbert Xu wrote: > > > 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 ... > 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 <ilpo.jarvinen@helsinki.fi> > > 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 -- 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
On Sun, Feb 22, 2009 at 3:53 PM, David Miller <davem@davemloft.net> wrote: > 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. 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
On Tue, Feb 24, 2009 at 02:40:30PM +0800, Jeff Chua wrote: > On Sun, Feb 22, 2009 at 3:53 PM, David Miller <davem@davemloft.net> wrote: > > > 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,
From: Jeff Chua <jeff.chua.linux@gmail.com> Date: Tue, 24 Feb 2009 14:40:30 +0800 > On Sun, Feb 22, 2009 at 3:53 PM, David Miller <davem@davemloft.net> wrote: > > > 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. 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. -- 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
On Tue, Feb 24, 2009 at 2:46 PM, David Miller <davem@davemloft.net> 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. -- 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 dda42f0..f5263c8 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -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; + } else if (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);