[PATCH v3 0/3] net/tcp: Dynamically disable TCP-MD5 static key

[PATCH v3 0/3] net/tcp: Dynamically disable TCP-MD5 static key

[Dmitry Safonov]

Changes from v2:
- Prevent key->enabled from turning negative by overflow from
  static_key_slow_inc() or static_key_fast_inc()
  (addressing Peter Zijlstra's review)
- Added checks if static_branch_inc() and static_key_fast_int()
  were successful to TCP-MD5 code.

Changes from v1:
- Add static_key_fast_inc() helper rather than open-coded atomic_inc()
  (as suggested by Eric Dumazet)

Version 2: 
https://lore.kernel.org/all/20221103212524.865762-1-dima@arista.com/T/#u
Version 1: 
https://lore.kernel.org/all/20221102211350.625011-1-dima@arista.com/T/#u

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 (3):
  jump_label: Prevent key->enabled int overflow
  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/linux/jump_label.h | 21 ++++++++--
 include/net/tcp.h          | 10 +++--
 kernel/jump_label.c        | 54 +++++++++++++++++-------
 net/ipv4/tcp.c             |  5 +--
 net/ipv4/tcp_ipv4.c        | 86 +++++++++++++++++++++++++++++++-------
 net/ipv4/tcp_minisocks.c   | 12 ++++--
 net/ipv4/tcp_output.c      |  4 +-
 net/ipv6/tcp_ipv6.c        | 10 ++---
 8 files changed, 150 insertions(+), 52 deletions(-)


base-commit: 4bbf3422df78029f03161640dcb1e9d1ed64d1ea
-- 
2.38.1

[PATCH v3 1/3] jump_label: Prevent key->enabled int overflow

[Dmitry Safonov]

1. With CONFIG_JUMP_LABEL=n static_key_slow_inc() doesn't have any
   protection against key->enabled refcounter overflow.
2. With CONFIG_JUMP_LABEL=y static_key_slow_inc_cpuslocked()
   still may turn the refcounter negative as (v + 1) may overflow.

key->enabled is indeed a ref-counter as it's documented in multiple
places: top comment in jump_label.h, Documentation/staging/static-keys.rst,
etc.

As -1 is reserved for static key that's in process of being enabled,
functions would break with negative key->enabled refcount:
- for CONFIG_JUMP_LABEL=n negative return of static_key_count()
  breaks static_key_false(), static_key_true()
- the ref counter may become 0 from negative side by too many
  static_key_slow_inc() calls and lead to use-after-free issues.

These flaws result in that some users have to introduce an additional
mutex and prevent the reference counter from overflowing themselves,
see bpf_enable_runtime_stats() checking the counter against INT_MAX / 2.

Prevent the reference counter overflow by checking if (v + 1) > 0.
Change functions API to return whether the increment was successful.

While at here, provide static_key_fast_inc() helper that does ref
counter increment in atomic fashion (without grabbing cpus_read_lock()
on CONFIG_JUMP_LABEL=y). This is needed to add a new user for
a static_key when the caller controls the lifetime of another user.
The exact detail where it will be used: if a listen socket with TCP-MD5
key receives SYN packet that passes the verification and in result
creates a request socket - it's all done from RX softirq. At that moment
userspace can't lock the listen socket and remove that TCP-MD5 key, so
the tcp_md5_needed static branch can't get disabled. But the refcounter
of the static key needs to be adjusted to account for a new user
(the request socket).

Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jason Baron <jbaron@akamai.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 include/linux/jump_label.h | 21 ++++++++++++---
 kernel/jump_label.c        | 54 ++++++++++++++++++++++++++++----------
 2 files changed, 57 insertions(+), 18 deletions(-)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 570831ca9951..ced5b49d24b4 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -224,9 +224,10 @@ extern bool arch_jump_label_transform_queue(struct jump_entry *entry,
 					    enum jump_label_type type);
 extern void arch_jump_label_transform_apply(void);
 extern int jump_label_text_reserved(void *start, void *end);
-extern void static_key_slow_inc(struct static_key *key);
+extern bool static_key_slow_inc(struct static_key *key);
+extern bool static_key_fast_inc(struct static_key *key);
 extern void static_key_slow_dec(struct static_key *key);
-extern void static_key_slow_inc_cpuslocked(struct static_key *key);
+extern bool static_key_slow_inc_cpuslocked(struct static_key *key);
 extern void static_key_slow_dec_cpuslocked(struct static_key *key);
 extern int static_key_count(struct static_key *key);
 extern void static_key_enable(struct static_key *key);
@@ -278,11 +279,23 @@ static __always_inline bool static_key_true(struct static_key *key)
 	return false;
 }
 
-static inline void static_key_slow_inc(struct static_key *key)
+static inline bool static_key_fast_inc(struct static_key *key)
 {
+	int v, v1;
+
 	STATIC_KEY_CHECK_USE(key);
-	atomic_inc(&key->enabled);
+	/*
+	 * Prevent key->enabled getting negative to follow the same semantics
+	 * as for CONFIG_JUMP_LABEL=y, see kernel/jump_label.c comment.
+	 */
+	for (v = atomic_read(&key->enabled); v >= 0 && (v + 1) > 0; v = v1) {
+		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
+		if (likely(v1 == v))
+			return true;
+	}
+	return false;
 }
+#define static_key_slow_inc(key)	static_key_fast_inc(key)
 
 static inline void static_key_slow_dec(struct static_key *key)
 {
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 714ac4c3b556..f2c1aa351d41 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -113,11 +113,38 @@ int static_key_count(struct static_key *key)
 }
 EXPORT_SYMBOL_GPL(static_key_count);
 
-void static_key_slow_inc_cpuslocked(struct static_key *key)
+/***
+ * static_key_fast_inc - adds a user for a static key
+ * @key: static key that must be already enabled
+ *
+ * The caller must make sure that the static key can't get disabled while
+ * in this function. It doesn't patch jump labels, only adds a user to
+ * an already enabled static key.
+ *
+ * Returns true if the increment was done.
+ */
+bool static_key_fast_inc(struct static_key *key)
 {
 	int v, v1;
 
 	STATIC_KEY_CHECK_USE(key);
+	/*
+	 * Negative key->enabled has a special meaning: it sends
+	 * static_key_slow_inc() down the slow path, and it is non-zero
+	 * so it counts as "enabled" in jump_label_update().  Note that
+	 * atomic_inc_unless_negative() checks >= 0, so roll our own.
+	 */
+	for (v = atomic_read(&key->enabled); v > 0 && (v + 1) > 0; v = v1) {
+		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
+		if (likely(v1 == v))
+			return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL_GPL(static_key_fast_inc);
+
+bool static_key_slow_inc_cpuslocked(struct static_key *key)
+{
 	lockdep_assert_cpus_held();
 
 	/*
@@ -126,17 +153,9 @@ void static_key_slow_inc_cpuslocked(struct static_key *key)
 	 * jump_label_update() process.  At the same time, however,
 	 * the jump_label_update() call below wants to see
 	 * static_key_enabled(&key) for jumps to be updated properly.
-	 *
-	 * So give a special meaning to negative key->enabled: it sends
-	 * static_key_slow_inc() down the slow path, and it is non-zero
-	 * so it counts as "enabled" in jump_label_update().  Note that
-	 * atomic_inc_unless_negative() checks >= 0, so roll our own.
 	 */
-	for (v = atomic_read(&key->enabled); v > 0; v = v1) {
-		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
-		if (likely(v1 == v))
-			return;
-	}
+	if (static_key_fast_inc(key))
+		return true;
 
 	jump_label_lock();
 	if (atomic_read(&key->enabled) == 0) {
@@ -148,16 +167,23 @@ void static_key_slow_inc_cpuslocked(struct static_key *key)
 		 */
 		atomic_set_release(&key->enabled, 1);
 	} else {
-		atomic_inc(&key->enabled);
+		if (WARN_ON_ONCE(static_key_fast_inc(key))) {
+			jump_label_unlock();
+			return false;
+		}
 	}
 	jump_label_unlock();
+	return true;
 }
 
-void static_key_slow_inc(struct static_key *key)
+bool static_key_slow_inc(struct static_key *key)
 {
+	bool ret;
+
 	cpus_read_lock();
-	static_key_slow_inc_cpuslocked(key);
+	ret = static_key_slow_inc_cpuslocked(key);
 	cpus_read_unlock();
+	return ret;
 }
 EXPORT_SYMBOL_GPL(static_key_slow_inc);
 
-- 
2.38.1

[PATCH v3 2/3] net/tcp: Separate tcp_md5sig_info allocation into tcp_md5sig_info_add()

[Dmitry Safonov]

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

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

[Dmitry Safonov]

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      | 68 ++++++++++++++++++++++++++++++++--------
 net/ipv4/tcp_minisocks.c | 12 +++++--
 net/ipv4/tcp_output.c    |  4 +--
 net/ipv6/tcp_ipv6.c      | 10 +++---
 6 files changed, 78 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 54836a6b81d6..07a73c9b49da 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..7c4cadbfdc1f 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,56 @@ 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;
+
+		if (!static_branch_inc(&tcp_md5_needed.key)) {
+			struct tcp_md5sig_info *md5sig = tp->md5sig_info;
+
+			rcu_assign_pointer(tp->md5sig_info, NULL);
+			kfree_rcu(md5sig);
+			return -EUSERS;
+		}
+	}
+
+	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;
+
+		if (!static_key_fast_inc(&tcp_md5_needed.key.key)) {
+			struct tcp_md5sig_info *md5sig = tp->md5sig_info;
+
+			rcu_assign_pointer(tp->md5sig_info, NULL);
+			kfree_rcu(md5sig);
+			return -EUSERS;
+		}
+	}
+
+	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 +1376,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 +1633,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 +2325,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..118fb2d939bb 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -291,13 +291,17 @@ 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());
+					if (!static_key_fast_inc(&tcp_md5_needed.key.key)) {
+						kfree(tcptw->tw_md5_key);
+						tcptw->tw_md5_key = NULL;
+					}
 				}
 			}
 		} while (0);
@@ -337,11 +341,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

Re: [PATCH v3 1/3] jump_label: Prevent key->enabled int overflow

[Peter Zijlstra]

On Fri, Nov 11, 2022 at 09:23:18PM +0000, Dmitry Safonov wrote:
> 1. With CONFIG_JUMP_LABEL=n static_key_slow_inc() doesn't have any
>    protection against key->enabled refcounter overflow.
> 2. With CONFIG_JUMP_LABEL=y static_key_slow_inc_cpuslocked()
>    still may turn the refcounter negative as (v + 1) may overflow.
> 
> key->enabled is indeed a ref-counter as it's documented in multiple
> places: top comment in jump_label.h, Documentation/staging/static-keys.rst,
> etc.
> 
> As -1 is reserved for static key that's in process of being enabled,
> functions would break with negative key->enabled refcount:
> - for CONFIG_JUMP_LABEL=n negative return of static_key_count()
>   breaks static_key_false(), static_key_true()
> - the ref counter may become 0 from negative side by too many
>   static_key_slow_inc() calls and lead to use-after-free issues.
> 
> These flaws result in that some users have to introduce an additional
> mutex and prevent the reference counter from overflowing themselves,
> see bpf_enable_runtime_stats() checking the counter against INT_MAX / 2.

Urgh,. nothing like working around defects instead of fixing them I
suppose :/

> Prevent the reference counter overflow by checking if (v + 1) > 0.
> Change functions API to return whether the increment was successful.
> 
> While at here, provide static_key_fast_inc() helper that does ref
> counter increment in atomic fashion (without grabbing cpus_read_lock()
> on CONFIG_JUMP_LABEL=y). This is needed to add a new user for

-ENOTHERE, did you forget to Cc me on all patches?

> a static_key when the caller controls the lifetime of another user.
> The exact detail where it will be used: if a listen socket with TCP-MD5
> key receives SYN packet that passes the verification and in result
> creates a request socket - it's all done from RX softirq. At that moment
> userspace can't lock the listen socket and remove that TCP-MD5 key, so
> the tcp_md5_needed static branch can't get disabled. But the refcounter
> of the static key needs to be adjusted to account for a new user
> (the request socket).

Arguably all this should be a separate patch. Also I'm hoping the caller
does something like WARN on failure?


> -static inline void static_key_slow_inc(struct static_key *key)
> +static inline bool static_key_fast_inc(struct static_key *key)
>  {
> +	int v, v1;
> +
>  	STATIC_KEY_CHECK_USE(key);
> -	atomic_inc(&key->enabled);
> +	/*
> +	 * Prevent key->enabled getting negative to follow the same semantics
> +	 * as for CONFIG_JUMP_LABEL=y, see kernel/jump_label.c comment.
> +	 */
> +	for (v = atomic_read(&key->enabled); v >= 0 && (v + 1) > 0; v = v1) {
> +		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
> +		if (likely(v1 == v))
> +			return true;
> +	}


Please, use atomic_try_cmpxchg(), it then turns into something like:

	int v = atomic_read(&key->enabled);

	do {
		if (v < 0 || (v + 1) < 0)
			return false;
	} while (!atomic_try_cmpxchg(&key->enabled, &v, v + 1))

	return true;

> +	return false;
>  }
> +#define static_key_slow_inc(key)	static_key_fast_inc(key)
>  
>  static inline void static_key_slow_dec(struct static_key *key)
>  {
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index 714ac4c3b556..f2c1aa351d41 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -113,11 +113,38 @@ int static_key_count(struct static_key *key)
>  }
>  EXPORT_SYMBOL_GPL(static_key_count);
>  
> -void static_key_slow_inc_cpuslocked(struct static_key *key)
> +/***
> + * static_key_fast_inc - adds a user for a static key
> + * @key: static key that must be already enabled
> + *
> + * The caller must make sure that the static key can't get disabled while
> + * in this function. It doesn't patch jump labels, only adds a user to
> + * an already enabled static key.
> + *
> + * Returns true if the increment was done.
> + */
> +bool static_key_fast_inc(struct static_key *key)

Typically this primitive is called something_inc_not_zero().

>  {
>  	int v, v1;
>  
>  	STATIC_KEY_CHECK_USE(key);
> +	/*
> +	 * Negative key->enabled has a special meaning: it sends
> +	 * static_key_slow_inc() down the slow path, and it is non-zero
> +	 * so it counts as "enabled" in jump_label_update().  Note that
> +	 * atomic_inc_unless_negative() checks >= 0, so roll our own.
> +	 */
> +	for (v = atomic_read(&key->enabled); v > 0 && (v + 1) > 0; v = v1) {
> +		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
> +		if (likely(v1 == v))
> +			return true;
> +	}

Idem on atomic_try_cmpxchg().

> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(static_key_fast_inc);
> +
> +bool static_key_slow_inc_cpuslocked(struct static_key *key)
> +{
>  	lockdep_assert_cpus_held();
>  
>  	/*
> @@ -126,17 +153,9 @@ void static_key_slow_inc_cpuslocked(struct static_key *key)
>  	 * jump_label_update() process.  At the same time, however,
>  	 * the jump_label_update() call below wants to see
>  	 * static_key_enabled(&key) for jumps to be updated properly.
> -	 *
> -	 * So give a special meaning to negative key->enabled: it sends
> -	 * static_key_slow_inc() down the slow path, and it is non-zero
> -	 * so it counts as "enabled" in jump_label_update().  Note that
> -	 * atomic_inc_unless_negative() checks >= 0, so roll our own.
>  	 */
> -	for (v = atomic_read(&key->enabled); v > 0; v = v1) {
> -		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
> -		if (likely(v1 == v))
> -			return;
> -	}

This does not in fact apply, since someone already converted to try_cmpxchg.

Re: [PATCH v3 1/3] jump_label: Prevent key->enabled int overflow

[Dmitry Safonov]

Hi Peter,

Thanks again for reviewing,

On 11/12/22 10:03, Peter Zijlstra wrote:
[..]
>> Prevent the reference counter overflow by checking if (v + 1) > 0.
>> Change functions API to return whether the increment was successful.
>>
>> While at here, provide static_key_fast_inc() helper that does ref
>> counter increment in atomic fashion (without grabbing cpus_read_lock()
>> on CONFIG_JUMP_LABEL=y). This is needed to add a new user for
> 
> -ENOTHERE, did you forget to Cc me on all patches?

I'll Cc you and static_key maintainers on all patches for v4.
Probably, my practice of Cc'ing maintainers only on patches for their
sub-system + cover-letter is a bit outdated and better to Cc on the
whole patch set.

>> a static_key when the caller controls the lifetime of another user.
>> The exact detail where it will be used: if a listen socket with TCP-MD5
>> key receives SYN packet that passes the verification and in result
>> creates a request socket - it's all done from RX softirq. At that moment
>> userspace can't lock the listen socket and remove that TCP-MD5 key, so
>> the tcp_md5_needed static branch can't get disabled. But the refcounter
>> of the static key needs to be adjusted to account for a new user
>> (the request socket).
> 
> Arguably all this should be a separate patch. Also I'm hoping the caller
> does something like WARN on failure?

I thought about it, but did add an error-fallback.
I'll add net_warn_ratelimited() for v4 for such cases.

>> -static inline void static_key_slow_inc(struct static_key *key)
>> +static inline bool static_key_fast_inc(struct static_key *key)
>>  {
>> +	int v, v1;
>> +
>>  	STATIC_KEY_CHECK_USE(key);
>> -	atomic_inc(&key->enabled);
>> +	/*
>> +	 * Prevent key->enabled getting negative to follow the same semantics
>> +	 * as for CONFIG_JUMP_LABEL=y, see kernel/jump_label.c comment.
>> +	 */
>> +	for (v = atomic_read(&key->enabled); v >= 0 && (v + 1) > 0; v = v1) {
>> +		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
>> +		if (likely(v1 == v))
>> +			return true;
>> +	}
> 
> 
> Please, use atomic_try_cmpxchg(), it then turns into something like:
> 
> 	int v = atomic_read(&key->enabled);
> 
> 	do {
> 		if (v < 0 || (v + 1) < 0)
> 			return false;
> 	} while (!atomic_try_cmpxchg(&key->enabled, &v, v + 1))
> 
> 	return true;

Thanks, will do.

> 
>> +	return false;
>>  }
>> +#define static_key_slow_inc(key)	static_key_fast_inc(key)
>>  
>>  static inline void static_key_slow_dec(struct static_key *key)
>>  {
>> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
>> index 714ac4c3b556..f2c1aa351d41 100644
>> --- a/kernel/jump_label.c
>> +++ b/kernel/jump_label.c
>> @@ -113,11 +113,38 @@ int static_key_count(struct static_key *key)
>>  }
>>  EXPORT_SYMBOL_GPL(static_key_count);
>>  
>> -void static_key_slow_inc_cpuslocked(struct static_key *key)
>> +/***
>> + * static_key_fast_inc - adds a user for a static key
>> + * @key: static key that must be already enabled
>> + *
>> + * The caller must make sure that the static key can't get disabled while
>> + * in this function. It doesn't patch jump labels, only adds a user to
>> + * an already enabled static key.
>> + *
>> + * Returns true if the increment was done.
>> + */
>> +bool static_key_fast_inc(struct static_key *key)
> 
> Typically this primitive is called something_inc_not_zero().

Hmm, maybe static_key_fast_inc_not_negative()?

> 
>>  {
>>  	int v, v1;
>>  
>>  	STATIC_KEY_CHECK_USE(key);
>> +	/*
>> +	 * Negative key->enabled has a special meaning: it sends
>> +	 * static_key_slow_inc() down the slow path, and it is non-zero
>> +	 * so it counts as "enabled" in jump_label_update().  Note that
>> +	 * atomic_inc_unless_negative() checks >= 0, so roll our own.
>> +	 */
>> +	for (v = atomic_read(&key->enabled); v > 0 && (v + 1) > 0; v = v1) {
>> +		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
>> +		if (likely(v1 == v))
>> +			return true;
>> +	}
> 
> Idem on atomic_try_cmpxchg().
> 
>> +	return false;
>> +}
>> +EXPORT_SYMBOL_GPL(static_key_fast_inc);
>> +
>> +bool static_key_slow_inc_cpuslocked(struct static_key *key)
>> +{
>>  	lockdep_assert_cpus_held();
>>  
>>  	/*
>> @@ -126,17 +153,9 @@ void static_key_slow_inc_cpuslocked(struct static_key *key)
>>  	 * jump_label_update() process.  At the same time, however,
>>  	 * the jump_label_update() call below wants to see
>>  	 * static_key_enabled(&key) for jumps to be updated properly.
>> -	 *
>> -	 * So give a special meaning to negative key->enabled: it sends
>> -	 * static_key_slow_inc() down the slow path, and it is non-zero
>> -	 * so it counts as "enabled" in jump_label_update().  Note that
>> -	 * atomic_inc_unless_negative() checks >= 0, so roll our own.
>>  	 */
>> -	for (v = atomic_read(&key->enabled); v > 0; v = v1) {
>> -		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
>> -		if (likely(v1 == v))
>> -			return;
>> -	}
> 
> This does not in fact apply, since someone already converted to try_cmpxchg.

Yeah, I based it on the current master, will take a look in linux-next.

Thanks,
          Dmitry

Re: [PATCH v3 1/3] jump_label: Prevent key->enabled int overflow

[Jason Baron]

On 11/11/22 16:23, Dmitry Safonov wrote:
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index 714ac4c3b556..f2c1aa351d41 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -113,11 +113,38 @@ int static_key_count(struct static_key *key)
>  }
>  EXPORT_SYMBOL_GPL(static_key_count);
>  
> -void static_key_slow_inc_cpuslocked(struct static_key *key)
> +/***
> + * static_key_fast_inc - adds a user for a static key
> + * @key: static key that must be already enabled
> + *
> + * The caller must make sure that the static key can't get disabled while
> + * in this function. It doesn't patch jump labels, only adds a user to
> + * an already enabled static key.
> + *
> + * Returns true if the increment was done.
> + */
> +bool static_key_fast_inc(struct static_key *key)
>  {
>  	int v, v1;
>  
>  	STATIC_KEY_CHECK_USE(key);
> +	/*
> +	 * Negative key->enabled has a special meaning: it sends
> +	 * static_key_slow_inc() down the slow path, and it is non-zero
> +	 * so it counts as "enabled" in jump_label_update().  Note that
> +	 * atomic_inc_unless_negative() checks >= 0, so roll our own.
> +	 */
> +	for (v = atomic_read(&key->enabled); v > 0 && (v + 1) > 0; v = v1) {
> +		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
> +		if (likely(v1 == v))
> +			return true;
> +	}
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(static_key_fast_inc);
> +
> +bool static_key_slow_inc_cpuslocked(struct static_key *key)
> +{
>  	lockdep_assert_cpus_held();
>  
>  	/*
> @@ -126,17 +153,9 @@ void static_key_slow_inc_cpuslocked(struct static_key *key)
>  	 * jump_label_update() process.  At the same time, however,
>  	 * the jump_label_update() call below wants to see
>  	 * static_key_enabled(&key) for jumps to be updated properly.
> -	 *
> -	 * So give a special meaning to negative key->enabled: it sends
> -	 * static_key_slow_inc() down the slow path, and it is non-zero
> -	 * so it counts as "enabled" in jump_label_update().  Note that
> -	 * atomic_inc_unless_negative() checks >= 0, so roll our own.
>  	 */
> -	for (v = atomic_read(&key->enabled); v > 0; v = v1) {
> -		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
> -		if (likely(v1 == v))
> -			return;
> -	}
> +	if (static_key_fast_inc(key))
> +		return true;
>  
>  	jump_label_lock();
>  	if (atomic_read(&key->enabled) == 0) {
> @@ -148,16 +167,23 @@ void static_key_slow_inc_cpuslocked(struct static_key *key)
>  		 */
>  		atomic_set_release(&key->enabled, 1);
>  	} else {
> -		atomic_inc(&key->enabled);
> +		if (WARN_ON_ONCE(static_key_fast_inc(key))) {

Shouldn't that be negated to catch the overflow:

if (WARN_ON_ONCE(!static_key_fast_inc(key)))



> +			jump_label_unlock();
> +			return false;
> +		}
>  	}
>  	jump_label_unlock();
> +	return true;
>  }
>  
> -void static_key_slow_inc(struct static_key *key)
> +bool static_key_slow_inc(struct static_key *key)
>  {
> +	bool ret;
> +
>  	cpus_read_lock();
> -	static_key_slow_inc_cpuslocked(key);
> +	ret = static_key_slow_inc_cpuslocked(key);
>  	cpus_read_unlock();
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(static_key_slow_inc);
>  

Re: [PATCH v3 1/3] jump_label: Prevent key->enabled int overflow

[Dmitry Safonov]

On 11/14/22 16:24, Jason Baron wrote:
> 
[..]
>> @@ -148,16 +167,23 @@ void static_key_slow_inc_cpuslocked(struct static_key *key)
>>  		 */
>>  		atomic_set_release(&key->enabled, 1);
>>  	} else {
>> -		atomic_inc(&key->enabled);
>> +		if (WARN_ON_ONCE(static_key_fast_inc(key))) {
> 
> Shouldn't that be negated to catch the overflow:
> 
> if (WARN_ON_ONCE(!static_key_fast_inc(key)))

Oh, that's just embarrassing!
I wonder how did I miss it during tests..

Thanks for spotting this, will fix in v4,
            Dmitry