[PATCH net-next] tcp: change source port selection at bind() time

[PATCH net-next] tcp: change source port selection at bind() time

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

This is a follow-up patch to an eariler commit 207184853dbd ("tcp/dccp:
change source port selection at connect() time").

This patch extends the use of IP_LOCAL_PORT_RANGE option, so that we
don't need to iterate every two ports which means only favouring odd
number like the old days before 2016, which can be good for some
users who want to keep in consistency with IP_LOCAL_PORT_RANGE in
connect().

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 net/ipv4/inet_connection_sock.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 64d07b842e73..6eb509832c90 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -333,19 +333,20 @@ inet_csk_find_open_port(const struct sock *sk, struct inet_bind_bucket **tb_ret,
 			struct inet_bind_hashbucket **head2_ret, int *port_ret)
 {
 	struct inet_hashinfo *hinfo = tcp_or_dccp_get_hashinfo(sk);
-	int i, low, high, attempt_half, port, l3mdev;
+	int i, low, high, attempt_half, port, l3mdev, step;
 	struct inet_bind_hashbucket *head, *head2;
+	bool local_ports, relax = false;
 	struct net *net = sock_net(sk);
 	struct inet_bind2_bucket *tb2;
 	struct inet_bind_bucket *tb;
 	u32 remaining, offset;
-	bool relax = false;
 
 	l3mdev = inet_sk_bound_l3mdev(sk);
 ports_exhausted:
 	attempt_half = (sk->sk_reuse == SK_CAN_REUSE) ? 1 : 0;
 other_half_scan:
-	inet_sk_get_local_port_range(sk, &low, &high);
+	local_ports = inet_sk_get_local_port_range(sk, &low, &high);
+	step = local_ports ? 1 : 2;
 	high++; /* [32768, 60999] -> [32768, 61000[ */
 	if (high - low < 4)
 		attempt_half = 0;
@@ -358,18 +359,19 @@ inet_csk_find_open_port(const struct sock *sk, struct inet_bind_bucket **tb_ret,
 			low = half;
 	}
 	remaining = high - low;
-	if (likely(remaining > 1))
+	if (!local_ports && remaining > 1)
 		remaining &= ~1U;
 
 	offset = get_random_u32_below(remaining);
 	/* __inet_hash_connect() favors ports having @low parity
 	 * We do the opposite to not pollute connect() users.
 	 */
-	offset |= 1U;
+	if (!local_ports)
+		offset |= 1U;
 
 other_parity_scan:
 	port = low + offset;
-	for (i = 0; i < remaining; i += 2, port += 2) {
+	for (i = 0; i < remaining; i += step, port += step) {
 		if (unlikely(port >= high))
 			port -= remaining;
 		if (inet_is_local_reserved_port(net, port))
@@ -400,9 +402,11 @@ inet_csk_find_open_port(const struct sock *sk, struct inet_bind_bucket **tb_ret,
 		cond_resched();
 	}
 
-	offset--;
-	if (!(offset & 1))
-		goto other_parity_scan;
+	if (!local_ports) {
+		offset--;
+		if (!(offset & 1))
+			goto other_parity_scan;
+	}
 
 	if (attempt_half == 1) {
 		/* OK we now try the upper half of the range */
-- 
2.37.3

Re: [PATCH net-next] tcp: change source port selection at bind() time

[Eric Dumazet]

On Fri, Aug 16, 2024 at 5:33 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> This is a follow-up patch to an eariler commit 207184853dbd ("tcp/dccp:
> change source port selection at connect() time").
>
> This patch extends the use of IP_LOCAL_PORT_RANGE option, so that we
> don't need to iterate every two ports which means only favouring odd
> number like the old days before 2016, which can be good for some
> users who want to keep in consistency with IP_LOCAL_PORT_RANGE in
> connect().

Except that bind() with a port reservation is not as common as a connect().
This is highly discouraged.

See IP_BIND_ADDRESS_NO_PORT

Can you provide a real use case ?

I really feel like you are trying to push patches 'just because you can'...

'The old days' before 2016 were not very nice, we had P0 all the time
because of port exhaustion.
Since 2016 and IP_BIND_ADDRESS_NO_PORT I no longer have war rooms stories.

Re: [PATCH net-next] tcp: change source port selection at bind() time

[Jason Xing]

On Mon, Aug 19, 2024 at 11:45 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Aug 16, 2024 at 5:33 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > This is a follow-up patch to an eariler commit 207184853dbd ("tcp/dccp:
> > change source port selection at connect() time").
> >
> > This patch extends the use of IP_LOCAL_PORT_RANGE option, so that we
> > don't need to iterate every two ports which means only favouring odd
> > number like the old days before 2016, which can be good for some
> > users who want to keep in consistency with IP_LOCAL_PORT_RANGE in
> > connect().
>
> Except that bind() with a port reservation is not as common as a connect().
> This is highly discouraged.
>
> See IP_BIND_ADDRESS_NO_PORT
>
> Can you provide a real use case ?
>
> I really feel like you are trying to push patches 'just because you can'...

You apparently got me wrong and hurt my feelings :/

Since you asked me about this one, I'm going to tell you the whole story.

A few years ago, one of my colleagues reached out to you and
complained about the issue of the new port selection algorithm
(odd/even port selection) to you. But a few years later, the community
finally implemented/extended such IP_LOCAL_PORT_RANGE option to
support finding a suitable port one by one. I'm not sure if you
remembered.

Nearly every month I get issue reports about why the latest kernel
applies a new algorithm because of the high cpu-load increasing so
much suddenly, then I will let them use the older one which I use a
sysctl knob to control internally.

If you pay more attention in these years, you've received more than
one patch trying to use the older algorithm provided for users, even
though they are not accepted. You may ignore the fact. For me, I just
want to provide a way/patch to use the older algorithm for connect()
and bind(), which can be accepted by the community relatively easily.
That's all. I don't believe we are the only special one! So I want to
change something!

Like that 'self-connection' issue, we are also not the only one, so I
exert/push myself to solve the issue thoroughly.

To be honest, whether you accept the patch, I cannot control it, but I
will try my best to do some useful reports and provide real use cases
which are derived from our production.

That's not bad, right?

Re: [PATCH net-next] tcp: change source port selection at bind() time

[Jason Xing]

Hello Eric,

On Mon, Aug 19, 2024 at 11:45 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Aug 16, 2024 at 5:33 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > This is a follow-up patch to an eariler commit 207184853dbd ("tcp/dccp:
> > change source port selection at connect() time").
> >
> > This patch extends the use of IP_LOCAL_PORT_RANGE option, so that we
> > don't need to iterate every two ports which means only favouring odd
> > number like the old days before 2016, which can be good for some
> > users who want to keep in consistency with IP_LOCAL_PORT_RANGE in
> > connect().
>
> Except that bind() with a port reservation is not as common as a connect().
> This is highly discouraged.
>
> See IP_BIND_ADDRESS_NO_PORT
>
> Can you provide a real use case ?
>
> I really feel like you are trying to push patches 'just because you can'...
>
> 'The old days' before 2016 were not very nice, we had P0 all the time
> because of port exhaustion.
> Since 2016 and IP_BIND_ADDRESS_NO_PORT I no longer have war rooms stories.

As you mentioned last night, the issues happening in connect() are
relatively more than in bind().

To be more concise, I would like to state 3 points to see if they are valid:
(1) Extending the option for bind() is the last puzzle of using an
older algorithm for some users. Since we have one in connect(), how
about adding it in bind() to provide for the people favouring the
older algorithm.
(2) This patch will not hurt any users like in Google as an example
which prefers odd/even port selection, which is, I admit, indeed more
advanced.
(3) This patch does not come out of thin air, but from some users who I contact.
?

In my opinion, using and adjusting to the new algorithm needs some
changes in applications. For some old applications, they still need
more time to keep pace with a more workable solution.

After considering it a whole night, I would like to push this tiny
feature into the upstream kernel, I wonder if you can help me review
it? Thanks in advance, Eric.

Thanks,
Jason

Re: [PATCH net-next] tcp: change source port selection at bind() time

[Kuniyuki Iwashima]

From: Jason Xing <kerneljasonxing@gmail.com>
Date: Tue, 20 Aug 2024 08:53:53 +0800
> Hello Eric,
> 
> On Mon, Aug 19, 2024 at 11:45 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Fri, Aug 16, 2024 at 5:33 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > >
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > This is a follow-up patch to an eariler commit 207184853dbd ("tcp/dccp:
> > > change source port selection at connect() time").
> > >
> > > This patch extends the use of IP_LOCAL_PORT_RANGE option, so that we
> > > don't need to iterate every two ports which means only favouring odd
> > > number like the old days before 2016, which can be good for some
> > > users who want to keep in consistency with IP_LOCAL_PORT_RANGE in
> > > connect().
> >
> > Except that bind() with a port reservation is not as common as a connect().
> > This is highly discouraged.
> >
> > See IP_BIND_ADDRESS_NO_PORT
> >
> > Can you provide a real use case ?
> >
> > I really feel like you are trying to push patches 'just because you can'...
> >
> > 'The old days' before 2016 were not very nice, we had P0 all the time
> > because of port exhaustion.
> > Since 2016 and IP_BIND_ADDRESS_NO_PORT I no longer have war rooms stories.
> 
> As you mentioned last night, the issues happening in connect() are
> relatively more than in bind().
> 
> To be more concise, I would like to state 3 points to see if they are valid:
> (1) Extending the option for bind() is the last puzzle of using an
> older algorithm for some users. Since we have one in connect(), how
> about adding it in bind() to provide for the people favouring the
> older algorithm.

Why do they want to use bind() to pick a random port in the first place ?

bind() behaviour is not strictly the same with connect(); the port reserved
by bind() is not reusable for connect().

Also, bind() requires SO_REUSEADDR to share a port, but by default, even
SO_REUSEADDR enabled sockets cannot share the same port if application
uses random-pick by bind((IP, 0)):

  # sysctl -w net.ipv4.ip_local_port_range="32768 32768"
  # python3
  >>> from socket import *
  >>> 
  >>> c1 = socket()
  >>> c1.setsockopt(SOL_SOCKET, SO_REUSEADDR, 1)
  >>> c1.bind(('', 0))
  >>> c1
  <socket.socket fd=4, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('0.0.0.0', 32768)>
  >>> 
  >>> c2 = socket()
  >>> c2.setsockopt(SOL_SOCKET, SO_REUSEADDR, 1)
  >>> c2.bind(('', 0))
  Traceback (most recent call last):
    File "<stdin>", line 1, in <module>
  OSError: [Errno 98] Address already in use

Then, net.ipv4.ip_autobind_reuse needs to be enabled at some risk.

bind()+connect() simply decreases the number of available 4-tuple on
the netns unless all applications use bind()+connect() instead of just
connect(), and it's unlikely.


> (2) This patch will not hurt any users like in Google as an example
> which prefers odd/even port selection, which is, I admit, indeed more
> advanced.

Indeed, it won't hurt existing users but will lead new users to the
wrong way.


> (3) This patch does not come out of thin air, but from some users who I contact.
> ?

Is someone who contacted to you really aware of all of the above and
even then in favor of bind() without IP_BIND_ADDRESS_NO_PORT ?


> In my opinion, using and adjusting to the new algorithm needs some
> changes in applications. For some old applications, they still need
> more time to keep pace with a more workable solution.

They will add setsockopt(IP_LOCAL_PORT_RANGE) whether your patch is
applied or not, then, only thing they need to do is replace SO_REUSEADDR
with IP_BIND_ADDRESS_NO_PORT, simple enough ?


> After considering it a whole night, I would like to push this tiny
> feature into the upstream kernel, I wonder if you can help me review
> it? Thanks in advance, Eric.

Re: [PATCH net-next] tcp: change source port selection at bind() time

[Jason Xing]

On Tue, Aug 20, 2024 at 12:53 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Jason Xing <kerneljasonxing@gmail.com>
> Date: Tue, 20 Aug 2024 08:53:53 +0800
> > Hello Eric,
> >
> > On Mon, Aug 19, 2024 at 11:45 PM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Fri, Aug 16, 2024 at 5:33 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > > >
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > This is a follow-up patch to an eariler commit 207184853dbd ("tcp/dccp:
> > > > change source port selection at connect() time").
> > > >
> > > > This patch extends the use of IP_LOCAL_PORT_RANGE option, so that we
> > > > don't need to iterate every two ports which means only favouring odd
> > > > number like the old days before 2016, which can be good for some
> > > > users who want to keep in consistency with IP_LOCAL_PORT_RANGE in
> > > > connect().
> > >
> > > Except that bind() with a port reservation is not as common as a connect().
> > > This is highly discouraged.
> > >
> > > See IP_BIND_ADDRESS_NO_PORT
> > >
> > > Can you provide a real use case ?
> > >
> > > I really feel like you are trying to push patches 'just because you can'...
> > >
> > > 'The old days' before 2016 were not very nice, we had P0 all the time
> > > because of port exhaustion.
> > > Since 2016 and IP_BIND_ADDRESS_NO_PORT I no longer have war rooms stories.
> >
> > As you mentioned last night, the issues happening in connect() are
> > relatively more than in bind().
> >
> > To be more concise, I would like to state 3 points to see if they are valid:
> > (1) Extending the option for bind() is the last puzzle of using an
> > older algorithm for some users. Since we have one in connect(), how
> > about adding it in bind() to provide for the people favouring the
> > older algorithm.
>
> Why do they want to use bind() to pick a random port in the first place ?
>
> bind() behaviour is not strictly the same with connect(); the port reserved
> by bind() is not reusable for connect().
>
> Also, bind() requires SO_REUSEADDR to share a port, but by default, even
> SO_REUSEADDR enabled sockets cannot share the same port if application
> uses random-pick by bind((IP, 0)):
>
>   # sysctl -w net.ipv4.ip_local_port_range="32768 32768"
>   # python3
>   >>> from socket import *
>   >>>
>   >>> c1 = socket()
>   >>> c1.setsockopt(SOL_SOCKET, SO_REUSEADDR, 1)
>   >>> c1.bind(('', 0))
>   >>> c1
>   <socket.socket fd=4, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('0.0.0.0', 32768)>
>   >>>
>   >>> c2 = socket()
>   >>> c2.setsockopt(SOL_SOCKET, SO_REUSEADDR, 1)
>   >>> c2.bind(('', 0))
>   Traceback (most recent call last):
>     File "<stdin>", line 1, in <module>
>   OSError: [Errno 98] Address already in use
>
> Then, net.ipv4.ip_autobind_reuse needs to be enabled at some risk.
>
> bind()+connect() simply decreases the number of available 4-tuple on
> the netns unless all applications use bind()+connect() instead of just
> connect(), and it's unlikely.
>
>
> > (2) This patch will not hurt any users like in Google as an example
> > which prefers odd/even port selection, which is, I admit, indeed more
> > advanced.
>
> Indeed, it won't hurt existing users but will lead new users to the
> wrong way.

Wrong way? You claim providing an old way for users to prevent the
'breakage' of applications is wrong regardless of those users' habits?

>
>
> > (3) This patch does not come out of thin air, but from some users who I contact.
> > ?
>
> Is someone who contacted to you really aware of all of the above and
> even then in favor of bind() without IP_BIND_ADDRESS_NO_PORT ?
>
>
> > In my opinion, using and adjusting to the new algorithm needs some
> > changes in applications. For some old applications, they still need
> > more time to keep pace with a more workable solution.
>
> They will add setsockopt(IP_LOCAL_PORT_RANGE) whether your patch is
> applied or not, then, only thing they need to do is replace SO_REUSEADDR
> with IP_BIND_ADDRESS_NO_PORT, simple enough ?

From the perspective of kernel developers, yes, it can work for sure.

I'm not discussing why exactly those applications design like this or
not like what we expect, but the fact is they do exist. If I were a
network engineer, I could probably know how to avoid this, but I'm
not. If you insist on asking why they use it like this, I can only
make myself clear by introducing another similar story to help me.

My original way is introducing a sysctl knob which can be tuned by
users to select one of those two algorithms, but it's not acceptable
by the upstream team from what I know. Then, I have to resort to other
ways of letting people have a chance to go back before 2016.

We cannot deny that the connect() using new algo causes some problems
which were missed two or three years ago, which may be considered an
incredible thing now, the same thing could happen again.

Probably we are not on the same page. I'm not unfamiliar with the
'correct' way to handle the issue. Also, I'm not arguing that we
should give up the way you mentioned. The only thing I'm saying is
whether we should provide a method using an old algo or not.

That's all, that's my final try on this patch. If you both disagree,
I'm totally fine with it.

Thanks for both of you getting involved into this discussion. I really
appreciate it :)

Thanks,
Jason

Re: [PATCH net-next] tcp: change source port selection at bind() time

[Jason Xing]

Hello Kuniyuki,

[...]
> > To be more concise, I would like to state 3 points to see if they are valid:
> > (1) Extending the option for bind() is the last puzzle of using an
> > older algorithm for some users. Since we have one in connect(), how
> > about adding it in bind() to provide for the people favouring the
> > older algorithm.
>
> Why do they want to use bind() to pick a random port in the first place ?
>

I feel sorry to bother you again.

Interesting thing is that I just found some of my personal records
that show to me: a lot of applications start active connections using
bind() to find a proper port in Google :) I'm not the only one :p
Probably coming up with the new algo selecting odd/even ports is the
reason/compromise for the bind() and non-bind() users. It gave range/2
for active flow using bind().

That's what I want to share with you :) Hope it will waste you and
Eric precious time :)

In my opinion, whatever the result is, technical communication is
important which can help the community grow. No hard feelings :)

Thanks,
Jason

Re: [PATCH net-next] tcp: change source port selection at bind() time

[Jason Xing]

On Tue, Aug 20, 2024 at 5:22 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> Hello Kuniyuki,
>
> [...]
> > > To be more concise, I would like to state 3 points to see if they are valid:
> > > (1) Extending the option for bind() is the last puzzle of using an
> > > older algorithm for some users. Since we have one in connect(), how
> > > about adding it in bind() to provide for the people favouring the
> > > older algorithm.
> >
> > Why do they want to use bind() to pick a random port in the first place ?
> >
>
> I feel sorry to bother you again.
>
> Interesting thing is that I just found some of my personal records
> that show to me: a lot of applications start active connections using
> bind() to find a proper port in Google :) I'm not the only one :p
> Probably coming up with the new algo selecting odd/even ports is the
> reason/compromise for the bind() and non-bind() users. It gave range/2
> for active flow using bind().
>
> That's what I want to share with you :) Hope it will waste you and
> Eric precious time :)

Sorry, it should be "it won't"...