[PATCH net] udp: fix l4 hash after reconnect

[PATCH net] udp: fix l4 hash after reconnect

[Paolo Abeni]

After the blamed commit below, udp_rehash() is supposed to be called
with both local and remote addresses set.

Currently that is already the case for IPv6 sockets, but for IPv4 the
destination address is updated after rehashing.

Address the issue moving the destination address and port initialization
before rehashing.

Fixes: 1b29a730ef8b ("ipv6/udp: Add 4-tuple hash for connected socket")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/ipv4/datagram.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/datagram.c b/net/ipv4/datagram.c
index cc6d0bd7b0a9..4aca1f05edd3 100644
--- a/net/ipv4/datagram.c
+++ b/net/ipv4/datagram.c
@@ -61,15 +61,17 @@ int __ip4_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len
 		err = -EACCES;
 		goto out;
 	}
+
+	/* Update addresses before rehashing */
+	inet->inet_daddr = fl4->daddr;
+	inet->inet_dport = usin->sin_port;
 	if (!inet->inet_saddr)
-		inet->inet_saddr = fl4->saddr;	/* Update source address */
+		inet->inet_saddr = fl4->saddr;
 	if (!inet->inet_rcv_saddr) {
 		inet->inet_rcv_saddr = fl4->saddr;
 		if (sk->sk_prot->rehash)
 			sk->sk_prot->rehash(sk);
 	}
-	inet->inet_daddr = fl4->daddr;
-	inet->inet_dport = usin->sin_port;
 	reuseport_has_conns_set(sk);
 	sk->sk_state = TCP_ESTABLISHED;
 	sk_set_txhash(sk);
-- 
2.45.2

Re: [PATCH net] udp: fix l4 hash after reconnect

[Eric Dumazet]

On Fri, Dec 6, 2024 at 4:50 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> After the blamed commit below, udp_rehash() is supposed to be called
> with both local and remote addresses set.
>
> Currently that is already the case for IPv6 sockets, but for IPv4 the
> destination address is updated after rehashing.
>
> Address the issue moving the destination address and port initialization
> before rehashing.
>
> Fixes: 1b29a730ef8b ("ipv6/udp: Add 4-tuple hash for connected socket")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Nice catch, thanks !

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net] udp: fix l4 hash after reconnect

[Eric Dumazet]

On Fri, Dec 6, 2024 at 4:57 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Dec 6, 2024 at 4:50 PM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > After the blamed commit below, udp_rehash() is supposed to be called
> > with both local and remote addresses set.
> >
> > Currently that is already the case for IPv6 sockets, but for IPv4 the
> > destination address is updated after rehashing.
> >
> > Address the issue moving the destination address and port initialization
> > before rehashing.
> >
> > Fixes: 1b29a730ef8b ("ipv6/udp: Add 4-tuple hash for connected socket")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>
> Nice catch, thanks !
>
> Reviewed-by: Eric Dumazet <edumazet@google.com>

BTW, it seems that udp_lib_rehash() does the udp_rehash4()
only if the hash2 has changed.

Re: [PATCH net] udp: fix l4 hash after reconnect

[Paolo Abeni]

On 12/6/24 17:01, Eric Dumazet wrote:
> On Fri, Dec 6, 2024 at 4:57 PM Eric Dumazet <edumazet@google.com> wrote:
>> On Fri, Dec 6, 2024 at 4:50 PM Paolo Abeni <pabeni@redhat.com> wrote:
>>>
>>> After the blamed commit below, udp_rehash() is supposed to be called
>>> with both local and remote addresses set.
>>>
>>> Currently that is already the case for IPv6 sockets, but for IPv4 the
>>> destination address is updated after rehashing.
>>>
>>> Address the issue moving the destination address and port initialization
>>> before rehashing.
>>>
>>> Fixes: 1b29a730ef8b ("ipv6/udp: Add 4-tuple hash for connected socket")
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>
>> Nice catch, thanks !
>>
>> Reviewed-by: Eric Dumazet <edumazet@google.com>
> 
> BTW, it seems that udp_lib_rehash() does the udp_rehash4()
> only if the hash2 has changed.

Oh, you are right, that requires a separate fix.

@Philo: could you please have a look at that? basically you need to
check separately for hash2 and hash4 changes.

Thanks!

Paolo

Re: [PATCH net] udp: fix l4 hash after reconnect

[Philo Lu]

On 2024/12/7 00:23, Paolo Abeni wrote:
> On 12/6/24 17:01, Eric Dumazet wrote:
>> On Fri, Dec 6, 2024 at 4:57 PM Eric Dumazet <edumazet@google.com> wrote:
>>> On Fri, Dec 6, 2024 at 4:50 PM Paolo Abeni <pabeni@redhat.com> wrote:
>>>>
>>>> After the blamed commit below, udp_rehash() is supposed to be called
>>>> with both local and remote addresses set.
>>>>
>>>> Currently that is already the case for IPv6 sockets, but for IPv4 the
>>>> destination address is updated after rehashing.
>>>>
>>>> Address the issue moving the destination address and port initialization
>>>> before rehashing.
>>>>
>>>> Fixes: 1b29a730ef8b ("ipv6/udp: Add 4-tuple hash for connected socket")
>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Thank you for this fix :)

>>>
>>> Nice catch, thanks !
>>>
>>> Reviewed-by: Eric Dumazet <edumazet@google.com>
>>
>> BTW, it seems that udp_lib_rehash() does the udp_rehash4()
>> only if the hash2 has changed.
> 
> Oh, you are right, that requires a separate fix.
> 
> @Philo: could you please have a look at that? basically you need to
> check separately for hash2 and hash4 changes.

This is a good question. IIUC, the only affected case is when trying to 
re-connect another remote address with the same local address (i.e., 
hash2 unchanged). And this will be handled by udp_lib_hash4(). So in 
udp_lib_rehash() I put rehash4() inside hash2 checking, which means a 
passive rehash4 following rehash2.

So I think it's more about the convention for rehash. We can choose the 
better one.

Thanks.
-- 
Philo

Re: [PATCH net] udp: fix l4 hash after reconnect

[Paolo Abeni]

On 12/7/24 03:34, Philo Lu wrote:
> On 2024/12/7 00:23, Paolo Abeni wrote:
>> On 12/6/24 17:01, Eric Dumazet wrote:
>>> BTW, it seems that udp_lib_rehash() does the udp_rehash4()
>>> only if the hash2 has changed.
>>
>> Oh, you are right, that requires a separate fix.
>>
>> @Philo: could you please have a look at that? basically you need to
>> check separately for hash2 and hash4 changes.
> 
> This is a good question. IIUC, the only affected case is when trying to 
> re-connect another remote address with the same local address 

AFAICS, there is also another case: when re-connection using a different
local addresses with the same l2 hash...

> (i.e., 
> hash2 unchanged). And this will be handled by udp_lib_hash4(). So in 
> udp_lib_rehash() I put rehash4() inside hash2 checking, which means a 
> passive rehash4 following rehash2.

... but even the latter case should be covered from the above.

> So I think it's more about the convention for rehash. We can choose the 
> better one.

IIRC a related question raised during code review for the udp L4 hash
patches. Perhaps refactoring the code slightly to let udp_rehash()
really doing the re-hashing and udp_hash really doing only the hashing
could be worth.

Cheers,

Paolo

Re: [PATCH net] udp: fix l4 hash after reconnect

[Philo Lu]

On 2024/12/10 16:32, Paolo Abeni wrote:
> On 12/7/24 03:34, Philo Lu wrote:
>> On 2024/12/7 00:23, Paolo Abeni wrote:
>>> On 12/6/24 17:01, Eric Dumazet wrote:
>>>> BTW, it seems that udp_lib_rehash() does the udp_rehash4()
>>>> only if the hash2 has changed.
>>>
>>> Oh, you are right, that requires a separate fix.
>>>
>>> @Philo: could you please have a look at that? basically you need to
>>> check separately for hash2 and hash4 changes.
>>
>> This is a good question. IIUC, the only affected case is when trying to
>> re-connect another remote address with the same local address
> 
> AFAICS, there is also another case: when re-connection using a different
> local addresses with the same l2 hash...
> 

Yes, you're right. I missed this case... Thank you for pointing out it.

>> (i.e.,
>> hash2 unchanged). And this will be handled by udp_lib_hash4(). So in
>> udp_lib_rehash() I put rehash4() inside hash2 checking, which means a
>> passive rehash4 following rehash2.
> 
> ... but even the latter case should be covered from the above.
> 
>> So I think it's more about the convention for rehash. We can choose the
>> better one.
> 
> IIRC a related question raised during code review for the udp L4 hash
> patches. Perhaps refactoring the code slightly to let udp_rehash()
> really doing the re-hashing and udp_hash really doing only the hashing
> could be worth.

Agreed. I'd appreciate it if someone helps to refactor it, or I can do 
this later myself.

-- 
Philo

Re: [PATCH net] udp: fix l4 hash after reconnect

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri,  6 Dec 2024 16:49:14 +0100 you wrote:
> After the blamed commit below, udp_rehash() is supposed to be called
> with both local and remote addresses set.
> 
> Currently that is already the case for IPv6 sockets, but for IPv4 the
> destination address is updated after rehashing.
> 
> Address the issue moving the destination address and port initialization
> before rehashing.
> 
> [...]

Here is the summary with links:
  - [net] udp: fix l4 hash after reconnect
    https://git.kernel.org/netdev/net/c/51a00be6a099

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] udp: fix l4 hash after reconnect

[Jakub Kicinski]

On Fri,  6 Dec 2024 16:49:14 +0100 Paolo Abeni wrote:
> After the blamed commit below, udp_rehash() is supposed to be called
> with both local and remote addresses set.
> 
> Currently that is already the case for IPv6 sockets, but for IPv4 the
> destination address is updated after rehashing.
> 
> Address the issue moving the destination address and port initialization
> before rehashing.
> 
> Fixes: 1b29a730ef8b ("ipv6/udp: Add 4-tuple hash for connected socket")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

I feel obliged to point out a lack of selftest both here and the series
under Fixes :(

Re: [PATCH net] udp: fix l4 hash after reconnect

[Philo Lu]

On 2024/12/11 08:59, Jakub Kicinski wrote:
> On Fri,  6 Dec 2024 16:49:14 +0100 Paolo Abeni wrote:
>> After the blamed commit below, udp_rehash() is supposed to be called
>> with both local and remote addresses set.
>>
>> Currently that is already the case for IPv6 sockets, but for IPv4 the
>> destination address is updated after rehashing.
>>
>> Address the issue moving the destination address and port initialization
>> before rehashing.
>>
>> Fixes: 1b29a730ef8b ("ipv6/udp: Add 4-tuple hash for connected socket")
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> I feel obliged to point out a lack of selftest both here and the series
> under Fixes :(

Sorry for the late reply.

Though I'm glad to add selftests, I don't have a good idea how to do it 
decently. Because the series (say uhash4) will take effect silently for 
connected udp sockets without any api change.

Maybe it's better to add KUnit tests for it? or any other suggestions?

Thanks.
-- 
Philo

Re: [PATCH net] udp: fix l4 hash after reconnect

[Philo Lu]

Hi Paolo, hi Eric,

On 2024/12/10 16:32, Paolo Abeni wrote:
> On 12/7/24 03:34, Philo Lu wrote:
>> On 2024/12/7 00:23, Paolo Abeni wrote:
>>> On 12/6/24 17:01, Eric Dumazet wrote:
>>>> BTW, it seems that udp_lib_rehash() does the udp_rehash4()
>>>> only if the hash2 has changed.
>>>
>>> Oh, you are right, that requires a separate fix.
>>>
>>> @Philo: could you please have a look at that? basically you need to
>>> check separately for hash2 and hash4 changes.
>>
>> This is a good question. IIUC, the only affected case is when trying to
>> re-connect another remote address with the same local address
> 
> AFAICS, there is also another case: when re-connection using a different
> local addresses with the same l2 hash...
> 
>> (i.e.,
>> hash2 unchanged). And this will be handled by udp_lib_hash4(). So in
>> udp_lib_rehash() I put rehash4() inside hash2 checking, which means a
>> passive rehash4 following rehash2.
> 
> ... but even the latter case should be covered from the above.
> 
>> So I think it's more about the convention for rehash. We can choose the
>> better one.
> 
> IIRC a related question raised during code review for the udp L4 hash
> patches. Perhaps refactoring the code slightly to let udp_rehash()
> really doing the re-hashing and udp_hash really doing only the hashing
> could be worth.
> 

I'm trying to unify rehash() for both hash2 and hash4 in 
__ip4_datagram_connect, when I noticed the inet_rcv_saddr checking 
before calling rehash():

```
if (!inet->inet_rcv_saddr) {
	inet->inet_rcv_saddr = fl4->saddr;
	if (sk->sk_prot->rehash)
		sk->sk_prot->rehash(sk);
}
```
This means inet_rcv_saddr is reset at most once no matter how many times 
connect() is called. I'm not sure if this is by-design for some reason? 
Or can I remove this checking? like:

--- a/net/ipv4/datagram.c
+++ b/net/ipv4/datagram.c
@@ -67,11 +67,9 @@ int __ip4_datagram_connect(struct sock *sk, struct 
sockaddr *uaddr, int addr_len
         inet->inet_dport = usin->sin_port;
         if (!inet->inet_saddr)
                 inet->inet_saddr = fl4->saddr;
-       if (!inet->inet_rcv_saddr) {
-               inet->inet_rcv_saddr = fl4->saddr;
-               if (sk->sk_prot->rehash)
-                       sk->sk_prot->rehash(sk);
-       }
+       inet->inet_rcv_saddr = fl4->saddr;
+       if (sk->sk_prot->rehash)
+               sk->sk_prot->rehash(sk);
         reuseport_has_conns_set(sk);
         sk->sk_state = TCP_ESTABLISHED;
         sk_set_txhash(sk);


Thanks.
-- 
Philo

Re: [PATCH net] udp: fix l4 hash after reconnect

[Paolo Abeni]

Hi,

I'm sorry for the latency, I was off in the past days.

On 12/31/24 8:55 AM, Philo Lu wrote:
> On 2024/12/10 16:32, Paolo Abeni wrote:
>> On 12/7/24 03:34, Philo Lu wrote:
>>> On 2024/12/7 00:23, Paolo Abeni wrote:
>>>> On 12/6/24 17:01, Eric Dumazet wrote:
>>>>> BTW, it seems that udp_lib_rehash() does the udp_rehash4()
>>>>> only if the hash2 has changed.
>>>>
>>>> Oh, you are right, that requires a separate fix.
>>>>
>>>> @Philo: could you please have a look at that? basically you need to
>>>> check separately for hash2 and hash4 changes.
>>>
>>> This is a good question. IIUC, the only affected case is when trying to
>>> re-connect another remote address with the same local address
>>
>> AFAICS, there is also another case: when re-connection using a different
>> local addresses with the same l2 hash...
>>
>>> (i.e.,
>>> hash2 unchanged). And this will be handled by udp_lib_hash4(). So in
>>> udp_lib_rehash() I put rehash4() inside hash2 checking, which means a
>>> passive rehash4 following rehash2.
>>
>> ... but even the latter case should be covered from the above.
>>
>>> So I think it's more about the convention for rehash. We can choose the
>>> better one.
>>
>> IIRC a related question raised during code review for the udp L4 hash
>> patches. Perhaps refactoring the code slightly to let udp_rehash()
>> really doing the re-hashing and udp_hash really doing only the hashing
>> could be worth.
>>
> 
> I'm trying to unify rehash() for both hash2 and hash4 in 
> __ip4_datagram_connect, when I noticed the inet_rcv_saddr checking 
> before calling rehash():
> 
> ```
> if (!inet->inet_rcv_saddr) {
> 	inet->inet_rcv_saddr = fl4->saddr;
> 	if (sk->sk_prot->rehash)
> 		sk->sk_prot->rehash(sk);
> }
> ```
> This means inet_rcv_saddr is reset at most once no matter how many times 
> connect() is called. 

... if you make consecutive connect(<dst address>) calls.

 __udp_disconnect() clears saddr, so:

connect(<AF_UNSPEC>); connect(<dst address>);

will yield the expected result

> I'm not sure if this is by-design for some reason? 
> Or can I remove this checking? like:
> 
> --- a/net/ipv4/datagram.c
> +++ b/net/ipv4/datagram.c
> @@ -67,11 +67,9 @@ int __ip4_datagram_connect(struct sock *sk, struct 
> sockaddr *uaddr, int addr_len
>          inet->inet_dport = usin->sin_port;
>          if (!inet->inet_saddr)
>                  inet->inet_saddr = fl4->saddr;
> -       if (!inet->inet_rcv_saddr) {
> -               inet->inet_rcv_saddr = fl4->saddr;
> -               if (sk->sk_prot->rehash)
> -                       sk->sk_prot->rehash(sk);
> -       }
> +       inet->inet_rcv_saddr = fl4->saddr;
> +       if (sk->sk_prot->rehash)
> +               sk->sk_prot->rehash(sk);
>          reuseport_has_conns_set(sk);
>          sk->sk_state = TCP_ESTABLISHED;
>          sk_set_txhash(sk);

This sounds like an unexpected behaviour change which may broke existing
applications.

I suggest retaining the current beheviour.

Thanks,

Paolo

Re: [PATCH net] udp: fix l4 hash after reconnect

[Philo Lu]

Hi,

On 2025/1/7 15:56, Paolo Abeni wrote:
> Hi,
> 
> I'm sorry for the latency, I was off in the past days.
> 
> On 12/31/24 8:55 AM, Philo Lu wrote:
>> On 2024/12/10 16:32, Paolo Abeni wrote:
>>> On 12/7/24 03:34, Philo Lu wrote:
>>>> On 2024/12/7 00:23, Paolo Abeni wrote:
>>>>> On 12/6/24 17:01, Eric Dumazet wrote:
>>>>>> BTW, it seems that udp_lib_rehash() does the udp_rehash4()
>>>>>> only if the hash2 has changed.
>>>>>
>>>>> Oh, you are right, that requires a separate fix.
>>>>>
>>>>> @Philo: could you please have a look at that? basically you need to
>>>>> check separately for hash2 and hash4 changes.
>>>>
>>>> This is a good question. IIUC, the only affected case is when trying to
>>>> re-connect another remote address with the same local address
>>>
>>> AFAICS, there is also another case: when re-connection using a different
>>> local addresses with the same l2 hash...
>>>
>>>> (i.e.,
>>>> hash2 unchanged). And this will be handled by udp_lib_hash4(). So in
>>>> udp_lib_rehash() I put rehash4() inside hash2 checking, which means a
>>>> passive rehash4 following rehash2.
>>>
>>> ... but even the latter case should be covered from the above.
>>>
>>>> So I think it's more about the convention for rehash. We can choose the
>>>> better one.
>>>
>>> IIRC a related question raised during code review for the udp L4 hash
>>> patches. Perhaps refactoring the code slightly to let udp_rehash()
>>> really doing the re-hashing and udp_hash really doing only the hashing
>>> could be worth.
>>>
>>
>> I'm trying to unify rehash() for both hash2 and hash4 in
>> __ip4_datagram_connect, when I noticed the inet_rcv_saddr checking
>> before calling rehash():
>>
>> ```
>> if (!inet->inet_rcv_saddr) {
>> 	inet->inet_rcv_saddr = fl4->saddr;
>> 	if (sk->sk_prot->rehash)
>> 		sk->sk_prot->rehash(sk);
>> }
>> ```
>> This means inet_rcv_saddr is reset at most once no matter how many times
>> connect() is called.
> 
> ... if you make consecutive connect(<dst address>) calls.
> 
>   __udp_disconnect() clears saddr, so:
> 
> connect(<AF_UNSPEC>); connect(<dst address>);
> 
> will yield the expected result
> 
>> I'm not sure if this is by-design for some reason?
>> Or can I remove this checking? like:
>>
>> --- a/net/ipv4/datagram.c
>> +++ b/net/ipv4/datagram.c
>> @@ -67,11 +67,9 @@ int __ip4_datagram_connect(struct sock *sk, struct
>> sockaddr *uaddr, int addr_len
>>           inet->inet_dport = usin->sin_port;
>>           if (!inet->inet_saddr)
>>                   inet->inet_saddr = fl4->saddr;
>> -       if (!inet->inet_rcv_saddr) {
>> -               inet->inet_rcv_saddr = fl4->saddr;
>> -               if (sk->sk_prot->rehash)
>> -                       sk->sk_prot->rehash(sk);
>> -       }
>> +       inet->inet_rcv_saddr = fl4->saddr;
>> +       if (sk->sk_prot->rehash)
>> +               sk->sk_prot->rehash(sk);
>>           reuseport_has_conns_set(sk);
>>           sk->sk_state = TCP_ESTABLISHED;
>>           sk_set_txhash(sk);
> 
> This sounds like an unexpected behaviour change which may broke existing
> applications.
> 
> I suggest retaining the current beheviour.
> 

Thank you for your suggestion. I've sent a fix patch for rehash4 (with 
few build errors I'll solve soon), leaving __ip4_datagram_connect() 
unchanged.

-- 
Philo