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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87qzyfdue6.fsf@cloudflare.com>
Date: Thu, 17 Jul 2025 11:44:01 +0200
From: Jakub Sitnicki <jakub@...udflare.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Eric Dumazet <edumazet@...gle.com>,  "David S. Miller"
 <davem@...emloft.net>,  Jakub Kicinski <kuba@...nel.org>,  Neal Cardwell
 <ncardwell@...gle.com>,  Kuniyuki Iwashima <kuniyu@...gle.com>,
  netdev@...r.kernel.org,  kernel-team@...udflare.com,  Lee Valentine
 <lvalentine@...udflare.com>
Subject: Re: [PATCH net-next v3 2/3] tcp: Consider every port when
 connecting with IP_LOCAL_PORT_RANGE

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?

>
> At very least you will need to update the man page.
>
>
>> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
>> index d3ce6d0a514e..9d8a9c7c8274 100644
>> --- a/net/ipv4/inet_hashtables.c
>> +++ b/net/ipv4/inet_hashtables.c
>> @@ -1005,6 +1005,52 @@ EXPORT_IPV6_MOD(inet_bhash2_reset_saddr);
>>  #define INET_TABLE_PERTURB_SIZE (1 << CONFIG_INET_TABLE_PERTURB_ORDER)
>>  static u32 *table_perturb;
>>  
>> +/* True on source address conflict with another socket. False otherwise. */
>> +static inline bool check_bind2_bucket(const struct sock *sk,
>> +				      const struct inet_bind2_bucket *tb2)
>
> Please no inline function in c files.

Will fix.

Thanks for feedback.