Introduce Simple atomic counters

Introduce Simple atomic counters.

There are a number of atomic_t usages in the kernel where atomic_t api is used strictly for counting and not for managing object lifetime. In some cases, atomic_t might not even be needed.

The purpose of these counters is to clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows tools that scan for underflow and overflow on atomic_t usages to detect overflow and underflows to scan just the cases that are prone to errors.

Simple atomic counters api provides interfaces for simple atomic counters that just count, and don't guard resource lifetimes. The interfaces are built on top of atomic_t api, providing a smaller subset of atomic_t interfaces necessary to support simple counters.

Counter wraps around to INT_MIN when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states. Overflowing to INT_MIN is consistent with the atomic_t api, which it is built on top of.

Using counter_atomic* to guard lifetimes could lead to use-after free when it overflows and undefined behavior when used to manage state changes and device usage/open states.

Reviewed-by Greg Kroah-Hartman <[email protected]>

Signed-off-by Shuah Khan <[email protected]>

Introduce Simple atomic counters.

There are a number of atomic_t usages in the kernel where atomic_t api is used strictly for counting and not for managing object lifetime. In some cases, atomic_t might not even be needed.

The purpose of these counters is to clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows tools that scan for underflow and overflow on atomic_t usages to detect overflow and underflows to scan just the cases that are prone to errors.

Introduce Simple atomic counters.

There are a number of atomic_t usages in the kernel where atomic_t api is used strictly for counting and not for managing object lifetime. In some cases, atomic_t might not even be needed.

The purpose of these counters is to clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows tools that scan for underflow and overflow on atomic_t usages to detect overflow and underflows to scan just the cases that are prone to errors.

Simple atomic counters api provides interfaces for simple atomic counters that just count, and don't guard resource lifetimes. The interfaces are built on top of atomic_t api, providing a smaller subset of atomic_t interfaces necessary to support simple counters.

Counter wraps around to INT_MIN when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states. Overflowing to INT_MIN is consistent with the atomic_t api, which it is built on top of.

Using counter_atomic* to guard lifetimes could lead to use-after free when it overflows and undefined behavior when used to manage state changes and device usage/open states.

Reviewed-by Greg Kroah-Hartman <[email protected]>

Signed-off-by Shuah Khan <[email protected]>

---
 Documentation/core-api/counters.rst | 109 +++++++++++++++++
 MAINTAINERS                         |   7 ++
 include/linux/counters.h            | 176 ++++++++++++++++++++++++++++
 lib/Kconfig                         |   9 ++
 lib/Makefile                        |   1 +
 lib/test_counters.c                 | 162 +++++++++++++++++++++++++
 6 files changed, 464 insertions(+)
 create mode 100644 Documentation/core-api/counters.rst
 create mode 100644 include/linux/counters.h
 create mode 100644 lib/test_counters.c

Documentation/core-api/counters.rst new file +109 -0

new file mode 100644
index 000000000000..642d907f4d3a
@@ -0,0 +1,109 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+======================
+Simple atomic counters
+======================
+
+:Author: Shuah Khan
+:Copyright: |copy| 2020, The Linux Foundation
+:Copyright: |copy| 2020, Shuah Khan <[email protected]>
+
+There are a number of atomic_t usages in the kernel where atomic_t api
+is used strictly for counting and not for managing object lifetime. In
+some cases, atomic_t might not even be needed.
+
+The purpose of these counters is to clearly differentiate atomic_t counters
+from atomic_t usages that guard object lifetimes, hence prone to overflow
+and underflow errors. It allows tools that scan for underflow and overflow
+on atomic_t usages to detect overflow and underflows to scan just the cases
+that are prone to errors.
+
+Simple atomic counters api provides interfaces for simple atomic counters
+that just count, and don't guard resource lifetimes. The interfaces are
+built on top of atomic_t api, providing a smaller subset of atomic_t
+interfaces necessary to support simple counters.
+
+Counter wraps around to INT_MIN when it overflows and should not be used
+to guard resource lifetimes, device usage and open counts that control
+state changes, and pm states. Overflowing to INT_MIN is consistent with
+the atomic_t api, which it is built on top of.
+
+Using counter_atomic32_* to guard lifetimes could lead to use-after free
+when it overflows and undefined behavior when used to manage state
+changes and device usage/open states.
+
+Use refcount_t interfaces for guarding resources.
+
+.. warning::
+        Counter wraps around to INT_MIN when it overflows.
+        Should not be used to guard resource lifetimes.
+        Should not be used to manage device state and pm state.
+
+Test Counters Module and selftest
+---------------------------------
+
+Please see :ref:`lib/test_counters.c <Test Counters Module>` for how to
+use these interfaces and also test them.
+
+Selftest for testing:
+:ref:`testing/selftests/lib/test_counters.sh <selftest for counters>`
+
+Atomic counter interfaces
+=========================
+
+counter_atomic32 and counter_atomic64 types use atomic_t and atomic64_t
+underneath to leverage atomic_t api,  providing a small subset of atomic_t
+interfaces necessary to support simple counters. ::
+
+        struct counter_atomic32 { atomic_t cnt; };
+        struct counter_atomic64 { atomic64_t cnt; };
+
+Please see :ref:`Documentation/core-api/atomic_ops.rst <atomic_ops>` for
+information on the Semantics and Behavior of Atomic operations.
+
+.. warning::
+        It is important to keep the ops to a very small subset to ensure
+        that the Counter API will never be used for guarding resource
+        lifetimes and state management.
+
+        inc_return() is added to support current atomic_inc_return()
+        usages and avoid forcing the use of _inc() followed by _read().
+
+Initializers
+------------
+
+Interfaces for initializing counters are write operations which in turn
+invoke their ``ATOMIC_INIT() and atomic_set()`` counterparts ::
+
+        #define COUNTER_ATOMIC_INIT(i)    { .cnt = ATOMIC_INIT(i) }
+        counter_atomic32_set() --> atomic_set()
+
+        static struct counter_atomic32 acnt = COUNTER_ATOMIC_INIT(0);
+        counter_atomic32_set(0);
+
+        static struct counter_atomic64 acnt = COUNTER_ATOMIC_INIT(0);
+        counter_atomic64_set(0);
+
+Increment interface
+-------------------
+
+Increments counter and doesn't return the new counter value. ::
+
+        counter_atomic32_inc() --> atomic_inc()
+        counter_atomic64_inc() --> atomic64_inc()
+
+Increment and return new counter value interface
+------------------------------------------------
+
+Increments counter and returns the new counter value. ::
+
+        counter_atomic32_inc_return() --> atomic_inc_return()
+        counter_atomic64_inc_return() --> atomic64_inc_return()
+
+Decrement interface
+-------------------
+
+Decrements counter and doesn't return the new counter value. ::
+
+        counter_atomic32_dec() --> atomic_dec()
+        counter_atomic64_dec() --> atomic64_dec()

MAINTAINERS +7 -0

index 33b27e62ce19..4e82d0ffcab0 100644
@@ -15839,6 +15839,13 @@ S:	Maintained
 F:	Documentation/fb/sm712fb.rst
 F:	drivers/video/fbdev/sm712*
 
+SIMPLE ATOMIC and NON-ATOMIC COUNTERS
+M:	Shuah Khan <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	include/linux/counters.h
+F:	lib/test_counters.c
+
 SIMPLE FIRMWARE INTERFACE (SFI)
 S:	Obsolete
 W:	http://simplefirmware.org/

include/linux/counters.h new file +176 -0

new file mode 100644
index 000000000000..bb96dd544553
@@ -0,0 +1,176 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * counters.h - Interface for simple atomic counters that just count.
+ *
+ * Copyright (c) 2020 Shuah Khan <[email protected]>
+ * Copyright (c) 2020 The Linux Foundation
+ *
+ * Counter wraps around to INT_MIN when it overflows and should not be
+ * used to guard resource lifetimes, device usage and open counts that
+ * control state changes, and pm states. Using counter_atomic to guard
+ * lifetimes could lead to use-after free when it overflows and undefined
+ * behavior when used to manage state changes and device usage/open states.
+ *
+ * Use refcount_t interfaces for guarding resources.
+ *
+ * The interface provides:
+ * atomic32 & atomic64 functions:
+ *	increment and no return
+ *	increment and return value
+ *	decrement and no return
+ *	read
+ *	set
+ *
+ * counter_atomic32 functions leverage/use atomic_t interfaces.
+ * counter_atomic64 functions leverage/use atomic64_t interfaces.
+ * The counter wraps around to INT_MIN when it overflows.
+ * These interfaces should not be used to guard resource lifetimes.
+ *
+ * Reference and API guide:
+ *	Documentation/core-api/counters.rst for more information.
+ *
+ */
+
+#ifndef __LINUX_COUNTERS_H
+#define __LINUX_COUNTERS_H
+
+#include <linux/atomic.h>
+
+/**
+ * struct counter_atomic32 - Simple atomic counter
+ * @cnt: int
+ *
+ * The counter wraps around to INT_MIN, when it overflows. Should not
+ * be used to guard object lifetimes.
+ **/
+struct counter_atomic32 {
+	atomic_t cnt;
+};
+
+#define COUNTER_ATOMIC_INIT(i)		{ .cnt = ATOMIC_INIT(i) }
+
+/*
+ * counter_atomic32_inc() - increment counter value
+ * @cntr: struct counter_atomic32 pointer
+ *
+ */
+static inline void counter_atomic32_inc(struct counter_atomic32 *cntr)
+{
+	atomic_inc(&cntr->cnt);
+}
+
+/*
+ * counter_atomic32_inc_return() - increment counter value and return it
+ * @cntr: struct counter_atomic32 pointer
+ *
+ * Return: returns the new counter value after incrementing it
+ */
+static inline int counter_atomic32_inc_return(struct counter_atomic32 *cntr)
+{
+	return atomic_inc_return(&cntr->cnt);
+}
+
+/*
+ * counter_atomic32_dec() - decrement counter value
+ * @cntr: struct counter_atomic32 pointer
+ *
+ */
+static inline void counter_atomic32_dec(struct counter_atomic32 *cntr)
+{
+	atomic_dec(&cntr->cnt);
+}
+
+/*
+ * counter_atomic32_read() - read counter value
+ * @cntr: struct counter_atomic32 pointer
+ *
+ * Return: return the counter value
+ */
+static inline int counter_atomic32_read(const struct counter_atomic32 *cntr)
+{
+	return atomic_read(&cntr->cnt);
+}
+
+/*
+ * counter_atomic32_set() - set counter value
+ * @cntr: struct counter_atomic32 pointer
+ * @val:  new counter value to set
+ *
+ */
+static inline void
+counter_atomic32_set(struct counter_atomic32 *cntr, int val)
+{
+	atomic_set(&cntr->cnt, val);
+}
+
+#ifdef CONFIG_64BIT
+/*
+ * struct counter_atomic64 - Simple atomic counter
+ * @cnt: atomic64_t
+ *
+ * The counter wraps around to INT_MIN, when it overflows. Should not
+ * be used to guard object lifetimes.
+ */
+struct counter_atomic64 {
+	atomic64_t cnt;
+};
+
+/*
+ * counter_atomic64_inc() - increment counter value
+ * @cntr: struct counter_atomic64 pointer
+ *
+ */
+static inline void counter_atomic64_inc(struct counter_atomic64 *cntr)
+{
+	atomic64_inc(&cntr->cnt);
+}
+
+/*
+ * counter_atomic64_inc_return() - increment counter value and return it
+ * @cntr: struct counter_atomic64 pointer
+ *
+ * Return: return the new counter value after incrementing it
+ */
+static inline s64
+counter_atomic64_inc_return(struct counter_atomic64 *cntr)
+{
+	return atomic64_inc_return(&cntr->cnt);
+}
+
+/*
+ * counter_atomic64_dec() - decrement counter value
+ * @cntr: struct counter_atomic64 pointer
+ *
+ */
+static inline void counter_atomic64_dec(
+				struct counter_atomic64 *cntr)
+{
+	atomic64_dec(&cntr->cnt);
+}
+
+/*
+ * counter_atomic64_read() - read counter value
+ * @cntr: struct counter_atomic64 pointer
+ *
+ * Return: return the counter value
+ */
+static inline s64
+counter_atomic64_read(const struct counter_atomic64 *cntr)
+{
+	return atomic64_read(&cntr->cnt);
+}
+
+/*
+ * counter_atomic64_set() - set counter value
+ * @cntr: struct counter_atomic64 pointer
+ * &val:  new counter value to set
+ *
+ */
+static inline void
+counter_atomic64_set(struct counter_atomic64 *cntr, s64 val)
+{
+	atomic64_set(&cntr->cnt, val);
+}
+
+#endif /* CONFIG_64BIT */
+#endif /* __LINUX_COUNTERS_H */

lib/Kconfig +9 -0

index b4b98a03ff98..e05e3d0a6d9c 100644
@@ -658,6 +658,15 @@ config OBJAGG
 config STRING_SELFTEST
 	tristate "Test string functions"
 
+config TEST_COUNTERS
+	tristate "Test Simple Atomic counter functions"
+	help
+	   A test module for Simple Atomic counter functions.
+	   A corresponding selftest can be used to test the
+	   counter functions.
+
+	   Select this if you would like to test counters.
+
 endmenu
 
 config GENERIC_IOREMAP

lib/Makefile +1 -0

index a4a4c6864f51..95b357bb5f3c 100644
@@ -99,6 +99,7 @@ obj-$(CONFIG_TEST_BLACKHOLE_DEV) += test_blackhole_dev.o
 obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o
 obj-$(CONFIG_TEST_LOCKUP) += test_lockup.o
 obj-$(CONFIG_TEST_HMM) += test_hmm.o
+obj-$(CONFIG_TEST_COUNTERS) += test_counters.o
 
 #
 # CFLAGS for compiling floating point code inside the kernel. x86/Makefile turns

lib/test_counters.c new file +152 -0

new file mode 100644
index 000000000000..7a7251273ba7
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * test_counters.c - Kernel module for testing Counters
+ *
+ * Copyright (c) 2020 Shuah Khan <[email protected]>
+ * Copyright (c) 2020 The Linux Foundation
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/counters.h>
+
+static inline void
+test_counter_result_print32(char *msg, int start, int end, int expected)
+{
+	pr_info("%s: %d to %d - %s\n",
+		msg, start, end,
+		((expected == end) ? "PASS" : "FAIL"));
+}
+
+
+static void test_counter_atomic32(void)
+{
+	static struct counter_atomic32 acnt = COUNTER_ATOMIC_INIT(0);
+	int start_val = counter_atomic32_read(&acnt);
+	int end_val;
+
+	counter_atomic32_inc(&acnt);
+	end_val = counter_atomic32_read(&acnt);
+	test_counter_result_print32("Test read and increment",
+				    start_val, end_val, start_val+1);
+
+	start_val = counter_atomic32_read(&acnt);
+	end_val = counter_atomic32_inc_return(&acnt);
+	test_counter_result_print32("Test read increment and return",
+				    start_val, end_val, start_val+1);
+
+	start_val = counter_atomic32_read(&acnt);
+	counter_atomic32_dec(&acnt);
+	end_val = counter_atomic32_read(&acnt);
+	test_counter_result_print32("Test read and decrement",
+				    start_val, end_val, start_val-1);
+
+	start_val = counter_atomic32_read(&acnt);
+	counter_atomic32_set(&acnt, INT_MAX);
+	end_val = counter_atomic32_read(&acnt);
+	test_counter_result_print32("Test set", start_val, end_val, INT_MAX);
+}
+
+static void test_counter_atomic32_overflow(void)
+{
+	static struct counter_atomic32 ucnt = COUNTER_ATOMIC_INIT(0);
+	static struct counter_atomic32 ocnt = COUNTER_ATOMIC_INIT(INT_MAX);
+	int start_val;
+	int end_val;
+
+	start_val = counter_atomic32_read(&ucnt);
+	counter_atomic32_dec(&ucnt);
+	end_val = counter_atomic32_read(&ucnt);
+	test_counter_result_print32("Test underflow (int)",
+				    start_val, end_val, start_val-1);
+	test_counter_result_print32("Test underflow (-1)",
+				    start_val, end_val, -1);
+
+	start_val = counter_atomic32_read(&ocnt);
+	end_val = counter_atomic32_inc_return(&ocnt);
+	test_counter_result_print32("Test overflow (int)",
+				    start_val, end_val, start_val+1);
+	test_counter_result_print32("Test overflow (INT_MIN)",
+				    start_val, end_val, INT_MIN);
+}
+
+#ifdef CONFIG_64BIT
+
+static inline void
+test_counter_result_print64(char *msg, s64 start, s64 end, s64 expected)
+{
+	pr_info("%s: %lld to %lld - %s\n",
+		msg, start, end,
+		((expected == end) ? "PASS" : "FAIL"));
+}
+
+static void test_counter_atomic64(void)
+{
+	static struct counter_atomic64 acnt = COUNTER_ATOMIC_INIT(0);
+	s64 start_val = counter_atomic64_read(&acnt);
+	s64 end_val;
+
+	counter_atomic64_inc(&acnt);
+	end_val = counter_atomic64_read(&acnt);
+	test_counter_result_print64("Test read and increment",
+				    start_val, end_val, start_val+1);
+
+	start_val = counter_atomic64_read(&acnt);
+	end_val = counter_atomic64_inc_return(&acnt);
+	test_counter_result_print64("Test read increment and return",
+				    start_val, end_val, start_val+1);
+
+	start_val = counter_atomic64_read(&acnt);
+	counter_atomic64_dec(&acnt);
+	end_val = counter_atomic64_read(&acnt);
+	test_counter_result_print64("Test read and decrement",
+				    start_val, end_val, start_val-1);
+
+	start_val = counter_atomic64_read(&acnt);
+	counter_atomic64_set(&acnt, INT_MAX);
+	end_val = counter_atomic64_read(&acnt);
+	test_counter_result_print64("Test set", start_val, end_val, INT_MAX);
+}
+
+static void test_counter_atomic64_overflow(void)
+{
+	static struct counter_atomic64 ucnt = COUNTER_ATOMIC_INIT(0);
+	static struct counter_atomic64 ocnt = COUNTER_ATOMIC_INIT(INT_MAX);
+	s64 start_val;
+	s64 end_val;
+
+	start_val = counter_atomic64_read(&ucnt);
+	counter_atomic64_dec(&ucnt);
+	end_val = counter_atomic64_read(&ucnt);
+	test_counter_result_print64("Test underflow",
+				    start_val, end_val, start_val-1);
+
+	start_val = counter_atomic64_read(&ocnt);
+	end_val = counter_atomic64_inc_return(&ocnt);
+	test_counter_result_print64("Test overflow",
+				    start_val, end_val, start_val+1);
+}
+
+#endif /* CONFIG_64BIT */
+
+static int __init test_counters_init(void)
+{
+	pr_info("Start counter_atomic32_*() interfaces test\n");
+	test_counter_atomic32();
+	test_counter_atomic32_overflow();
+	pr_info("End counter_atomic32_*() interfaces test\n\n");
+
+#ifdef CONFIG_64BIT
+	pr_info("Start counter_atomic64_*() interfaces test\n");
+	test_counter_atomic64();
+	test_counter_atomic64_overflow();
+	pr_info("End counter_atomic64_*() interfaces test\n\n");
+
+#endif /* CONFIG_64BIT */
+
+	return 0;
+}
+
+module_init(test_counters_init);

Em Fri, 9 Oct 2020 09:55:56 -0600
Shuah Khan <[email protected]> escreveu:

Quoted 22 lines

Introduce Simple atomic counters.

There are a number of atomic_t usages in the kernel where atomic_t api is used strictly for counting and not for managing object lifetime. In some cases, atomic_t might not even be needed.

The purpose of these counters is to clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows tools that scan for underflow and overflow on atomic_t usages to detect overflow and underflows to scan just the cases that are prone to errors.

Simple atomic counters api provides interfaces for simple atomic counters that just count, and don't guard resource lifetimes. The interfaces are built on top of atomic_t api, providing a smaller subset of atomic_t interfaces necessary to support simple counters.

Counter wraps around to INT_MIN when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states. Overflowing to INT_MIN is consistent with the atomic_t api, which it is built on top of.

Quoted 6 lines

Using counter_atomic* to guard lifetimes could lead to use-after free when it overflows and undefined behavior when used to manage state changes and device usage/open states.

Reviewed-by Greg Kroah-Hartman <[email protected]>

Signed-off-by Shuah Khan <[email protected]>

Did you try building this with htmldocs? It produces 3 new warnings due to wrong usage of the "ref" tag:

.../Documentation/core-api/counters.rst:46: WARNING: undefined label: test counters module (if the link has no caption the label must precede a section header) .../Documentation/core-api/counters.rst:49: WARNING: undefined label: selftest for counters (if the link has no caption the label must precede a section header) .../Documentation/core-api/counters.rst:62: WARNING: undefined label: atomic_ops (if the link has no caption the label must precede a section header)

(plus another one that I'll be sending a fixup patch anytime soon)

Are those referring to some documents that don't exist yet uptream? Or are you trying to force Sphinx to generate a cross-reference for a C file at the tree? If it is the latter, then this won't work, as it will only generate cross-references for files that are placed inside the documentation output dir (Documentation/output, by default).

Add a new selftest for testing counter_atomic* Counters API. This test load test_counters test modules and unloads.

The test module runs tests and prints results in dmesg.

There are a number of atomic_t usages in the kernel where atomic_t api is used strictly for counting and not for managing object lifetime. In some cases, atomic_t might not even be needed.

The purpose of these counters is to clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows tools that scan for underflow and overflow on atomic_t usages to detect overflow and underflows to scan just the cases that are prone to errors.

Simple atomic counters api provides interfaces for simple atomic counters that just count, and don't guard resource lifetimes. The interfaces are built on top of atomic_t api, providing a smaller subset of atomic_t interfaces necessary to support simple counters.

Counter wraps around to INT_MIN when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states. Overflowing to INT_MIN is consistent with the atomic_t api, which it is built on top of.

Using counter_atomic* to guard lifetimes could lead to use-after free when it overflows and undefined behavior when used to manage state changes and device usage/open states.

Signed-off-by Shuah Khan <[email protected]>

counter_atomic* is introduced to be used when a variable is used as a simple counter and doesn't guard object lifetimes. This clearly differentiates atomic_t usages that guard object lifetimes.

counter_atomic* variables wrap around to INT_MIN when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.

devcd_count is used to track dev_coredumpm device count and used in device name string. It doesn't guard object lifetimes, device usage counts, device open counts, and pm states. There is very little chance of this counter overflowing. Convert it to use counter_atomic32.

This conversion doesn't change the overflow wrap around behavior.

Reviewed-by Greg Kroah-Hartman <[email protected]>

Signed-off-by Shuah Khan <[email protected]>

counter_atomic* is introduced to be used when a variable is used as a simple counter and doesn't guard object lifetimes. This clearly differentiates atomic_t usages that guard object lifetimes.

counter_atomic* variables wrap around to INT_MIN when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.

devcd_count is used to track dev_coredumpm device count and used in device name string. It doesn't guard object lifetimes, device usage counts, device open counts, and pm states. There is very little chance of this counter overflowing. Convert it to use counter_atomic32.

This conversion doesn't change the overflow wrap around behavior.

Reviewed-by Greg Kroah-Hartman <[email protected]>

Signed-off-by Shuah Khan <[email protected]>

---
 drivers/base/devcoredump.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/base/devcoredump.c +2 -1

index e42d0b514384..59bc48ee44af 100644
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/fs.h>
 #include <linux/workqueue.h>
+#include <linux/counters.h>
 
 static struct class devcd_class;
 

@@ -255,7 +256,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
 				   void *data, size_t datalen),
 		   void (*free)(void *data))
 {
-	static atomic_t devcd_count = ATOMIC_INIT(0);
+	static struct counter_atomic32 devcd_count = COUNTER_ATOMIC_INIT(0);
 	struct devcd_entry *devcd;
 	struct device *existing;
 

Note: Would like to get this into Linux 5.10-rc1 so we can continue updating drivers that can be updated to use this API. If this all looks good, Kees, would you like to take this through your tree or would you like to take this through mine.

Quoted 4 lines · Shuah Khan

Note: Would like to get this into Linux 5.10-rc1 so we can continue updating drivers that can be updated to use this API. If this all looks good, Kees, would you like to take this through your tree or would you like to take this through mine.

I'd mentioned this in the v2, but yes, please take via your trees. :)

I'm glad to see this landing!

This patch series is a result of discussion at the refcount_t BOF the Linux Plumbers Conference. In this discussion, we identified a need for looking closely and investigating atomic_t usages in the kernel when it is used strictly as a counter without it controlling object lifetimes and state changes.

There are a number of atomic_t usages in the kernel where atomic_t api is used strictly for counting and not for managing object lifetime. In some cases, atomic_t might not even be needed.

The purpose of these counters is to clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows tools that scan for underflow and overflow on atomic_t usages to detect overflow and underflows to scan just the cases that are prone to errors.

Simple atomic counters api provides interfaces for simple atomic counters that just count, and don't guard resource lifetimes. The interfaces are built on top of atomic_t api, providing a smaller subset of atomic_t interfaces necessary to support simple counters.

Counter wraps around to INT_MIN when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states. Overflowing to INT_MIN is consistent with the atomic_t api, which it is built on top of.

Using counter_atomic* to guard lifetimes could lead to use-after free when it overflows and undefined behavior when used to manage state changes and device usage/open states.

This patch series introduces Simple atomic counters. Counter atomic ops leverage atomic_t and provide a sub-set of atomic_t ops.

Quoted 4 lines · Shuah Khan

Simple atomic counters api provides interfaces for simple atomic counters that just count, and don't guard resource lifetimes. The interfaces are built on top of atomic_t api, providing a smaller subset of atomic_t interfaces necessary to support simple counters.

To what actual purpose?!? AFACIT its pointless wrappery, it gets us nothing.

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines · Shuah Khan

Simple atomic counters api provides interfaces for simple atomic counters that just count, and don't guard resource lifetimes. The interfaces are built on top of atomic_t api, providing a smaller subset of atomic_t interfaces necessary to support simple counters.

To what actual purpose?!? AFACIT its pointless wrappery, it gets us nothing.

It's not pointless. There is value is separating types for behavioral constraint to avoid flaws. atomic_t provides a native operation. We gained refcount_t for the "must not wrap" type, and this gets us the other side of that behavioral type, which is "wrapping is expected". Separating the atomic_t uses allows for a clearer path to being able to reason about code flow, whether it be a human or a static analyzer.

The counter wrappers add nothing to the image size, and only serve to confine the API to one that cannot be used for lifetime management.

Once conversions are done, we have a clean line between refcounting and statistical atomics, which means we have a much lower chance of introducing new flaws (and maybe we'll fix flaws during the conversion, which we've certainly seen before when doing this stricter type/language changes).

I don't see why this is an objectionable goal.

Quoted 10 lines · Kees Cook

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines · Shuah Khan

Simple atomic counters api provides interfaces for simple atomic counters that just count, and don't guard resource lifetimes. The interfaces are built on top of atomic_t api, providing a smaller subset of atomic_t interfaces necessary to support simple counters.

To what actual purpose?!? AFACIT its pointless wrappery, it gets us nothing.

Quoted 6 lines

It's not pointless. There is value is separating types for behavioral constraint to avoid flaws. atomic_t provides a native operation. We gained refcount_t for the "must not wrap" type, and this gets us the other side of that behavioral type, which is "wrapping is expected". Separating the atomic_t uses allows for a clearer path to being able to reason about code flow, whether it be a human or a static analyzer.

refcount_t got us actual rutime exceptions that atomic_t doesn't. This propsal gets us nothing.

atomic_t is very much expected to wrap.

Quoted 2 lines

The counter wrappers add nothing to the image size, and only serve to confine the API to one that cannot be used for lifetime management.

It doesn't add anything period. It doesn't get us new behaviour, it splits a 'can wrap' use-case from a 'can wrap' type. That's sodding pointless.

Worse, it mixes 2 unrelated cases into one type, which just makes a mockery of things (all the inc_return users are not statistics, some might even mis-behave if they wrap).

They don't add any new behavior, As Kees mentioned they do give us a way to clearly differentiate atomic usages that can wrap.

Quoted 4 lines

Worse, it mixes 2 unrelated cases into one type, which just makes a mockery of things (all the inc_return users are not statistics, some might even mis-behave if they wrap).

You are right that all inc_return usages aren't statistics. There are 3 distinct usages:

1. Stats
2. Cases where wrapping is fine
3. Cases where wrapping could be a problem. In which case, this API shouldn't be used.

There is no need to keep inc_return in this API as such. I included it so it can be used for above cases 1 and 2, so the users don't have to call inc() followed by read(). It can be left out of the API.

The atomic_t usages in the kernel fall into the following categories:

1. Stats (tolerance for accuracy determines whether they need to be atomic or not). RFC version included non-atomic API for cases when lossiness is acceptable. All these cases use/need just init and inc. There are two variations in this case:

a. No checks for wrapping. Use signed value. b. No checks for wrapping, but return unsigned.

2. Reference counters that release resource and rapping could result in use-after-free type problems. There are two variations in this case:

a. Increments and decrements aren't bounded.
b. Increments and decrements are bounded.

Currently tools that flag unsafe atomic_t usages that are candidates for refcount_t conversions don't make a distinction between the two.

The second case, since increments and decrements are bounded, it is safe to continue to use it. At the moment there is no good way to tell them apart other than looking at each of these cases.

3. Reference counters that manage/control states. Wrapping is a problem in this case, as it could lead to undefined behavior. These cases don't use test and free, use inc/dec. At the moment there is no good way to tell them apart other than looking at each of these cases. This is addressed by REFCOUNT_SATURATED case.

Quoted 2 lines · Shuah Khan

They don't add any new behavior, As Kees mentioned they do give us a way to clearly differentiate atomic usages that can wrap.

No it doesn't! atomic_t can wrap, this thing can wrap, no distinction.

All it does is fragment the API and sow confusion. FOR NO BENEFIT.

Quoted 3 lines

There is no need to keep inc_return in this API as such. I included it so it can be used for above cases 1 and 2, so the users don't have to call inc() followed by read(). It can be left out of the API.

Wrong! The atomic usage in mutex doesn't fall in any of those categories.

The only thing you're all saying that makes sense is that unintentional wrapping can have bad consequences, the rest is pure confusion.

Focus on the non-wrapping cases, _everything_ else is not going anywhere.

So audit the kernel, find the cases that should not wrap, categorize and create APIs for them that trap the wrapping. But don't go around confusing things that don't need confusion.

Quoted 8 lines · Peter Zijlstra

Quoted 2 lines · Shuah Khan

They don't add any new behavior, As Kees mentioned they do give us a way to clearly differentiate atomic usages that can wrap.

No it doesn't! atomic_t can wrap, this thing can wrap, no distinction.

All it does is fragment the API and sow confusion. FOR NO BENEFIT.

I really don't see it this way. It's a distinct subset of the atomic_t API. The trouble that has existed here has been with an atomic_t being originally used NOT for lifetime management, that mutates into something like that because of the available API, but doing so without realizing it. atomic_t gets used for all kinds of algorithms, and the "counter" type is way too easily accidentally transformed into a "lifetime tracker" and we get bugs.

If we have a distinct type for wrapping-counters that limits the API, then it is much harder for folks to shoot themselves in the foot. I don't see why this is so bad: we end up with safer usage, more easily auditable code behavior ("how was this atomic_t instance _intended_ to be used?"), and no change in binary size.

Quoted 3 lines

Quoted 3 lines

There is no need to keep inc_return in this API as such. I included it so it can be used for above cases 1 and 2, so the users don't have to call inc() followed by read(). It can be left out of the API.

I go back and forth on this, but after looking at these instances, it makes sense to have inc_return(), for where counters are actually "serial numbers". An argument could be made[1], however, that such uses should not end up in the position of _reusing_ earlier identifiers, which means it's actually can't wrap. (And some cases just need u64 to make this happen[2] -- and in that specific case, don't even need to be atomic_t).

[1] https://lore.kernel.org/lkml/202010071334.8298F3FA7@keescook/
[2] https://git.kernel.org/linus/d1e7fd6462ca9fc76650fbe6ca800e35b24267da

Quoted 2 lines

Wrong! The atomic usage in mutex doesn't fall in any of those categories.

But the atomic usage in mutex is *IN* mutex -- it's a separate data type, etc. We don't build mutexes manually, so why build counters manually?

Quoted 5 lines

The only thing you're all saying that makes sense is that unintentional wrapping can have bad consequences, the rest is pure confusion.

Focus on the non-wrapping cases, _everything_ else is not going anywhere.

I view this as a way to do so: this subset of wrapping cases is being identified and removed from the pool of all the atomic_t cases so that they will have been classified, and we can continue to narrow down all the atomic_t uses to find any potentially mis-used non-wrapping cases.

The other option is adding some kind of attribute to the declarations (which gets us the annotation) but doesn't provide a limit to the API. (e.g. no counter should ever call dec_return).

Quoted 3 lines

So audit the kernel, find the cases that should not wrap, categorize and create APIs for them that trap the wrapping. But don't go around confusing things that don't need confusion.

That's what's happening here. But as it turns out, it's easier to do this by employing both the process of elimination (mark the counters) and direct identification (mark the refcount_t). Then the pool of "unannotated" atomic_t instances continues to shrink.

That's like saying: "I'm too lazy to track what I've looked at already". You're basically proposing to graffiti "Kees was here -- 16/10/2020" all over the kernel. Just so you can see where you still need to go.

It says the code was (assuming your audit was correct) good at that date, but has no guarantees for any moment after that.

Quoted 6 lines · Peter Zijlstra

That's like saying: "I'm too lazy to track what I've looked at already". You're basically proposing to graffiti "Kees was here -- 16/10/2020" all over the kernel. Just so you can see where you still need to go.

It says the code was (assuming your audit was correct) good at that date, but has no guarantees for any moment after that.

That kind of bit-rot marking is exactly what I would like to avoid: just putting a comment in is pointless. Making the expectations of the usage become _enforced_ is the goal. And having it enforced by the _compiler_ is key. Just adding a meaningless attribute that a static checker will notice some time and hope people fix them doesn't scale either (just look at how many sparse warnings there are).

Quoted 8 lines · Peter Zijlstra

Quoted 2 lines · Shuah Khan

They don't add any new behavior, As Kees mentioned they do give us a way to clearly differentiate atomic usages that can wrap.

No it doesn't! atomic_t can wrap, this thing can wrap, no distinction.

All it does is fragment the API and sow confusion. FOR NO BENEFIT.

I really don't see it this way. It's a distinct subset of the atomic_t API. The trouble that has existed here has been with an atomic_t being originally used NOT for lifetime management, that mutates into something like that because of the available API, but doing so without realizing it. atomic_t gets used for all kinds of algorithms, and the "counter" type is way too easily accidentally transformed into a "lifetime tracker" and we get bugs.

If we have a distinct type for wrapping-counters that limits the API, then it is much harder for folks to shoot themselves in the foot. I don't see why this is so bad: we end up with safer usage, more easily auditable code behavior ("how was this atomic_t instance _intended_ to be used?"), and no change in binary size.

Quoted 3 lines

Quoted 3 lines

There is no need to keep inc_return in this API as such. I included it so it can be used for above cases 1 and 2, so the users don't have to call inc() followed by read(). It can be left out of the API.

I go back and forth on this, but after looking at these instances, it makes sense to have inc_return(), for where counters are actually "serial numbers". An argument could be made[1], however, that such uses should not end up in the position of _reusing_ earlier identifiers, which means it's actually can't wrap. (And some cases just need u64 to make this happen[2] -- and in that specific case, don't even need to be atomic_t).

[1] https://lore.kernel.org/lkml/202010071334.8298F3FA7@keescook/
[2] https://git.kernel.org/linus/d1e7fd6462ca9fc76650fbe6ca800e35b24267da

Quoted 2 lines

Wrong! The atomic usage in mutex doesn't fall in any of those categories.

But the atomic usage in mutex is *IN* mutex -- it's a separate data type, etc. We don't build mutexes manually, so why build counters manually?

Quoted 5 lines

The only thing you're all saying that makes sense is that unintentional wrapping can have bad consequences, the rest is pure confusion.

Focus on the non-wrapping cases, _everything_ else is not going anywhere.

I view this as a way to do so: this subset of wrapping cases is being identified and removed from the pool of all the atomic_t cases so that they will have been classified, and we can continue to narrow down all the atomic_t uses to find any potentially mis-used non-wrapping cases.

The other option is adding some kind of attribute to the declarations (which gets us the annotation) but doesn't provide a limit to the API. (e.g. no counter should ever call dec_return).

Quoted 3 lines

So audit the kernel, find the cases that should not wrap, categorize and create APIs for them that trap the wrapping. But don't go around confusing things that don't need confusion.

That's what's happening here. But as it turns out, it's easier to do this by employing both the process of elimination (mark the counters) and direct identification (mark the refcount_t). Then the pool of "unannotated" atomic_t instances continues to shrink.