Re: [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction

[Eric Dumazet]

On Wed, Nov 2, 2022 at 2:40 PM Dmitry Safonov <dima@xxxxxxxxxx> wrote:
>
> On 11/2/22 21:25, Eric Dumazet wrote:
> > On Wed, Nov 2, 2022 at 2:14 PM Dmitry Safonov <dima@xxxxxxxxxx> 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.