Re: [PATCH net-next v4 1/2] tcp: Update bind bucket state on port release

[Jakub Sitnicki <jakub@cloudflare.com>]

On Tue, Sep 16, 2025 at 12:14 PM +02, Paolo Abeni wrote:
> On 9/13/25 12:09 PM, Jakub Sitnicki wrote:
>> Today, once an inet_bind_bucket enters a state where fastreuse >= 0 or
>> fastreuseport >= 0 after a socket is explicitly bound to a port, it remains
>> in that state until all 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 reduced ephemeral port
>> range (IP_LOCAL_PORT_RANGE socket option), this can cause faster port
>> exhaustion since blocked buckets are excluded from reuse.
>> 
>> The reason the bucket state isn't updated on port release is unclear.
>> Possibly a performance trade-off to avoid scanning bucket owners, or just
>> an oversight.
>> 
>> Fix it by recalculating the bucket state when a socket releases a port. To
>> limit overhead, each inet_bind2_bucket stores its own (fastreuse,
>> fastreuseport) state. On port release, only the relevant port-addr bucket
>> is scanned, and the overall state is derived from these.
>
> I'm possibly likely lost, but I think that the bucket state could change
> even after inet_bhash2_update_saddr(), but AFAICS it's not updated there.

Let me double check if I understand what you have in mind because now I
also feel a bit lost :-)

We already update the bucket state in inet_bhash2_update_saddr(). I
assume we are talking about the main body, not the early bailout path
when the socket is not bound yet [1].

This code gets called only in the obscure (?) case when ip_dynaddr [2]
sysctl is set, and we have a routing failure during connection setup
phase (SYN-SENT).

In such case, on source address update, call to
inet_bind2_bucket_destroy() will recalculate port-addr bucket state,
potentially "downgrading" it to (fastreuse=-1, fastreuseport=-1).

But if the "downgrade" happens, it changes nothing for the port bucket
state, as we are about to re-add the socket into another port-addr
bucket.

Now, adding a CONNECT_BIND socket to an existing port-addr bucket, that
also has no side effects. We can't "upgrade" the bucket to the shareable
state (fastreuse=-1, fastreuseport=-1).

That said, I do see an unaddressed corner case now that I audit this
code again. If we end up _creating_ a new inet_bind2_bucket
(__inet_bhash2_update_saddr->inet_bind2_bucket_init), then the bucket
state should be initialized to (fastreuse=-1, fastreuseport=-1) when the
socket has the CONNECT_BIND flag set.

The call chain I'm referring to:

tcp_connect / __tcp_retransmit_skb
  ->rebuild_header
    inet_sk_rebuild_header
      inet_sk_reselect_saddr IFF sysctl_ip_dynaddr != 0
        inet_bhash2_update_saddr
          __inet_bhash2_update_saddr
            inet_bind2_bucket_init

I propose to handle that by checking if the socket has CONNECT_BIND flag
set and overwriting the port-addr bucket state similiar to like I did in
__inet_hash_connect:

	tb2 = inet_bind2_bucket_find(head2, net, port, l3mdev, sk);
	if (!tb2) {
		tb2 = inet_bind2_bucket_create(hinfo->bind2_bucket_cachep, net,
					       head2, tb, sk);
		if (!tb2)
			goto error;
		tb2->fastreuse = -1;
		tb2->fastreuseport = -1;
	}

So the obscure ip_dynadr path does need a fixup. Other than that I'm not
able to poke any other holes in how we manage the bucket state.

Was that your concern or you had something else in mind?

Thanks,
-jkbs

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ipv4/inet_hashtables.c?h=v6.17-rc6#n914
[2] https://docs.kernel.org/networking/ip_dynaddr.html