Re: [v1,net] udp: Unhash auto-bound connected sk from 4-tuple hash table when disconnected.

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
udp: Unhash auto-bound connected sk from 4-tuple hash table when disconnected.

This commit fixes a bug where disconnecting an auto-bound connected UDP
socket leaves a garbage entry in the 4-tuple hash table. The socket is
now properly removed from the hash table when disconnected.

> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index b96e47f1c8a2..3388e4ab9056 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2321,15 +2321,19 @@ void udp_lib_rehash(struct sock *sk, u16 newhash, u16 newhash4)
>  		if (udp_hashed4(sk)) {
>  			spin_lock_bh(&hslot->lock);
>
> -			udp_rehash4(udptable, sk, newhash4);
> -			if (hslot2 != nhslot2) {
> -				spin_lock(&hslot2->lock);
> -				udp_hash4_dec(hslot2);
> -				spin_unlock(&hslot2->lock);
> -
> -				spin_lock(&nhslot2->lock);
> -				udp_hash4_inc(nhslot2);
> -				spin_unlock(&nhslot2->lock);
> +			if (inet_rcv_saddr_any(sk)) {
> +				udp_unhash4(udptable, sk);
> +			} else {
> +				udp_rehash4(udptable, sk, newhash4);
> +				if (hslot2 != nhslot2) {
> +					spin_lock(&hslot2->lock);
> +					udp_hash4_dec(hslot2);
> +					spin_unlock(&hslot2->lock);
> +
> +					spin_lock(&nhslot2->lock);
> +					udp_hash4_inc(nhslot2);
> +					spin_unlock(&nhslot2->lock);
> +				}
>  			}
>
>  			spin_unlock_bh(&hslot->lock);

Is there a hash4_cnt accounting mismatch when inet_rcv_saddr_any(sk)
is true? The udp_sk(sk)->udp_portaddr_hash is updated to newhash at
the start of the function (before this block):

void udp_lib_rehash(struct sock *sk, u16 newhash, u16 newhash4)
{
	struct udp_table *udptable = udp_get_table_prot(sk);
	...
	udp_sk(sk)->udp_portaddr_hash = newhash;
	...
}

When the new code calls udp_unhash4() on the inet_rcv_saddr_any(sk)
path, udp_unhash4() computes its hslot2 from the already-updated
udp_portaddr_hash (nhslot2):

void udp_unhash4(struct udp_table *udptable, struct sock *sk)
{
	struct udp_hslot *hslot2 = udp_hashslot2(udptable, udp_sk(sk)->udp_portaddr_hash);
	...
	udp_hash4_dec(hslot2);
}

But the original udp_lib_hash4() call during connect() would have
incremented hash4_cnt on the old hslot2, not nhslot2. This seems to
decrement hash4_cnt on the wrong 2-tuple slot when hslot2 != nhslot2,
which is common for wildcard-bind + connect + disconnect.

The else branch correctly uses udp_hash4_dec(hslot2) on the old slot
and udp_hash4_inc(nhslot2) on the new slot.

While the impact is minor since hash4_cnt is a u32 optimization hint,
the counter becomes permanently incorrect and accumulates error across
connect/disconnect cycles.