[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

[RFC PATCH 02/11] selftests:lib: add new test for counters

[Shuah Khan]

Add a new selftest for testing counter and 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 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>
---
 MAINTAINERS                                  | 1 +
 tools/testing/selftests/lib/Makefile         | 1 +
 tools/testing/selftests/lib/config           | 1 +
 tools/testing/selftests/lib/test_counters.sh | 5 +++++
 4 files changed, 8 insertions(+)
 create mode 100755 tools/testing/selftests/lib/test_counters.sh

diff --git a/MAINTAINERS b/MAINTAINERS
index 1d3abcfa76ab..fc802ef0ee95 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15847,6 +15847,7 @@ L:	linux-kernel@vger.kernel.org
 S:	Maintained
 F:	include/linux/counters.h
 F:	lib/test_counters.c
+F:	tools/testing/selftests/lib/test_counters.sh
 
 SIMPLE FIRMWARE INTERFACE (SFI)
 S:	Obsolete
diff --git a/tools/testing/selftests/lib/Makefile b/tools/testing/selftests/lib/Makefile
index a105f094676e..e8960d7934e2 100644
--- a/tools/testing/selftests/lib/Makefile
+++ b/tools/testing/selftests/lib/Makefile
@@ -5,5 +5,6 @@
 all:
 
 TEST_PROGS := printf.sh bitmap.sh prime_numbers.sh strscpy.sh
+TEST_PROGS += test_counters.sh
 
 include ../lib.mk
diff --git a/tools/testing/selftests/lib/config b/tools/testing/selftests/lib/config
index b80ee3f6e265..6ed25024d371 100644
--- a/tools/testing/selftests/lib/config
+++ b/tools/testing/selftests/lib/config
@@ -3,3 +3,4 @@ CONFIG_TEST_BITMAP=m
 CONFIG_PRIME_NUMBERS=m
 CONFIG_TEST_STRSCPY=m
 CONFIG_TEST_BITOPS=m
+CONFIG_TEST_COUNTERS=m
diff --git a/tools/testing/selftests/lib/test_counters.sh b/tools/testing/selftests/lib/test_counters.sh
new file mode 100755
index 000000000000..d1a130190e3f
--- /dev/null
+++ b/tools/testing/selftests/lib/test_counters.sh
@@ -0,0 +1,5 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Tests the Simple Atomic and Non-atomic Counters interfaces using
+# test_counters kernel module
+$(dirname $0)/../kselftest/module.sh "test_counters" test_counters
-- 
2.25.1

[RFC PATCH 03/11] drivers/base: convert deferred_trigger_count and probe_count to counter_atomic

[Shuah Khan]

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

deferred_trigger_count gets incremented and read. 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_atomic.

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

probe_count gets incremented, decremented, read. It doesn't guard
object lifetimes, device usage counts, device open counts, and pm
states. There is very little chance of this counter overflowing.
This counter controls the wait for known devices to complete their
probing, and probe_count == 0 ends the wait. Other than that it
meets the other criteria to be converted. Convert it to use
counter_atomic.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/base/dd.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 857b0a928e8d..f49fe45960ac 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -28,6 +28,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
 #include <linux/slab.h>
+#include <linux/counters.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -54,7 +55,7 @@
 static DEFINE_MUTEX(deferred_probe_mutex);
 static LIST_HEAD(deferred_probe_pending_list);
 static LIST_HEAD(deferred_probe_active_list);
-static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
+static struct counter_atomic deferred_trigger_count = COUNTER_ATOMIC_INIT(0);
 static struct dentry *deferred_devices;
 static bool initcalls_done;
 
@@ -173,7 +174,7 @@ static void driver_deferred_probe_trigger(void)
 	 * into the active list so they can be retried by the workqueue
 	 */
 	mutex_lock(&deferred_probe_mutex);
-	atomic_inc(&deferred_trigger_count);
+	counter_atomic_inc(&deferred_trigger_count);
 	list_splice_tail_init(&deferred_probe_pending_list,
 			      &deferred_probe_active_list);
 	mutex_unlock(&deferred_probe_mutex);
@@ -466,7 +467,7 @@ int device_bind_driver(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(device_bind_driver);
 
-static atomic_t probe_count = ATOMIC_INIT(0);
+static struct counter_atomic probe_count = COUNTER_ATOMIC_INIT(0);
 static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
 
 static void driver_deferred_probe_add_trigger(struct device *dev,
@@ -474,7 +475,7 @@ static void driver_deferred_probe_add_trigger(struct device *dev,
 {
 	driver_deferred_probe_add(dev);
 	/* Did a trigger occur while probing? Need to re-trigger if yes */
-	if (local_trigger_count != atomic_read(&deferred_trigger_count))
+	if (local_trigger_count != counter_atomic_read(&deferred_trigger_count))
 		driver_deferred_probe_trigger();
 }
 
@@ -493,7 +494,7 @@ static DEVICE_ATTR_RO(state_synced);
 static int really_probe(struct device *dev, struct device_driver *drv)
 {
 	int ret = -EPROBE_DEFER;
-	int local_trigger_count = atomic_read(&deferred_trigger_count);
+	int local_trigger_count = counter_atomic_read(&deferred_trigger_count);
 	bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
 			   !drv->suppress_bind_attrs;
 
@@ -514,7 +515,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	if (ret)
 		return ret;
 
-	atomic_inc(&probe_count);
+	counter_atomic_inc(&probe_count);
 	pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
 		 drv->bus->name, __func__, drv->name, dev_name(dev));
 	if (!list_empty(&dev->devres_head)) {
@@ -648,7 +649,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	 */
 	ret = 0;
 done:
-	atomic_dec(&probe_count);
+	counter_atomic_dec(&probe_count);
 	wake_up_all(&probe_waitqueue);
 	return ret;
 }
@@ -678,7 +679,7 @@ static int really_probe_debug(struct device *dev, struct device_driver *drv)
  */
 int driver_probe_done(void)
 {
-	int local_probe_count = atomic_read(&probe_count);
+	int local_probe_count = counter_atomic_read(&probe_count);
 
 	pr_debug("%s: probe_count = %d\n", __func__, local_probe_count);
 	if (local_probe_count)
@@ -699,7 +700,7 @@ void wait_for_device_probe(void)
 	flush_work(&deferred_probe_work);
 
 	/* wait for the known devices to complete their probing */
-	wait_event(probe_waitqueue, atomic_read(&probe_count) == 0);
+	wait_event(probe_waitqueue, counter_atomic_read(&probe_count) == 0);
 	async_synchronize_full();
 }
 EXPORT_SYMBOL_GPL(wait_for_device_probe);
-- 
2.25.1

[RFC PATCH 04/11] drivers/base/devcoredump: convert devcd_count to counter_atomic

[Shuah Khan]

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

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

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

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/base/devcoredump.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
index e42d0b514384..d719424d1e0b 100644
--- a/drivers/base/devcoredump.c
+++ b/drivers/base/devcoredump.c
@@ -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_atomic devcd_count = COUNTER_ATOMIC_INIT(0);
 	struct devcd_entry *devcd;
 	struct device *existing;
 
@@ -286,7 +287,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
 	device_initialize(&devcd->devcd_dev);
 
 	dev_set_name(&devcd->devcd_dev, "devcd%d",
-		     atomic_inc_return(&devcd_count));
+		     counter_atomic_inc_return(&devcd_count));
 	devcd->devcd_dev.class = &devcd_class;
 
 	if (device_add(&devcd->devcd_dev))
-- 
2.25.1

[RFC PATCH 05/11] drivers/acpi: convert seqno counter_atomic

[Shuah Khan]

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

seqno is a sequence number counter for logging. This counter gets
incremented. Unsure if there is a chance of this overflowing. It
doesn't look like overflowing causes any problems since it is used
to tag the log messages and nothing more.

Convert it to use counter_atomic.

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

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/acpi/acpi_extlog.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index f138e12b7b82..23b696b7eb14 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -12,6 +12,7 @@
 #include <linux/ratelimit.h>
 #include <linux/edac.h>
 #include <linux/ras.h>
+#include <linux/counters.h>
 #include <asm/cpu.h>
 #include <asm/mce.h>
 
@@ -93,7 +94,7 @@ static struct acpi_hest_generic_status *extlog_elog_entry_check(int cpu, int ban
 static void __print_extlog_rcd(const char *pfx,
 			       struct acpi_hest_generic_status *estatus, int cpu)
 {
-	static atomic_t seqno;
+	static struct counter_atomic seqno;
 	unsigned int curr_seqno;
 	char pfx_seq[64];
 
@@ -103,7 +104,7 @@ static void __print_extlog_rcd(const char *pfx,
 		else
 			pfx = KERN_ERR;
 	}
-	curr_seqno = atomic_inc_return(&seqno);
+	curr_seqno = counter_atomic_inc_return(&seqno);
 	snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}", pfx, curr_seqno);
 	printk("%s""Hardware error detected on CPU%d\n", pfx_seq, cpu);
 	cper_estatus_print(pfx_seq, estatus);
-- 
2.25.1

[RFC PATCH 06/11] drivers/acpi/apei: convert seqno counter_atomic

[Shuah Khan]

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

seqno is a sequence number counter for logging. This counter gets
incremented. Unsure if there is a chance of this overflowing. It
doesn't look like overflowing causes any problems since it is used
to tag the log messages and nothing more.

Convert it to use counter_atomic.

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

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/acpi/apei/ghes.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 81bf71b10d44..88a660f9c22c 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -41,6 +41,7 @@
 #include <linux/uuid.h>
 #include <linux/ras.h>
 #include <linux/task_work.h>
+#include <linux/counters.h>
 
 #include <acpi/actbl1.h>
 #include <acpi/ghes.h>
@@ -562,7 +563,7 @@ static void __ghes_print_estatus(const char *pfx,
 				 const struct acpi_hest_generic *generic,
 				 const struct acpi_hest_generic_status *estatus)
 {
-	static atomic_t seqno;
+	static struct counter_atomic seqno = COUNTER_ATOMIC_INIT(0);
 	unsigned int curr_seqno;
 	char pfx_seq[64];
 
@@ -573,7 +574,7 @@ static void __ghes_print_estatus(const char *pfx,
 		else
 			pfx = KERN_ERR;
 	}
-	curr_seqno = atomic_inc_return(&seqno);
+	curr_seqno = counter_atomic_inc_return(&seqno);
 	snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}" HW_ERR, pfx, curr_seqno);
 	printk("%s""Hardware error from APEI Generic Hardware Error Source: %d\n",
 	       pfx_seq, generic->header.source_id);
-- 
2.25.1

[RFC PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic

[Shuah Khan]

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

stats tracks per-process binder statistics. Unsure if there is a chance
of this overflowing, other than stats getting reset to 0. Convert it to
use counter_atomic.

binder_transaction_log:cur is used to keep track of the current log entry
location. Overflow is handled in the code. Since it is used as a
counter, convert it to use counter_atomic.

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

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/android/binder.c          | 41 ++++++++++++++++---------------
 drivers/android/binder_internal.h |  3 ++-
 2 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f936530a19b0..11a0407c46df 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -66,6 +66,7 @@
 #include <linux/syscalls.h>
 #include <linux/task_work.h>
 #include <linux/sizes.h>
+#include <linux/counters.h>
 
 #include <uapi/linux/android/binder.h>
 #include <uapi/linux/android/binderfs.h>
@@ -172,22 +173,22 @@ enum binder_stat_types {
 };
 
 struct binder_stats {
-	atomic_t br[_IOC_NR(BR_FAILED_REPLY) + 1];
-	atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
-	atomic_t obj_created[BINDER_STAT_COUNT];
-	atomic_t obj_deleted[BINDER_STAT_COUNT];
+	struct counter_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
+	struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
+	struct counter_atomic obj_created[BINDER_STAT_COUNT];
+	struct counter_atomic obj_deleted[BINDER_STAT_COUNT];
 };
 
 static struct binder_stats binder_stats;
 
 static inline void binder_stats_deleted(enum binder_stat_types type)
 {
-	atomic_inc(&binder_stats.obj_deleted[type]);
+	counter_atomic_inc(&binder_stats.obj_deleted[type]);
 }
 
 static inline void binder_stats_created(enum binder_stat_types type)
 {
-	atomic_inc(&binder_stats.obj_created[type]);
+	counter_atomic_inc(&binder_stats.obj_created[type]);
 }
 
 struct binder_transaction_log binder_transaction_log;
@@ -197,7 +198,7 @@ static struct binder_transaction_log_entry *binder_transaction_log_add(
 	struct binder_transaction_log *log)
 {
 	struct binder_transaction_log_entry *e;
-	unsigned int cur = atomic_inc_return(&log->cur);
+	unsigned int cur = counter_atomic_inc_return(&log->cur);
 
 	if (cur >= ARRAY_SIZE(log->entry))
 		log->full = true;
@@ -3615,9 +3616,9 @@ static int binder_thread_write(struct binder_proc *proc,
 		ptr += sizeof(uint32_t);
 		trace_binder_command(cmd);
 		if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.bc)) {
-			atomic_inc(&binder_stats.bc[_IOC_NR(cmd)]);
-			atomic_inc(&proc->stats.bc[_IOC_NR(cmd)]);
-			atomic_inc(&thread->stats.bc[_IOC_NR(cmd)]);
+			counter_atomic_inc(&binder_stats.bc[_IOC_NR(cmd)]);
+			counter_atomic_inc(&proc->stats.bc[_IOC_NR(cmd)]);
+			counter_atomic_inc(&thread->stats.bc[_IOC_NR(cmd)]);
 		}
 		switch (cmd) {
 		case BC_INCREFS:
@@ -4047,9 +4048,9 @@ static void binder_stat_br(struct binder_proc *proc,
 {
 	trace_binder_return(cmd);
 	if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.br)) {
-		atomic_inc(&binder_stats.br[_IOC_NR(cmd)]);
-		atomic_inc(&proc->stats.br[_IOC_NR(cmd)]);
-		atomic_inc(&thread->stats.br[_IOC_NR(cmd)]);
+		counter_atomic_inc(&binder_stats.br[_IOC_NR(cmd)]);
+		counter_atomic_inc(&proc->stats.br[_IOC_NR(cmd)]);
+		counter_atomic_inc(&thread->stats.br[_IOC_NR(cmd)]);
 	}
 }
 
@@ -5841,7 +5842,7 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
 	BUILD_BUG_ON(ARRAY_SIZE(stats->bc) !=
 		     ARRAY_SIZE(binder_command_strings));
 	for (i = 0; i < ARRAY_SIZE(stats->bc); i++) {
-		int temp = atomic_read(&stats->bc[i]);
+		int temp = counter_atomic_read(&stats->bc[i]);
 
 		if (temp)
 			seq_printf(m, "%s%s: %d\n", prefix,
@@ -5851,7 +5852,7 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
 	BUILD_BUG_ON(ARRAY_SIZE(stats->br) !=
 		     ARRAY_SIZE(binder_return_strings));
 	for (i = 0; i < ARRAY_SIZE(stats->br); i++) {
-		int temp = atomic_read(&stats->br[i]);
+		int temp = counter_atomic_read(&stats->br[i]);
 
 		if (temp)
 			seq_printf(m, "%s%s: %d\n", prefix,
@@ -5863,8 +5864,8 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
 	BUILD_BUG_ON(ARRAY_SIZE(stats->obj_created) !=
 		     ARRAY_SIZE(stats->obj_deleted));
 	for (i = 0; i < ARRAY_SIZE(stats->obj_created); i++) {
-		int created = atomic_read(&stats->obj_created[i]);
-		int deleted = atomic_read(&stats->obj_deleted[i]);
+		int created = counter_atomic_read(&stats->obj_created[i]);
+		int deleted = counter_atomic_read(&stats->obj_deleted[i]);
 
 		if (created || deleted)
 			seq_printf(m, "%s%s: active %d total %d\n",
@@ -6054,7 +6055,7 @@ static void print_binder_transaction_log_entry(struct seq_file *m,
 int binder_transaction_log_show(struct seq_file *m, void *unused)
 {
 	struct binder_transaction_log *log = m->private;
-	unsigned int log_cur = atomic_read(&log->cur);
+	unsigned int log_cur = counter_atomic_read(&log->cur);
 	unsigned int count;
 	unsigned int cur;
 	int i;
@@ -6124,8 +6125,8 @@ static int __init binder_init(void)
 	if (ret)
 		return ret;
 
-	atomic_set(&binder_transaction_log.cur, ~0U);
-	atomic_set(&binder_transaction_log_failed.cur, ~0U);
+	counter_atomic_set(&binder_transaction_log.cur, ~0U);
+	counter_atomic_set(&binder_transaction_log_failed.cur, ~0U);
 
 	binder_debugfs_dir_entry_root = debugfs_create_dir("binder", NULL);
 	if (binder_debugfs_dir_entry_root)
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 283d3cb9c16e..9f5ef382e761 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -12,6 +12,7 @@
 #include <linux/stddef.h>
 #include <linux/types.h>
 #include <linux/uidgid.h>
+#include <linux/counters.h>
 
 struct binder_context {
 	struct binder_node *binder_context_mgr_node;
@@ -136,7 +137,7 @@ struct binder_transaction_log_entry {
 };
 
 struct binder_transaction_log {
-	atomic_t cur;
+	struct counter_atomic cur;
 	bool full;
 	struct binder_transaction_log_entry entry[32];
 };
-- 
2.25.1

[RFC PATCH 08/11] drivers/base/test/test_async_driver_probe: convert to use counter_atomic

[Shuah Khan]

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

atomic_t variables used to count errors, warns, keep track of timeout,
and async completion are counters.

Unsure overflow is a concern for timeout and async completion, and there
are no checks for overflow to hold them to upper bounds. Overflow and
wrap around doesn't impact errors, and warns.

Convert them to use counter_atomic.

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

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/base/test/test_async_driver_probe.c | 23 ++++++++++++---------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/base/test/test_async_driver_probe.c b/drivers/base/test/test_async_driver_probe.c
index 3bb7beb127a9..def08cd03eb5 100644
--- a/drivers/base/test/test_async_driver_probe.c
+++ b/drivers/base/test/test_async_driver_probe.c
@@ -14,11 +14,12 @@
 #include <linux/numa.h>
 #include <linux/nodemask.h>
 #include <linux/topology.h>
+#include <linux/counters.h>
 
 #define TEST_PROBE_DELAY	(5 * 1000)	/* 5 sec */
 #define TEST_PROBE_THRESHOLD	(TEST_PROBE_DELAY / 2)
 
-static atomic_t warnings, errors, timeout, async_completed;
+static struct counter_atomic warnings, errors, timeout, async_completed;
 
 static int test_probe(struct platform_device *pdev)
 {
@@ -29,9 +30,9 @@ static int test_probe(struct platform_device *pdev)
 	 * have then report it as an error, otherwise we wil sleep for the
 	 * required amount of time and then report completion.
 	 */
-	if (atomic_read(&timeout)) {
+	if (counter_atomic_read(&timeout)) {
 		dev_err(dev, "async probe took too long\n");
-		atomic_inc(&errors);
+		counter_atomic_inc(&errors);
 	} else {
 		dev_dbg(&pdev->dev, "sleeping for %d msecs in probe\n",
 			 TEST_PROBE_DELAY);
@@ -48,10 +49,10 @@ static int test_probe(struct platform_device *pdev)
 		    dev_to_node(dev) != numa_node_id()) {
 			dev_warn(dev, "NUMA node mismatch %d != %d\n",
 				 dev_to_node(dev), numa_node_id());
-			atomic_inc(&warnings);
+			counter_atomic_inc(&warnings);
 		}
 
-		atomic_inc(&async_completed);
+		counter_atomic_inc(&async_completed);
 	}
 
 	return 0;
@@ -244,11 +245,12 @@ static int __init test_async_probe_init(void)
 	 * Otherwise if they completed without errors or warnings then
 	 * report successful completion.
 	 */
-	if (atomic_read(&async_completed) != async_id) {
+	if (counter_atomic_read(&async_completed) != async_id) {
 		pr_err("async events still pending, forcing timeout\n");
-		atomic_inc(&timeout);
+		counter_atomic_inc(&timeout);
 		err = -ETIMEDOUT;
-	} else if (!atomic_read(&errors) && !atomic_read(&warnings)) {
+	} else if (!counter_atomic_read(&errors) &&
+		   !counter_atomic_read(&warnings)) {
 		pr_info("completed successfully\n");
 		return 0;
 	}
@@ -271,12 +273,13 @@ static int __init test_async_probe_init(void)
 	 * errors or warnings being reported by the probe routine.
 	 */
 	if (err)
-		atomic_inc(&errors);
+		counter_atomic_inc(&errors);
 	else
 		err = -EINVAL;
 
 	pr_err("Test failed with %d errors and %d warnings\n",
-	       atomic_read(&errors), atomic_read(&warnings));
+	       counter_atomic_read(&errors),
+	       counter_atomic_read(&warnings));
 
 	return err;
 }
-- 
2.25.1

[RFC PATCH 09/11] drivers/char/ipmi: convert stats to use counter_atomic

[Shuah Khan]

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

atomic_t variables used for stats are atomic counters. Overflow will
wrap around and reset the stats and no change with the conversion.

Convert them to use counter_atomic.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/char/ipmi/ipmi_msghandler.c | 9 +++++----
 drivers/char/ipmi/ipmi_si_intf.c    | 9 +++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 737c0b6b24ea..c3ad8edd1da3 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -34,6 +34,7 @@
 #include <linux/uuid.h>
 #include <linux/nospec.h>
 #include <linux/vmalloc.h>
+#include <linux/counters.h>
 
 #define IPMI_DRIVER_VERSION "39.2"
 
@@ -584,7 +585,7 @@ struct ipmi_smi {
 	struct ipmi_my_addrinfo addrinfo[IPMI_MAX_CHANNELS];
 	bool channels_ready;
 
-	atomic_t stats[IPMI_NUM_STATS];
+	struct counter_atomic stats[IPMI_NUM_STATS];
 
 	/*
 	 * run_to_completion duplicate of smb_info, smi_info
@@ -630,9 +631,9 @@ static LIST_HEAD(smi_watchers);
 static DEFINE_MUTEX(smi_watchers_mutex);
 
 #define ipmi_inc_stat(intf, stat) \
-	atomic_inc(&(intf)->stats[IPMI_STAT_ ## stat])
+	counter_atomic_inc(&(intf)->stats[IPMI_STAT_ ## stat])
 #define ipmi_get_stat(intf, stat) \
-	((unsigned int) atomic_read(&(intf)->stats[IPMI_STAT_ ## stat]))
+	((unsigned int) counter_atomic_read(&(intf)->stats[IPMI_STAT_ ## stat]))
 
 static const char * const addr_src_to_str[] = {
 	"invalid", "hotmod", "hardcoded", "SPMI", "ACPI", "SMBIOS", "PCI",
@@ -3448,7 +3449,7 @@ int ipmi_add_smi(struct module         *owner,
 	INIT_LIST_HEAD(&intf->cmd_rcvrs);
 	init_waitqueue_head(&intf->waitq);
 	for (i = 0; i < IPMI_NUM_STATS; i++)
-		atomic_set(&intf->stats[i], 0);
+		counter_atomic_set(&intf->stats[i], 0);
 
 	mutex_lock(&ipmi_interfaces_mutex);
 	/* Look for a hole in the numbers. */
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 77b8d551ae7f..cea1131f7639 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -43,6 +43,7 @@
 #include "ipmi_si_sm.h"
 #include <linux/string.h>
 #include <linux/ctype.h>
+#include <linux/counters.h>
 
 /* Measure times between events in the driver. */
 #undef DEBUG_TIMING
@@ -237,7 +238,7 @@ struct smi_info {
 	bool dev_group_added;
 
 	/* Counters and things for the proc filesystem. */
-	atomic_t stats[SI_NUM_STATS];
+	struct counter_atomic stats[SI_NUM_STATS];
 
 	struct task_struct *thread;
 
@@ -245,9 +246,9 @@ struct smi_info {
 };
 
 #define smi_inc_stat(smi, stat) \
-	atomic_inc(&(smi)->stats[SI_STAT_ ## stat])
+	counter_atomic_inc(&(smi)->stats[SI_STAT_ ## stat])
 #define smi_get_stat(smi, stat) \
-	((unsigned int) atomic_read(&(smi)->stats[SI_STAT_ ## stat]))
+	((unsigned int) counter_atomic_read(&(smi)->stats[SI_STAT_ ## stat]))
 
 #define IPMI_MAX_INTFS 4
 static int force_kipmid[IPMI_MAX_INTFS];
@@ -2013,7 +2014,7 @@ static int try_smi_init(struct smi_info *new_smi)
 	atomic_set(&new_smi->req_events, 0);
 	new_smi->run_to_completion = false;
 	for (i = 0; i < SI_NUM_STATS; i++)
-		atomic_set(&new_smi->stats[i], 0);
+		counter_atomic_set(&new_smi->stats[i], 0);
 
 	new_smi->interrupt_disabled = true;
 	atomic_set(&new_smi->need_watch, 0);
-- 
2.25.1

[RFC PATCH 10/11] drivers/misc/vmw_vmci: convert num guest devices counter to counter_atomic

[Shuah Khan]

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

atomic_t variable used to count number of vmci guest devices is used
as just as counter and it doesn't control object lifetimes or state
management. Overflow doesn't appear to be problem for this use.

Convert it to use counter_atomic.

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

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/misc/vmw_vmci/vmci_guest.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c
index cc8eeb361fcd..2c21448af730 100644
--- a/drivers/misc/vmw_vmci/vmci_guest.c
+++ b/drivers/misc/vmw_vmci/vmci_guest.c
@@ -20,6 +20,7 @@
 #include <linux/smp.h>
 #include <linux/io.h>
 #include <linux/vmalloc.h>
+#include <linux/counters.h>
 
 #include "vmci_datagram.h"
 #include "vmci_doorbell.h"
@@ -68,11 +69,11 @@ struct pci_dev *vmci_pdev;
 static struct vmci_guest_device *vmci_dev_g;
 static DEFINE_SPINLOCK(vmci_dev_spinlock);
 
-static atomic_t vmci_num_guest_devices = ATOMIC_INIT(0);
+static struct counter_atomic vmci_num_guest_devices = COUNTER_ATOMIC_INIT(0);
 
 bool vmci_guest_code_active(void)
 {
-	return atomic_read(&vmci_num_guest_devices) != 0;
+	return counter_atomic_read(&vmci_num_guest_devices) != 0;
 }
 
 u32 vmci_get_vm_context_id(void)
@@ -624,7 +625,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
 
 	dev_dbg(&pdev->dev, "Registered device\n");
 
-	atomic_inc(&vmci_num_guest_devices);
+	counter_atomic_inc(&vmci_num_guest_devices);
 
 	/* Enable specific interrupt bits. */
 	cmd = VMCI_IMR_DATAGRAM;
@@ -684,7 +685,7 @@ static void vmci_guest_remove_device(struct pci_dev *pdev)
 
 	dev_dbg(&pdev->dev, "Removing device\n");
 
-	atomic_dec(&vmci_num_guest_devices);
+	counter_atomic_dec(&vmci_num_guest_devices);
 
 	vmci_qp_guest_endpoints_exit();
 
-- 
2.25.1

[RFC PATCH 11/11] drivers/edac: convert pci counters to counter_atomic

[Shuah Khan]

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

atomic_t variables used for pci counters keep track of pci parity and
non-parity errors. Overflow will wrap around and reset the counts as
was the case prior to the conversion.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/edac/edac_pci.h       |  5 +++--
 drivers/edac/edac_pci_sysfs.c | 28 ++++++++++++++--------------
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/edac/edac_pci.h b/drivers/edac/edac_pci.h
index 5175f5724cfa..2f8a21e56377 100644
--- a/drivers/edac/edac_pci.h
+++ b/drivers/edac/edac_pci.h
@@ -30,12 +30,13 @@
 #include <linux/pci.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
+#include <linux/counters.h>
 
 #ifdef CONFIG_PCI
 
 struct edac_pci_counter {
-	atomic_t pe_count;
-	atomic_t npe_count;
+	struct counter_atomic pe_count;
+	struct counter_atomic npe_count;
 };
 
 /*
diff --git a/drivers/edac/edac_pci_sysfs.c b/drivers/edac/edac_pci_sysfs.c
index 53042af7262e..fccec2677907 100644
--- a/drivers/edac/edac_pci_sysfs.c
+++ b/drivers/edac/edac_pci_sysfs.c
@@ -23,8 +23,8 @@ static int edac_pci_log_pe = 1;		/* log PCI parity errors */
 static int edac_pci_log_npe = 1;	/* log PCI non-parity error errors */
 static int edac_pci_poll_msec = 1000;	/* one second workq period */
 
-static atomic_t pci_parity_count = ATOMIC_INIT(0);
-static atomic_t pci_nonparity_count = ATOMIC_INIT(0);
+static struct counter_atomic pci_parity_count = COUNTER_ATOMIC_INIT(0);
+static struct counter_atomic pci_nonparity_count = COUNTER_ATOMIC_INIT(0);
 
 static struct kobject *edac_pci_top_main_kobj;
 static atomic_t edac_pci_sysfs_refcount = ATOMIC_INIT(0);
@@ -58,13 +58,13 @@ int edac_pci_get_poll_msec(void)
 /**************************** EDAC PCI sysfs instance *******************/
 static ssize_t instance_pe_count_show(struct edac_pci_ctl_info *pci, char *data)
 {
-	return sprintf(data, "%u\n", atomic_read(&pci->counters.pe_count));
+	return sprintf(data, "%u\n", counter_atomic_read(&pci->counters.pe_count));
 }
 
 static ssize_t instance_npe_count_show(struct edac_pci_ctl_info *pci,
 				char *data)
 {
-	return sprintf(data, "%u\n", atomic_read(&pci->counters.npe_count));
+	return sprintf(data, "%u\n", counter_atomic_read(&pci->counters.npe_count));
 }
 
 #define to_instance(k) container_of(k, struct edac_pci_ctl_info, kobj)
@@ -553,7 +553,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 			edac_printk(KERN_CRIT, EDAC_PCI,
 				"Signaled System Error on %s\n",
 				pci_name(dev));
-			atomic_inc(&pci_nonparity_count);
+			counter_atomic_inc(&pci_nonparity_count);
 		}
 
 		if (status & (PCI_STATUS_PARITY)) {
@@ -561,7 +561,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 				"Master Data Parity Error on %s\n",
 				pci_name(dev));
 
-			atomic_inc(&pci_parity_count);
+			counter_atomic_inc(&pci_parity_count);
 		}
 
 		if (status & (PCI_STATUS_DETECTED_PARITY)) {
@@ -569,7 +569,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 				"Detected Parity Error on %s\n",
 				pci_name(dev));
 
-			atomic_inc(&pci_parity_count);
+			counter_atomic_inc(&pci_parity_count);
 		}
 	}
 
@@ -592,7 +592,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 				edac_printk(KERN_CRIT, EDAC_PCI, "Bridge "
 					"Signaled System Error on %s\n",
 					pci_name(dev));
-				atomic_inc(&pci_nonparity_count);
+				counter_atomic_inc(&pci_nonparity_count);
 			}
 
 			if (status & (PCI_STATUS_PARITY)) {
@@ -600,7 +600,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 					"Master Data Parity Error on "
 					"%s\n", pci_name(dev));
 
-				atomic_inc(&pci_parity_count);
+				counter_atomic_inc(&pci_parity_count);
 			}
 
 			if (status & (PCI_STATUS_DETECTED_PARITY)) {
@@ -608,7 +608,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 					"Detected Parity Error on %s\n",
 					pci_name(dev));
 
-				atomic_inc(&pci_parity_count);
+				counter_atomic_inc(&pci_parity_count);
 			}
 		}
 	}
@@ -646,7 +646,7 @@ void edac_pci_do_parity_check(void)
 	if (!check_pci_errors)
 		return;
 
-	before_count = atomic_read(&pci_parity_count);
+	before_count = counter_atomic_read(&pci_parity_count);
 
 	/* scan all PCI devices looking for a Parity Error on devices and
 	 * bridges.
@@ -658,7 +658,7 @@ void edac_pci_do_parity_check(void)
 	/* Only if operator has selected panic on PCI Error */
 	if (edac_pci_get_panic_on_pe()) {
 		/* If the count is different 'after' from 'before' */
-		if (before_count != atomic_read(&pci_parity_count))
+		if (before_count != counter_atomic_read(&pci_parity_count))
 			panic("EDAC: PCI Parity Error");
 	}
 }
@@ -686,7 +686,7 @@ void edac_pci_handle_pe(struct edac_pci_ctl_info *pci, const char *msg)
 {
 
 	/* global PE counter incremented by edac_pci_do_parity_check() */
-	atomic_inc(&pci->counters.pe_count);
+	counter_atomic_inc(&pci->counters.pe_count);
 
 	if (edac_pci_get_log_pe())
 		edac_pci_printk(pci, KERN_WARNING,
@@ -711,7 +711,7 @@ void edac_pci_handle_npe(struct edac_pci_ctl_info *pci, const char *msg)
 {
 
 	/* global NPE counter incremented by edac_pci_do_parity_check() */
-	atomic_inc(&pci->counters.npe_count);
+	counter_atomic_inc(&pci->counters.npe_count);
 
 	if (edac_pci_get_log_npe())
 		edac_pci_printk(pci, KERN_WARNING,
-- 
2.25.1

Re: [RFC PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic

[Greg KH]

On Tue, Sep 22, 2020 at 07:43:36PM -0600, Shuah Khan wrote:
> 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 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.
> 
> stats tracks per-process binder statistics. Unsure if there is a chance
> of this overflowing, other than stats getting reset to 0. Convert it to
> use counter_atomic.
> 
> binder_transaction_log:cur is used to keep track of the current log entry
> location. Overflow is handled in the code. Since it is used as a
> counter, convert it to use counter_atomic.
> 
> This conversion doesn't change the oveflow wrap around behavior.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  drivers/android/binder.c          | 41 ++++++++++++++++---------------
>  drivers/android/binder_internal.h |  3 ++-
>  2 files changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index f936530a19b0..11a0407c46df 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -66,6 +66,7 @@
>  #include <linux/syscalls.h>
>  #include <linux/task_work.h>
>  #include <linux/sizes.h>
> +#include <linux/counters.h>
>  
>  #include <uapi/linux/android/binder.h>
>  #include <uapi/linux/android/binderfs.h>
> @@ -172,22 +173,22 @@ enum binder_stat_types {
>  };
>  
>  struct binder_stats {
> -	atomic_t br[_IOC_NR(BR_FAILED_REPLY) + 1];
> -	atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
> -	atomic_t obj_created[BINDER_STAT_COUNT];
> -	atomic_t obj_deleted[BINDER_STAT_COUNT];
> +	struct counter_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
> +	struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
> +	struct counter_atomic obj_created[BINDER_STAT_COUNT];
> +	struct counter_atomic obj_deleted[BINDER_STAT_COUNT];

These are just debugging statistics, no reason they have to be atomic
variables at all and they should be able to just be "struct counter"
variables instead.

thanks for looking into all of these!

greg k-h

Re: [RFC PATCH 10/11] drivers/misc/vmw_vmci: convert num guest devices counter to counter_atomic

[Greg KH]

On Tue, Sep 22, 2020 at 07:43:39PM -0600, Shuah Khan wrote:
> 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 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.
> 
> atomic_t variable used to count number of vmci guest devices is used
> as just as counter and it doesn't control object lifetimes or state
> management. Overflow doesn't appear to be problem for this use.
> 
> Convert it to use counter_atomic.
> 
> This conversion doesn't change the oveflow wrap around behavior.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  drivers/misc/vmw_vmci/vmci_guest.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c
> index cc8eeb361fcd..2c21448af730 100644
> --- a/drivers/misc/vmw_vmci/vmci_guest.c
> +++ b/drivers/misc/vmw_vmci/vmci_guest.c
> @@ -20,6 +20,7 @@
>  #include <linux/smp.h>
>  #include <linux/io.h>
>  #include <linux/vmalloc.h>
> +#include <linux/counters.h>
>  
>  #include "vmci_datagram.h"
>  #include "vmci_doorbell.h"
> @@ -68,11 +69,11 @@ struct pci_dev *vmci_pdev;
>  static struct vmci_guest_device *vmci_dev_g;
>  static DEFINE_SPINLOCK(vmci_dev_spinlock);
>  
> -static atomic_t vmci_num_guest_devices = ATOMIC_INIT(0);
> +static struct counter_atomic vmci_num_guest_devices = COUNTER_ATOMIC_INIT(0);
>  
>  bool vmci_guest_code_active(void)
>  {
> -	return atomic_read(&vmci_num_guest_devices) != 0;
> +	return counter_atomic_read(&vmci_num_guest_devices) != 0;
>  }
>  
>  u32 vmci_get_vm_context_id(void)
> @@ -624,7 +625,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
>  
>  	dev_dbg(&pdev->dev, "Registered device\n");
>  
> -	atomic_inc(&vmci_num_guest_devices);
> +	counter_atomic_inc(&vmci_num_guest_devices);
>  
>  	/* Enable specific interrupt bits. */
>  	cmd = VMCI_IMR_DATAGRAM;
> @@ -684,7 +685,7 @@ static void vmci_guest_remove_device(struct pci_dev *pdev)
>  
>  	dev_dbg(&pdev->dev, "Removing device\n");
>  
> -	atomic_dec(&vmci_num_guest_devices);
> +	counter_atomic_dec(&vmci_num_guest_devices);
>  
>  	vmci_qp_guest_endpoints_exit();
>  

While this conversion looks fine to me, wow, the code does not really
seem to be doing the right thing.  So that's something to fix up
independantly of this change, not your fault at all :)

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

Re: [RFC PATCH 03/11] drivers/base: convert deferred_trigger_count and probe_count to counter_atomic

[Greg KH]

On Tue, Sep 22, 2020 at 07:43:32PM -0600, Shuah Khan wrote:
> 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 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.
> 
> deferred_trigger_count gets incremented and read. 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_atomic.
> 
> This conversion doesn't change the oveflow wrap around behavior.
> 
> probe_count gets incremented, decremented, read. It doesn't guard
> object lifetimes, device usage counts, device open counts, and pm
> states. There is very little chance of this counter overflowing.
> This counter controls the wait for known devices to complete their
> probing, and probe_count == 0 ends the wait. Other than that it
> meets the other criteria to be converted. Convert it to use
> counter_atomic.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  drivers/base/dd.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 857b0a928e8d..f49fe45960ac 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -28,6 +28,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/pinctrl/devinfo.h>
>  #include <linux/slab.h>
> +#include <linux/counters.h>
>  
>  #include "base.h"
>  #include "power/power.h"
> @@ -54,7 +55,7 @@
>  static DEFINE_MUTEX(deferred_probe_mutex);
>  static LIST_HEAD(deferred_probe_pending_list);
>  static LIST_HEAD(deferred_probe_active_list);
> -static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
> +static struct counter_atomic deferred_trigger_count = COUNTER_ATOMIC_INIT(0);
>  static struct dentry *deferred_devices;
>  static bool initcalls_done;
>  
> @@ -173,7 +174,7 @@ static void driver_deferred_probe_trigger(void)
>  	 * into the active list so they can be retried by the workqueue
>  	 */
>  	mutex_lock(&deferred_probe_mutex);
> -	atomic_inc(&deferred_trigger_count);
> +	counter_atomic_inc(&deferred_trigger_count);
>  	list_splice_tail_init(&deferred_probe_pending_list,
>  			      &deferred_probe_active_list);
>  	mutex_unlock(&deferred_probe_mutex);
> @@ -466,7 +467,7 @@ int device_bind_driver(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(device_bind_driver);
>  
> -static atomic_t probe_count = ATOMIC_INIT(0);
> +static struct counter_atomic probe_count = COUNTER_ATOMIC_INIT(0);
>  static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
>  
>  static void driver_deferred_probe_add_trigger(struct device *dev,
> @@ -474,7 +475,7 @@ static void driver_deferred_probe_add_trigger(struct device *dev,
>  {
>  	driver_deferred_probe_add(dev);
>  	/* Did a trigger occur while probing? Need to re-trigger if yes */
> -	if (local_trigger_count != atomic_read(&deferred_trigger_count))
> +	if (local_trigger_count != counter_atomic_read(&deferred_trigger_count))
>  		driver_deferred_probe_trigger();
>  }
>  
> @@ -493,7 +494,7 @@ static DEVICE_ATTR_RO(state_synced);
>  static int really_probe(struct device *dev, struct device_driver *drv)
>  {
>  	int ret = -EPROBE_DEFER;
> -	int local_trigger_count = atomic_read(&deferred_trigger_count);
> +	int local_trigger_count = counter_atomic_read(&deferred_trigger_count);
>  	bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
>  			   !drv->suppress_bind_attrs;
>  
> @@ -514,7 +515,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  	if (ret)
>  		return ret;
>  
> -	atomic_inc(&probe_count);
> +	counter_atomic_inc(&probe_count);
>  	pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
>  		 drv->bus->name, __func__, drv->name, dev_name(dev));
>  	if (!list_empty(&dev->devres_head)) {
> @@ -648,7 +649,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  	 */
>  	ret = 0;
>  done:
> -	atomic_dec(&probe_count);
> +	counter_atomic_dec(&probe_count);
>  	wake_up_all(&probe_waitqueue);
>  	return ret;
>  }
> @@ -678,7 +679,7 @@ static int really_probe_debug(struct device *dev, struct device_driver *drv)
>   */
>  int driver_probe_done(void)
>  {
> -	int local_probe_count = atomic_read(&probe_count);
> +	int local_probe_count = counter_atomic_read(&probe_count);
>  
>  	pr_debug("%s: probe_count = %d\n", __func__, local_probe_count);
>  	if (local_probe_count)
> @@ -699,7 +700,7 @@ void wait_for_device_probe(void)
>  	flush_work(&deferred_probe_work);
>  
>  	/* wait for the known devices to complete their probing */
> -	wait_event(probe_waitqueue, atomic_read(&probe_count) == 0);
> +	wait_event(probe_waitqueue, counter_atomic_read(&probe_count) == 0);
>  	async_synchronize_full();
>  }
>  EXPORT_SYMBOL_GPL(wait_for_device_probe);
> -- 
> 2.25.1
> 

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

Re: [RFC PATCH 04/11] drivers/base/devcoredump: convert devcd_count to counter_atomic

[Greg KH]

On Tue, Sep 22, 2020 at 07:43:33PM -0600, Shuah Khan wrote:
> 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 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.
> 
> 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_atomic.
> 
> This conversion doesn't change the oveflow wrap around behavior.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  drivers/base/devcoredump.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
> index e42d0b514384..d719424d1e0b 100644
> --- a/drivers/base/devcoredump.c
> +++ b/drivers/base/devcoredump.c
> @@ -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_atomic devcd_count = COUNTER_ATOMIC_INIT(0);
>  	struct devcd_entry *devcd;
>  	struct device *existing;
>  
> @@ -286,7 +287,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
>  	device_initialize(&devcd->devcd_dev);
>  
>  	dev_set_name(&devcd->devcd_dev, "devcd%d",
> -		     atomic_inc_return(&devcd_count));
> +		     counter_atomic_inc_return(&devcd_count));
>  	devcd->devcd_dev.class = &devcd_class;
>  
>  	if (device_add(&devcd->devcd_dev))
> -- 
> 2.25.1
> 

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

Re: [RFC PATCH 08/11] drivers/base/test/test_async_driver_probe: convert to use counter_atomic

[Greg KH]

On Tue, Sep 22, 2020 at 07:43:37PM -0600, Shuah Khan wrote:
> 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 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.
> 
> atomic_t variables used to count errors, warns, keep track of timeout,
> and async completion are counters.
> 
> Unsure overflow is a concern for timeout and async completion, and there
> are no checks for overflow to hold them to upper bounds. Overflow and
> wrap around doesn't impact errors, and warns.
> 
> Convert them to use counter_atomic.
> 
> This conversion doesn't change the oveflow wrap around behavior.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  drivers/base/test/test_async_driver_probe.c | 23 ++++++++++++---------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/base/test/test_async_driver_probe.c b/drivers/base/test/test_async_driver_probe.c
> index 3bb7beb127a9..def08cd03eb5 100644
> --- a/drivers/base/test/test_async_driver_probe.c
> +++ b/drivers/base/test/test_async_driver_probe.c
> @@ -14,11 +14,12 @@
>  #include <linux/numa.h>
>  #include <linux/nodemask.h>
>  #include <linux/topology.h>
> +#include <linux/counters.h>
>  
>  #define TEST_PROBE_DELAY	(5 * 1000)	/* 5 sec */
>  #define TEST_PROBE_THRESHOLD	(TEST_PROBE_DELAY / 2)
>  
> -static atomic_t warnings, errors, timeout, async_completed;
> +static struct counter_atomic warnings, errors, timeout, async_completed;

Having 3 atomic variables here feels like something is not right and we
should switch the code over to using a single lock, and 3 variables.

But that's not the fault of your conversion, it looks fine.

It is interesting that this is digging up all sorts of "odd, why is this
code written like that???" issues with the conversion, which means it's
a good thing to do :)

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

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 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic

[Kees Cook]

On Wed, Sep 23, 2020 at 07:10:27AM +0200, Greg KH wrote:
> On Tue, Sep 22, 2020 at 07:43:36PM -0600, Shuah Khan wrote:
> > 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 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.
> > 
> > stats tracks per-process binder statistics. Unsure if there is a chance
> > of this overflowing, other than stats getting reset to 0. Convert it to
> > use counter_atomic.
> > 
> > binder_transaction_log:cur is used to keep track of the current log entry
> > location. Overflow is handled in the code. Since it is used as a
> > counter, convert it to use counter_atomic.
> > 
> > This conversion doesn't change the oveflow wrap around behavior.
> > 
> > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> > ---
> >  drivers/android/binder.c          | 41 ++++++++++++++++---------------
> >  drivers/android/binder_internal.h |  3 ++-
> >  2 files changed, 23 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index f936530a19b0..11a0407c46df 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -66,6 +66,7 @@
> >  #include <linux/syscalls.h>
> >  #include <linux/task_work.h>
> >  #include <linux/sizes.h>
> > +#include <linux/counters.h>
> >  
> >  #include <uapi/linux/android/binder.h>
> >  #include <uapi/linux/android/binderfs.h>
> > @@ -172,22 +173,22 @@ enum binder_stat_types {
> >  };
> >  
> >  struct binder_stats {
> > -	atomic_t br[_IOC_NR(BR_FAILED_REPLY) + 1];
> > -	atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
> > -	atomic_t obj_created[BINDER_STAT_COUNT];
> > -	atomic_t obj_deleted[BINDER_STAT_COUNT];
> > +	struct counter_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
> > +	struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
> > +	struct counter_atomic obj_created[BINDER_STAT_COUNT];
> > +	struct counter_atomic obj_deleted[BINDER_STAT_COUNT];
> 
> These are just debugging statistics, no reason they have to be atomic
> variables at all and they should be able to just be "struct counter"
> variables instead.

But there's no reason for them _not_ to be atomic. Please let's keep
this API as always safe. Why even provide a new foot-gun here?

-- 
Kees Cook

Re: [RFC PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic

[Greg KH]

On Wed, Sep 23, 2020 at 12:04:58PM -0700, Kees Cook wrote:
> On Wed, Sep 23, 2020 at 07:10:27AM +0200, Greg KH wrote:
> > On Tue, Sep 22, 2020 at 07:43:36PM -0600, Shuah Khan wrote:
> > > 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 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.
> > > 
> > > stats tracks per-process binder statistics. Unsure if there is a chance
> > > of this overflowing, other than stats getting reset to 0. Convert it to
> > > use counter_atomic.
> > > 
> > > binder_transaction_log:cur is used to keep track of the current log entry
> > > location. Overflow is handled in the code. Since it is used as a
> > > counter, convert it to use counter_atomic.
> > > 
> > > This conversion doesn't change the oveflow wrap around behavior.
> > > 
> > > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> > > ---
> > >  drivers/android/binder.c          | 41 ++++++++++++++++---------------
> > >  drivers/android/binder_internal.h |  3 ++-
> > >  2 files changed, 23 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index f936530a19b0..11a0407c46df 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -66,6 +66,7 @@
> > >  #include <linux/syscalls.h>
> > >  #include <linux/task_work.h>
> > >  #include <linux/sizes.h>
> > > +#include <linux/counters.h>
> > >  
> > >  #include <uapi/linux/android/binder.h>
> > >  #include <uapi/linux/android/binderfs.h>
> > > @@ -172,22 +173,22 @@ enum binder_stat_types {
> > >  };
> > >  
> > >  struct binder_stats {
> > > -	atomic_t br[_IOC_NR(BR_FAILED_REPLY) + 1];
> > > -	atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
> > > -	atomic_t obj_created[BINDER_STAT_COUNT];
> > > -	atomic_t obj_deleted[BINDER_STAT_COUNT];
> > > +	struct counter_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
> > > +	struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
> > > +	struct counter_atomic obj_created[BINDER_STAT_COUNT];
> > > +	struct counter_atomic obj_deleted[BINDER_STAT_COUNT];
> > 
> > These are just debugging statistics, no reason they have to be atomic
> > variables at all and they should be able to just be "struct counter"
> > variables instead.
> 
> But there's no reason for them _not_ to be atomic. Please let's keep
> this API as always safe. Why even provide a new foot-gun here?

These are debugging things, how can you shoot yourself in the foot with
that???

thanks,

greg k-h

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 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic

[Kees Cook]

On Wed, Sep 23, 2020 at 09:31:34PM +0200, Greg KH wrote:
> On Wed, Sep 23, 2020 at 12:04:58PM -0700, Kees Cook wrote:
> > On Wed, Sep 23, 2020 at 07:10:27AM +0200, Greg KH wrote:
> > > On Tue, Sep 22, 2020 at 07:43:36PM -0600, Shuah Khan wrote:
> > > >  struct binder_stats {
> > > > -	atomic_t br[_IOC_NR(BR_FAILED_REPLY) + 1];
> > > > -	atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
> > > > -	atomic_t obj_created[BINDER_STAT_COUNT];
> > > > -	atomic_t obj_deleted[BINDER_STAT_COUNT];
> > > > +	struct counter_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
> > > > +	struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
> > > > +	struct counter_atomic obj_created[BINDER_STAT_COUNT];
> > > > +	struct counter_atomic obj_deleted[BINDER_STAT_COUNT];
> > > 
> > > These are just debugging statistics, no reason they have to be atomic
> > > variables at all and they should be able to just be "struct counter"
> > > variables instead.
> > 
> > But there's no reason for them _not_ to be atomic. Please let's keep
> > this API as always safe. Why even provide a new foot-gun here?
> 
> These are debugging things, how can you shoot yourself in the foot with
> that???

Because suddenly you might be trying to use these values for debugging
only to dig and dig to discover that because they were non-atomic, some
parallel race cause a counter to get dropped, etc.

Since we can design this API robustly, let's take the opportunity to do
so.

-- 
Kees Cook

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

Re: [RFC PATCH 05/11] drivers/acpi: convert seqno counter_atomic

[Rafael J. Wysocki]

On Wed, Sep 23, 2020 at 3:44 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> 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 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.
>
> seqno is a sequence number counter for logging. This counter gets
> incremented. Unsure if there is a chance of this overflowing. It
> doesn't look like overflowing causes any problems since it is used
> to tag the log messages and nothing more.
>
> Convert it to use counter_atomic.
>
> This conversion doesn't change the oveflow wrap around behavior.
>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

Both this change and the next patch are fine by me.

Thanks!

> ---
>  drivers/acpi/acpi_extlog.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index f138e12b7b82..23b696b7eb14 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -12,6 +12,7 @@
>  #include <linux/ratelimit.h>
>  #include <linux/edac.h>
>  #include <linux/ras.h>
> +#include <linux/counters.h>
>  #include <asm/cpu.h>
>  #include <asm/mce.h>
>
> @@ -93,7 +94,7 @@ static struct acpi_hest_generic_status *extlog_elog_entry_check(int cpu, int ban
>  static void __print_extlog_rcd(const char *pfx,
>                                struct acpi_hest_generic_status *estatus, int cpu)
>  {
> -       static atomic_t seqno;
> +       static struct counter_atomic seqno;
>         unsigned int curr_seqno;
>         char pfx_seq[64];
>
> @@ -103,7 +104,7 @@ static void __print_extlog_rcd(const char *pfx,
>                 else
>                         pfx = KERN_ERR;
>         }
> -       curr_seqno = atomic_inc_return(&seqno);
> +       curr_seqno = counter_atomic_inc_return(&seqno);
>         snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}", pfx, curr_seqno);
>         printk("%s""Hardware error detected on CPU%d\n", pfx_seq, cpu);
>         cper_estatus_print(pfx_seq, estatus);
> --
> 2.25.1
>

Re: [RFC PATCH 05/11] drivers/acpi: convert seqno counter_atomic

[Shuah Khan]

On 9/24/20 5:13 AM, Rafael J. Wysocki wrote:
> On Wed, Sep 23, 2020 at 3:44 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
>>
>> 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 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.
>>
>> seqno is a sequence number counter for logging. This counter gets
>> incremented. Unsure if there is a chance of this overflowing. It
>> doesn't look like overflowing causes any problems since it is used
>> to tag the log messages and nothing more.
>>
>> Convert it to use counter_atomic.
>>
>> This conversion doesn't change the oveflow wrap around behavior.

I see typo here. Will fix it.

>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> Both this change and the next patch are fine by me.
> 

Thanks Rafael. Okay to add your Acked-by?

thanks,
-- Shuah

Re: [RFC PATCH 05/11] drivers/acpi: convert seqno counter_atomic

[Rafael J. Wysocki]

On Thu, Sep 24, 2020 at 5:08 PM Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 9/24/20 5:13 AM, Rafael J. Wysocki wrote:
> > On Wed, Sep 23, 2020 at 3:44 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
> >>
> >> 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 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.
> >>
> >> seqno is a sequence number counter for logging. This counter gets
> >> incremented. Unsure if there is a chance of this overflowing. It
> >> doesn't look like overflowing causes any problems since it is used
> >> to tag the log messages and nothing more.
> >>
> >> Convert it to use counter_atomic.
> >>
> >> This conversion doesn't change the oveflow wrap around behavior.
>
> I see typo here. Will fix it.
>
> >>
> >> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> >
> > Both this change and the next patch are fine by me.
> >
>
> Thanks Rafael. Okay to add your Acked-by?

Sure.

Thanks!