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 [off-list ref] wrote:

On 11/2/22 21:25, Eric Dumazet wrote:

quoted

On Wed, Nov 2, 2022 at 2:14 PM Dmitry Safonov [off-list ref] wrote:

[..]

quoted

quoted

+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.