| Message ID | 4761e466ab9f7542c68cdc95f248987d127044d2.1733499715.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 51a00be6a0994da2ba6b4ace3b7a0d9373b4b25e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] udp: fix l4 hash after reconnect | expand |
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>
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.
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
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.
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
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.
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!
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 :(
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);
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(-)