Messages in this thread | | | From | Eric Dumazet <> | Date | Thu, 3 Nov 2022 10:04:19 -0700 | Subject | Re: [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction |
| |
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.
| |