Re: [PATCH v5 1/5] jump_label: Prevent key->enabled int overflow

On 11/23/22 09:55, Peter Zijlstra wrote:

On Tue, Nov 22, 2022 at 06:55:30PM +0000, Dmitry Safonov wrote:

quoted

+/***
+ * static_key_fast_inc_not_negative - 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.
+ */

I don't normally do kerneldoc style comments, and this is the first in
the whole file. The moment I get a docs person complaining about some
markup issue I just take the ** off.

The only reason I used kerneldoc style is that otherwise usually someone
would come and complain. I'll convert it to a regular comment.

One more thing; it might be useful to point out that unlike refcount_t
this thing does not saturate but will fail to increment on overflow.

Will add it as well.

quoted

+static bool static_key_fast_inc_not_negative(struct static_key *key)
 {
+	int v;
+
 	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.
+	 */
+	v = atomic_read(&key->enabled);
+	do {
+		if (v <= 0 || (v + 1) < 0)
+			return false;
+	} while (!likely(atomic_try_cmpxchg(&key->enabled, &v, v + 1)));
+
+	return true;
+}

( vexing how this function and the JUMP_LABEL=n static_key_slow_inc() are
  only a single character different )

Yeah, also another reason for it was that when JUMP_LABEL=y jump_label.h
doesn't include <linux/atomic.h> and <linux/bug.h> because of the
inclusion hell:
commit 1f69bf9c6137 ("jump_label: remove bug.h, atomic.h dependencies
for HAVE_JUMP_LABEL")
and I can't move JUMP_LABEL=n version of static_key_slow_inc() to
jump_label.c as it is not being built without the config set.

So, in result I was looking into macro-define for both cases, but that
adds quite some ugliness and has no type checks for just reusing 10
lines, where 1 differs...

So while strictly accurate, I dislike this name (and I see I was not
quick enough responding to your earlier suggestion :/). The whole
negative thing is an implementation detail that should not spread
outside of jump_label.c.

Since you did not like the canonical _inc_not_zero(), how about
inc_not_disabled() ?

Ok, that sounds good, I'll rename in v6.

Also, perhaps expose this function in this patch, instead of hiding that
in patch 3?

Will do.

Otherwise, things look good.

Thanks!

Thanks again for the review,
          Dmitry