Patchwork commit 64ff3b938ec6782e6585a83d5459b98b0c3f6eb8 breaks rlogin

login
register
mail settings
Submitter Herbert Xu
Date Feb. 21, 2009, 1:24 p.m.
Message ID <20090221132424.GA28861@gondor.apana.org.au>
Download mbox | patch
Permalink /patch/23518/
State Accepted
Delegated to: David Miller
Headers show

Comments

Herbert Xu - Feb. 21, 2009, 1:24 p.m.
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>


Cheers,
Ilpo Järvinen - Feb. 21, 2009, 6:54 p.m.
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.
David Miller - Feb. 22, 2009, 7:53 a.m.
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
Jeff Chua - Feb. 24, 2009, 6:40 a.m.
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
Herbert Xu - Feb. 24, 2009, 6:44 a.m.
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,
David Miller - Feb. 24, 2009, 6:46 a.m.
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
Jeff Chua - Feb. 24, 2009, 8:01 a.m.
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

Patch

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