netdev - Re: [PATCH] tcp: make urg+gso work for real this time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.64.0812032025570.26311@wrl-59.cs.helsinki.fi>
Date:	Wed, 3 Dec 2008 20:31:09 +0200 (EET)
From:	"Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
To:	Petr Tesarik <ptesarik@...e.cz>
cc:	David Miller <davem@...emloft.net>, Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] tcp: make urg+gso work for real this time

On Wed, 3 Dec 2008, Petr Tesarik wrote:

> Ilpo Järvinen píse v St 03. 12. 2008 v 13:22 +0200:
> > I should have noticed this earlier... :-) The previous solution
> > to URG+GSO/TSO will cause SACK block tcp_fragment to do zig-zig
> > patterns, or even worse, a steep downward slope into packet
> > counting because each skb pcount would be truncated to pcount
> > of 2 and then the following fragments of the later portion would
> > restore the window again.
> 
> Oops, not nice. :(
> 
> > Basically this reverts "tcp: Do not use TSO/GSO when there is
> > urgent data" (33cf71cee1). It also removes some unnecessary code
> > from tcp_current_mss that didn't work as intented either (could
> > be that something was changed down the road, or it might have
> > been broken since the dawn of time) because it only works once
> > urg is already written while this bug shows up starting from
> > ~64k before the urg point.
> > 
> > The retransmissions already are split to mss sized chunks, so
> > only new data sending paths need splitting in case they have
> > a segment otherwise suitable for gso/tso. The actually check
> > can be improved to be more narrow but since this is late -rc
> > already, I'll postpone thinking the more fine-grained things.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
> > Cc: Petr Tesarik <ptesarik@...e.cz>
> 
> Yes, re-fragmenting the packet here is probably the best way to go. But
> repeating the same condition on multiple places is not so nice. What
> about this little improvement?

Yeah, this too will work... Though not of too much significance since
I've already most of unification of those two functions in a local
queue awaiting testing, polish up and submit to net-next.

When sending an updated version davem prefers to have full changelog in it 
btw, it saves some work for him.

> (I didn't roll the revert of 33cf71cee1 into it, as it makes the actual
> change less obvious. I think there are better ways of doing a revert in
> git.)

Whatever, I've no strong opinion either way though it's just broken with 
and without the revert anyway, the full thing is needed to actually fix 
it.

> Signed-off-by: Petr Tesarik <ptesarik@...e.cz>
> Cc: Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
>
> --
>  tcp_output.c |   13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index ba85d88..a45c62d 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1028,10 +1028,6 @@ unsigned int tcp_sync_mss(struct sock *sk, u32 pmtu)
>  
>  /* Compute the current effective MSS, taking SACKs and IP options,
>   * and even PMTU discovery events into account.
> - *
> - * LARGESEND note: !tcp_urg_mode is overkill, only frames up to snd_up
> - * cannot be large. However, taking into account rare use of URG, this
> - * is not a big flaw.
>   */
>  unsigned int tcp_current_mss(struct sock *sk, int large_allowed)
>  {
> @@ -1046,7 +1042,7 @@ unsigned int tcp_current_mss(struct sock *sk, int large_allowed)
>  
>  	mss_now = tp->mss_cache;
>  
> -	if (large_allowed && sk_can_gso(sk) && !tcp_urg_mode(tp))
> +	if (large_allowed && sk_can_gso(sk))
>  		doing_tso = 1;
>  
>  	if (dst) {
> @@ -1113,6 +1109,10 @@ static void tcp_cwnd_validate(struct sock *sk)
>   * return whenever allowed by the other factors). Basically we need the
>   * modulo only when the receiver window alone is the limiting factor or
>   * when we would be allowed to send the split-due-to-Nagle skb fully.
> + *
> + * LARGESEND note: !tcp_urg_mode is overkill, only frames between
> + * snd_up-64k-mss .. snd_up cannot be large. However, taking into
> + * account rare use of URG, this is not a big flaw.
>   */
>  static unsigned int tcp_mss_split_point(struct sock *sk, struct sk_buff *skb,
>  					unsigned int mss_now, unsigned int cwnd)
> @@ -1120,6 +1120,9 @@ static unsigned int tcp_mss_split_point(struct sock *sk, struct sk_buff *skb,
>  	struct tcp_sock *tp = tcp_sk(sk);
>  	u32 needed, window, cwnd_len;
>  
> +	if (unlikely(tcp_urg_mode(tp)))
> +		return mss_now;
> +
>  	window = tcp_wnd_end(tp) - TCP_SKB_CB(skb)->seq;
>  	cwnd_len = mss_now * cwnd;

Acked-by: Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>

-- 
 i.