Skip to contentSkip to conversation

__inet_hash_connect port_offset parameter

PATCH In review Crestez Dan Leonard 3 participants 5 messages May 27, 2015

Subsystems (3)
Networking Network drivers TCP
lore
Share & tools
lore

Crestez Dan Leonard

author
#0 lore raw

Hello,

I'm confused about the port_offset parameter to __inet_hash_connect.

When allocating the local port for an outgoing TCP connection the port search looks something like this:

    static u32 hint;
    u32 offset = hint + port_offset;

    inet_get_local_port_range(net, &low, &high);
    remaining = (high - low) + 1;

    for (i = 1; i <= remaining; i++) {
        port = low + (i + offset) % remaining;
        /* check port is free */

The port_offset is calculated for v4 and v6 based on a hash of src/dst addresses, presumably in order to improve security.

I see a few issues with this:
- The port_offset is calculated even if the local port was already assigned via bind. This wastes a few cycles.
- Keeping the last searched port as a static variable is a bad idea on multicore cpus. Starting a lot of connections to the same target will result in lock contention in the bind hash. This is probably only visible in highly synthetic tests.
- When doing a port search at bind() time the search starts from "prandom_32()". Is this "less secure" for port allocation? I bet most applications are not aware of this difference.

Wouldn't it be better to use the same local port search mechanism at both bind (inet_csk_get_port) and connect (__inet_hash_connect) time, based on starting from a random point? It would also make connecting slightly faster.

Regards,
Leonard

#1 lore raw
Quoted 17 lines · Crestez Dan Leonard

Hello,

I'm confused about the port_offset parameter to __inet_hash_connect.

When allocating the local port for an outgoing TCP connection the port search looks something like this:

    static u32 hint;
    u32 offset = hint + port_offset;

    inet_get_local_port_range(net, &low, &high);
    remaining = (high - low) + 1;

    for (i = 1; i <= remaining; i++) {
        port = low + (i + offset) % remaining;
        /* check port is free */
Quoted 6 lines

The port_offset is calculated for v4 and v6 based on a hash of src/dst addresses, presumably in order to improve security.

I see a few issues with this:
- The port_offset is calculated even if the local port was already
assigned via bind. This wastes a few cycles.

OK. Not a big deal I guess.

Quoted 4 lines

- Keeping the last searched port as a static variable is a bad idea
on multicore cpus. Starting a lot of connections to the same target will result in lock contention in the bind hash. This is probably only visible in highly synthetic tests.

Not really. This is a hint only.

I have one patch adding an array of hints, but this does not change behavior if connecting to same target.

u32 key = port_offset % HINTS_SZ;

Quoted 3 lines

- When doing a port search at bind() time the search starts from
"prandom_32()". Is this "less secure" for port allocation? I bet most applications are not aware of this difference.

I am afraid we need to keep a sequential search for ephemeral port selection.

This known behavior is described in RFC 6056

Quoted 5 lines

Wouldn't it be better to use the same local port search mechanism at both bind (inet_csk_get_port) and connect (__inet_hash_connect) time, based on starting from a random point? It would also make connecting slightly faster.

Nope, bind() and connect() have different semantics.

The randomization is only good for bind(port=0) users.

Have you seen my proposal ?

https://patchwork.ozlabs.org/patch/476002/

#2 lore raw
Quoted 18 lines · Eric Dumazet
Quoted 17 lines · Crestez Dan Leonard

Hello,

I'm confused about the port_offset parameter to __inet_hash_connect.

When allocating the local port for an outgoing TCP connection the port search looks something like this:

    static u32 hint;
    u32 offset = hint + port_offset;

    inet_get_local_port_range(net, &low, &high);
    remaining = (high - low) + 1;

    for (i = 1; i <= remaining; i++) {
        port = low + (i + offset) % remaining;
        /* check port is free */
Quoted 8 lines
Quoted 6 lines

The port_offset is calculated for v4 and v6 based on a hash of src/dst addresses, presumably in order to improve security.

I see a few issues with this:
- The port_offset is calculated even if the local port was already
assigned via bind. This wastes a few cycles.

OK. Not a big deal I guess.

Patch for IPv4 would be :

net/ipv4/inet_hashtables.c +5 -1
index 185efef0f1251ba9d45fabb3ed51777a8be097a6..be4bac368b6bfb8a1eca429cce415da99adc5515 100644
@@ -594,7 +594,11 @@ out:
 int inet_hash_connect(struct inet_timewait_death_row *death_row,
 		      struct sock *sk)
 {
-	return __inet_hash_connect(death_row, sk, inet_sk_port_offset(sk),
+	u32 port_offset = 0;
+
+	if (!inet_sk(sk)->inet_num)
+		port_offset = inet_sk_port_offset(sk);
+	return __inet_hash_connect(death_row, sk, port_offset,
 				   __inet_check_established);
 }
 EXPORT_SYMBOL_GPL(inet_hash_connect);
#3 lore raw

PATCH 1/1 tcp: connect() from bound sockets can be faster

From: Eric Dumazet <[email protected]>

__inet_hash_connect() does not use its third argument (port_offset) if socket was already bound to a source port.

No need to perform useless but expensive md5 computations.

Reported-by Crestez Dan Leonard <[email protected]>
Signed-off-by Eric Dumazet <[email protected]>
---
 net/ipv4/inet_hashtables.c  |    9 +++++++--
 net/ipv6/inet6_hashtables.c |    8 ++++++--
 2 files changed, 13 insertions(+), 4 deletions(-)
net/ipv4/inet_hashtables.c +7 -2
index 185efef0f1251ba9d45fabb3ed51777a8be097a6..e36942f0725d2e2d15f8d248b19a34c3487528ef 100644
@@ -394,9 +394,10 @@ not_unique:
 	return -EADDRNOTAVAIL;
 }
 
-static inline u32 inet_sk_port_offset(const struct sock *sk)
+static u32 inet_sk_port_offset(const struct sock *sk)
 {
 	const struct inet_sock *inet = inet_sk(sk);
+
 	return secure_ipv4_port_ephemeral(inet->inet_rcv_saddr,
 					  inet->inet_daddr,
 					  inet->inet_dport);

@@ -594,7 +595,11 @@ out:
 int inet_hash_connect(struct inet_timewait_death_row *death_row,
 		      struct sock *sk)
 {
-	return __inet_hash_connect(death_row, sk, inet_sk_port_offset(sk),
+	u32 port_offset = 0;
+
+	if (!inet_sk(sk)->inet_num)
+		port_offset = inet_sk_port_offset(sk);
+	return __inet_hash_connect(death_row, sk, port_offset,
 				   __inet_check_established);
 }
 EXPORT_SYMBOL_GPL(inet_hash_connect);
net/ipv6/inet6_hashtables.c +6 -2
index 871641bc1ed4eb5b8f5f554c9efb75f2419fe5b6..b4fd96de97e61627003eff220e10bdd05a899e28 100644
@@ -257,7 +257,7 @@ not_unique:
 	return -EADDRNOTAVAIL;
 }
 
-static inline u32 inet6_sk_port_offset(const struct sock *sk)
+static u32 inet6_sk_port_offset(const struct sock *sk)
 {
 	const struct inet_sock *inet = inet_sk(sk);
 

@@ -269,7 +269,11 @@ static inline u32 inet6_sk_port_offset(const struct sock *sk)
 int inet6_hash_connect(struct inet_timewait_death_row *death_row,
 		       struct sock *sk)
 {
-	return __inet_hash_connect(death_row, sk, inet6_sk_port_offset(sk),
+	u32 port_offset = 0;
+
+	if (!inet_sk(sk)->inet_num)
+		port_offset = inet6_sk_port_offset(sk);
+	return __inet_hash_connect(death_row, sk, port_offset,
 				   __inet6_check_established);
 }
 EXPORT_SYMBOL_GPL(inet6_hash_connect);
maintainer
#4 lore raw

Re: [PATCH net-next] tcp: connect() from bound sockets can be faster

From: Eric Dumazet <[email protected]>
Date: Wed, 27 May 2015 10:46:02 -0700

Quoted 9 lines

From: Eric Dumazet <[email protected]>

__inet_hash_connect() does not use its third argument (port_offset) if socket was already bound to a source port.

No need to perform useless but expensive md5 computations.

Reported-by Crestez Dan Leonard <[email protected]>
Signed-off-by Eric Dumazet <[email protected]>

Looks great, applied, thanks Eric!