| 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
| ||
|
Message-ID: <874itzzdcy.fsf@cloudflare.com>
Date: Fri, 22 Aug 2025 15:37:33 +0200
From: Jakub Sitnicki <jakub@...udflare.com>
To: Kuniyuki Iwashima <kuniyu@...gle.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, Eric
Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Neal
Cardwell <ncardwell@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
kernel-team@...udflare.com, Lee Valentine <lvalentine@...udflare.com>
Subject: Re: [PATCH net-next v2 1/2] tcp: Update bind bucket state on port
release
On Thu, Aug 21, 2025 at 08:58 PM -07, Kuniyuki Iwashima wrote:
> On Thu, Aug 21, 2025 at 4:09 AM Jakub Sitnicki <jakub@...udflare.com> wrote:
>>
>> Currently, when an inet_bind_bucket enters a state where fastreuse >= 0 or
>> fastreuseport >= 0, after a socket explicitly binds to a port, it stays in
>> that state until all associated sockets are removed and the bucket is
>> destroyed.
>>
>> In this state, the bucket is skipped during ephemeral port selection in
>> connect(). For applications using a small ephemeral port range (via
>> IP_LOCAL_PORT_RANGE option), this can lead to quicker port exhaustion
>> because "blocked" buckets remain excluded from reuse.
>>
>> The reason for not updating the bucket state on port release is unclear. It
>> may have been a performance trade-off to avoid scanning bucket owners, or
>> simply an oversight.
>>
>> Address it by recalculating the bind bucket state when a socket releases a
>> port. To minimize overhead, use a divide-and-conquer strategy: duplicate
>> the (fastreuse, fastreuseport) state in each inet_bind2_bucket. On port
>> release, we only need to scan the relevant port-addr bucket, and the
>> overall port bucket state can be derived from those.
>>
>> Signed-off-by: Jakub Sitnicki <jakub@...udflare.com>
>> ---
>> include/net/inet_connection_sock.h | 5 +++--
>> include/net/inet_hashtables.h | 2 ++
>> include/net/inet_sock.h | 2 ++
>> include/net/inet_timewait_sock.h | 3 ++-
>> include/net/tcp.h | 15 +++++++++++++++
>> net/ipv4/inet_connection_sock.c | 12 ++++++++----
>> net/ipv4/inet_hashtables.c | 32 +++++++++++++++++++++++++++++++-
>> net/ipv4/inet_timewait_sock.c | 1 +
>> 8 files changed, 64 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
>> index 1735db332aab..072347f16483 100644
>> --- a/include/net/inet_connection_sock.h
>> +++ b/include/net/inet_connection_sock.h
>> @@ -322,8 +322,9 @@ int inet_csk_listen_start(struct sock *sk);
>> void inet_csk_listen_stop(struct sock *sk);
>>
>> /* update the fast reuse flag when adding a socket */
>> -void inet_csk_update_fastreuse(struct inet_bind_bucket *tb,
>> - struct sock *sk);
>> +void inet_csk_update_fastreuse(const struct sock *sk,
>> + struct inet_bind_bucket *tb,
>> + struct inet_bind2_bucket *tb2);
>>
>> struct dst_entry *inet_csk_update_pmtu(struct sock *sk, u32 mtu);
>>
>> diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
>> index 19dbd9081d5a..d6676746dabf 100644
>> --- a/include/net/inet_hashtables.h
>> +++ b/include/net/inet_hashtables.h
>> @@ -108,6 +108,8 @@ struct inet_bind2_bucket {
>> struct hlist_node bhash_node;
>> /* List of sockets hashed to this bucket */
>> struct hlist_head owners;
>> + signed char fastreuse;
>> + signed char fastreuseport;
>> };
>>
>> static inline struct net *ib_net(const struct inet_bind_bucket *ib)
>> diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
>> index 1086256549fa..9614d0430471 100644
>> --- a/include/net/inet_sock.h
>> +++ b/include/net/inet_sock.h
>> @@ -279,6 +279,8 @@ enum {
>> INET_FLAGS_RTALERT_ISOLATE = 28,
>> INET_FLAGS_SNDFLOW = 29,
>> INET_FLAGS_RTALERT = 30,
>> + /* socket bound to a port at connect() time */
>> + INET_FLAGS_AUTOBIND = 31,
>
> AUTOBIND sounds like inet_autobind() was called.
That was intentional. I was going for an analogy to
inet_dgram_connect->inet_autobind, but I see how it can also be
confusing.
> __inet_bind() saves similar flags in sk->sk_userlocks and
> it has 3 bits available.
>
> How about flagging SOCK_BINDPORT_CONNECT in
> sk->sk_userlocks ?
I was on the fence whether to put the bit flag in sk_userlocks or
inet_flags. Treating it as a variant of BINDPORT lock also makes sense.
>> };
>>
>> /* cmsg flags for inet */
>> diff --git a/include/net/inet_timewait_sock.h b/include/net/inet_timewait_sock.h
>> index 67a313575780..ec99176d576f 100644
>> --- a/include/net/inet_timewait_sock.h
>> +++ b/include/net/inet_timewait_sock.h
>> @@ -70,7 +70,8 @@ struct inet_timewait_sock {
>> unsigned int tw_transparent : 1,
>> tw_flowlabel : 20,
>> tw_usec_ts : 1,
>> - tw_pad : 2, /* 2 bits hole */
>> + tw_autobind : 1,
>> + tw_pad : 1, /* 1 bit hole */
>> tw_tos : 8;
>> u32 tw_txhash;
>> u32 tw_priority;
>> diff --git a/include/net/tcp.h b/include/net/tcp.h
>> index 2936b8175950..c4bb6e56a668 100644
>> --- a/include/net/tcp.h
>> +++ b/include/net/tcp.h
>> @@ -2225,6 +2225,21 @@ static inline bool inet_sk_transparent(const struct sock *sk)
>> return inet_test_bit(TRANSPARENT, sk);
>> }
>>
>> +/**
>> + * inet_sk_autobind - Check if socket was bound to a port at connect() time.
>> + * @sk: &struct inet_connection_sock or &struct inet_timewait_sock
>> + */
>> +static inline bool inet_sk_autobind(const struct sock *sk)
>> +{
>> + switch (sk->sk_state) {
>> + case TCP_TIME_WAIT:
>> + return inet_twsk(sk)->tw_autobind;
>> + case TCP_NEW_SYN_RECV:
>> + return false; /* n/a to request sock */
>
> This never happens. Maybe remove the case
> or add DEBUG_NET_WARN_ON_ONCE(1) ?
Will probably just remove it.
Thanks for reviewing!
>> + }
>> + return inet_test_bit(AUTOBIND, sk);
>> +}
>> +
[...]
Powered by blists - more mailing lists