Message-ID: <CAL+tcoCa1MCHnyd8Qy2LE5e6mUe8PLPp=tXAO56z7S34TXNuFA@mail.gmail.com>
Date: Tue, 20 Aug 2024 13:34:41 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com, 
	kernelxing@...cent.com, kuba@...nel.org, ncardwell@...gle.com, 
	netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next] tcp: change source port selection at bind() time

On Tue, Aug 20, 2024 at 12:53 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Jason Xing <kerneljasonxing@...il.com>
> Date: Tue, 20 Aug 2024 08:53:53 +0800
> > Hello Eric,
> >
> > On Mon, Aug 19, 2024 at 11:45 PM Eric Dumazet <edumazet@...gle.com> wrote:
> > >
> > > On Fri, Aug 16, 2024 at 5:33 PM Jason Xing <kerneljasonxing@...il.com> wrote:
> > > >
> > > > From: Jason Xing <kernelxing@...cent.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

Powered by blists - more mailing lists