[RFC PATCH 00/11] Introduce Simple atomic and non-atomic counters

[RFC PATCH 00/11] Introduce Simple atomic and non-atomic counters

[Shuah Khan]

This patch series is a result of discussion at the refcount_t BOF
the Linux Plumbers Conference. In this discussion, we identifed
a need for looking closely and investigating atomic_t usages in
the kernel when it is used strictly as a counter wothout 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 twofold: 1. 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. 2. provides
non-atomic counters for cases where atomic isn't necessary.
    
Simple atomic and non-atomic counters api provides interfaces for simple
atomic and non-atomic counters that just count, and don't guard resource
lifetimes. Counters will wrap around to 0 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.

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

In addition this patch series converts a few drivers to use the new api.
The following criteria is used for select variables for conversion:

1. Variable doesn't guard object lifetimes, manage state changes e.g:
   device usage counts, device open counts, and pm states.
2. Variable is used for stats and counters.
3. The conversion doesn't change the overflow behavior.

Please review and let me know if non-stat conversions e.g: probe_count,
deferred_trigger_count make sense.

Shuah Khan (11):
  counters: Introduce counter and counter_atomic counters
  selftests:lib:test_counters: add new test for counters
  drivers/base: convert deferred_trigger_count and probe_count to
    counter_atomic
  drivers/base/devcoredump: convert devcd_count to counter_atomic
  drivers/acpi: convert seqno counter_atomic
  drivers/acpi/apei: convert seqno counter_atomic
  drivers/android/binder: convert stats, transaction_log to
    counter_atomic
  drivers/base/test/test_async_driver_probe: convert to use
    counter_atomic
  drivers/char/ipmi: convert stats to use counter_atomic
  drivers/misc/vmw_vmci: convert num guest devices counter to
    counter_atomic
  drivers/edac: convert pci counters to counter_atomic

 Documentation/core-api/counters.rst          | 158 +++++++++
 MAINTAINERS                                  |   8 +
 drivers/acpi/acpi_extlog.c                   |   5 +-
 drivers/acpi/apei/ghes.c                     |   5 +-
 drivers/android/binder.c                     |  41 +--
 drivers/android/binder_internal.h            |   3 +-
 drivers/base/dd.c                            |  19 +-
 drivers/base/devcoredump.c                   |   5 +-
 drivers/base/test/test_async_driver_probe.c  |  23 +-
 drivers/char/ipmi/ipmi_msghandler.c          |   9 +-
 drivers/char/ipmi/ipmi_si_intf.c             |   9 +-
 drivers/edac/edac_pci.h                      |   5 +-
 drivers/edac/edac_pci_sysfs.c                |  28 +-
 drivers/misc/vmw_vmci/vmci_guest.c           |   9 +-
 include/linux/counters.h                     | 343 +++++++++++++++++++
 lib/Kconfig                                  |  10 +
 lib/Makefile                                 |   1 +
 lib/test_counters.c                          | 283 +++++++++++++++
 tools/testing/selftests/lib/Makefile         |   1 +
 tools/testing/selftests/lib/config           |   1 +
 tools/testing/selftests/lib/test_counters.sh |   5 +
 21 files changed, 897 insertions(+), 74 deletions(-)
 create mode 100644 Documentation/core-api/counters.rst
 create mode 100644 include/linux/counters.h
 create mode 100644 lib/test_counters.c
 create mode 100755 tools/testing/selftests/lib/test_counters.sh

-- 
2.25.1

[RFC PATCH 01/11] counters: Introduce counter and counter_atomic

[Shuah Khan]

Introduce Simple atomic and non-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 twofold: 1. 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. 2. provides
non-atomic counters for cases where atomic isn't necessary.

Simple atomic and non-atomic counters api provides interfaces for simple
atomic and non-atomic counters that just count, and don't guard resource
lifetimes. Counters will wrap around to 0 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.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 Documentation/core-api/counters.rst | 158 +++++++++++++
 MAINTAINERS                         |   7 +
 include/linux/counters.h            | 343 ++++++++++++++++++++++++++++
 lib/Kconfig                         |  10 +
 lib/Makefile                        |   1 +
 lib/test_counters.c                 | 283 +++++++++++++++++++++++
 6 files changed, 802 insertions(+)
 create mode 100644 Documentation/core-api/counters.rst
 create mode 100644 include/linux/counters.h
 create mode 100644 lib/test_counters.c

diff --git a/Documentation/core-api/counters.rst b/Documentation/core-api/counters.rst
new file mode 100644
index 000000000000..86c90de6cb6b
--- /dev/null
+++ b/Documentation/core-api/counters.rst
@@ -0,0 +1,158 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=====================================
+Simple atomic and non-atomic counters
+=====================================
+
+:Author: Shuah Khan
+
+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 twofold: 1. 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. 2. provides
+non-atomic counters for cases where atomic isn't necessary.
+
+Simple atomic and non-atomic counters api provides interfaces for simple
+atomic and non-atomic counters that just count, and don't guard resource
+lifetimes. Counters will wrap around to 0 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 refcnt_t interfaces for guarding resources.
+
+.. warning::
+        Counter will wrap around to 0 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_atomic and counter_atomic_long types use atomic_t and atomic_long_t
+underneath to leverage atomic_t api,  providing a small subset of atomic_t
+interfaces necessary to support simple counters. ::
+
+        struct counter_atomic { atomic_t cnt; };
+        struct counter_atomic_long { atomic_long_t cnt; };
+
+Please see :ref:`Documentation/core-api/atomic_ops.rst <atomic_ops>` for
+information on the Semantics and Behavior of Atomic operations.
+
+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_atomic_set() --> atomic_set()
+
+        static struct counter_atomic acnt = COUNTER_ATOMIC_INIT(0);
+        counter_atomic_set(0);
+
+        static struct counter_atomic_long acnt = COUNTER_ATOMIC_INIT(0);
+        counter_atomic_long_set(0);
+
+Increment interface
+-------------------
+
+Increments counter and doesn't return the new counter value. ::
+
+        counter_atomic_inc() --> atomic_inc()
+        counter_atomic_long_inc() --> atomic_long_inc()
+
+Increment and return new counter value interface
+------------------------------------------------
+
+Increments counter and returns the new counter value. ::
+
+        counter_atomic_inc_return() --> atomic_inc_return()
+        counter_atomic_long_inc_return() --> atomic_long_inc_return()
+
+Decrement interface
+-------------------
+
+Decrements counter and doesn't return the new counter value. ::
+
+        counter_atomic_dec() --> atomic_dec()
+        counter_atomic_long_dec() --> atomic_long_dec()
+
+Decrement and return new counter value interface
+------------------------------------------------
+
+Decrements counter and returns the new counter value. ::
+
+        counter_atomic_dec_return() --> atomic_dec_return()
+        counter_atomic_long_dec_return() --> atomic_long_dec_return()
+
+Non-atomic counter operations
+=============================
+
+counter and counter_long types are non-atomic types. ::
+
+        struct counter { int cnt; };
+        struct counter_long { long cnt; };
+
+Initializers
+------------
+
+Interfaces for initializing counters ::
+
+        #define COUNTER_INIT(i) { (i) }
+        counter_set();
+
+        static struct counter acnt = COUNTER_INIT(0);
+        counter_set(0);
+
+        static struct counter_long acnt = COUNTER_INIT(0);
+        counter_long_set(0);
+
+Increment interface
+-------------------
+
+Increments counter and doesn't return the new counter value. ::
+
+        counter_inc()
+        counter_long_inc()
+
+Increment and return new counter value interface
+------------------------------------------------
+
+Increments counter and returns the new counter value. ::
+
+        counter_inc_return()
+        counter_long_inc_return()
+
+Decrement interface
+-------------------
+
+Decrements counter and doesn't return the new counter value. ::
+
+        counter_dec()
+        counter_long_dec()
+
+Decrement and return new counter value interface
+------------------------------------------------
+
+Decrements counter and returns the new counter value. ::
+
+        counter_dec_return()
+        counter_long_dec_return()
diff --git a/MAINTAINERS b/MAINTAINERS
index 0d0862b19ce5..1d3abcfa76ab 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15841,6 +15841,13 @@ S:	Maintained
 F:	Documentation/fb/sm712fb.rst
 F:	drivers/video/fbdev/sm712*
 
+SIMPLE ATOMIC and NON-ATOMIC COUNTERS
+M:	Shuah Khan <skhan@linuxfoundation.org>
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	include/linux/counters.h
+F:	lib/test_counters.c
+
 SIMPLE FIRMWARE INTERFACE (SFI)
 S:	Obsolete
 W:	http://simplefirmware.org/
diff --git a/include/linux/counters.h b/include/linux/counters.h
new file mode 100644
index 000000000000..3c0813e3ccdd
--- /dev/null
+++ b/include/linux/counters.h
@@ -0,0 +1,343 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Interface for simple atomic and non-atomic counters that just count,
+ * and should not be used guard resource lifetimes and device states.
+ * Counters will wrap around to 0 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 refcnt_t interfaces for guarding resources.
+ *
+ * The interface provides:
+ * atomic & atomic_long and
+ * non-atomic & non-atomic_long
+ *	increment and no return
+ *	increment and return value
+ *	decrement and no return
+ *	decrement and return value
+ *	read
+ *	set
+ * functions.
+ *
+ * atomic and atomic_long functions use atomic_t interfaces.
+ * The counter will wrap around to 0 when it overflows.
+ * These interfaces should not be used to guard resource lifetimes.
+ *
+ */
+
+#ifndef __LINUX_COUNTERS_H
+#define __LINUX_COUNTERS_H
+
+#include <linux/atomic.h>
+
+/**
+ * struct counter_atomic - Simple atomic counter
+ * @cnt: int
+ *
+ * The counter wraps around to 0, when it overflows. Should not
+ * be used to guard object lifetimes.
+ **/
+struct counter_atomic {
+	atomic_t cnt;
+};
+
+#define COUNTER_ATOMIC_INIT(i)		{ .cnt = ATOMIC_INIT(i) }
+
+/*
+ * counter_atomic_inc() - increment counter value
+ * @cntr: struct counter_atomic pointer
+ *
+ */
+static inline void counter_atomic_inc(struct counter_atomic *cntr)
+{
+	atomic_inc(&cntr->cnt);
+}
+
+/*
+ * counter_atomic_inc_return() - increment counter value and return it
+ * @cntr: struct counter_atomic pointer
+ *
+ * Return: returns the new counter value after incrementing it
+ */
+static inline int counter_atomic_inc_return(struct counter_atomic *cntr)
+{
+	return atomic_inc_return(&cntr->cnt);
+}
+
+/*
+ * counter_atomic_dec() - decrement counter value
+ * @cntr: struct counter_atomic pointer
+ *
+ */
+static inline void counter_atomic_dec(struct counter_atomic *cntr)
+{
+	atomic_dec(&cntr->cnt);
+}
+
+/*
+ * counter_atomic_dec_return() - decrement counter value and return it
+ * @cntr: struct counter_atomic pointer
+ *
+ * Return: return the new counter value after decrementing it
+ */
+static inline int counter_atomic_dec_return(struct counter_atomic *cntr)
+{
+	return atomic_dec_return(&cntr->cnt);
+}
+
+/*
+ * counter_atomic_read() - read counter value
+ * @cntr: struct counter_atomic pointer
+ *
+ * Return: return the counter value
+ */
+static inline int counter_atomic_read(const struct counter_atomic *cntr)
+{
+	return atomic_read(&cntr->cnt);
+}
+
+/*
+ * counter_atomic_set() - set counter value
+ * @cntr: struct counter_atomic pointer
+ * @val:  new counter value to set
+ *
+ */
+static inline void counter_atomic_set(struct counter_atomic *cntr, int val)
+{
+	atomic_set(&cntr->cnt, val);
+}
+
+/**
+ * struct counter - Simple counter
+ * @cnt: int
+ *
+ * The counter wraps around to 0, when it overflows. Should not
+ * be used to guard object lifetimes.
+ */
+struct counter {
+	int cnt;
+};
+
+#define COUNTER_INIT(i)	{ (i) }
+
+/*
+ * counter_inc() - increment counter value
+ * @cntr: struct counter pointer
+ *
+ */
+static inline void counter_inc(struct counter *cntr)
+{
+	cntr->cnt++;
+}
+
+/*
+ * counter_inc_return() - increment counter value and return it
+ * @cntr: struct counter pointer
+ *
+ * Return: return the new counter value after incrementing it
+ */
+static inline int counter_inc_return(struct counter *cntr)
+{
+	return ++cntr->cnt;
+}
+
+/*
+ * counter_dec() - decrement counter value
+ * @cntr: struct counter_atomic pointer
+ *
+ */
+static inline void counter_dec(struct counter *cntr)
+{
+	cntr->cnt--;
+}
+
+/*
+ * counter_dec_return() - decrement counter value and return it
+ * @cntr: struct counter pointer
+ *
+ * Return: return the new counter value after decrementing it
+ */
+static inline int counter_dec_return(struct counter *cntr)
+{
+	return --cntr->cnt;
+}
+
+/*
+ * counter_read() - read counter value
+ * @cntr: struct counter pointer
+ *
+ * Return: return the counter value
+ */
+static inline int counter_read(const struct counter *cntr)
+{
+	return cntr->cnt;
+}
+
+/*
+ * counter_set() - set counter value
+ * @cntr: struct counter pointer
+ * @val:  new counter value to set
+ *
+ */
+static inline void counter_set(struct counter *cntr, int val)
+{
+	cntr->cnt = val;
+}
+
+/*
+ * struct counter_atomic_long - Simple atomic counter
+ * @cnt: atomic_long_t
+ *
+ * The counter wraps around to 0, when it overflows. Should not
+ * be used to guard object lifetimes.
+ */
+struct counter_atomic_long {
+	atomic_long_t cnt;
+};
+
+/*
+ * counter_atomic_long_inc() - increment counter value
+ * @cntr: struct counter_atomic_long pointer
+ *
+ */
+static inline void counter_atomic_long_inc(struct counter_atomic_long *cntr)
+{
+	atomic_long_inc(&cntr->cnt);
+}
+
+/*
+ * counter_atomic_long_inc_return() - increment counter value and return it
+ * @cntr: struct counter_atomic_long pointer
+ *
+ * Return: return the new counter value after incrementing it
+ */
+static inline long
+counter_atomic_long_inc_return(struct counter_atomic_long *cntr)
+{
+	return atomic_long_inc_return(&cntr->cnt);
+}
+
+/*
+ * counter_atomic_long() - decrement counter value
+ * @cntr: struct counter_atomic_long pointer
+ *
+ */
+static inline void counter_atomic_long_dec(
+				struct counter_atomic_long *cntr)
+{
+	atomic_long_dec(&cntr->cnt);
+}
+
+/*
+ * counter_atomic_long_dec_return() - decrement counter value and return it
+ * @cntr: struct counter_atomic_long pointer
+ *
+ * Return: return the new counter value after decrementing it
+ */
+static inline long
+counter_atomic_long_dec_return(struct counter_atomic_long *cntr)
+{
+	return atomic_long_dec_return(&cntr->cnt);
+}
+
+/*
+ * counter_atomic_long_read() - read counter value
+ * @cntr: struct counter_atomic_long pointer
+ *
+ * Return: return the counter value
+ */
+static inline long
+counter_atomic_long_read(const struct counter_atomic_long *cntr)
+{
+	return atomic_long_read(&cntr->cnt);
+}
+
+/*
+ * counter_atomic_long_set() - set counter value
+ * @cntr: struct counter_atomic pointer
+ * &val:  new counter value to set
+ *
+ */
+static inline void
+counter_atomic_long_set(struct counter_atomic_long *cntr, long val)
+{
+	atomic_long_set(&cntr->cnt, val);
+}
+
+/*
+ * struct counter - Simple counter
+ * @cnt: long
+ *
+ * The counter wraps around to 0, when it overflows. Should not
+ * be used to guard object lifetimes.
+ */
+struct counter_long {
+	long cnt;
+};
+
+/*
+ * counter_long_inc() - increment counter value
+ * @cntr: struct counter_long pointer
+ *
+ */
+static inline void counter_long_inc(struct counter_long *cntr)
+{
+	cntr->cnt++;
+}
+
+/*
+ * counter_long_inc_return() - increment counter value and return it
+ * @cntr: struct counter_long pointer
+ *
+ * Return: return the counter value after incrementing it
+ */
+static inline long counter_long_inc_return(struct counter_long *cntr)
+{
+	return ++cntr->cnt;
+}
+
+/*
+ * counter_long_dec() - decrement counter value
+ * @cntr: struct counter_long pointer
+ *
+ */
+static inline void counter_long_dec(struct counter_long *cntr)
+{
+	cntr->cnt--;
+}
+
+/*
+ * counter_long_dec_return() - decrement counter value
+ * @cntr: counter_long pointer
+ *
+ */
+static inline long counter_long_dec_return(struct counter_long *cntr)
+{
+	return --cntr->cnt;
+}
+
+/*
+ * counter_long_read() - read counter value
+ * @cntr: struct counter_long pointer
+ *
+ * Return: return the new counter value
+ */
+static inline long counter_long_read(const struct counter_long *cntr)
+{
+	return cntr->cnt;
+}
+
+/*
+ * counter_long_set() - set counter value
+ * @cntr: struct counter pointer
+ * @val:  new counter value to set
+ *
+ */
+static inline void counter_long_set(struct counter_long *cntr, long val)
+{
+	cntr->cnt = val;
+}
+
+#endif /* __LINUX_COUNTERS_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index b4b98a03ff98..956063ea7aa8 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -658,6 +658,16 @@ config OBJAGG
 config STRING_SELFTEST
 	tristate "Test string functions"
 
+config TEST_COUNTERS
+	tristate "Test Simple Atomic and Non-atomic counter functions"
+	default n
+	help
+	   A test module for Simple Atomic and Non-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
diff --git a/lib/Makefile b/lib/Makefile
index a4a4c6864f51..95b357bb5f3c 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -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
diff --git a/lib/test_counters.c b/lib/test_counters.c
new file mode 100644
index 000000000000..24ec4a8c057a
--- /dev/null
+++ b/lib/test_counters.c
@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Kernel module for testing Counters
+ *
+ * Authors:
+ *	Shuah Khan	<skhan@linuxfoundation.org>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/counters.h>
+
+void test_counter_atomic(void)
+{
+	static struct counter_atomic acnt = COUNTER_ATOMIC_INIT(0);
+	int start_val = counter_atomic_read(&acnt);
+	int end_val;
+
+	counter_atomic_inc(&acnt);
+	end_val = counter_atomic_read(&acnt);
+	pr_info("Test read and increment: %d to %d - %s\n",
+		start_val, end_val,
+		((start_val+1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_atomic_read(&acnt);
+	end_val = counter_atomic_inc_return(&acnt);
+	pr_info("Test read increment and return: %d to %d - %s\n",
+		start_val, end_val,
+		((start_val+1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_atomic_read(&acnt);
+	counter_atomic_dec(&acnt);
+	end_val = counter_atomic_read(&acnt);
+	pr_info("Test read and decrement: %d to %d - %s\n",
+		start_val, end_val,
+		((start_val-1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_atomic_read(&acnt);
+	end_val = counter_atomic_dec_return(&acnt);
+	pr_info("Test read decrement and return: %d to %d - %s\n",
+		start_val, end_val,
+		((start_val-1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_atomic_read(&acnt);
+	counter_atomic_set(&acnt, INT_MAX);
+	end_val = counter_atomic_read(&acnt);
+	pr_info("Test set: %d to %d - %s\n",
+		start_val, end_val,
+		((end_val == INT_MAX) ? "PASS" : "FAIL"));
+}
+
+void test_counter_atomic_overflow(void)
+{
+	static struct counter_atomic ucnt = COUNTER_ATOMIC_INIT(0);
+	static struct counter_atomic ocnt = COUNTER_ATOMIC_INIT(INT_MAX);
+	int start_val;
+	int end_val;
+
+	start_val = counter_atomic_read(&ucnt);
+	end_val = counter_atomic_dec_return(&ucnt);
+	pr_info("Test underflow: %d to %d\n",
+		start_val, end_val);
+
+	start_val = counter_atomic_read(&ocnt);
+	end_val = counter_atomic_inc_return(&ocnt);
+	pr_info("Test overflow: %d to %d\n",
+		start_val, end_val);
+}
+
+void test_counter(void)
+{
+	static struct counter acnt = COUNTER_INIT(0);
+	int start_val = counter_read(&acnt);
+	int end_val;
+
+	counter_inc(&acnt);
+	end_val = counter_read(&acnt);
+	pr_info("Test read and increment: %d to %d - %s\n",
+		start_val, end_val,
+		((start_val+1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_read(&acnt);
+	end_val = counter_inc_return(&acnt);
+	pr_info("Test read increment and return: %d to %d - %s\n",
+		start_val, end_val,
+		((start_val+1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_read(&acnt);
+	counter_dec(&acnt);
+	end_val = counter_read(&acnt);
+	pr_info("Test read and decrement: %d to %d - %s\n",
+		start_val, end_val,
+		((start_val-1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_read(&acnt);
+	end_val = counter_dec_return(&acnt);
+	pr_info("Test read decrement and return: %d to %d - %s\n",
+		start_val, end_val,
+		((start_val-1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_read(&acnt);
+	counter_set(&acnt, INT_MAX);
+	end_val = counter_read(&acnt);
+	pr_info("Test set: %d to %d - %s\n",
+		start_val, end_val,
+		((end_val == INT_MAX) ? "PASS" : "FAIL"));
+}
+
+void test_counter_overflow(void)
+{
+	static struct counter ucnt = COUNTER_INIT(0);
+	static struct counter ocnt = COUNTER_INIT(INT_MAX);
+	int start_val;
+	int end_val;
+
+	start_val = counter_read(&ucnt);
+	end_val = counter_dec_return(&ucnt);
+	pr_info("Test underflow: %d to %d - %s\n",
+		start_val, end_val,
+		((start_val-1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_read(&ocnt);
+	end_val = counter_inc_return(&ocnt);
+	pr_info("Test overflow: %d to %d - %s\n",
+		start_val, end_val,
+		((start_val+1 == end_val) ? "PASS" : "FAIL"));
+}
+
+void test_counter_atomic_long(void)
+{
+	static struct counter_atomic_long acnt = COUNTER_ATOMIC_INIT(0);
+	long start_val = counter_atomic_long_read(&acnt);
+	long end_val;
+
+	counter_atomic_long_inc(&acnt);
+	end_val = counter_atomic_long_read(&acnt);
+	pr_info("Test read and increment: %ld to %ld - %s\n",
+		start_val, end_val,
+		((start_val+1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_atomic_long_read(&acnt);
+	end_val = counter_atomic_long_inc_return(&acnt);
+	pr_info("Test read increment and return: %ld to %ld - %s\n",
+		start_val, end_val,
+		((start_val+1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_atomic_long_read(&acnt);
+	counter_atomic_long_dec(&acnt);
+	end_val = counter_atomic_long_read(&acnt);
+	pr_info("Test read and decrement: %ld to %ld - %s\n",
+		start_val, end_val,
+		((start_val-1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_atomic_long_read(&acnt);
+	end_val = counter_atomic_long_dec_return(&acnt);
+	pr_info("Test read decrement and return: %ld to %ld - %s\n",
+		start_val, end_val,
+		((start_val-1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_atomic_long_read(&acnt);
+	counter_atomic_long_set(&acnt, INT_MAX);
+	end_val = counter_atomic_long_read(&acnt);
+	pr_info("Test set: %ld to %ld - %s\n",
+		start_val, end_val,
+		((end_val == INT_MAX) ? "PASS" : "FAIL"));
+}
+
+void test_counter_atomic_long_overflow(void)
+{
+	static struct counter_atomic_long ucnt = COUNTER_ATOMIC_INIT(0);
+	static struct counter_atomic_long ocnt = COUNTER_ATOMIC_INIT(INT_MAX);
+	long start_val;
+	long end_val;
+
+	start_val = counter_atomic_long_read(&ucnt);
+	end_val = counter_atomic_long_dec_return(&ucnt);
+	pr_info("Test underflow: %ld to %ld - %s\n",
+		start_val, end_val,
+		((start_val-1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_atomic_long_read(&ocnt);
+	end_val = counter_atomic_long_inc_return(&ocnt);
+	pr_info("Test overflow: %ld to %ld - %s\n",
+		start_val, end_val,
+		((start_val+1 == end_val) ? "PASS" : "FAIL"));
+}
+
+void test_counter_long(void)
+{
+	static struct counter_long acnt = COUNTER_INIT(0);
+	long start_val = counter_long_read(&acnt);
+	long end_val;
+
+	counter_long_inc(&acnt);
+	end_val = counter_long_read(&acnt);
+	pr_info("Test read and increment: %ld to %ld - %s\n",
+		start_val, end_val,
+		((start_val+1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_long_read(&acnt);
+	end_val = counter_long_inc_return(&acnt);
+	pr_info("Test read increment and return: %ld to %ld - %s\n",
+		start_val, end_val,
+		((start_val+1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_long_read(&acnt);
+	counter_long_dec(&acnt);
+	end_val = counter_long_read(&acnt);
+	pr_info("Test read and decrement: %ld to %ld - %s\n",
+		start_val, end_val,
+		((start_val-1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_long_read(&acnt);
+	end_val = counter_long_dec_return(&acnt);
+	pr_info("Test read decrement and return: %ld to %ld - %s\n",
+		start_val, end_val,
+		((start_val-1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_long_read(&acnt);
+	counter_long_set(&acnt, INT_MAX);
+	end_val = counter_long_read(&acnt);
+	pr_info("Test set: %ld to %ld - %s\n",
+		start_val, end_val,
+		((end_val == INT_MAX) ? "PASS" : "FAIL"));
+}
+
+void test_counter_long_overflow(void)
+{
+	static struct counter_long ucnt = COUNTER_INIT(0);
+	static struct counter_long ocnt = COUNTER_INIT(INT_MAX);
+	long start_val;
+	long end_val;
+
+	start_val = counter_long_read(&ucnt);
+	end_val = counter_long_dec_return(&ucnt);
+	pr_info("Test underflow: %ld to %ld - %s\n",
+		start_val, end_val,
+		((start_val-1 == end_val) ? "PASS" : "FAIL"));
+
+	start_val = counter_long_read(&ocnt);
+	end_val = counter_long_inc_return(&ocnt);
+	pr_info("Test overflow: %ld to %ld - %s\n",
+		start_val, end_val,
+		((start_val+1 == end_val) ? "PASS" : "FAIL"));
+}
+
+static int __init test_counters_init(void)
+{
+	pr_info("Start counter_atomic_*() interfaces test\n");
+	test_counter_atomic();
+	test_counter_atomic_overflow();
+	pr_info("End counter_atomic_*() interfaces test\n\n");
+
+	pr_info("Start counter_*() interfaces test\n");
+	test_counter();
+	test_counter_overflow();
+	pr_info("End counter_*() interfaces test\n\n");
+
+	pr_info("Start counter_atomic_long_*() interfaces test\n");
+	test_counter_atomic_long();
+	test_counter_atomic_long_overflow();
+	pr_info("End counter_atomic_*() interfaces test\n\n");
+
+	pr_info("Start counter_long_*() interfaces test\n");
+	test_counter_long();
+	test_counter_long_overflow();
+	pr_info("End counter_long_*() interfaces test\n\n");
+
+	return 0;
+}
+
+module_init(test_counters_init);
+
+static void __exit test_counters_exit(void)
+{
+	pr_info("exiting.\n");
+}
+
+module_exit(test_counters_exit);
+
+MODULE_AUTHOR("Shuah Khan <skhan@linuxfoundation.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

Re: [RFC PATCH 01/11] counters: Introduce counter and counter_atomic

[Greg KH]

On Tue, Sep 22, 2020 at 07:43:30PM -0600, Shuah Khan wrote:
> Introduce Simple atomic and non-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 twofold: 1. 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. 2. provides
> non-atomic counters for cases where atomic isn't necessary.
> 
> Simple atomic and non-atomic counters api provides interfaces for simple
> atomic and non-atomic counters that just count, and don't guard resource
> lifetimes. Counters will wrap around to 0 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.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  Documentation/core-api/counters.rst | 158 +++++++++++++
>  MAINTAINERS                         |   7 +
>  include/linux/counters.h            | 343 ++++++++++++++++++++++++++++
>  lib/Kconfig                         |  10 +
>  lib/Makefile                        |   1 +
>  lib/test_counters.c                 | 283 +++++++++++++++++++++++

Tests for new apis, nice!

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [RFC PATCH 01/11] counters: Introduce counter and counter_atomic

[Kees Cook]

On Tue, Sep 22, 2020 at 07:43:30PM -0600, Shuah Khan wrote:
> Introduce Simple atomic and non-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.

Thank you for working on a counter API! I'm glad to see work here,
though I have some pretty significant changes to request; see below...

> 
> The purpose of these counters is twofold: 1. 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. 2. provides
> non-atomic counters for cases where atomic isn't necessary.
> 
> Simple atomic and non-atomic counters api provides interfaces for simple
> atomic and non-atomic counters that just count, and don't guard resource
> lifetimes. Counters will wrap around to 0 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.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

I would really like these APIs to be _impossible_ to use for object
lifetime management. To that end, I would like to have all of the
*_return() functions removed. It should be strictly init, inc, dec,
read.

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

Why even force the distinction? I think all the counters should be
atomic and then there is no chance they will get accidentally used in
places where someone *thinks* it's safe to use a non-atomic. So,
"_atomic" can be removed from the name and the non-atomic implementation
can get removed. Anyone already using non-atomic counters is just using
"int" and "long" anyway. Let's please only create APIs that are always
safe to use, and provide some benefit over a native time.

> +Simple atomic and non-atomic counters api provides interfaces for simple
> +atomic and non-atomic counters that just count, and don't guard resource
> +lifetimes. Counters will wrap around to 0 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 refcnt_t interfaces for guarding resources.

typo: refcount_t (this typo is repeated in a few places)

> +
> +.. warning::
> +        Counter will wrap around to 0 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_atomic and counter_atomic_long types use atomic_t and atomic_long_t
> +underneath to leverage atomic_t api,  providing a small subset of atomic_t
> +interfaces necessary to support simple counters. ::
> +
> +        struct counter_atomic { atomic_t cnt; };
> +        struct counter_atomic_long { atomic_long_t cnt; };

"Unsized" and "Long" are both unhelpful here. If it's unsized, that
tells nothing about the counter size. And "long" changes with word size.
I think counters should either _all_ be 64-bit, or they should be
explicitly sized in their name. Either:

struct counter;  /* unsigned 64-bit, wraps back around to 0 */

or

struct counter32; /* unsigned 32-bit, wraps back around to 0 */
struct counter64; /* unsigned 64-bit, wraps back around to 0 */

> --- /dev/null
> +++ b/lib/test_counters.c
> @@ -0,0 +1,283 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Kernel module for testing Counters
> + *
> + * Authors:
> + *	Shuah Khan	<skhan@linuxfoundation.org>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/counters.h>
> +
> +void test_counter_atomic(void)
> +{
> +	static struct counter_atomic acnt = COUNTER_ATOMIC_INIT(0);
> +	int start_val = counter_atomic_read(&acnt);
> +	int end_val;

Please build this test using KUnit.

> +	start_val = counter_long_read(&acnt);
> +	end_val = counter_long_dec_return(&acnt);
> +	pr_info("Test read decrement and return: %ld to %ld - %s\n",
> +		start_val, end_val,
> +		((start_val-1 == end_val) ? "PASS" : "FAIL"));

I also see a lot of copy/paste patterns here. These should all use a
common helper.

-- 
Kees Cook

Re: [RFC PATCH 01/11] counters: Introduce counter and counter_atomic

[Greg KH]

On Wed, Sep 23, 2020 at 12:04:08PM -0700, Kees Cook wrote:
> On Tue, Sep 22, 2020 at 07:43:30PM -0600, Shuah Khan wrote:
> > Introduce Simple atomic and non-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.
> 
> Thank you for working on a counter API! I'm glad to see work here,
> though I have some pretty significant changes to request; see below...
> 
> > 
> > The purpose of these counters is twofold: 1. 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. 2. provides
> > non-atomic counters for cases where atomic isn't necessary.
> > 
> > Simple atomic and non-atomic counters api provides interfaces for simple
> > atomic and non-atomic counters that just count, and don't guard resource
> > lifetimes. Counters will wrap around to 0 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.
> > 
> > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> I would really like these APIs to be _impossible_ to use for object
> lifetime management. To that end, I would like to have all of the
> *_return() functions removed. It should be strictly init, inc, dec,
> read.
> 
> > +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.
> 
> Why even force the distinction? I think all the counters should be
> atomic and then there is no chance they will get accidentally used in
> places where someone *thinks* it's safe to use a non-atomic. So,
> "_atomic" can be removed from the name and the non-atomic implementation
> can get removed. Anyone already using non-atomic counters is just using
> "int" and "long" anyway. Let's please only create APIs that are always
> safe to use, and provide some benefit over a native time.

For "statistics", why take the extra overhead for an atomic variable
just to be able to show to a debugging file the number of USB packets
have been sent through the system (a current use of an atomic variable
for some odd reason...)

And really, a "int" should be pretty safe to write from multiple places,
you aren't going to get "tearing" on any processors that run Linux,
worst case you get a stale value when reading them.

So I would argue that the default for a counter be just an int, not
atomic, as odds are, most atomics are not really needed for this type of
thing at all.

thanks,

greg k-h

Re: [RFC PATCH 01/11] counters: Introduce counter and counter_atomic

[Shuah Khan]

On 9/23/20 1:04 PM, Kees Cook wrote:
> On Tue, Sep 22, 2020 at 07:43:30PM -0600, Shuah Khan wrote:
>> Introduce Simple atomic and non-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.
> 
> Thank you for working on a counter API! I'm glad to see work here,
> though I have some pretty significant changes to request; see below...
> 

Thanks for the review.

>>
>> The purpose of these counters is twofold: 1. 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. 2. provides
>> non-atomic counters for cases where atomic isn't necessary.
>>
>> Simple atomic and non-atomic counters api provides interfaces for simple
>> atomic and non-atomic counters that just count, and don't guard resource
>> lifetimes. Counters will wrap around to 0 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.
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> I would really like these APIs to be _impossible_ to use for object
> lifetime management. To that end, I would like to have all of the
> *_return() functions removed. It should be strictly init, inc, dec,
> read.
> 

Yes. I am with you on making this API as small as possible so it won't
be used for lifetime mgmt. That means no support for:

*_test, add_negative etc.

I started out with just init, inc, dec, read. As I started looking
for candidates that can be converted to counters, I found inc_return()
usages. I think we need inc_return() for sure. I haven't come across
atomic_dec_return() yet.

I would say we will need at least inc_return() for being able to convert
all counter atomic_t usages.

>> +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.
> 
> Why even force the distinction? I think all the counters should be
> atomic and then there is no chance they will get accidentally used in
> places where someone *thinks* it's safe to use a non-atomic. So,
> "_atomic" can be removed from the name and the non-atomic implementation
> can get removed. Anyone already using non-atomic counters is just using
> "int" and "long" anyway. Let's please only create APIs that are always
> safe to use, and provide some benefit over a native time.
> 

I am with Greg on this. I think we will find several atomic_t usages
that don't need atomicity.

>> +Simple atomic and non-atomic counters api provides interfaces for simple
>> +atomic and non-atomic counters that just count, and don't guard resource
>> +lifetimes. Counters will wrap around to 0 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 refcnt_t interfaces for guarding resources.
>  > typo: refcount_t (this typo is repeated in a few places)
> 

Thanks for the catch. Will fit it.

>> +
>> +.. warning::
>> +        Counter will wrap around to 0 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_atomic and counter_atomic_long types use atomic_t and atomic_long_t
>> +underneath to leverage atomic_t api,  providing a small subset of atomic_t
>> +interfaces necessary to support simple counters. ::
>> +
>> +        struct counter_atomic { atomic_t cnt; };
>> +        struct counter_atomic_long { atomic_long_t cnt; };
> 
> "Unsized" and "Long" are both unhelpful here. If it's unsized, that
> tells nothing about the counter size. And "long" changes with word size.
> I think counters should either _all_ be 64-bit, or they should be
> explicitly sized in their name. Either:
> 
> struct counter;  /* unsigned 64-bit, wraps back around to 0 */
> 
> or
> 
> struct counter32; /* unsigned 32-bit, wraps back around to 0 */
> struct counter64; /* unsigned 64-bit, wraps back around to 0 */
> 

Will do.

>> --- /dev/null
>> +++ b/lib/test_counters.c
>> @@ -0,0 +1,283 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Kernel module for testing Counters
>> + *
>> + * Authors:
>> + *	Shuah Khan	<skhan@linuxfoundation.org>
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/module.h>
>> +#include <linux/counters.h>
>> +
>> +void test_counter_atomic(void)
>> +{
>> +	static struct counter_atomic acnt = COUNTER_ATOMIC_INIT(0);
>> +	int start_val = counter_atomic_read(&acnt);
>> +	int end_val;
> 
> Please build this test using KUnit.
> 

Sounds good.

>> +	start_val = counter_long_read(&acnt);
>> +	end_val = counter_long_dec_return(&acnt);
>> +	pr_info("Test read decrement and return: %ld to %ld - %s\n",
>> +		start_val, end_val,
>> +		((start_val-1 == end_val) ? "PASS" : "FAIL"));
> 
> I also see a lot of copy/paste patterns here. These should all use a
> common helper.

I knew you would ask for helpers. :)

Yeah will do.

thanks,
-- Shuah

Re: [RFC PATCH 01/11] counters: Introduce counter and counter_atomic

[Kees Cook]

On Wed, Sep 23, 2020 at 09:34:48PM +0200, Greg KH wrote:
> On Wed, Sep 23, 2020 at 12:04:08PM -0700, Kees Cook wrote:
> > On Tue, Sep 22, 2020 at 07:43:30PM -0600, Shuah Khan wrote:
> > > Introduce Simple atomic and non-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.
> > 
> > Thank you for working on a counter API! I'm glad to see work here,
> > though I have some pretty significant changes to request; see below...
> > 
> > > 
> > > The purpose of these counters is twofold: 1. 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. 2. provides
> > > non-atomic counters for cases where atomic isn't necessary.
> > > 
> > > Simple atomic and non-atomic counters api provides interfaces for simple
> > > atomic and non-atomic counters that just count, and don't guard resource
> > > lifetimes. Counters will wrap around to 0 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.
> > > 
> > > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> > 
> > I would really like these APIs to be _impossible_ to use for object
> > lifetime management. To that end, I would like to have all of the
> > *_return() functions removed. It should be strictly init, inc, dec,
> > read.
> > 
> > > +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.
> > 
> > Why even force the distinction? I think all the counters should be
> > atomic and then there is no chance they will get accidentally used in
> > places where someone *thinks* it's safe to use a non-atomic. So,
> > "_atomic" can be removed from the name and the non-atomic implementation
> > can get removed. Anyone already using non-atomic counters is just using
> > "int" and "long" anyway. Let's please only create APIs that are always
> > safe to use, and provide some benefit over a native time.
> 
> For "statistics", why take the extra overhead for an atomic variable
> just to be able to show to a debugging file the number of USB packets
> have been sent through the system (a current use of an atomic variable
> for some odd reason...)
> 
> And really, a "int" should be pretty safe to write from multiple places,
> you aren't going to get "tearing" on any processors that run Linux,
> worst case you get a stale value when reading them.
> 
> So I would argue that the default for a counter be just an int, not
> atomic, as odds are, most atomics are not really needed for this type of
> thing at all.

If the atomicity isn't needed, then they can just use an int. ;)

I think the _counter_ type should be robust. We're specifically looking
at replacing the users who are already using atomic_t for counting. The
idea is to separate all the atomic_t doing ref counting into refcount_t
and all the atomic_t doing statistics into "struct counter", and then
what's left can meaningfully be reasoned about. i.e. "why is this a raw
atomic)t?"

But creating "struct counter" with a non-atomic API doesn't make sense
to me. And it certainly doesn't make sense for replacing existing
atomic_t statistics use cases.

-- 
Kees Cook

Re: [RFC PATCH 01/11] counters: Introduce counter and counter_atomic

[Kees Cook]

On Wed, Sep 23, 2020 at 02:48:22PM -0600, Shuah Khan wrote:
> On 9/23/20 1:04 PM, Kees Cook wrote:
> > On Tue, Sep 22, 2020 at 07:43:30PM -0600, Shuah Khan wrote:
> > I would really like these APIs to be _impossible_ to use for object
> > lifetime management. To that end, I would like to have all of the
> > *_return() functions removed. It should be strictly init, inc, dec,
> > read.
> > 
> 
> Yes. I am with you on making this API as small as possible so it won't
> be used for lifetime mgmt. That means no support for:
> 
> *_test, add_negative etc.
> 
> I started out with just init, inc, dec, read. As I started looking
> for candidates that can be converted to counters, I found inc_return()
> usages. I think we need inc_return() for sure. I haven't come across
> atomic_dec_return() yet.

What are the inc_return() cases? If they're not "safe" to use inc() and
then read(), then those likely need a closer look at what they're doing.

> > > +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.
> > 
> > Why even force the distinction? I think all the counters should be
> > atomic and then there is no chance they will get accidentally used in
> > places where someone *thinks* it's safe to use a non-atomic. So,
> > "_atomic" can be removed from the name and the non-atomic implementation
> > can get removed. Anyone already using non-atomic counters is just using
> > "int" and "long" anyway. Let's please only create APIs that are always
> > safe to use, and provide some benefit over a native time.
> > 
> 
> I am with Greg on this. I think we will find several atomic_t usages
> that don't need atomicity.

If you want to distinguish from atomic and create a wrapping "int", how
about making "counter" be the atomic and name the other "counter_unsafe"
(or "counter_best_effort", "counter_simple", ...) etc?

> > > +	end_val = counter_long_dec_return(&acnt);
> > > +	pr_info("Test read decrement and return: %ld to %ld - %s\n",
> > > +		start_val, end_val,
> > > +		((start_val-1 == end_val) ? "PASS" : "FAIL"));
> > 
> > I also see a lot of copy/paste patterns here. These should all use a
> > common helper.
> 
> I knew you would ask for helpers. :)

Heh. inlines for everyone! ;)

> Yeah will do.

Awesome!

-- 
Kees Cook

Re: [RFC PATCH 01/11] counters: Introduce counter and counter_atomic

[Shuah Khan]

On 9/23/20 2:58 PM, Kees Cook wrote:
> On Wed, Sep 23, 2020 at 02:48:22PM -0600, Shuah Khan wrote:
>> On 9/23/20 1:04 PM, Kees Cook wrote:
>>> On Tue, Sep 22, 2020 at 07:43:30PM -0600, Shuah Khan wrote:
>>> I would really like these APIs to be _impossible_ to use for object
>>> lifetime management. To that end, I would like to have all of the
>>> *_return() functions removed. It should be strictly init, inc, dec,
>>> read.
>>>
>>
>> Yes. I am with you on making this API as small as possible so it won't
>> be used for lifetime mgmt. That means no support for:
>>
>> *_test, add_negative etc.
>>
>> I started out with just init, inc, dec, read. As I started looking
>> for candidates that can be converted to counters, I found inc_return()
>> usages. I think we need inc_return() for sure. I haven't come across
>> atomic_dec_return() yet.
> 
> What are the inc_return() cases? If they're not "safe" to use inc() and
> then read(), then those likely need a closer look at what they're doing.
> 

3 in this series I sent. I would say I barely scratched the surface
when it comes to finding candidates for converting.

drivers/android/binder.c
drivers/acpi/acpi_extlog.c
drivers/acpi/apei/ghes.c

These uses look reasonable to me. Having this inc_return() will save
making _inc() followed by _read()

>>>> +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.
>>>
>>> Why even force the distinction? I think all the counters should be
>>> atomic and then there is no chance they will get accidentally used in
>>> places where someone *thinks* it's safe to use a non-atomic. So,
>>> "_atomic" can be removed from the name and the non-atomic implementation
>>> can get removed. Anyone already using non-atomic counters is just using
>>> "int" and "long" anyway. Let's please only create APIs that are always
>>> safe to use, and provide some benefit over a native time.
>>>
>>
>> I am with Greg on this. I think we will find several atomic_t usages
>> that don't need atomicity.
> 
> If you want to distinguish from atomic and create a wrapping "int", how
> about making "counter" be the atomic and name the other "counter_unsafe"
> (or "counter_best_effort", "counter_simple", ...) etc?
> 

I will change counter to counter_simple and add a warning that this
should only be used when atomic isn't needed. I can outline some
tips for choosing the right one.

thanks,
-- Shuah

Re: [RFC PATCH 01/11] counters: Introduce counter and counter_atomic

[Kees Cook]

On Wed, Sep 23, 2020 at 03:19:08PM -0600, Shuah Khan wrote:
> On 9/23/20 2:58 PM, Kees Cook wrote:
> > On Wed, Sep 23, 2020 at 02:48:22PM -0600, Shuah Khan wrote:
> > > On 9/23/20 1:04 PM, Kees Cook wrote:
> > > > On Tue, Sep 22, 2020 at 07:43:30PM -0600, Shuah Khan wrote:
> > > > I would really like these APIs to be _impossible_ to use for object
> > > > lifetime management. To that end, I would like to have all of the
> > > > *_return() functions removed. It should be strictly init, inc, dec,
> > > > read.
> > > > 
> > > 
> > > Yes. I am with you on making this API as small as possible so it won't
> > > be used for lifetime mgmt. That means no support for:
> > > 
> > > *_test, add_negative etc.
> > > 
> > > I started out with just init, inc, dec, read. As I started looking
> > > for candidates that can be converted to counters, I found inc_return()
> > > usages. I think we need inc_return() for sure. I haven't come across
> > > atomic_dec_return() yet.
> > 
> > What are the inc_return() cases? If they're not "safe" to use inc() and
> > then read(), then those likely need a closer look at what they're doing.
> > 
> 
> 3 in this series I sent. I would say I barely scratched the surface
> when it comes to finding candidates for converting.
> 
> drivers/android/binder.c
> drivers/acpi/acpi_extlog.c
> drivers/acpi/apei/ghes.c
> 
> These uses look reasonable to me. Having this inc_return() will save
> making _inc() followed by _read()

I'd like to make sure it's clear that it should not be treated as atomic
(even if it is), so a separate _read(), I think, makes that clear. And
hopefully it'll keep people from ever trying to sneak a _dec_return()
in. :)

> I will change counter to counter_simple and add a warning that this
> should only be used when atomic isn't needed. I can outline some
> tips for choosing the right one.

Okay.

-- 
Kees Cook