Skip to contentSkip to conversation

net/ipv4/tcp.c little cleanup

PATCH In review Eduard Roccatello 2 participants 4 messages Jan 22, 2004 – Jan 23, 2004

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Eduard Roccatello

author
#0 lore raw

PATCH 1/1 net/ipv4/tcp.c little cleanup

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

I hope it is ok :-)

Best regards,
Eduard Roccatello

ipv4/tcp.c renamed from ipv4/tcp.c.orig +3 -3
@@ -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);
#1 lore raw

Hi !

Quoted 4 lines · Eduard Roccatello

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

Quoted 16 lines
ipv4/tcp.c renamed from ipv4/tcp.c.orig +3 -3
@@ -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);
author
#2 lore raw
Quoted 11 lines · Willy Tarreau

Hi !

Quoted 4 lines · Eduard Roccatello

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?

Quoted 15 lines
Quoted 15 lines
ipv4/tcp.c renamed from ipv4/tcp.c.orig +3 -3
@@ -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);
#3 lore raw

Hi,

Quoted 5 lines · Eduard Roccatello

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