* [PATCH 0/2] net/tcp: Dynamically disable TCP-MD5 static key
@ 2022-11-02 21:13 Dmitry Safonov
2022-11-02 21:13 ` [PATCH 1/2] net/tcp: Separate tcp_md5sig_info allocation into tcp_md5sig_info_add() Dmitry Safonov
2022-11-02 21:13 ` [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction Dmitry Safonov
0 siblings, 2 replies; 11+ messages in thread
From: Dmitry Safonov @ 2022-11-02 21:13 UTC (permalink / raw)
To: linux-kernel, David Ahern, Eric Dumazet
Cc: Dmitry Safonov, Bob Gilligan, David S. Miller, Dmitry Safonov,
Francesco Ruggeri, Hideaki YOSHIFUJI, Jakub Kicinski,
Paolo Abeni, Salam Noureddine, netdev
The static key introduced by commit 6015c71e656b ("tcp: md5: add
tcp_md5_needed jump label") is a fast-path optimization aimed at
avoiding a cache line miss.
Once an MD5 key is introduced in the system the static key is enabled
and never disabled. Address this by disabling the static key when
the last tcp_md5sig_info in system is destroyed.
Previously it was submitted as a part of TCP-AO patches set [1].
Now in attempt to split 36 patches submission, I send this independently.
Cc: Bob Gilligan <gilligan@arista.com>
Cc: David Ahern <dsahern@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Francesco Ruggeri <fruggeri@arista.com>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Salam Noureddine <noureddine@arista.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
[1]: https://lore.kernel.org/all/20221027204347.529913-1-dima@arista.com/T/#u
Thanks,
Dmitry
Dmitry Safonov (2):
net/tcp: Separate tcp_md5sig_info allocation into
tcp_md5sig_info_add()
net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction
include/net/tcp.h | 10 ++++--
net/ipv4/tcp.c | 5 +--
net/ipv4/tcp_ipv4.c | 74 +++++++++++++++++++++++++++++++---------
net/ipv4/tcp_minisocks.c | 9 +++--
net/ipv4/tcp_output.c | 4 +--
net/ipv6/tcp_ipv6.c | 10 +++---
6 files changed, 78 insertions(+), 34 deletions(-)
base-commit: 8f71a2b3f435f29b787537d1abedaa7d8ebe6647
--
2.38.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] net/tcp: Separate tcp_md5sig_info allocation into tcp_md5sig_info_add() 2022-11-02 21:13 [PATCH 0/2] net/tcp: Dynamically disable TCP-MD5 static key Dmitry Safonov @ 2022-11-02 21:13 ` Dmitry Safonov 2022-11-02 21:27 ` Eric Dumazet 2022-11-02 21:13 ` [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction Dmitry Safonov 1 sibling, 1 reply; 11+ messages in thread From: Dmitry Safonov @ 2022-11-02 21:13 UTC (permalink / raw) To: linux-kernel, David Ahern, Eric Dumazet Cc: Dmitry Safonov, Bob Gilligan, David S. Miller, Dmitry Safonov, Francesco Ruggeri, Hideaki YOSHIFUJI, Jakub Kicinski, Paolo Abeni, Salam Noureddine, netdev Add a helper to allocate tcp_md5sig_info, that will help later to do/allocate things when info allocated, once per socket. Signed-off-by: Dmitry Safonov <dima@arista.com> --- net/ipv4/tcp_ipv4.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 87d440f47a70..fae80b1a1796 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -1172,6 +1172,24 @@ struct tcp_md5sig_key *tcp_v4_md5_lookup(const struct sock *sk, } EXPORT_SYMBOL(tcp_v4_md5_lookup); +static int tcp_md5sig_info_add(struct sock *sk, gfp_t gfp) +{ + struct tcp_sock *tp = tcp_sk(sk); + struct tcp_md5sig_info *md5sig; + + if (rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk))) + return 0; + + md5sig = kmalloc(sizeof(*md5sig), gfp); + if (!md5sig) + return -ENOMEM; + + sk_gso_disable(sk); + INIT_HLIST_HEAD(&md5sig->head); + rcu_assign_pointer(tp->md5sig_info, md5sig); + return 0; +} + /* This can be called on a newly created socket, from other files */ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, int family, u8 prefixlen, int l3index, u8 flags, @@ -1202,17 +1220,11 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, return 0; } + if (tcp_md5sig_info_add(sk, gfp)) + return -ENOMEM; + md5sig = rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk)); - if (!md5sig) { - md5sig = kmalloc(sizeof(*md5sig), gfp); - if (!md5sig) - return -ENOMEM; - - sk_gso_disable(sk); - INIT_HLIST_HEAD(&md5sig->head); - rcu_assign_pointer(tp->md5sig_info, md5sig); - } key = sock_kmalloc(sk, sizeof(*key), gfp | __GFP_ZERO); if (!key) -- 2.38.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] net/tcp: Separate tcp_md5sig_info allocation into tcp_md5sig_info_add() 2022-11-02 21:13 ` [PATCH 1/2] net/tcp: Separate tcp_md5sig_info allocation into tcp_md5sig_info_add() Dmitry Safonov @ 2022-11-02 21:27 ` Eric Dumazet 0 siblings, 0 replies; 11+ messages in thread From: Eric Dumazet @ 2022-11-02 21:27 UTC (permalink / raw) To: Dmitry Safonov Cc: linux-kernel, David Ahern, Bob Gilligan, David S. Miller, Dmitry Safonov, Francesco Ruggeri, Hideaki YOSHIFUJI, Jakub Kicinski, Paolo Abeni, Salam Noureddine, netdev On Wed, Nov 2, 2022 at 2:14 PM Dmitry Safonov <dima@arista.com> wrote: > > Add a helper to allocate tcp_md5sig_info, that will help later to > do/allocate things when info allocated, once per socket. > > Signed-off-by: Dmitry Safonov <dima@arista.com> > --- Reviewed-by: Eric Dumazet <edumazet@google.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction 2022-11-02 21:13 [PATCH 0/2] net/tcp: Dynamically disable TCP-MD5 static key Dmitry Safonov 2022-11-02 21:13 ` [PATCH 1/2] net/tcp: Separate tcp_md5sig_info allocation into tcp_md5sig_info_add() Dmitry Safonov @ 2022-11-02 21:13 ` Dmitry Safonov 2022-11-02 21:25 ` Eric Dumazet 1 sibling, 1 reply; 11+ messages in thread From: Dmitry Safonov @ 2022-11-02 21:13 UTC (permalink / raw) To: linux-kernel, David Ahern, Eric Dumazet Cc: Dmitry Safonov, Bob Gilligan, David S. Miller, Dmitry Safonov, Francesco Ruggeri, Hideaki YOSHIFUJI, Jakub Kicinski, Paolo Abeni, Salam Noureddine, netdev To do that, separate two scenarios: - where it's the first MD5 key on the system, which means that enabling of the static key may need to sleep; - copying of an existing key from a listening socket to the request socket upon receiving a signed TCP segment, where static key was already enabled (when the key was added to the listening socket). Now the life-time of the static branch for TCP-MD5 is until: - last tcp_md5sig_info is destroyed - last socket in time-wait state with MD5 key is closed. Which means that after all sockets with TCP-MD5 keys are gone, the system gets back the performance of disabled md5-key static branch. Signed-off-by: Dmitry Safonov <dima@arista.com> --- include/net/tcp.h | 10 ++++--- net/ipv4/tcp.c | 5 +--- net/ipv4/tcp_ipv4.c | 56 ++++++++++++++++++++++++++++++---------- net/ipv4/tcp_minisocks.c | 9 ++++--- net/ipv4/tcp_output.c | 4 +-- net/ipv6/tcp_ipv6.c | 10 +++---- 6 files changed, 63 insertions(+), 31 deletions(-) diff --git a/include/net/tcp.h b/include/net/tcp.h index 14d45661a84d..a0cdf013782a 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -1675,7 +1675,11 @@ int tcp_v4_md5_hash_skb(char *md5_hash, const struct tcp_md5sig_key *key, const struct sock *sk, const struct sk_buff *skb); int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, int family, u8 prefixlen, int l3index, u8 flags, - const u8 *newkey, u8 newkeylen, gfp_t gfp); + const u8 *newkey, u8 newkeylen); +int tcp_md5_key_copy(struct sock *sk, const union tcp_md5_addr *addr, + int family, u8 prefixlen, int l3index, + struct tcp_md5sig_key *key); + int tcp_md5_do_del(struct sock *sk, const union tcp_md5_addr *addr, int family, u8 prefixlen, int l3index, u8 flags); struct tcp_md5sig_key *tcp_v4_md5_lookup(const struct sock *sk, @@ -1683,7 +1687,7 @@ struct tcp_md5sig_key *tcp_v4_md5_lookup(const struct sock *sk, #ifdef CONFIG_TCP_MD5SIG #include <linux/jump_label.h> -extern struct static_key_false tcp_md5_needed; +extern struct static_key_false_deferred tcp_md5_needed; struct tcp_md5sig_key *__tcp_md5_do_lookup(const struct sock *sk, int l3index, const union tcp_md5_addr *addr, int family); @@ -1691,7 +1695,7 @@ static inline struct tcp_md5sig_key * tcp_md5_do_lookup(const struct sock *sk, int l3index, const union tcp_md5_addr *addr, int family) { - if (!static_branch_unlikely(&tcp_md5_needed)) + if (!static_branch_unlikely(&tcp_md5_needed.key)) return NULL; return __tcp_md5_do_lookup(sk, l3index, addr, family); } diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index ef14efa1fb70..936ed566cc89 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -4460,11 +4460,8 @@ bool tcp_alloc_md5sig_pool(void) if (unlikely(!READ_ONCE(tcp_md5sig_pool_populated))) { mutex_lock(&tcp_md5sig_mutex); - if (!tcp_md5sig_pool_populated) { + if (!tcp_md5sig_pool_populated) __tcp_alloc_md5sig_pool(); - if (tcp_md5sig_pool_populated) - static_branch_inc(&tcp_md5_needed); - } mutex_unlock(&tcp_md5sig_mutex); } diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index fae80b1a1796..f812d507fc9a 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -1064,7 +1064,7 @@ static void tcp_v4_reqsk_destructor(struct request_sock *req) * We need to maintain these in the sk structure. */ -DEFINE_STATIC_KEY_FALSE(tcp_md5_needed); +DEFINE_STATIC_KEY_DEFERRED_FALSE(tcp_md5_needed, HZ); EXPORT_SYMBOL(tcp_md5_needed); static bool better_md5_match(struct tcp_md5sig_key *old, struct tcp_md5sig_key *new) @@ -1177,9 +1177,6 @@ static int tcp_md5sig_info_add(struct sock *sk, gfp_t gfp) struct tcp_sock *tp = tcp_sk(sk); struct tcp_md5sig_info *md5sig; - if (rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk))) - return 0; - md5sig = kmalloc(sizeof(*md5sig), gfp); if (!md5sig) return -ENOMEM; @@ -1191,9 +1188,9 @@ static int tcp_md5sig_info_add(struct sock *sk, gfp_t gfp) } /* This can be called on a newly created socket, from other files */ -int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, - int family, u8 prefixlen, int l3index, u8 flags, - const u8 *newkey, u8 newkeylen, gfp_t gfp) +static int __tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, + int family, u8 prefixlen, int l3index, u8 flags, + const u8 *newkey, u8 newkeylen, gfp_t gfp) { /* Add Key to the list */ struct tcp_md5sig_key *key; @@ -1220,9 +1217,6 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, return 0; } - if (tcp_md5sig_info_add(sk, gfp)) - return -ENOMEM; - md5sig = rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk)); @@ -1246,8 +1240,44 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, hlist_add_head_rcu(&key->node, &md5sig->head); return 0; } + +int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, + int family, u8 prefixlen, int l3index, u8 flags, + const u8 *newkey, u8 newkeylen) +{ + struct tcp_sock *tp = tcp_sk(sk); + + if (!rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk))) { + if (tcp_md5sig_info_add(sk, GFP_KERNEL)) + return -ENOMEM; + + static_branch_inc(&tcp_md5_needed.key); + } + + return __tcp_md5_do_add(sk, addr, family, prefixlen, l3index, flags, + newkey, newkeylen, GFP_KERNEL); +} EXPORT_SYMBOL(tcp_md5_do_add); +int tcp_md5_key_copy(struct sock *sk, const union tcp_md5_addr *addr, + int family, u8 prefixlen, int l3index, + struct tcp_md5sig_key *key) +{ + struct tcp_sock *tp = tcp_sk(sk); + + if (!rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk))) { + if (tcp_md5sig_info_add(sk, sk_gfp_mask(sk, GFP_ATOMIC))) + return -ENOMEM; + + atomic_inc(&tcp_md5_needed.key.key.enabled); + } + + return __tcp_md5_do_add(sk, addr, family, prefixlen, l3index, + key->flags, key->key, key->keylen, + sk_gfp_mask(sk, GFP_ATOMIC)); +} +EXPORT_SYMBOL(tcp_md5_key_copy); + int tcp_md5_do_del(struct sock *sk, const union tcp_md5_addr *addr, int family, u8 prefixlen, int l3index, u8 flags) { @@ -1334,7 +1364,7 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, int optname, return -EINVAL; return tcp_md5_do_add(sk, addr, AF_INET, prefixlen, l3index, flags, - cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL); + cmd.tcpm_key, cmd.tcpm_keylen); } static int tcp_v4_md5_hash_headers(struct tcp_md5sig_pool *hp, @@ -1591,8 +1621,7 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb, * memory, then we end up not copying the key * across. Shucks. */ - tcp_md5_do_add(newsk, addr, AF_INET, 32, l3index, key->flags, - key->key, key->keylen, GFP_ATOMIC); + tcp_md5_key_copy(newsk, addr, AF_INET, 32, l3index, key); sk_gso_disable(newsk); } #endif @@ -2284,6 +2313,7 @@ void tcp_v4_destroy_sock(struct sock *sk) tcp_clear_md5_list(sk); kfree_rcu(rcu_dereference_protected(tp->md5sig_info, 1), rcu); tp->md5sig_info = NULL; + static_branch_slow_dec_deferred(&tcp_md5_needed); } #endif diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c index c375f603a16c..fb500160b8d2 100644 --- a/net/ipv4/tcp_minisocks.c +++ b/net/ipv4/tcp_minisocks.c @@ -291,13 +291,14 @@ void tcp_time_wait(struct sock *sk, int state, int timeo) */ do { tcptw->tw_md5_key = NULL; - if (static_branch_unlikely(&tcp_md5_needed)) { + if (static_branch_unlikely(&tcp_md5_needed.key)) { struct tcp_md5sig_key *key; key = tp->af_specific->md5_lookup(sk, sk); if (key) { tcptw->tw_md5_key = kmemdup(key, sizeof(*key), GFP_ATOMIC); BUG_ON(tcptw->tw_md5_key && !tcp_alloc_md5sig_pool()); + atomic_inc(&tcp_md5_needed.key.key.enabled); } } } while (0); @@ -337,11 +338,13 @@ EXPORT_SYMBOL(tcp_time_wait); void tcp_twsk_destructor(struct sock *sk) { #ifdef CONFIG_TCP_MD5SIG - if (static_branch_unlikely(&tcp_md5_needed)) { + if (static_branch_unlikely(&tcp_md5_needed.key)) { struct tcp_timewait_sock *twsk = tcp_twsk(sk); - if (twsk->tw_md5_key) + if (twsk->tw_md5_key) { kfree_rcu(twsk->tw_md5_key, rcu); + static_branch_slow_dec_deferred(&tcp_md5_needed); + } } #endif } diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index c69f4d966024..86e71c8c76bc 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -766,7 +766,7 @@ static unsigned int tcp_syn_options(struct sock *sk, struct sk_buff *skb, *md5 = NULL; #ifdef CONFIG_TCP_MD5SIG - if (static_branch_unlikely(&tcp_md5_needed) && + if (static_branch_unlikely(&tcp_md5_needed.key) && rcu_access_pointer(tp->md5sig_info)) { *md5 = tp->af_specific->md5_lookup(sk, sk); if (*md5) { @@ -922,7 +922,7 @@ static unsigned int tcp_established_options(struct sock *sk, struct sk_buff *skb *md5 = NULL; #ifdef CONFIG_TCP_MD5SIG - if (static_branch_unlikely(&tcp_md5_needed) && + if (static_branch_unlikely(&tcp_md5_needed.key) && rcu_access_pointer(tp->md5sig_info)) { *md5 = tp->af_specific->md5_lookup(sk, sk); if (*md5) { diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c index 2a3f9296df1e..3e3bdc120fc8 100644 --- a/net/ipv6/tcp_ipv6.c +++ b/net/ipv6/tcp_ipv6.c @@ -677,12 +677,11 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, int optname, if (ipv6_addr_v4mapped(&sin6->sin6_addr)) return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3], AF_INET, prefixlen, l3index, flags, - cmd.tcpm_key, cmd.tcpm_keylen, - GFP_KERNEL); + cmd.tcpm_key, cmd.tcpm_keylen); return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr, AF_INET6, prefixlen, l3index, flags, - cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL); + cmd.tcpm_key, cmd.tcpm_keylen); } static int tcp_v6_md5_hash_headers(struct tcp_md5sig_pool *hp, @@ -1382,9 +1381,8 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff * * memory, then we end up not copying the key * across. Shucks. */ - tcp_md5_do_add(newsk, (union tcp_md5_addr *)&newsk->sk_v6_daddr, - AF_INET6, 128, l3index, key->flags, key->key, key->keylen, - sk_gfp_mask(sk, GFP_ATOMIC)); + tcp_md5_key_copy(newsk, (union tcp_md5_addr *)&newsk->sk_v6_daddr, + AF_INET6, 128, l3index, key); } #endif -- 2.38.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction 2022-11-02 21:13 ` [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction Dmitry Safonov @ 2022-11-02 21:25 ` Eric Dumazet 2022-11-02 21:40 ` Dmitry Safonov 2022-11-03 16:53 ` Dmitry Safonov 0 siblings, 2 replies; 11+ messages in thread From: Eric Dumazet @ 2022-11-02 21:25 UTC (permalink / raw) To: Dmitry Safonov Cc: linux-kernel, David Ahern, Bob Gilligan, David S. Miller, Dmitry Safonov, Francesco Ruggeri, Hideaki YOSHIFUJI, Jakub Kicinski, Paolo Abeni, Salam Noureddine, netdev On Wed, Nov 2, 2022 at 2:14 PM Dmitry Safonov <dima@arista.com> wrote: > > To do that, separate two scenarios: > - where it's the first MD5 key on the system, which means that enabling > of the static key may need to sleep; > - copying of an existing key from a listening socket to the request > socket upon receiving a signed TCP segment, where static key was > already enabled (when the key was added to the listening socket). > > Now the life-time of the static branch for TCP-MD5 is until: > - last tcp_md5sig_info is destroyed > - last socket in time-wait state with MD5 key is closed. > > Which means that after all sockets with TCP-MD5 keys are gone, the > system gets back the performance of disabled md5-key static branch. > > Signed-off-by: Dmitry Safonov <dima@arista.com> > --- > include/net/tcp.h | 10 ++++--- > net/ipv4/tcp.c | 5 +--- > net/ipv4/tcp_ipv4.c | 56 ++++++++++++++++++++++++++++++---------- > net/ipv4/tcp_minisocks.c | 9 ++++--- > net/ipv4/tcp_output.c | 4 +-- > net/ipv6/tcp_ipv6.c | 10 +++---- > 6 files changed, 63 insertions(+), 31 deletions(-) > > diff --git a/include/net/tcp.h b/include/net/tcp.h > index 14d45661a84d..a0cdf013782a 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -1675,7 +1675,11 @@ int tcp_v4_md5_hash_skb(char *md5_hash, const struct tcp_md5sig_key *key, > const struct sock *sk, const struct sk_buff *skb); > int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, > int family, u8 prefixlen, int l3index, u8 flags, > - const u8 *newkey, u8 newkeylen, gfp_t gfp); > + const u8 *newkey, u8 newkeylen); > +int tcp_md5_key_copy(struct sock *sk, const union tcp_md5_addr *addr, > + int family, u8 prefixlen, int l3index, > + struct tcp_md5sig_key *key); > + > int tcp_md5_do_del(struct sock *sk, const union tcp_md5_addr *addr, > int family, u8 prefixlen, int l3index, u8 flags); > struct tcp_md5sig_key *tcp_v4_md5_lookup(const struct sock *sk, > @@ -1683,7 +1687,7 @@ struct tcp_md5sig_key *tcp_v4_md5_lookup(const struct sock *sk, > > #ifdef CONFIG_TCP_MD5SIG > #include <linux/jump_label.h> > -extern struct static_key_false tcp_md5_needed; > +extern struct static_key_false_deferred tcp_md5_needed; > struct tcp_md5sig_key *__tcp_md5_do_lookup(const struct sock *sk, int l3index, > const union tcp_md5_addr *addr, > int family); > @@ -1691,7 +1695,7 @@ static inline struct tcp_md5sig_key * > tcp_md5_do_lookup(const struct sock *sk, int l3index, > const union tcp_md5_addr *addr, int family) > { > - if (!static_branch_unlikely(&tcp_md5_needed)) > + if (!static_branch_unlikely(&tcp_md5_needed.key)) > return NULL; > return __tcp_md5_do_lookup(sk, l3index, addr, family); > } > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index ef14efa1fb70..936ed566cc89 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -4460,11 +4460,8 @@ bool tcp_alloc_md5sig_pool(void) > if (unlikely(!READ_ONCE(tcp_md5sig_pool_populated))) { > mutex_lock(&tcp_md5sig_mutex); > > - if (!tcp_md5sig_pool_populated) { > + if (!tcp_md5sig_pool_populated) > __tcp_alloc_md5sig_pool(); > - if (tcp_md5sig_pool_populated) > - static_branch_inc(&tcp_md5_needed); > - } > > mutex_unlock(&tcp_md5sig_mutex); > } > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > index fae80b1a1796..f812d507fc9a 100644 > --- a/net/ipv4/tcp_ipv4.c > +++ b/net/ipv4/tcp_ipv4.c > @@ -1064,7 +1064,7 @@ static void tcp_v4_reqsk_destructor(struct request_sock *req) > * We need to maintain these in the sk structure. > */ > > -DEFINE_STATIC_KEY_FALSE(tcp_md5_needed); > +DEFINE_STATIC_KEY_DEFERRED_FALSE(tcp_md5_needed, HZ); > EXPORT_SYMBOL(tcp_md5_needed); > > static bool better_md5_match(struct tcp_md5sig_key *old, struct tcp_md5sig_key *new) > @@ -1177,9 +1177,6 @@ static int tcp_md5sig_info_add(struct sock *sk, gfp_t gfp) > struct tcp_sock *tp = tcp_sk(sk); > struct tcp_md5sig_info *md5sig; > > - if (rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk))) > - return 0; > - > md5sig = kmalloc(sizeof(*md5sig), gfp); > if (!md5sig) > return -ENOMEM; > @@ -1191,9 +1188,9 @@ static int tcp_md5sig_info_add(struct sock *sk, gfp_t gfp) > } > > /* This can be called on a newly created socket, from other files */ > -int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, > - int family, u8 prefixlen, int l3index, u8 flags, > - const u8 *newkey, u8 newkeylen, gfp_t gfp) > +static int __tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, > + int family, u8 prefixlen, int l3index, u8 flags, > + const u8 *newkey, u8 newkeylen, gfp_t gfp) > { > /* Add Key to the list */ > struct tcp_md5sig_key *key; > @@ -1220,9 +1217,6 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, > return 0; > } > > - if (tcp_md5sig_info_add(sk, gfp)) > - return -ENOMEM; > - > md5sig = rcu_dereference_protected(tp->md5sig_info, > lockdep_sock_is_held(sk)); > > @@ -1246,8 +1240,44 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, > hlist_add_head_rcu(&key->node, &md5sig->head); > return 0; > } > + > +int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, > + int family, u8 prefixlen, int l3index, u8 flags, > + const u8 *newkey, u8 newkeylen) > +{ > + struct tcp_sock *tp = tcp_sk(sk); > + > + if (!rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk))) { > + if (tcp_md5sig_info_add(sk, GFP_KERNEL)) > + return -ENOMEM; > + > + static_branch_inc(&tcp_md5_needed.key); > + } > + > + return __tcp_md5_do_add(sk, addr, family, prefixlen, l3index, flags, > + newkey, newkeylen, GFP_KERNEL); > +} > EXPORT_SYMBOL(tcp_md5_do_add); > > +int tcp_md5_key_copy(struct sock *sk, const union tcp_md5_addr *addr, > + int family, u8 prefixlen, int l3index, > + struct tcp_md5sig_key *key) > +{ > + struct tcp_sock *tp = tcp_sk(sk); > + > + if (!rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk))) { > + if (tcp_md5sig_info_add(sk, sk_gfp_mask(sk, GFP_ATOMIC))) > + return -ENOMEM; > + > + atomic_inc(&tcp_md5_needed.key.key.enabled); static_branch_inc ? > + } > + > + return __tcp_md5_do_add(sk, addr, family, prefixlen, l3index, > + key->flags, key->key, key->keylen, > + sk_gfp_mask(sk, GFP_ATOMIC)); > +} > +EXPORT_SYMBOL(tcp_md5_key_copy); > + > int tcp_md5_do_del(struct sock *sk, const union tcp_md5_addr *addr, int family, > u8 prefixlen, int l3index, u8 flags) > { > @@ -1334,7 +1364,7 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, int optname, > return -EINVAL; > > return tcp_md5_do_add(sk, addr, AF_INET, prefixlen, l3index, flags, > - cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL); > + cmd.tcpm_key, cmd.tcpm_keylen); > } > > static int tcp_v4_md5_hash_headers(struct tcp_md5sig_pool *hp, > @@ -1591,8 +1621,7 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb, > * memory, then we end up not copying the key > * across. Shucks. > */ > - tcp_md5_do_add(newsk, addr, AF_INET, 32, l3index, key->flags, > - key->key, key->keylen, GFP_ATOMIC); > + tcp_md5_key_copy(newsk, addr, AF_INET, 32, l3index, key); > sk_gso_disable(newsk); > } > #endif > @@ -2284,6 +2313,7 @@ void tcp_v4_destroy_sock(struct sock *sk) > tcp_clear_md5_list(sk); > kfree_rcu(rcu_dereference_protected(tp->md5sig_info, 1), rcu); > tp->md5sig_info = NULL; > + static_branch_slow_dec_deferred(&tcp_md5_needed); > } > #endif > > diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c > index c375f603a16c..fb500160b8d2 100644 > --- a/net/ipv4/tcp_minisocks.c > +++ b/net/ipv4/tcp_minisocks.c > @@ -291,13 +291,14 @@ void tcp_time_wait(struct sock *sk, int state, int timeo) > */ > do { > tcptw->tw_md5_key = NULL; > - if (static_branch_unlikely(&tcp_md5_needed)) { > + if (static_branch_unlikely(&tcp_md5_needed.key)) { > struct tcp_md5sig_key *key; > > key = tp->af_specific->md5_lookup(sk, sk); > if (key) { > tcptw->tw_md5_key = kmemdup(key, sizeof(*key), GFP_ATOMIC); > BUG_ON(tcptw->tw_md5_key && !tcp_alloc_md5sig_pool()); > + atomic_inc(&tcp_md5_needed.key.key.enabled); static_branch_inc > } > } > } while (0); > @@ -337,11 +338,13 @@ EXPORT_SYMBOL(tcp_time_wait); > void tcp_twsk_destructor(struct sock *sk) > { > #ifdef CONFIG_TCP_MD5SIG > - if (static_branch_unlikely(&tcp_md5_needed)) { > + if (static_branch_unlikely(&tcp_md5_needed.key)) { > struct tcp_timewait_sock *twsk = tcp_twsk(sk); > > - if (twsk->tw_md5_key) > + if (twsk->tw_md5_key) { Orthogonal to this patch, but I wonder why we do not clear twsk->tw_md5_key before kfree_rcu() It seems a lookup could catch the invalid pointer. > kfree_rcu(twsk->tw_md5_key, rcu); > + static_branch_slow_dec_deferred(&tcp_md5_needed); > + } > } > #endif > } > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index c69f4d966024..86e71c8c76bc 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -766,7 +766,7 @@ static unsigned int tcp_syn_options(struct sock *sk, struct sk_buff *skb, > > *md5 = NULL; > #ifdef CONFIG_TCP_MD5SIG > - if (static_branch_unlikely(&tcp_md5_needed) && > + if (static_branch_unlikely(&tcp_md5_needed.key) && > rcu_access_pointer(tp->md5sig_info)) { > *md5 = tp->af_specific->md5_lookup(sk, sk); > if (*md5) { > @@ -922,7 +922,7 @@ static unsigned int tcp_established_options(struct sock *sk, struct sk_buff *skb > > *md5 = NULL; > #ifdef CONFIG_TCP_MD5SIG > - if (static_branch_unlikely(&tcp_md5_needed) && > + if (static_branch_unlikely(&tcp_md5_needed.key) && > rcu_access_pointer(tp->md5sig_info)) { > *md5 = tp->af_specific->md5_lookup(sk, sk); > if (*md5) { > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c > index 2a3f9296df1e..3e3bdc120fc8 100644 > --- a/net/ipv6/tcp_ipv6.c > +++ b/net/ipv6/tcp_ipv6.c > @@ -677,12 +677,11 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, int optname, > if (ipv6_addr_v4mapped(&sin6->sin6_addr)) > return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3], > AF_INET, prefixlen, l3index, flags, > - cmd.tcpm_key, cmd.tcpm_keylen, > - GFP_KERNEL); > + cmd.tcpm_key, cmd.tcpm_keylen); > > return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr, > AF_INET6, prefixlen, l3index, flags, > - cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL); > + cmd.tcpm_key, cmd.tcpm_keylen); > } > > static int tcp_v6_md5_hash_headers(struct tcp_md5sig_pool *hp, > @@ -1382,9 +1381,8 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff * > * memory, then we end up not copying the key > * across. Shucks. > */ > - tcp_md5_do_add(newsk, (union tcp_md5_addr *)&newsk->sk_v6_daddr, > - AF_INET6, 128, l3index, key->flags, key->key, key->keylen, > - sk_gfp_mask(sk, GFP_ATOMIC)); > + tcp_md5_key_copy(newsk, (union tcp_md5_addr *)&newsk->sk_v6_daddr, > + AF_INET6, 128, l3index, key); > } > #endif > > -- > 2.38.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction 2022-11-02 21:25 ` Eric Dumazet @ 2022-11-02 21:40 ` Dmitry Safonov 2022-11-02 21:49 ` Eric Dumazet 2022-11-03 16:53 ` Dmitry Safonov 1 sibling, 1 reply; 11+ messages in thread From: Dmitry Safonov @ 2022-11-02 21:40 UTC (permalink / raw) To: Eric Dumazet Cc: linux-kernel, David Ahern, Bob Gilligan, David S. Miller, Dmitry Safonov, Francesco Ruggeri, Hideaki YOSHIFUJI, Jakub Kicinski, Paolo Abeni, Salam Noureddine, netdev On 11/2/22 21:25, Eric Dumazet wrote: > On Wed, Nov 2, 2022 at 2:14 PM Dmitry Safonov <dima@arista.com> wrote: [..] >> +int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, >> + int family, u8 prefixlen, int l3index, u8 flags, >> + const u8 *newkey, u8 newkeylen) >> +{ >> + struct tcp_sock *tp = tcp_sk(sk); >> + >> + if (!rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk))) { >> + if (tcp_md5sig_info_add(sk, GFP_KERNEL)) >> + return -ENOMEM; >> + >> + static_branch_inc(&tcp_md5_needed.key); >> + } >> + >> + return __tcp_md5_do_add(sk, addr, family, prefixlen, l3index, flags, >> + newkey, newkeylen, GFP_KERNEL); >> +} >> EXPORT_SYMBOL(tcp_md5_do_add); >> >> +int tcp_md5_key_copy(struct sock *sk, const union tcp_md5_addr *addr, >> + int family, u8 prefixlen, int l3index, >> + struct tcp_md5sig_key *key) >> +{ >> + struct tcp_sock *tp = tcp_sk(sk); >> + >> + if (!rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk))) { >> + if (tcp_md5sig_info_add(sk, sk_gfp_mask(sk, GFP_ATOMIC))) >> + return -ENOMEM; >> + >> + atomic_inc(&tcp_md5_needed.key.key.enabled); > > static_branch_inc ? That's the difference between tcp_md5_do_add() and tcp_md5_key_copy(): the first one can sleep on either allocation or static branch patching, while the second one is used where there is md5 key and it can't get destroyed during the function call. tcp_md5_key_copy() is called somewhere from the softirq handler so it needs an atomic allocation as well as this a little bit hacky part. [..] >> @@ -337,11 +338,13 @@ EXPORT_SYMBOL(tcp_time_wait); >> void tcp_twsk_destructor(struct sock *sk) >> { >> #ifdef CONFIG_TCP_MD5SIG >> - if (static_branch_unlikely(&tcp_md5_needed)) { >> + if (static_branch_unlikely(&tcp_md5_needed.key)) { >> struct tcp_timewait_sock *twsk = tcp_twsk(sk); >> >> - if (twsk->tw_md5_key) >> + if (twsk->tw_md5_key) { > > Orthogonal to this patch, but I wonder why we do not clear > twsk->tw_md5_key before kfree_rcu() > > It seems a lookup could catch the invalid pointer. > >> kfree_rcu(twsk->tw_md5_key, rcu); >> + static_branch_slow_dec_deferred(&tcp_md5_needed); >> + } >> } >> #endif A good question, let me check on this. Thanks for the review, Dmitry ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction 2022-11-02 21:40 ` Dmitry Safonov @ 2022-11-02 21:49 ` Eric Dumazet 2022-11-02 21:53 ` Eric Dumazet 0 siblings, 1 reply; 11+ messages in thread From: Eric Dumazet @ 2022-11-02 21:49 UTC (permalink / raw) To: Dmitry Safonov Cc: linux-kernel, David Ahern, Bob Gilligan, David S. Miller, Dmitry Safonov, Francesco Ruggeri, Hideaki YOSHIFUJI, Jakub Kicinski, Paolo Abeni, Salam Noureddine, netdev On Wed, Nov 2, 2022 at 2:40 PM Dmitry Safonov <dima@arista.com> wrote: > > On 11/2/22 21:25, Eric Dumazet wrote: > > On Wed, Nov 2, 2022 at 2:14 PM Dmitry Safonov <dima@arista.com> wrote: > [..] > >> +int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, > >> + int family, u8 prefixlen, int l3index, u8 flags, > >> + const u8 *newkey, u8 newkeylen) > >> +{ > >> + struct tcp_sock *tp = tcp_sk(sk); > >> + > >> + if (!rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk))) { > >> + if (tcp_md5sig_info_add(sk, GFP_KERNEL)) > >> + return -ENOMEM; > >> + > >> + static_branch_inc(&tcp_md5_needed.key); > >> + } > >> + > >> + return __tcp_md5_do_add(sk, addr, family, prefixlen, l3index, flags, > >> + newkey, newkeylen, GFP_KERNEL); > >> +} > >> EXPORT_SYMBOL(tcp_md5_do_add); > >> > >> +int tcp_md5_key_copy(struct sock *sk, const union tcp_md5_addr *addr, > >> + int family, u8 prefixlen, int l3index, > >> + struct tcp_md5sig_key *key) > >> +{ > >> + struct tcp_sock *tp = tcp_sk(sk); > >> + > >> + if (!rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk))) { > >> + if (tcp_md5sig_info_add(sk, sk_gfp_mask(sk, GFP_ATOMIC))) > >> + return -ENOMEM; > >> + > >> + atomic_inc(&tcp_md5_needed.key.key.enabled); > > > > static_branch_inc ? > > That's the difference between tcp_md5_do_add() and tcp_md5_key_copy(): > the first one can sleep on either allocation or static branch patching, > while the second one is used where there is md5 key and it can't get > destroyed during the function call. tcp_md5_key_copy() is called > somewhere from the softirq handler so it needs an atomic allocation as > well as this a little bit hacky part. > Are you sure ? static_branch_inc() is what we want here, it is a nice wrapper around the correct internal details, and ultimately boils to an atomic_inc(). It is safe for all contexts. But if/when jump labels get refcount_t one day, we will not have to change TCP stack because it made some implementation assumptions. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction 2022-11-02 21:49 ` Eric Dumazet @ 2022-11-02 21:53 ` Eric Dumazet 2022-11-03 15:40 ` Dmitry Safonov 0 siblings, 1 reply; 11+ messages in thread From: Eric Dumazet @ 2022-11-02 21:53 UTC (permalink / raw) To: Dmitry Safonov Cc: linux-kernel, David Ahern, Bob Gilligan, David S. Miller, Dmitry Safonov, Francesco Ruggeri, Hideaki YOSHIFUJI, Jakub Kicinski, Paolo Abeni, Salam Noureddine, netdev On Wed, Nov 2, 2022 at 2:49 PM Eric Dumazet <edumazet@google.com> wrote: > > Are you sure ? > > static_branch_inc() is what we want here, it is a nice wrapper around > the correct internal details, > and ultimately boils to an atomic_inc(). It is safe for all contexts. > > But if/when jump labels get refcount_t one day, we will not have to > change TCP stack because > it made some implementation assumptions. Oh, I think I understand this better now. Please provide a helper like static inline void static_key_fast_inc(struct static_key *key) { atomic_inc(&key->enabled); } Something like that. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction 2022-11-02 21:53 ` Eric Dumazet @ 2022-11-03 15:40 ` Dmitry Safonov 0 siblings, 0 replies; 11+ messages in thread From: Dmitry Safonov @ 2022-11-03 15:40 UTC (permalink / raw) To: Eric Dumazet Cc: linux-kernel, David Ahern, Bob Gilligan, David S. Miller, Dmitry Safonov, Francesco Ruggeri, Hideaki YOSHIFUJI, Jakub Kicinski, Paolo Abeni, Salam Noureddine, netdev On 11/2/22 21:53, Eric Dumazet wrote: > On Wed, Nov 2, 2022 at 2:49 PM Eric Dumazet <edumazet@google.com> wrote: > >> >> Are you sure ? >> >> static_branch_inc() is what we want here, it is a nice wrapper around >> the correct internal details, >> and ultimately boils to an atomic_inc(). It is safe for all contexts. >> >> But if/when jump labels get refcount_t one day, we will not have to >> change TCP stack because >> it made some implementation assumptions. > > Oh, I think I understand this better now. > > Please provide a helper like > > static inline void static_key_fast_inc(struct static_key *key) > { > atomic_inc(&key->enabled); > } > > Something like that. Sure, that sounds like a better thing to do, rather than the hack I had. Thanks, will send v2 soon, Dmitry ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction 2022-11-02 21:25 ` Eric Dumazet 2022-11-02 21:40 ` Dmitry Safonov @ 2022-11-03 16:53 ` Dmitry Safonov 2022-11-03 17:04 ` Eric Dumazet 1 sibling, 1 reply; 11+ messages in thread From: Dmitry Safonov @ 2022-11-03 16:53 UTC (permalink / raw) To: Eric Dumazet Cc: linux-kernel, David Ahern, Bob Gilligan, David S. Miller, Dmitry Safonov, Francesco Ruggeri, Hideaki YOSHIFUJI, Jakub Kicinski, Paolo Abeni, Salam Noureddine, netdev On 11/2/22 21:25, Eric Dumazet wrote: > On Wed, Nov 2, 2022 at 2:14 PM Dmitry Safonov <dima@arista.com> wrote: [..] >> @@ -337,11 +338,13 @@ EXPORT_SYMBOL(tcp_time_wait); >> void tcp_twsk_destructor(struct sock *sk) >> { >> #ifdef CONFIG_TCP_MD5SIG >> - if (static_branch_unlikely(&tcp_md5_needed)) { >> + if (static_branch_unlikely(&tcp_md5_needed.key)) { >> struct tcp_timewait_sock *twsk = tcp_twsk(sk); >> >> - if (twsk->tw_md5_key) >> + if (twsk->tw_md5_key) { > > Orthogonal to this patch, but I wonder why we do not clear > twsk->tw_md5_key before kfree_rcu() > > It seems a lookup could catch the invalid pointer. > >> kfree_rcu(twsk->tw_md5_key, rcu); >> + static_branch_slow_dec_deferred(&tcp_md5_needed); >> + } >> } I looked into that, it seems tcp_twsk_destructor() is called from inet_twsk_free(), which is either called from: 1. inet_twsk_put(), protected by tw->tw_refcnt 2. sock_gen_put(), protected by the same sk->sk_refcnt So, in result, if I understand correctly, lookups should fail on ref counter check. Maybe I'm missing something, but clearing here seems not necessary? I can add rcu_assign_pointer() just in case the destruction path changes in v2 if you think it's worth it :-) Thanks, Dmitry ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction 2022-11-03 16:53 ` Dmitry Safonov @ 2022-11-03 17:04 ` Eric Dumazet 0 siblings, 0 replies; 11+ messages in thread From: Eric Dumazet @ 2022-11-03 17:04 UTC (permalink / raw) To: Dmitry Safonov Cc: linux-kernel, David Ahern, Bob Gilligan, David S. Miller, Dmitry Safonov, Francesco Ruggeri, Hideaki YOSHIFUJI, Jakub Kicinski, Paolo Abeni, Salam Noureddine, netdev On Thu, Nov 3, 2022 at 9:53 AM Dmitry Safonov <dima@arista.com> wrote: > > On 11/2/22 21:25, Eric Dumazet wrote: > > On Wed, Nov 2, 2022 at 2:14 PM Dmitry Safonov <dima@arista.com> wrote: > [..] > >> @@ -337,11 +338,13 @@ EXPORT_SYMBOL(tcp_time_wait); > >> void tcp_twsk_destructor(struct sock *sk) > >> { > >> #ifdef CONFIG_TCP_MD5SIG > >> - if (static_branch_unlikely(&tcp_md5_needed)) { > >> + if (static_branch_unlikely(&tcp_md5_needed.key)) { > >> struct tcp_timewait_sock *twsk = tcp_twsk(sk); > >> > >> - if (twsk->tw_md5_key) > >> + if (twsk->tw_md5_key) { > > > > Orthogonal to this patch, but I wonder why we do not clear > > twsk->tw_md5_key before kfree_rcu() > > > > It seems a lookup could catch the invalid pointer. > > > >> kfree_rcu(twsk->tw_md5_key, rcu); > >> + static_branch_slow_dec_deferred(&tcp_md5_needed); > >> + } > >> } > > I looked into that, it seems tcp_twsk_destructor() is called from > inet_twsk_free(), which is either called from: > 1. inet_twsk_put(), protected by tw->tw_refcnt > 2. sock_gen_put(), protected by the same sk->sk_refcnt > > So, in result, if I understand correctly, lookups should fail on ref > counter check. Maybe I'm missing something, but clearing here seems not > necessary? > > I can add rcu_assign_pointer() just in case the destruction path changes > in v2 if you think it's worth it :-) Agree, this seems fine. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-11-03 17:05 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-02 21:13 [PATCH 0/2] net/tcp: Dynamically disable TCP-MD5 static key Dmitry Safonov 2022-11-02 21:13 ` [PATCH 1/2] net/tcp: Separate tcp_md5sig_info allocation into tcp_md5sig_info_add() Dmitry Safonov 2022-11-02 21:27 ` Eric Dumazet 2022-11-02 21:13 ` [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction Dmitry Safonov 2022-11-02 21:25 ` Eric Dumazet 2022-11-02 21:40 ` Dmitry Safonov 2022-11-02 21:49 ` Eric Dumazet 2022-11-02 21:53 ` Eric Dumazet 2022-11-03 15:40 ` Dmitry Safonov 2022-11-03 16:53 ` Dmitry Safonov 2022-11-03 17:04 ` Eric Dumazet
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.