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