[PATCH] net/ipv4/tcp.c little cleanup

[PATCH] net/ipv4/tcp.c little cleanup

[Eduard Roccatello]

[-- Attachment #1: Type: text/plain, Size: 106 bytes --]

Hello,
i've done a little cleanup to net/ipv4/tcp.c

I hope it is ok :-)

Best regards,
Eduard Roccatello

[-- Attachment #2: tcp.c.patch --]
[-- Type: text/x-diff, Size: 561 bytes --]

--- net/ipv4/tcp.c.orig	2004-01-22 22:49:38.000000000 +0100
+++ net/ipv4/tcp.c	2004-01-22 22:42:38.000000000 +0100
@@ -549,9 +549,9 @@ int tcp_listen_start(struct sock *sk)
 		return -ENOMEM;
 
 	memset(lopt, 0, sizeof(struct tcp_listen_opt));
-	for (lopt->max_qlen_log = 6; ; lopt->max_qlen_log++)
-		if ((1 << lopt->max_qlen_log) >= sysctl_max_syn_backlog)
-			break;
+	lopt->max_qlen_log = 6;
+	while (sysctl_max_syn_backlog > (1 << lopt->max_qlen_log))
+		lopt->max_qlen_log++;
 	get_random_bytes(&lopt->hash_rnd, 4);
 
 	write_lock_bh(&tp->syn_wait_lock);

Re: [PATCH] net/ipv4/tcp.c little cleanup

[Willy Tarreau]

Hi !

On Thu, Jan 22, 2004 at 10:53:37PM +0100, Eduard Roccatello wrote:
> Hello,
> i've done a little cleanup to net/ipv4/tcp.c
> 
> I hope it is ok :-)

I haven't looked at sysctl_max_syn_backlog type, but if it's unsigned, there's
a risk of infinite loop for values above 2^31 on 32 bits machines, or 2^63 on
64 bits machine. This is because many processors leave the result undefined
when you shift more bits that the word size. Often, only the lowest bits are
used for the shift count, resulting in a modulo.

Eg, on ia32, if sysctl_max_syn_backlog is >2^31, the test will never work, and
when max_qlen_log becomes 32, it will give the same result as if it were 0.
Another case is if sysctl_max_syn_backlog is above 2^30, and the shift returns
a signed result, because 1<<31 will be negative, thus validating the test and
maintain the loop.

Note that this potential problem is also present in the code you replaced.

Cheers,
Willy

> --- net/ipv4/tcp.c.orig	2004-01-22 22:49:38.000000000 +0100
> +++ net/ipv4/tcp.c	2004-01-22 22:42:38.000000000 +0100
> @@ -549,9 +549,9 @@ int tcp_listen_start(struct sock *sk)
>  	 	return -ENOMEM;
>  
> 	memset(lopt, 0, sizeof(struct tcp_listen_opt));
> -	for (lopt->max_qlen_log = 6; ; lopt->max_qlen_log++)
> -		if ((1 << lopt->max_qlen_log) >= sysctl_max_syn_backlog)
> -			break;
> +	lopt->max_qlen_log = 6;
> +	while (sysctl_max_syn_backlog > (1 << lopt->max_qlen_log))
> +		lopt->max_qlen_log++;
>  	get_random_bytes(&lopt->hash_rnd, 4);
>  
>  	write_lock_bh(&tp->syn_wait_lock);
> 

Re: [PATCH] net/ipv4/tcp.c little cleanup

[Eduard Roccatello]

On Friday 23 January 2004 00:48, Willy Tarreau wrote:
> Hi !
>
> On Thu, Jan 22, 2004 at 10:53:37PM +0100, Eduard Roccatello wrote:
> > Hello,
> > i've done a little cleanup to net/ipv4/tcp.c
> >
> > I hope it is ok :-)
>
> I haven't looked at sysctl_max_syn_backlog type, but if it's unsigned,
> there's a risk of infinite loop for values above 2^31 on 32 bits
> machines, or 2^63 on 64 bits machine.
sysctl_max_syn_backlog is an int and max_qlen_log is a u8 (uint8_t).
i think there is no problem with them.
sysctl_max_syn_backlog max value is 1024 so max_qlen_log is just 9.

is it ok for you?

> > --- net/ipv4/tcp.c.orig	2004-01-22 22:49:38.000000000 +0100
> > +++ net/ipv4/tcp.c	2004-01-22 22:42:38.000000000 +0100
> > @@ -549,9 +549,9 @@ int tcp_listen_start(struct sock *sk)
> >  	 	return -ENOMEM;
> >
> > 	memset(lopt, 0, sizeof(struct tcp_listen_opt));
> > -	for (lopt->max_qlen_log = 6; ; lopt->max_qlen_log++)
> > -		if ((1 << lopt->max_qlen_log) >= sysctl_max_syn_backlog)
> > -			break;
> > +	lopt->max_qlen_log = 6;
> > +	while (sysctl_max_syn_backlog > (1 << lopt->max_qlen_log))
> > +		lopt->max_qlen_log++;
> >  	get_random_bytes(&lopt->hash_rnd, 4);
> >
> >  	write_lock_bh(&tp->syn_wait_lock);

Re: [PATCH] net/ipv4/tcp.c little cleanup

[Willy Tarreau]

Hi,

On Fri, Jan 23, 2004 at 09:03:05PM +0100, Eduard Roccatello wrote:
> sysctl_max_syn_backlog is an int and max_qlen_log is a u8 (uint8_t).
> i think there is no problem with them.
> sysctl_max_syn_backlog max value is 1024 so max_qlen_log is just 9.
> 
> is it ok for you?

it's ok, thanks Eduard for this clarification.

Cheers,
Willy