Re: [PATCH net-next v3 2/3] tcp: Consider every port when connecting with IP_LOCAL_PORT_RANGE

[Jakub Sitnicki <jakub@cloudflare.com>]

On Thu, Jul 17, 2025 at 11:44 AM +02, Jakub Sitnicki wrote:
> On Thu, Jul 17, 2025 at 11:23 AM +02, Paolo Abeni wrote:
>> On 7/14/25 6:03 PM, Jakub Sitnicki wrote:
>>> Solution
>>> --------
>>> 
>>> If there is no IP address conflict with any socket bound to a given local
>>> port, then from the protocol's perspective, the port can be safely shared.
>>> 
>>> With that in mind, modify the port search during connect(), that is
>>> __inet_hash_connect, to consider all bind buckets (ports) when looking for
>>> a local port for egress.
>>> 
>>> To achieve this, add an extra walk over bhash2 buckets for the port to
>>> check for IP conflicts. The additional walk is not free, so perform it only
>>> once per port - during the second phase of conflict checking, when the
>>> bhash bucket is locked.
>>> 
>>> We enable this changed behavior only if the IP_LOCAL_PORT_RANGE socket
>>> option is set. The rationale is that users are likely to care about using
>>> every possible local port only when they have deliberately constrained the
>>> ephemeral port range.
>>
>> I'm not a big fan of piggybacking additional semantic on existing
>> socketopt, have you considered a new one?
>
> That's a fair point. Though a dedicated sysctl seems more appropriate in
> this case. Akin to how we have ip_autobind_reuse to enable amore
> aggresive port sharing strategy on bind() side. How does that sound?

Thinking about this some more - if we're considering a dedicated sysctl
guard for this, perhaps this merits giving a shot to the more
comprehensive fix first.

That is to update the inet_bind_bucket state (fastreuse, fastreuseport)
on socket unbind to reflect the change in bucket owners. IOW, pivot to
one of the alternatives that I've highlighted:

| Alternatives
| ------------
| 
| * Update bind bucket state on port release
| 
| A valid solution to the described problem would also be to walk the bind
| bucket owners when releasing the port and recalculate the
| tb->{reuse,reuseport} state.
| 
| However, in comparison to the proposed solution, this alone would not allow
| sharing the local port with other sockets bound to non-conflicting IPs for
| as long as they exist.
| 
| Another downside is that we'd pay the extra cost on each unbind (more
| frequent) rather than only when connecting with IP_LOCAL_PORT_RANGE
| set (less frequent). Due to that we would also likely need to guard it
| behind a sysctl (see below).

Right now the inet_bind_bucket fastreuse{,port} state is being
mismanaged, IMO. This would be the fix for the actual root cause here.