* [PATCH net-next v3] inet: fix fast path in __inet_hash_connect()
@ 2023-01-14 13:11 Pietro Borrello
2023-01-16 16:19 ` Kuniyuki Iwashima
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Pietro Borrello @ 2023-01-14 13:11 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Hideaki YOSHIFUJI, David Ahern
Cc: Cristiano Giuffrida, Bos, H.J.,
Jakob Koschel, Kuniyuki Iwashima, netdev, linux-kernel,
Pietro Borrello
__inet_hash_connect() has a fast path taken if sk_head(&tb->owners) is
equal to the sk parameter.
sk_head() returns the hlist_entry() with respect to the sk_node field.
However entries in the tb->owners list are inserted with respect to the
sk_bind_node field with sk_add_bind_node().
Thus the check would never pass and the fast path never execute.
This fast path has never been executed or tested as this bug seems
to be present since commit 1da177e4c3f4 ("Linux-2.6.12-rc2"), thus
remove it to reduce code complexity.
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
Changes in v3:
- remove the fast path to reduce code complexity
- Link to v2: https://lore.kernel.org/r/20230112-inet_hash_connect_bind_head-v2-1-5ec926ddd985@diag.uniroma1.it
Changes in v2:
- nit: s/list_entry/hlist_entry/
- Link to v1: https://lore.kernel.org/r/20230112-inet_hash_connect_bind_head-v1-1-7e3c770157c8@diag.uniroma1.it
---
net/ipv4/inet_hashtables.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index d039b4e732a3..b832e7a545d4 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -994,17 +994,7 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
u32 index;
if (port) {
- head = &hinfo->bhash[inet_bhashfn(net, port,
- hinfo->bhash_size)];
- tb = inet_csk(sk)->icsk_bind_hash;
- spin_lock_bh(&head->lock);
- if (sk_head(&tb->owners) == sk && !sk->sk_bind_node.next) {
- inet_ehash_nolisten(sk, NULL, NULL);
- spin_unlock_bh(&head->lock);
- return 0;
- }
- spin_unlock(&head->lock);
- /* No definite answer... Walk to established hash table */
+ local_bh_disable();
ret = check_established(death_row, sk, port, NULL);
local_bh_enable();
return ret;
---
base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
change-id: 20230112-inet_hash_connect_bind_head-8f2dc98f08b1
Best regards,
--
Pietro Borrello <borrello@diag.uniroma1.it>
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v3] inet: fix fast path in __inet_hash_connect()
2023-01-14 13:11 [PATCH net-next v3] inet: fix fast path in __inet_hash_connect() Pietro Borrello
@ 2023-01-16 16:19 ` Kuniyuki Iwashima
2023-01-17 10:21 ` Paolo Abeni
2023-01-17 11:30 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 5+ messages in thread
From: Kuniyuki Iwashima @ 2023-01-16 16:19 UTC (permalink / raw)
To: borrello
Cc: c.giuffrida, davem, dsahern, edumazet, h.j.bos, jkl820.git, kuba,
kuniyu, linux-kernel, netdev, pabeni, yoshfuji
From: Pietro Borrello <borrello@diag.uniroma1.it>
Date: Sat, 14 Jan 2023 13:11:41 +0000
> __inet_hash_connect() has a fast path taken if sk_head(&tb->owners) is
> equal to the sk parameter.
> sk_head() returns the hlist_entry() with respect to the sk_node field.
> However entries in the tb->owners list are inserted with respect to the
> sk_bind_node field with sk_add_bind_node().
> Thus the check would never pass and the fast path never execute.
>
> This fast path has never been executed or tested as this bug seems
> to be present since commit 1da177e4c3f4 ("Linux-2.6.12-rc2"), thus
> remove it to reduce code complexity.
>
> Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Thank you!
> ---
> Changes in v3:
> - remove the fast path to reduce code complexity
> - Link to v2: https://lore.kernel.org/r/20230112-inet_hash_connect_bind_head-v2-1-5ec926ddd985@diag.uniroma1.it
>
> Changes in v2:
> - nit: s/list_entry/hlist_entry/
> - Link to v1: https://lore.kernel.org/r/20230112-inet_hash_connect_bind_head-v1-1-7e3c770157c8@diag.uniroma1.it
> ---
> net/ipv4/inet_hashtables.c | 12 +-----------
> 1 file changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index d039b4e732a3..b832e7a545d4 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -994,17 +994,7 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
> u32 index;
>
> if (port) {
> - head = &hinfo->bhash[inet_bhashfn(net, port,
> - hinfo->bhash_size)];
> - tb = inet_csk(sk)->icsk_bind_hash;
> - spin_lock_bh(&head->lock);
> - if (sk_head(&tb->owners) == sk && !sk->sk_bind_node.next) {
> - inet_ehash_nolisten(sk, NULL, NULL);
> - spin_unlock_bh(&head->lock);
> - return 0;
> - }
> - spin_unlock(&head->lock);
> - /* No definite answer... Walk to established hash table */
> + local_bh_disable();
> ret = check_established(death_row, sk, port, NULL);
> local_bh_enable();
> return ret;
>
> ---
> base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
> change-id: 20230112-inet_hash_connect_bind_head-8f2dc98f08b1
>
> Best regards,
> --
> Pietro Borrello <borrello@diag.uniroma1.it>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v3] inet: fix fast path in __inet_hash_connect()
2023-01-14 13:11 [PATCH net-next v3] inet: fix fast path in __inet_hash_connect() Pietro Borrello
2023-01-16 16:19 ` Kuniyuki Iwashima
@ 2023-01-17 10:21 ` Paolo Abeni
2023-01-17 10:27 ` Eric Dumazet
2023-01-17 11:30 ` patchwork-bot+netdevbpf
2 siblings, 1 reply; 5+ messages in thread
From: Paolo Abeni @ 2023-01-17 10:21 UTC (permalink / raw)
To: Pietro Borrello, David S. Miller, Eric Dumazet, Jakub Kicinski,
Hideaki YOSHIFUJI, David Ahern
Cc: Cristiano Giuffrida, Bos, H.J.,
Jakob Koschel, Kuniyuki Iwashima, netdev, linux-kernel
On Sat, 2023-01-14 at 13:11 +0000, Pietro Borrello wrote:
> __inet_hash_connect() has a fast path taken if sk_head(&tb->owners) is
> equal to the sk parameter.
> sk_head() returns the hlist_entry() with respect to the sk_node field.
> However entries in the tb->owners list are inserted with respect to the
> sk_bind_node field with sk_add_bind_node().
> Thus the check would never pass and the fast path never execute.
>
> This fast path has never been executed or tested as this bug seems
> to be present since commit 1da177e4c3f4 ("Linux-2.6.12-rc2"), thus
> remove it to reduce code complexity.
>
> Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> ---
> Changes in v3:
> - remove the fast path to reduce code complexity
> - Link to v2: https://lore.kernel.org/r/20230112-inet_hash_connect_bind_head-v2-1-5ec926ddd985@diag.uniroma1.it
>
> Changes in v2:
> - nit: s/list_entry/hlist_entry/
> - Link to v1: https://lore.kernel.org/r/20230112-inet_hash_connect_bind_head-v1-1-7e3c770157c8@diag.uniroma1.it
> ---
> net/ipv4/inet_hashtables.c | 12 +-----------
> 1 file changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index d039b4e732a3..b832e7a545d4 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -994,17 +994,7 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
> u32 index;
>
> if (port) {
> - head = &hinfo->bhash[inet_bhashfn(net, port,
> - hinfo->bhash_size)];
> - tb = inet_csk(sk)->icsk_bind_hash;
> - spin_lock_bh(&head->lock);
> - if (sk_head(&tb->owners) == sk && !sk->sk_bind_node.next) {
> - inet_ehash_nolisten(sk, NULL, NULL);
> - spin_unlock_bh(&head->lock);
> - return 0;
> - }
> - spin_unlock(&head->lock);
> - /* No definite answer... Walk to established hash table */
> + local_bh_disable();
> ret = check_established(death_row, sk, port, NULL);
> local_bh_enable();
> return ret;
LGTM, thanks!
Eric, do you have any additional comment?
Cheers,
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v3] inet: fix fast path in __inet_hash_connect()
2023-01-17 10:21 ` Paolo Abeni
@ 2023-01-17 10:27 ` Eric Dumazet
0 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2023-01-17 10:27 UTC (permalink / raw)
To: Paolo Abeni
Cc: Pietro Borrello, David S. Miller, Jakub Kicinski,
Hideaki YOSHIFUJI, David Ahern, Cristiano Giuffrida, Bos, H.J.,
Jakob Koschel, Kuniyuki Iwashima, netdev, linux-kernel
On Tue, Jan 17, 2023 at 11:21 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Sat, 2023-01-14 at 13:11 +0000, Pietro Borrello wrote:
> > __inet_hash_connect() has a fast path taken if sk_head(&tb->owners) is
> > equal to the sk parameter.
> > sk_head() returns the hlist_entry() with respect to the sk_node field.
> > However entries in the tb->owners list are inserted with respect to the
> > sk_bind_node field with sk_add_bind_node().
> > Thus the check would never pass and the fast path never execute.
> >
> > This fast path has never been executed or tested as this bug seems
> > to be present since commit 1da177e4c3f4 ("Linux-2.6.12-rc2"), thus
> > remove it to reduce code complexity.
> >
> > Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> > ---
> > Changes in v3:
> > - remove the fast path to reduce code complexity
> > - Link to v2: https://lore.kernel.org/r/20230112-inet_hash_connect_bind_head-v2-1-5ec926ddd985@diag.uniroma1.it
> >
> > Changes in v2:
> > - nit: s/list_entry/hlist_entry/
> > - Link to v1: https://lore.kernel.org/r/20230112-inet_hash_connect_bind_head-v1-1-7e3c770157c8@diag.uniroma1.it
> > ---
> > net/ipv4/inet_hashtables.c | 12 +-----------
> > 1 file changed, 1 insertion(+), 11 deletions(-)
> >
> > diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> > index d039b4e732a3..b832e7a545d4 100644
> > --- a/net/ipv4/inet_hashtables.c
> > +++ b/net/ipv4/inet_hashtables.c
> > @@ -994,17 +994,7 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
> > u32 index;
> >
> > if (port) {
> > - head = &hinfo->bhash[inet_bhashfn(net, port,
> > - hinfo->bhash_size)];
> > - tb = inet_csk(sk)->icsk_bind_hash;
> > - spin_lock_bh(&head->lock);
> > - if (sk_head(&tb->owners) == sk && !sk->sk_bind_node.next) {
> > - inet_ehash_nolisten(sk, NULL, NULL);
> > - spin_unlock_bh(&head->lock);
> > - return 0;
> > - }
> > - spin_unlock(&head->lock);
> > - /* No definite answer... Walk to established hash table */
> > + local_bh_disable();
> > ret = check_established(death_row, sk, port, NULL);
> > local_bh_enable();
> > return ret;
>
> LGTM, thanks!
>
> Eric, do you have any additional comment?
Oh right, I have missed this version.
Reviewed-by: Eric Dumazet <edumazet@google.com>
Thanks !
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v3] inet: fix fast path in __inet_hash_connect()
2023-01-14 13:11 [PATCH net-next v3] inet: fix fast path in __inet_hash_connect() Pietro Borrello
2023-01-16 16:19 ` Kuniyuki Iwashima
2023-01-17 10:21 ` Paolo Abeni
@ 2023-01-17 11:30 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-01-17 11:30 UTC (permalink / raw)
To: Pietro Borrello
Cc: davem, edumazet, kuba, pabeni, yoshfuji, dsahern, c.giuffrida,
h.j.bos, jkl820.git, kuniyu, netdev, linux-kernel
Hello:
This patch was applied to netdev/net-next.git (master)
by Paolo Abeni <pabeni@redhat.com>:
On Sat, 14 Jan 2023 13:11:41 +0000 you wrote:
> __inet_hash_connect() has a fast path taken if sk_head(&tb->owners) is
> equal to the sk parameter.
> sk_head() returns the hlist_entry() with respect to the sk_node field.
> However entries in the tb->owners list are inserted with respect to the
> sk_bind_node field with sk_add_bind_node().
> Thus the check would never pass and the fast path never execute.
>
> [...]
Here is the summary with links:
- [net-next,v3] inet: fix fast path in __inet_hash_connect()
https://git.kernel.org/netdev/net-next/c/21cbd90a6fab
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-01-17 11:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-14 13:11 [PATCH net-next v3] inet: fix fast path in __inet_hash_connect() Pietro Borrello
2023-01-16 16:19 ` Kuniyuki Iwashima
2023-01-17 10:21 ` Paolo Abeni
2023-01-17 10:27 ` Eric Dumazet
2023-01-17 11:30 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).