linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/11] Introduce Simple atomic counters
@ 2020-10-06 20:44 Shuah Khan
  2020-10-06 20:44 ` [PATCH v2 01/11] counters: Introduce counter_atomic* counters Shuah Khan
                   ` (11 more replies)
  0 siblings, 12 replies; 36+ messages in thread
From: Shuah Khan @ 2020-10-06 20:44 UTC (permalink / raw)
  To: corbet, keescook, gregkh, shuah, rafael, johannes, lenb,
	james.morse, tony.luck, bp, arve, tkjos, maco, joel, christian,
	hridya, surenb, minyard, arnd, mchehab, rric
  Cc: Shuah Khan, linux-doc, linux-kernel, linux-kselftest, linux-acpi,
	devel, openipmi-developer, linux-edac

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

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

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

Simple atomic counters api provides interfaces for simple atomic counters
that just count, and don't guard resource lifetimes. Counter 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 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.

Changes since Patch v1
-- Thanks for reviews and reviewed-by, and Acked-by tags. Updated
   the patches with the tags.
-- Addressed Kees's  and Joel's comments:
   1. Removed dec_return interfaces (Patch 1/11)
   2. Removed counter_simple interfaces to be added later with changes
      to drivers that use them (if any) (Patch 1/11)
   3. Comment and Changelogs updates to Patch 2/11

Kees, if this series is good, would you like to take this through your
tree or would you like to take this through mine?

Changes since RFC:
-- Thanks for reviews and reviewed-by, and Acked-by tags. Updated
   the patches with the tags.
-- Addressed Kees's comments:
   1. Non-atomic counters renamed to counter_simple32 and counter_simple64
      to clearly indicate size.
   2. Added warning for counter_simple* usage and it should be used only
      when there is no need for atomicity.
   3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
   4. Renamed counter_atomic_long to counter_atomic64 and it now uses
      atomic64_t ops and indicates size.
   5. Test updated for the API renames.
   6. Added helper functions for test results printing
   7. Verified that the test module compiles in kunit env. and test
      module can be loaded to run the test.
   8. Updated Documentation to reflect the intent to make the API
      restricted so it can never be used to guard object lifetimes
      and state management. I left _return ops for now, inc_return
      is necessary for now as per the discussion we had on this topic.
-- Updated driver patches with API name changes.
-- We discussed if binder counters can be non-atomic. For now I left
   them the same as the RFC patch - using counter_atomic32
-- Unrelated to this patch series:
   The patch series review uncovered improvements could be made to
   test_async_driver_probe and vmw_vmci/vmci_guest. I will track
   these for fixing later.

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

 Documentation/core-api/counters.rst          | 103 +++++++++++
 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                     | 173 +++++++++++++++++++
 lib/Kconfig                                  |  10 ++
 lib/Makefile                                 |   1 +
 lib/test_counters.c                          | 157 +++++++++++++++++
 tools/testing/selftests/lib/Makefile         |   1 +
 tools/testing/selftests/lib/config           |   1 +
 tools/testing/selftests/lib/test_counters.sh |   5 +
 21 files changed, 546 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


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v2 01/11] counters: Introduce counter_atomic* counters
  2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
@ 2020-10-06 20:44 ` Shuah Khan
  2020-10-07  9:04   ` Greg KH
  2020-10-07 18:11   ` Kees Cook
  2020-10-06 20:44 ` [PATCH v2 02/11] selftests:lib:test_counters: add new test for counters Shuah Khan
                   ` (10 subsequent siblings)
  11 siblings, 2 replies; 36+ messages in thread
From: Shuah Khan @ 2020-10-06 20:44 UTC (permalink / raw)
  To: corbet, keescook, gregkh; +Cc: Shuah Khan, linux-doc, linux-kernel

Introduce Simple atomic counters.

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

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

Simple atomic counters api provides interfaces for simple atomic counters
that just count, and don't guard resource lifetimes. Counter 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.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 Documentation/core-api/counters.rst | 103 +++++++++++++++++
 MAINTAINERS                         |   7 ++
 include/linux/counters.h            | 173 ++++++++++++++++++++++++++++
 lib/Kconfig                         |  10 ++
 lib/Makefile                        |   1 +
 lib/test_counters.c                 | 157 +++++++++++++++++++++++++
 6 files changed, 451 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..ba1ce325b639
--- /dev/null
+++ b/Documentation/core-api/counters.rst
@@ -0,0 +1,103 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+======================
+Simple 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 to clearly differentiate atomic_t counters
+from atomic_t usages that guard object lifetimes, hence prone to overflow
+and underflow errors. It allows tools that scan for underflow and overflow
+on atomic_t usages to detect overflow and underflows to scan just the cases
+that are prone to errors.
+
+Simple atomic counters api provides interfaces for simple atomic counters
+that just count, and don't guard resource lifetimes. Counter 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_atomic32_* to guard lifetimes could lead to use-after free
+when it overflows and undefined behavior when used to manage state
+changes and device usage/open states.
+
+Use refcount_t interfaces for guarding resources.
+
+.. warning::
+        Counter 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_atomic32 and counter_atomic64 types use atomic_t and atomic64_t
+underneath to leverage atomic_t api,  providing a small subset of atomic_t
+interfaces necessary to support simple counters. ::
+
+        struct counter_atomic32 { atomic_t cnt; };
+        struct counter_atomic64 { atomic64_t cnt; };
+
+Please see :ref:`Documentation/core-api/atomic_ops.rst <atomic_ops>` for
+information on the Semantics and Behavior of Atomic operations.
+
+.. warning::
+        It is important to keep the ops to a very small subset to ensure
+        that the Counter API will never be used for guarding resource
+        lifetimes and state management.
+
+        inc_return() is added to support current atomic_inc_return()
+        usages and avoid forcing the use of _inc() followed by _read().
+
+Initializers
+------------
+
+Interfaces for initializing counters are write operations which in turn
+invoke their ``ATOMIC_INIT() and atomic_set()`` counterparts ::
+
+        #define COUNTER_ATOMIC_INIT(i)    { .cnt = ATOMIC_INIT(i) }
+        counter_atomic32_set() --> atomic_set()
+
+        static struct counter_atomic32 acnt = COUNTER_ATOMIC_INIT(0);
+        counter_atomic32_set(0);
+
+        static struct counter_atomic64 acnt = COUNTER_ATOMIC_INIT(0);
+        counter_atomic64_set(0);
+
+Increment interface
+-------------------
+
+Increments counter and doesn't return the new counter value. ::
+
+        counter_atomic32_inc() --> atomic_inc()
+        counter_atomic64_inc() --> atomic64_inc()
+
+Increment and return new counter value interface
+------------------------------------------------
+
+Increments counter and returns the new counter value. ::
+
+        counter_atomic32_inc_return() --> atomic_inc_return()
+        counter_atomic64_inc_return() --> atomic64_inc_return()
+
+Decrement interface
+-------------------
+
+Decrements counter and doesn't return the new counter value. ::
+
+        counter_atomic32_dec() --> atomic_dec()
+        counter_atomic64_dec() --> atomic64_dec()
diff --git a/MAINTAINERS b/MAINTAINERS
index 33b27e62ce19..4e82d0ffcab0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15839,6 +15839,13 @@ S:	Maintained
 F:	Documentation/fb/sm712fb.rst
 F:	drivers/video/fbdev/sm712*
 
+SIMPLE ATOMIC and NON-ATOMIC COUNTERS
+M:	Shuah Khan <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..c0c26a13f768
--- /dev/null
+++ b/include/linux/counters.h
@@ -0,0 +1,173 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Interface for simple atomic counters that just count.
+ *
+ * Counter 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 refcount_t interfaces for guarding resources.
+ *
+ * The interface provides:
+ * atomic32 & atomic64 functions:
+ *	increment and no return
+ *	increment and return value
+ *	decrement and no return
+ *	read
+ *	set
+ *
+ * counter_atomic32 unctions leverage/use atomic_t interfaces.
+ * counter_atomic64 functions leverage/use atomic64_t interfaces.
+ * The counter will wrap around to 0 when it overflows.
+ * These interfaces should not be used to guard resource lifetimes.
+ *
+ * Reference and API guide:
+ *	Documentation/core-api/counters.rst for more information.
+ *
+ */
+
+#ifndef __LINUX_COUNTERS_H
+#define __LINUX_COUNTERS_H
+
+#include <linux/atomic.h>
+
+/**
+ * struct counter_atomic32 - Simple atomic counter
+ * @cnt: int
+ *
+ * The counter wraps around to 0, when it overflows. Should not
+ * be used to guard object lifetimes.
+ **/
+struct counter_atomic32 {
+	atomic_t cnt;
+};
+
+#define COUNTER_ATOMIC_INIT(i)		{ .cnt = ATOMIC_INIT(i) }
+
+/*
+ * counter_atomic32_inc() - increment counter value
+ * @cntr: struct counter_atomic32 pointer
+ *
+ */
+static inline void counter_atomic32_inc(struct counter_atomic32 *cntr)
+{
+	atomic_inc(&cntr->cnt);
+}
+
+/*
+ * counter_atomic32_inc_return() - increment counter value and return it
+ * @cntr: struct counter_atomic32 pointer
+ *
+ * Return: returns the new counter value after incrementing it
+ */
+static inline int counter_atomic32_inc_return(struct counter_atomic32 *cntr)
+{
+	return atomic_inc_return(&cntr->cnt);
+}
+
+/*
+ * counter_atomic32_dec() - decrement counter value
+ * @cntr: struct counter_atomic32 pointer
+ *
+ */
+static inline void counter_atomic32_dec(struct counter_atomic32 *cntr)
+{
+	atomic_dec(&cntr->cnt);
+}
+
+/*
+ * counter_atomic32_read() - read counter value
+ * @cntr: struct counter_atomic32 pointer
+ *
+ * Return: return the counter value
+ */
+static inline int counter_atomic32_read(const struct counter_atomic32 *cntr)
+{
+	return atomic_read(&cntr->cnt);
+}
+
+/*
+ * counter_atomic32_set() - set counter value
+ * @cntr: struct counter_atomic32 pointer
+ * @val:  new counter value to set
+ *
+ */
+static inline void
+counter_atomic32_set(struct counter_atomic32 *cntr, int val)
+{
+	atomic_set(&cntr->cnt, val);
+}
+
+#ifdef CONFIG_64BIT
+/*
+ * struct counter_atomic64 - Simple atomic counter
+ * @cnt: atomic64_t
+ *
+ * The counter wraps around to 0, when it overflows. Should not
+ * be used to guard object lifetimes.
+ */
+struct counter_atomic64 {
+	atomic64_t cnt;
+};
+
+/*
+ * counter_atomic64_inc() - increment counter value
+ * @cntr: struct counter_atomic64 pointer
+ *
+ */
+static inline void counter_atomic64_inc(struct counter_atomic64 *cntr)
+{
+	atomic64_inc(&cntr->cnt);
+}
+
+/*
+ * counter_atomic64_inc_return() - increment counter value and return it
+ * @cntr: struct counter_atomic64 pointer
+ *
+ * Return: return the new counter value after incrementing it
+ */
+static inline s64
+counter_atomic64_inc_return(struct counter_atomic64 *cntr)
+{
+	return atomic64_inc_return(&cntr->cnt);
+}
+
+/*
+ * counter_atomic64_dec() - decrement counter value
+ * @cntr: struct counter_atomic64 pointer
+ *
+ */
+static inline void counter_atomic64_dec(
+				struct counter_atomic64 *cntr)
+{
+	atomic64_dec(&cntr->cnt);
+}
+
+/*
+ * counter_atomic64_read() - read counter value
+ * @cntr: struct counter_atomic64 pointer
+ *
+ * Return: return the counter value
+ */
+static inline s64
+counter_atomic64_read(const struct counter_atomic64 *cntr)
+{
+	return atomic64_read(&cntr->cnt);
+}
+
+/*
+ * counter_atomic64_set() - set counter value
+ * @cntr: struct counter_atomic64 pointer
+ * &val:  new counter value to set
+ *
+ */
+static inline void
+counter_atomic64_set(struct counter_atomic64 *cntr, s64 val)
+{
+	atomic64_set(&cntr->cnt, val);
+}
+
+#endif /* CONFIG_64BIT */
+#endif /* __LINUX_COUNTERS_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index b4b98a03ff98..00cb4264bd8b 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 counter functions"
+	default n
+	help
+	   A test module for Simple Atomic counter functions.
+	   A corresponding selftest can be used to test the
+	   counter functions.
+
+	   Select this if you would like to test counters.
+
 endmenu
 
 config GENERIC_IOREMAP
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..c80e812b523e
--- /dev/null
+++ b/lib/test_counters.c
@@ -0,0 +1,157 @@
+// 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>
+
+static inline void
+test_counter_result_print32(char *msg, int start, int end, int expected)
+{
+	pr_info("%s: %d to %d - %s\n",
+		msg, start, end,
+		((expected == end) ? "PASS" : "FAIL"));
+}
+
+
+static void test_counter_atomic32(void)
+{
+	static struct counter_atomic32 acnt = COUNTER_ATOMIC_INIT(0);
+	int start_val = counter_atomic32_read(&acnt);
+	int end_val;
+
+	counter_atomic32_inc(&acnt);
+	end_val = counter_atomic32_read(&acnt);
+	test_counter_result_print32("Test read and increment",
+				    start_val, end_val, start_val+1);
+
+	start_val = counter_atomic32_read(&acnt);
+	end_val = counter_atomic32_inc_return(&acnt);
+	test_counter_result_print32("Test read increment and return",
+				    start_val, end_val, start_val+1);
+
+	start_val = counter_atomic32_read(&acnt);
+	counter_atomic32_dec(&acnt);
+	end_val = counter_atomic32_read(&acnt);
+	test_counter_result_print32("Test read and decrement",
+				    start_val, end_val, start_val-1);
+
+	start_val = counter_atomic32_read(&acnt);
+	counter_atomic32_set(&acnt, INT_MAX);
+	end_val = counter_atomic32_read(&acnt);
+	test_counter_result_print32("Test set", start_val, end_val, INT_MAX);
+}
+
+static void test_counter_atomic32_overflow(void)
+{
+	static struct counter_atomic32 ucnt = COUNTER_ATOMIC_INIT(0);
+	static struct counter_atomic32 ocnt = COUNTER_ATOMIC_INIT(INT_MAX);
+	int start_val;
+	int end_val;
+
+	start_val = counter_atomic32_read(&ucnt);
+	counter_atomic32_dec(&ucnt);
+	end_val = counter_atomic32_read(&ucnt);
+	test_counter_result_print32("Test underflow",
+				    start_val, end_val, start_val-1);
+
+	start_val = counter_atomic32_read(&ocnt);
+	end_val = counter_atomic32_inc_return(&ocnt);
+	test_counter_result_print32("Test overflow",
+				    start_val, end_val, start_val+1);
+}
+
+#ifdef CONFIG_64BIT
+
+static inline void
+test_counter_result_print64(char *msg, s64 start, s64 end, s64 expected)
+{
+	pr_info("%s: %lld to %lld - %s\n",
+		msg, start, end,
+		((expected == end) ? "PASS" : "FAIL"));
+}
+
+static void test_counter_atomic64(void)
+{
+	static struct counter_atomic64 acnt = COUNTER_ATOMIC_INIT(0);
+	s64 start_val = counter_atomic64_read(&acnt);
+	s64 end_val;
+
+	counter_atomic64_inc(&acnt);
+	end_val = counter_atomic64_read(&acnt);
+	test_counter_result_print64("Test read and increment",
+				    start_val, end_val, start_val+1);
+
+	start_val = counter_atomic64_read(&acnt);
+	end_val = counter_atomic64_inc_return(&acnt);
+	test_counter_result_print64("Test read increment and return",
+				    start_val, end_val, start_val+1);
+
+	start_val = counter_atomic64_read(&acnt);
+	counter_atomic64_dec(&acnt);
+	end_val = counter_atomic64_read(&acnt);
+	test_counter_result_print64("Test read and decrement",
+				    start_val, end_val, start_val-1);
+
+	start_val = counter_atomic64_read(&acnt);
+	counter_atomic64_set(&acnt, INT_MAX);
+	end_val = counter_atomic64_read(&acnt);
+	test_counter_result_print64("Test set", start_val, end_val, INT_MAX);
+}
+
+static void test_counter_atomic64_overflow(void)
+{
+	static struct counter_atomic64 ucnt = COUNTER_ATOMIC_INIT(0);
+	static struct counter_atomic64 ocnt = COUNTER_ATOMIC_INIT(INT_MAX);
+	s64 start_val;
+	s64 end_val;
+
+	start_val = counter_atomic64_read(&ucnt);
+	counter_atomic64_dec(&ucnt);
+	end_val = counter_atomic64_read(&ucnt);
+	test_counter_result_print64("Test underflow",
+				    start_val, end_val, start_val-1);
+
+	start_val = counter_atomic64_read(&ocnt);
+	end_val = counter_atomic64_inc_return(&ocnt);
+	test_counter_result_print64("Test overflow",
+				    start_val, end_val, start_val+1);
+}
+
+#endif /* CONFIG_64BIT */
+
+static int __init test_counters_init(void)
+{
+	pr_info("Start counter_atomic32_*() interfaces test\n");
+	test_counter_atomic32();
+	test_counter_atomic32_overflow();
+	pr_info("End counter_atomic32_*() interfaces test\n\n");
+
+#ifdef CONFIG_64BIT
+	pr_info("Start counter_atomic64_*() interfaces test\n");
+	test_counter_atomic64();
+	test_counter_atomic64_overflow();
+	pr_info("End counter_atomic64_*() interfaces test\n\n");
+
+#endif /* CONFIG_64BIT */
+
+	return 0;
+}
+
+module_init(test_counters_init);
+
+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


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 02/11] selftests:lib:test_counters: add new test for counters
  2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
  2020-10-06 20:44 ` [PATCH v2 01/11] counters: Introduce counter_atomic* counters Shuah Khan
@ 2020-10-06 20:44 ` Shuah Khan
  2020-10-07 18:12   ` Kees Cook
  2020-10-06 20:44 ` [PATCH v2 03/11] drivers/base: convert deferred_trigger_count and probe_count to counter_atomic32 Shuah Khan
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 36+ messages in thread
From: Shuah Khan @ 2020-10-06 20:44 UTC (permalink / raw)
  To: shuah, keescook, gregkh; +Cc: Shuah Khan, linux-kernel, linux-kselftest

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

The test module runs tests and prints results in dmesg.

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

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

Simple atomic counters api provides interfaces for simple atomic counters
that just count, and don't guard resource lifetimes. Counter 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 4e82d0ffcab0..26719b8dd48e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15845,6 +15845,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..c8731aef2498
--- /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 Counters interfaces using test_counters
+# kernel module
+$(dirname $0)/../kselftest/module.sh "test_counters" test_counters
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 03/11] drivers/base: convert deferred_trigger_count and probe_count to counter_atomic32
  2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
  2020-10-06 20:44 ` [PATCH v2 01/11] counters: Introduce counter_atomic* counters Shuah Khan
  2020-10-06 20:44 ` [PATCH v2 02/11] selftests:lib:test_counters: add new test for counters Shuah Khan
@ 2020-10-06 20:44 ` Shuah Khan
  2020-10-07 18:13   ` Kees Cook
  2020-10-06 20:44 ` [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count " Shuah Khan
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 36+ messages in thread
From: Shuah Khan @ 2020-10-06 20:44 UTC (permalink / raw)
  To: gregkh, rafael, keescook; +Cc: Shuah Khan, linux-kernel

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

This conversion doesn't change the overflow 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_atomic32.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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..cdb310aca74f 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_atomic32 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_atomic32_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_atomic32 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_atomic32_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_atomic32_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_atomic32_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_atomic32_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_atomic32_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_atomic32_read(&probe_count) == 0);
 	async_synchronize_full();
 }
 EXPORT_SYMBOL_GPL(wait_for_device_probe);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count to counter_atomic32
  2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
                   ` (2 preceding siblings ...)
  2020-10-06 20:44 ` [PATCH v2 03/11] drivers/base: convert deferred_trigger_count and probe_count to counter_atomic32 Shuah Khan
@ 2020-10-06 20:44 ` Shuah Khan
  2020-10-07 18:15   ` Kees Cook
  2020-10-06 20:44 ` [PATCH v2 05/11] drivers/acpi: convert seqno counter_atomic32 Shuah Khan
                   ` (7 subsequent siblings)
  11 siblings, 1 reply; 36+ messages in thread
From: Shuah Khan @ 2020-10-06 20:44 UTC (permalink / raw)
  To: johannes, gregkh, rafael, keescook; +Cc: Shuah Khan, linux-kernel

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

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

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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..59bc48ee44af 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_atomic32 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_atomic32_inc_return(&devcd_count));
 	devcd->devcd_dev.class = &devcd_class;
 
 	if (device_add(&devcd->devcd_dev))
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 05/11] drivers/acpi: convert seqno counter_atomic32
  2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
                   ` (3 preceding siblings ...)
  2020-10-06 20:44 ` [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count " Shuah Khan
@ 2020-10-06 20:44 ` Shuah Khan
  2020-10-07 18:16   ` Kees Cook
  2020-10-06 20:44 ` [PATCH v2 06/11] drivers/acpi/apei: " Shuah Khan
                   ` (6 subsequent siblings)
  11 siblings, 1 reply; 36+ messages in thread
From: Shuah Khan @ 2020-10-06 20:44 UTC (permalink / raw)
  To: rafael, lenb, gregkh, keescook; +Cc: Shuah Khan, linux-acpi, linux-kernel

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

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

Acked-by: Rafael J. Wysocki <rafael@kernel.org>
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..d1e733f15cf5 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_atomic32 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_atomic32_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


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 06/11] drivers/acpi/apei: convert seqno counter_atomic32
  2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
                   ` (4 preceding siblings ...)
  2020-10-06 20:44 ` [PATCH v2 05/11] drivers/acpi: convert seqno counter_atomic32 Shuah Khan
@ 2020-10-06 20:44 ` Shuah Khan
  2020-10-07 18:17   ` Kees Cook
  2020-10-06 20:44 ` [PATCH v2 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32 Shuah Khan
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 36+ messages in thread
From: Shuah Khan @ 2020-10-06 20:44 UTC (permalink / raw)
  To: rafael, james.morse, tony.luck, bp, gregkh, keescook
  Cc: Shuah Khan, linux-acpi, linux-kernel, Borislav Petkov

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

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

Acked-by: Rafael J. Wysocki <rafael@kernel.org>
Acked-by: Borislav Petkov <bp@suse.de>
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..92169436be18 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_atomic32 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_atomic32_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


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32
  2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
                   ` (5 preceding siblings ...)
  2020-10-06 20:44 ` [PATCH v2 06/11] drivers/acpi/apei: " Shuah Khan
@ 2020-10-06 20:44 ` Shuah Khan
  2020-10-07 18:18   ` Kees Cook
  2020-10-09 12:39   ` Christian Brauner
  2020-10-06 20:44 ` [PATCH v2 08/11] drivers/base/test/test_async_driver_probe: convert to use counter_atomic32 Shuah Khan
                   ` (4 subsequent siblings)
  11 siblings, 2 replies; 36+ messages in thread
From: Shuah Khan @ 2020-10-06 20:44 UTC (permalink / raw)
  To: gregkh, arve, tkjos, maco, joel, christian, hridya, surenb, keescook
  Cc: Shuah Khan, devel, linux-kernel

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

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

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
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..52175cd6a62b 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_atomic32 br[_IOC_NR(BR_FAILED_REPLY) + 1];
+	struct counter_atomic32 bc[_IOC_NR(BC_REPLY_SG) + 1];
+	struct counter_atomic32 obj_created[BINDER_STAT_COUNT];
+	struct counter_atomic32 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_atomic32_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_atomic32_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_atomic32_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_atomic32_inc(&binder_stats.bc[_IOC_NR(cmd)]);
+			counter_atomic32_inc(&proc->stats.bc[_IOC_NR(cmd)]);
+			counter_atomic32_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_atomic32_inc(&binder_stats.br[_IOC_NR(cmd)]);
+		counter_atomic32_inc(&proc->stats.br[_IOC_NR(cmd)]);
+		counter_atomic32_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_atomic32_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_atomic32_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_atomic32_read(&stats->obj_created[i]);
+		int deleted = counter_atomic32_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_atomic32_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_atomic32_set(&binder_transaction_log.cur, ~0U);
+	counter_atomic32_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..c77960c01430 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_atomic32 cur;
 	bool full;
 	struct binder_transaction_log_entry entry[32];
 };
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 08/11] drivers/base/test/test_async_driver_probe: convert to use counter_atomic32
  2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
                   ` (6 preceding siblings ...)
  2020-10-06 20:44 ` [PATCH v2 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32 Shuah Khan
@ 2020-10-06 20:44 ` Shuah Khan
  2020-10-07 18:20   ` Kees Cook
  2020-10-06 20:44 ` [PATCH v2 09/11] drivers/char/ipmi: convert stats " Shuah Khan
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 36+ messages in thread
From: Shuah Khan @ 2020-10-06 20:44 UTC (permalink / raw)
  To: gregkh, rafael, keescook; +Cc: Shuah Khan, linux-kernel

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

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

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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..e969c1b09426 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_atomic32 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_atomic32_read(&timeout)) {
 		dev_err(dev, "async probe took too long\n");
-		atomic_inc(&errors);
+		counter_atomic32_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_atomic32_inc(&warnings);
 		}
 
-		atomic_inc(&async_completed);
+		counter_atomic32_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_atomic32_read(&async_completed) != async_id) {
 		pr_err("async events still pending, forcing timeout\n");
-		atomic_inc(&timeout);
+		counter_atomic32_inc(&timeout);
 		err = -ETIMEDOUT;
-	} else if (!atomic_read(&errors) && !atomic_read(&warnings)) {
+	} else if (!counter_atomic32_read(&errors) &&
+		   !counter_atomic32_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_atomic32_inc(&errors);
 	else
 		err = -EINVAL;
 
 	pr_err("Test failed with %d errors and %d warnings\n",
-	       atomic_read(&errors), atomic_read(&warnings));
+	       counter_atomic32_read(&errors),
+	       counter_atomic32_read(&warnings));
 
 	return err;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 09/11] drivers/char/ipmi: convert stats to use counter_atomic32
  2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
                   ` (7 preceding siblings ...)
  2020-10-06 20:44 ` [PATCH v2 08/11] drivers/base/test/test_async_driver_probe: convert to use counter_atomic32 Shuah Khan
@ 2020-10-06 20:44 ` Shuah Khan
  2020-10-07 18:21   ` Kees Cook
  2020-10-06 20:44 ` [PATCH v2 10/11] drivers/misc/vmw_vmci: convert num guest devices counter to counter_atomic32 Shuah Khan
                   ` (2 subsequent siblings)
  11 siblings, 1 reply; 36+ messages in thread
From: Shuah Khan @ 2020-10-06 20:44 UTC (permalink / raw)
  To: minyard, arnd, gregkh, keescook
  Cc: Shuah Khan, openipmi-developer, linux-kernel, Corey Minyard

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

Reviewed-by: Corey Minyard <cminyard@mvista.com>
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..36c0b1be22fb 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_atomic32 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_atomic32_inc(&(intf)->stats[IPMI_STAT_ ## stat])
 #define ipmi_get_stat(intf, stat) \
-	((unsigned int) atomic_read(&(intf)->stats[IPMI_STAT_ ## stat]))
+	((unsigned int) counter_atomic32_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_atomic32_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..0909a3461f05 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_atomic32 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_atomic32_inc(&(smi)->stats[SI_STAT_ ## stat])
 #define smi_get_stat(smi, stat) \
-	((unsigned int) atomic_read(&(smi)->stats[SI_STAT_ ## stat]))
+	((unsigned int) counter_atomic32_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_atomic32_set(&new_smi->stats[i], 0);
 
 	new_smi->interrupt_disabled = true;
 	atomic_set(&new_smi->need_watch, 0);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 10/11] drivers/misc/vmw_vmci: convert num guest devices counter to counter_atomic32
  2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
                   ` (8 preceding siblings ...)
  2020-10-06 20:44 ` [PATCH v2 09/11] drivers/char/ipmi: convert stats " Shuah Khan
@ 2020-10-06 20:44 ` Shuah Khan
  2020-10-07 18:27   ` Kees Cook
  2020-10-06 20:44 ` [PATCH v2 11/11] drivers/edac: convert pci counters " Shuah Khan
  2020-10-07 18:30 ` [PATCH v2 00/11] Introduce Simple atomic counters Kees Cook
  11 siblings, 1 reply; 36+ messages in thread
From: Shuah Khan @ 2020-10-06 20:44 UTC (permalink / raw)
  To: arnd, gregkh, keescook; +Cc: Shuah Khan, linux-kernel

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

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

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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..86ae27b05fc2 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_atomic32 vmci_num_guest_devices = COUNTER_ATOMIC_INIT(0);
 
 bool vmci_guest_code_active(void)
 {
-	return atomic_read(&vmci_num_guest_devices) != 0;
+	return counter_atomic32_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_atomic32_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_atomic32_dec(&vmci_num_guest_devices);
 
 	vmci_qp_guest_endpoints_exit();
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 11/11] drivers/edac: convert pci counters to counter_atomic32
  2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
                   ` (9 preceding siblings ...)
  2020-10-06 20:44 ` [PATCH v2 10/11] drivers/misc/vmw_vmci: convert num guest devices counter to counter_atomic32 Shuah Khan
@ 2020-10-06 20:44 ` Shuah Khan
  2020-10-07 18:28   ` Kees Cook
  2020-10-07 18:30 ` [PATCH v2 00/11] Introduce Simple atomic counters Kees Cook
  11 siblings, 1 reply; 36+ messages in thread
From: Shuah Khan @ 2020-10-06 20:44 UTC (permalink / raw)
  To: bp, mchehab, tony.luck, james.morse, rric, gregkh, keescook
  Cc: Shuah Khan, linux-edac, linux-kernel, Borislav Petkov

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. Convert them to use counter_atomic32.

Overflow will wrap around and reset the counts as was the case prior to
the conversion.

Acked-by: Borislav Petkov <bp@suse.de>
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..797b25a6afc0 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_atomic32 pe_count;
+	struct counter_atomic32 npe_count;
 };
 
 /*
diff --git a/drivers/edac/edac_pci_sysfs.c b/drivers/edac/edac_pci_sysfs.c
index 53042af7262e..d33a726234c0 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_atomic32 pci_parity_count = COUNTER_ATOMIC_INIT(0);
+static struct counter_atomic32 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_atomic32_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_atomic32_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_atomic32_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_atomic32_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_atomic32_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_atomic32_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_atomic32_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_atomic32_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_atomic32_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_atomic32_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_atomic32_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_atomic32_inc(&pci->counters.npe_count);
 
 	if (edac_pci_get_log_npe())
 		edac_pci_printk(pci, KERN_WARNING,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 01/11] counters: Introduce counter_atomic* counters
  2020-10-06 20:44 ` [PATCH v2 01/11] counters: Introduce counter_atomic* counters Shuah Khan
@ 2020-10-07  9:04   ` Greg KH
  2020-10-08 17:18     ` Shuah Khan
  2020-10-07 18:11   ` Kees Cook
  1 sibling, 1 reply; 36+ messages in thread
From: Greg KH @ 2020-10-07  9:04 UTC (permalink / raw)
  To: Shuah Khan; +Cc: corbet, keescook, linux-doc, linux-kernel

On Tue, Oct 06, 2020 at 02:44:32PM -0600, Shuah Khan wrote:
> Introduce Simple atomic counters.
> 
> There are a number of atomic_t usages in the kernel where atomic_t api
> is used strictly for counting and not for managing object lifetime. In
> some cases, atomic_t might not even be needed.
> 
> The purpose of these counters is to clearly differentiate atomic_t
> counters from atomic_t usages that guard object lifetimes, hence prone
> to overflow and underflow errors. It allows tools that scan for underflow
> and overflow on atomic_t usages to detect overflow and underflows to scan
> just the cases that are prone to errors.
> 
> Simple atomic counters api provides interfaces for simple atomic counters
> that just count, and don't guard resource lifetimes. Counter 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.
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  Documentation/core-api/counters.rst | 103 +++++++++++++++++
>  MAINTAINERS                         |   7 ++
>  include/linux/counters.h            | 173 ++++++++++++++++++++++++++++
>  lib/Kconfig                         |  10 ++
>  lib/Makefile                        |   1 +
>  lib/test_counters.c                 | 157 +++++++++++++++++++++++++
>  6 files changed, 451 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..ba1ce325b639
> --- /dev/null
> +++ b/Documentation/core-api/counters.rst
> @@ -0,0 +1,103 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +======================
> +Simple 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 to clearly differentiate atomic_t counters
> +from atomic_t usages that guard object lifetimes, hence prone to overflow
> +and underflow errors. It allows tools that scan for underflow and overflow
> +on atomic_t usages to detect overflow and underflows to scan just the cases
> +that are prone to errors.
> +
> +Simple atomic counters api provides interfaces for simple atomic counters
> +that just count, and don't guard resource lifetimes. Counter 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_atomic32_* to guard lifetimes could lead to use-after free
> +when it overflows and undefined behavior when used to manage state
> +changes and device usage/open states.
> +
> +Use refcount_t interfaces for guarding resources.
> +
> +.. warning::
> +        Counter 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_atomic32 and counter_atomic64 types use atomic_t and atomic64_t
> +underneath to leverage atomic_t api,  providing a small subset of atomic_t
> +interfaces necessary to support simple counters. ::
> +
> +        struct counter_atomic32 { atomic_t cnt; };
> +        struct counter_atomic64 { atomic64_t cnt; };
> +
> +Please see :ref:`Documentation/core-api/atomic_ops.rst <atomic_ops>` for
> +information on the Semantics and Behavior of Atomic operations.
> +
> +.. warning::
> +        It is important to keep the ops to a very small subset to ensure
> +        that the Counter API will never be used for guarding resource
> +        lifetimes and state management.
> +
> +        inc_return() is added to support current atomic_inc_return()
> +        usages and avoid forcing the use of _inc() followed by _read().
> +
> +Initializers
> +------------
> +
> +Interfaces for initializing counters are write operations which in turn
> +invoke their ``ATOMIC_INIT() and atomic_set()`` counterparts ::
> +
> +        #define COUNTER_ATOMIC_INIT(i)    { .cnt = ATOMIC_INIT(i) }
> +        counter_atomic32_set() --> atomic_set()
> +
> +        static struct counter_atomic32 acnt = COUNTER_ATOMIC_INIT(0);
> +        counter_atomic32_set(0);
> +
> +        static struct counter_atomic64 acnt = COUNTER_ATOMIC_INIT(0);
> +        counter_atomic64_set(0);
> +
> +Increment interface
> +-------------------
> +
> +Increments counter and doesn't return the new counter value. ::
> +
> +        counter_atomic32_inc() --> atomic_inc()
> +        counter_atomic64_inc() --> atomic64_inc()
> +
> +Increment and return new counter value interface
> +------------------------------------------------
> +
> +Increments counter and returns the new counter value. ::
> +
> +        counter_atomic32_inc_return() --> atomic_inc_return()
> +        counter_atomic64_inc_return() --> atomic64_inc_return()
> +
> +Decrement interface
> +-------------------
> +
> +Decrements counter and doesn't return the new counter value. ::
> +
> +        counter_atomic32_dec() --> atomic_dec()
> +        counter_atomic64_dec() --> atomic64_dec()
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 33b27e62ce19..4e82d0ffcab0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15839,6 +15839,13 @@ S:	Maintained
>  F:	Documentation/fb/sm712fb.rst
>  F:	drivers/video/fbdev/sm712*
>  
> +SIMPLE ATOMIC and NON-ATOMIC COUNTERS
> +M:	Shuah Khan <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..c0c26a13f768
> --- /dev/null
> +++ b/include/linux/counters.h
> @@ -0,0 +1,173 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Interface for simple atomic counters that just count.
> + *
> + * Counter 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 refcount_t interfaces for guarding resources.
> + *
> + * The interface provides:
> + * atomic32 & atomic64 functions:
> + *	increment and no return
> + *	increment and return value
> + *	decrement and no return
> + *	read
> + *	set
> + *
> + * counter_atomic32 unctions leverage/use atomic_t interfaces.
> + * counter_atomic64 functions leverage/use atomic64_t interfaces.
> + * The counter will wrap around to 0 when it overflows.
> + * These interfaces should not be used to guard resource lifetimes.
> + *
> + * Reference and API guide:
> + *	Documentation/core-api/counters.rst for more information.
> + *
> + */
> +
> +#ifndef __LINUX_COUNTERS_H
> +#define __LINUX_COUNTERS_H
> +
> +#include <linux/atomic.h>
> +
> +/**
> + * struct counter_atomic32 - Simple atomic counter
> + * @cnt: int
> + *
> + * The counter wraps around to 0, when it overflows. Should not
> + * be used to guard object lifetimes.
> + **/
> +struct counter_atomic32 {
> +	atomic_t cnt;
> +};
> +
> +#define COUNTER_ATOMIC_INIT(i)		{ .cnt = ATOMIC_INIT(i) }
> +
> +/*
> + * counter_atomic32_inc() - increment counter value
> + * @cntr: struct counter_atomic32 pointer
> + *
> + */
> +static inline void counter_atomic32_inc(struct counter_atomic32 *cntr)
> +{
> +	atomic_inc(&cntr->cnt);
> +}
> +
> +/*
> + * counter_atomic32_inc_return() - increment counter value and return it
> + * @cntr: struct counter_atomic32 pointer
> + *
> + * Return: returns the new counter value after incrementing it
> + */
> +static inline int counter_atomic32_inc_return(struct counter_atomic32 *cntr)
> +{
> +	return atomic_inc_return(&cntr->cnt);
> +}
> +
> +/*
> + * counter_atomic32_dec() - decrement counter value
> + * @cntr: struct counter_atomic32 pointer
> + *
> + */
> +static inline void counter_atomic32_dec(struct counter_atomic32 *cntr)
> +{
> +	atomic_dec(&cntr->cnt);
> +}
> +
> +/*
> + * counter_atomic32_read() - read counter value
> + * @cntr: struct counter_atomic32 pointer
> + *
> + * Return: return the counter value
> + */
> +static inline int counter_atomic32_read(const struct counter_atomic32 *cntr)
> +{
> +	return atomic_read(&cntr->cnt);
> +}
> +
> +/*
> + * counter_atomic32_set() - set counter value
> + * @cntr: struct counter_atomic32 pointer
> + * @val:  new counter value to set
> + *
> + */
> +static inline void
> +counter_atomic32_set(struct counter_atomic32 *cntr, int val)
> +{
> +	atomic_set(&cntr->cnt, val);
> +}
> +
> +#ifdef CONFIG_64BIT
> +/*
> + * struct counter_atomic64 - Simple atomic counter
> + * @cnt: atomic64_t
> + *
> + * The counter wraps around to 0, when it overflows. Should not
> + * be used to guard object lifetimes.
> + */
> +struct counter_atomic64 {
> +	atomic64_t cnt;
> +};
> +
> +/*
> + * counter_atomic64_inc() - increment counter value
> + * @cntr: struct counter_atomic64 pointer
> + *
> + */
> +static inline void counter_atomic64_inc(struct counter_atomic64 *cntr)
> +{
> +	atomic64_inc(&cntr->cnt);
> +}
> +
> +/*
> + * counter_atomic64_inc_return() - increment counter value and return it
> + * @cntr: struct counter_atomic64 pointer
> + *
> + * Return: return the new counter value after incrementing it
> + */
> +static inline s64
> +counter_atomic64_inc_return(struct counter_atomic64 *cntr)
> +{
> +	return atomic64_inc_return(&cntr->cnt);
> +}
> +
> +/*
> + * counter_atomic64_dec() - decrement counter value
> + * @cntr: struct counter_atomic64 pointer
> + *
> + */
> +static inline void counter_atomic64_dec(
> +				struct counter_atomic64 *cntr)
> +{
> +	atomic64_dec(&cntr->cnt);
> +}
> +
> +/*
> + * counter_atomic64_read() - read counter value
> + * @cntr: struct counter_atomic64 pointer
> + *
> + * Return: return the counter value
> + */
> +static inline s64
> +counter_atomic64_read(const struct counter_atomic64 *cntr)
> +{
> +	return atomic64_read(&cntr->cnt);
> +}
> +
> +/*
> + * counter_atomic64_set() - set counter value
> + * @cntr: struct counter_atomic64 pointer
> + * &val:  new counter value to set
> + *
> + */
> +static inline void
> +counter_atomic64_set(struct counter_atomic64 *cntr, s64 val)
> +{
> +	atomic64_set(&cntr->cnt, val);
> +}
> +
> +#endif /* CONFIG_64BIT */
> +#endif /* __LINUX_COUNTERS_H */
> diff --git a/lib/Kconfig b/lib/Kconfig
> index b4b98a03ff98..00cb4264bd8b 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 counter functions"
> +	default n

Nit, if you end up doing another version, this "default n" isn't needed,
it's the default already :)

Other than that tiny thing, still looks good to me, thanks for doing
this work.

greg k-h

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 01/11] counters: Introduce counter_atomic* counters
  2020-10-06 20:44 ` [PATCH v2 01/11] counters: Introduce counter_atomic* counters Shuah Khan
  2020-10-07  9:04   ` Greg KH
@ 2020-10-07 18:11   ` Kees Cook
  2020-10-07 19:26     ` Shuah Khan
  1 sibling, 1 reply; 36+ messages in thread
From: Kees Cook @ 2020-10-07 18:11 UTC (permalink / raw)
  To: Shuah Khan; +Cc: corbet, gregkh, linux-doc, linux-kernel

On Tue, Oct 06, 2020 at 02:44:32PM -0600, Shuah Khan wrote:
> Introduce Simple atomic counters.
> 
> There are a number of atomic_t usages in the kernel where atomic_t api
> is used strictly for counting and not for managing object lifetime. In
> some cases, atomic_t might not even be needed.
> 
> The purpose of these counters is to clearly differentiate atomic_t
> counters from atomic_t usages that guard object lifetimes, hence prone
> to overflow and underflow errors. It allows tools that scan for underflow
> and overflow on atomic_t usages to detect overflow and underflows to scan
> just the cases that are prone to errors.
> 
> Simple atomic counters api provides interfaces for simple atomic counters
> that just count, and don't guard resource lifetimes. Counter 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.
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  Documentation/core-api/counters.rst | 103 +++++++++++++++++
>  MAINTAINERS                         |   7 ++
>  include/linux/counters.h            | 173 ++++++++++++++++++++++++++++
>  lib/Kconfig                         |  10 ++
>  lib/Makefile                        |   1 +
>  lib/test_counters.c                 | 157 +++++++++++++++++++++++++
>  6 files changed, 451 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..ba1ce325b639
> --- /dev/null
> +++ b/Documentation/core-api/counters.rst
> @@ -0,0 +1,103 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +======================
> +Simple 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 to clearly differentiate atomic_t counters
> +from atomic_t usages that guard object lifetimes, hence prone to overflow
> +and underflow errors. It allows tools that scan for underflow and overflow
> +on atomic_t usages to detect overflow and underflows to scan just the cases
> +that are prone to errors.
> +
> +Simple atomic counters api provides interfaces for simple atomic counters
> +that just count, and don't guard resource lifetimes. Counter 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_atomic32_* to guard lifetimes could lead to use-after free
> +when it overflows and undefined behavior when used to manage state
> +changes and device usage/open states.
> +
> +Use refcount_t interfaces for guarding resources.
> +
> +.. warning::
> +        Counter 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_atomic32 and counter_atomic64 types use atomic_t and atomic64_t
> +underneath to leverage atomic_t api,  providing a small subset of atomic_t
> +interfaces necessary to support simple counters. ::
> +
> +        struct counter_atomic32 { atomic_t cnt; };
> +        struct counter_atomic64 { atomic64_t cnt; };
> +
> +Please see :ref:`Documentation/core-api/atomic_ops.rst <atomic_ops>` for
> +information on the Semantics and Behavior of Atomic operations.
> +
> +.. warning::
> +        It is important to keep the ops to a very small subset to ensure
> +        that the Counter API will never be used for guarding resource
> +        lifetimes and state management.
> +
> +        inc_return() is added to support current atomic_inc_return()
> +        usages and avoid forcing the use of _inc() followed by _read().
> +
> +Initializers
> +------------
> +
> +Interfaces for initializing counters are write operations which in turn
> +invoke their ``ATOMIC_INIT() and atomic_set()`` counterparts ::
> +
> +        #define COUNTER_ATOMIC_INIT(i)    { .cnt = ATOMIC_INIT(i) }
> +        counter_atomic32_set() --> atomic_set()
> +
> +        static struct counter_atomic32 acnt = COUNTER_ATOMIC_INIT(0);
> +        counter_atomic32_set(0);
> +
> +        static struct counter_atomic64 acnt = COUNTER_ATOMIC_INIT(0);
> +        counter_atomic64_set(0);
> +
> +Increment interface
> +-------------------
> +
> +Increments counter and doesn't return the new counter value. ::
> +
> +        counter_atomic32_inc() --> atomic_inc()
> +        counter_atomic64_inc() --> atomic64_inc()
> +
> +Increment and return new counter value interface
> +------------------------------------------------
> +
> +Increments counter and returns the new counter value. ::
> +
> +        counter_atomic32_inc_return() --> atomic_inc_return()
> +        counter_atomic64_inc_return() --> atomic64_inc_return()
> +
> +Decrement interface
> +-------------------
> +
> +Decrements counter and doesn't return the new counter value. ::
> +
> +        counter_atomic32_dec() --> atomic_dec()
> +        counter_atomic64_dec() --> atomic64_dec()
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 33b27e62ce19..4e82d0ffcab0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15839,6 +15839,13 @@ S:	Maintained
>  F:	Documentation/fb/sm712fb.rst
>  F:	drivers/video/fbdev/sm712*
>  
> +SIMPLE ATOMIC and NON-ATOMIC COUNTERS
> +M:	Shuah Khan <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..c0c26a13f768
> --- /dev/null
> +++ b/include/linux/counters.h
> @@ -0,0 +1,173 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Interface for simple atomic counters that just count.
> + *
> + * Counter 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 refcount_t interfaces for guarding resources.
> + *
> + * The interface provides:
> + * atomic32 & atomic64 functions:
> + *	increment and no return
> + *	increment and return value
> + *	decrement and no return
> + *	read
> + *	set
> + *
> + * counter_atomic32 unctions leverage/use atomic_t interfaces.

typo: functions

> + * counter_atomic64 functions leverage/use atomic64_t interfaces.
> + * The counter will wrap around to 0 when it overflows.
> + * These interfaces should not be used to guard resource lifetimes.
> + *
> + * Reference and API guide:
> + *	Documentation/core-api/counters.rst for more information.
> + *
> + */
> +
> +#ifndef __LINUX_COUNTERS_H
> +#define __LINUX_COUNTERS_H
> +
> +#include <linux/atomic.h>
> +
> +/**
> + * struct counter_atomic32 - Simple atomic counter
> + * @cnt: int
> + *
> + * The counter wraps around to 0, when it overflows. Should not
> + * be used to guard object lifetimes.
> + **/
> +struct counter_atomic32 {
> +	atomic_t cnt;
> +};
> +
> +#define COUNTER_ATOMIC_INIT(i)		{ .cnt = ATOMIC_INIT(i) }
> +
> +/*
> + * counter_atomic32_inc() - increment counter value
> + * @cntr: struct counter_atomic32 pointer
> + *
> + */
> +static inline void counter_atomic32_inc(struct counter_atomic32 *cntr)
> +{
> +	atomic_inc(&cntr->cnt);
> +}
> +
> +/*
> + * counter_atomic32_inc_return() - increment counter value and return it
> + * @cntr: struct counter_atomic32 pointer
> + *
> + * Return: returns the new counter value after incrementing it
> + */
> +static inline int counter_atomic32_inc_return(struct counter_atomic32 *cntr)
> +{
> +	return atomic_inc_return(&cntr->cnt);
> +}

So, there's an issue here between the types and the documentation: while
this will eventually wrap around to 0, it will first go through the
negative value space (i.e. INT_MAX + 1 == INT_MIN, INT_MIN < 0).

Current users of atomic_t should already be expecting this, but does it
make sense?

i.e. should the documentation be updated to "wraps around to negative
values", or should the counter API be updated to force the unsigned
value:

+static inline u32 counter_atomic32_inc_return(struct counter_atomic32 *cntr)
+{
+	return (u32)atomic_inc_return(&cntr->cnt);
+}

I see many forcing the return type from atomic_*{read,return}*():

$ git grep -E '\((unsigned|unsigned int|u32)\).*\batomic.*(read|return)' | wc -l
67

My instinct is to say leave it "int" and adjust documentation, which is
the least disruptive, but I am enticed by the desire to make sure a
counter doesn't "misbehave" and go negative when the usage wants it
always positive.

> +static void test_counter_atomic32_overflow(void)
> +{
> +	static struct counter_atomic32 ucnt = COUNTER_ATOMIC_INIT(0);
> +	static struct counter_atomic32 ocnt = COUNTER_ATOMIC_INIT(INT_MAX);
> +	int start_val;
> +	int end_val;
> +
> +	start_val = counter_atomic32_read(&ucnt);
> +	counter_atomic32_dec(&ucnt);
> +	end_val = counter_atomic32_read(&ucnt);

This is testing that counter operations match native int operations,
which seems fine. I wonder if hard-coded values should be added too, to
just more directly map the explicit expectations? E.g. adding a second
test with each:

	test_counter_result_print32("Test underflow (int)",
				    start_val, end_val, start_val-1);
	test_counter_result_print32("Test underflow (-1)",
				    start_val, end_val, -1);


> +
> +	start_val = counter_atomic32_read(&ocnt);
> +	end_val = counter_atomic32_inc_return(&ocnt);

and:

	test_counter_result_print32("Test overflow (int)",
				    start_val, end_val, start_val+1);
	test_counter_result_print32("Test underflow (INT_MIN)",
				    start_val, end_val, INT_MIN);


Otherwise, yes, looks great; thank you!

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/11] selftests:lib:test_counters: add new test for counters
  2020-10-06 20:44 ` [PATCH v2 02/11] selftests:lib:test_counters: add new test for counters Shuah Khan
@ 2020-10-07 18:12   ` Kees Cook
  0 siblings, 0 replies; 36+ messages in thread
From: Kees Cook @ 2020-10-07 18:12 UTC (permalink / raw)
  To: Shuah Khan; +Cc: shuah, gregkh, linux-kernel, linux-kselftest

On Tue, Oct 06, 2020 at 02:44:33PM -0600, Shuah Khan wrote:
> Add a new selftest for testing counter_atomic* Counters API. This test
> load test_counters test modules and unloads.
> 
> The test module runs tests and prints results in dmesg.
> 
> There are a number of atomic_t usages in the kernel where atomic_t api
> is used strictly for counting and not for managing object lifetime. In
> some cases, atomic_t might not even be needed.
> 
> The purpose of these counters is to clearly differentiate atomic_t
> counters from atomic_t usages that guard object lifetimes, hence prone
> to overflow and underflow errors. It allows tools that scan for underflow
> and overflow on atomic_t usages to detect overflow and underflows to scan
> just the cases that are prone to errors.
> 
> Simple atomic counters api provides interfaces for simple atomic counters
> that just count, and don't guard resource lifetimes. Counter 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>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 03/11] drivers/base: convert deferred_trigger_count and probe_count to counter_atomic32
  2020-10-06 20:44 ` [PATCH v2 03/11] drivers/base: convert deferred_trigger_count and probe_count to counter_atomic32 Shuah Khan
@ 2020-10-07 18:13   ` Kees Cook
  0 siblings, 0 replies; 36+ messages in thread
From: Kees Cook @ 2020-10-07 18:13 UTC (permalink / raw)
  To: Shuah Khan; +Cc: gregkh, rafael, linux-kernel

On Tue, Oct 06, 2020 at 02:44:34PM -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_atomic32.
> 
> This conversion doesn't change the overflow 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_atomic32.
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

With the "wrap around to 0" commit log detail fixed, look good:

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count to counter_atomic32
  2020-10-06 20:44 ` [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count " Shuah Khan
@ 2020-10-07 18:15   ` Kees Cook
  2020-10-07 19:33     ` Shuah Khan
  0 siblings, 1 reply; 36+ messages in thread
From: Kees Cook @ 2020-10-07 18:15 UTC (permalink / raw)
  To: Shuah Khan; +Cc: johannes, gregkh, rafael, linux-kernel

On Tue, Oct 06, 2020 at 02:44:35PM -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_atomic32.
> 
> This conversion doesn't change the overflow wrap around behavior.
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

I actually wonder if this should use refcount_t just because it is
designed to be an alway-unique value. It is hard to imagine ever causing
this to overflow, but why not let it be protected?

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 05/11] drivers/acpi: convert seqno counter_atomic32
  2020-10-06 20:44 ` [PATCH v2 05/11] drivers/acpi: convert seqno counter_atomic32 Shuah Khan
@ 2020-10-07 18:16   ` Kees Cook
  0 siblings, 0 replies; 36+ messages in thread
From: Kees Cook @ 2020-10-07 18:16 UTC (permalink / raw)
  To: Shuah Khan; +Cc: rafael, lenb, gregkh, linux-acpi, linux-kernel

On Tue, Oct 06, 2020 at 02:44: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.
> 
> 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_atomic32.
> 
> This conversion doesn't change the overflow wrap around behavior.
> 
> Acked-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

Agreed: this looks like logging only.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 06/11] drivers/acpi/apei: convert seqno counter_atomic32
  2020-10-06 20:44 ` [PATCH v2 06/11] drivers/acpi/apei: " Shuah Khan
@ 2020-10-07 18:17   ` Kees Cook
  0 siblings, 0 replies; 36+ messages in thread
From: Kees Cook @ 2020-10-07 18:17 UTC (permalink / raw)
  To: Shuah Khan
  Cc: rafael, james.morse, tony.luck, bp, gregkh, linux-acpi,
	linux-kernel, Borislav Petkov

On Tue, Oct 06, 2020 at 02:44: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.
> 
> 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_atomic32.
> 
> This conversion doesn't change the overflow wrap around behavior.
> 
> Acked-by: Rafael J. Wysocki <rafael@kernel.org>
> Acked-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

Yup, also logging only, it seems.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32
  2020-10-06 20:44 ` [PATCH v2 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32 Shuah Khan
@ 2020-10-07 18:18   ` Kees Cook
  2020-10-09 12:39   ` Christian Brauner
  1 sibling, 0 replies; 36+ messages in thread
From: Kees Cook @ 2020-10-07 18:18 UTC (permalink / raw)
  To: Shuah Khan
  Cc: gregkh, arve, tkjos, maco, joel, christian, hridya, surenb,
	devel, linux-kernel

On Tue, Oct 06, 2020 at 02:44:38PM -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_atomic32.
> 
> This conversion doesn't change the overflow wrap around behavior.
> 
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

Yup, seems to be stats-only.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 08/11] drivers/base/test/test_async_driver_probe: convert to use counter_atomic32
  2020-10-06 20:44 ` [PATCH v2 08/11] drivers/base/test/test_async_driver_probe: convert to use counter_atomic32 Shuah Khan
@ 2020-10-07 18:20   ` Kees Cook
  0 siblings, 0 replies; 36+ messages in thread
From: Kees Cook @ 2020-10-07 18:20 UTC (permalink / raw)
  To: Shuah Khan; +Cc: gregkh, rafael, linux-kernel

On Tue, Oct 06, 2020 at 02:44: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 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_atomic32.
> 
> This conversion doesn't change the overflow wrap around behavior.
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 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..e969c1b09426 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_atomic32 warnings, errors, timeout, async_completed;

While it's currently harmless, this should likely be using
COUNTER_ATOMIC_INIT(0)s...

>  
>  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_atomic32_read(&timeout)) {
>  		dev_err(dev, "async probe took too long\n");
> -		atomic_inc(&errors);
> +		counter_atomic32_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_atomic32_inc(&warnings);
>  		}
>  
> -		atomic_inc(&async_completed);
> +		counter_atomic32_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_atomic32_read(&async_completed) != async_id) {
>  		pr_err("async events still pending, forcing timeout\n");
> -		atomic_inc(&timeout);
> +		counter_atomic32_inc(&timeout);
>  		err = -ETIMEDOUT;
> -	} else if (!atomic_read(&errors) && !atomic_read(&warnings)) {
> +	} else if (!counter_atomic32_read(&errors) &&
> +		   !counter_atomic32_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_atomic32_inc(&errors);
>  	else
>  		err = -EINVAL;
>  
>  	pr_err("Test failed with %d errors and %d warnings\n",
> -	       atomic_read(&errors), atomic_read(&warnings));
> +	       counter_atomic32_read(&errors),
> +	       counter_atomic32_read(&warnings));
>  
>  	return err;
>  }
> -- 
> 2.25.1
> 

But yeah, this is otherwise just reporting and simple one-time synchronization.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 09/11] drivers/char/ipmi: convert stats to use counter_atomic32
  2020-10-06 20:44 ` [PATCH v2 09/11] drivers/char/ipmi: convert stats " Shuah Khan
@ 2020-10-07 18:21   ` Kees Cook
  0 siblings, 0 replies; 36+ messages in thread
From: Kees Cook @ 2020-10-07 18:21 UTC (permalink / raw)
  To: Shuah Khan
  Cc: minyard, arnd, gregkh, openipmi-developer, linux-kernel, Corey Minyard

On Tue, Oct 06, 2020 at 02:44:40PM -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 for stats are atomic counters. Overflow will
> wrap around and reset the stats and no change with the conversion.
> 
> Convert them to use counter_atomic32.
> 
> Reviewed-by: Corey Minyard <cminyard@mvista.com>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 10/11] drivers/misc/vmw_vmci: convert num guest devices counter to counter_atomic32
  2020-10-06 20:44 ` [PATCH v2 10/11] drivers/misc/vmw_vmci: convert num guest devices counter to counter_atomic32 Shuah Khan
@ 2020-10-07 18:27   ` Kees Cook
  2020-10-08 17:12     ` Shuah Khan
  0 siblings, 1 reply; 36+ messages in thread
From: Kees Cook @ 2020-10-07 18:27 UTC (permalink / raw)
  To: Shuah Khan; +Cc: arnd, gregkh, linux-kernel

On Tue, Oct 06, 2020 at 02:44:41PM -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_atomic32.
> 
> This conversion doesn't change the overflow wrap around behavior.
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

I'm not convinced this isn't both managing lifetime and already buggy.
Specifically, I'm looking at how vmci_guest_code_active() is used --
it's being tested before making calls? Is this safe?

> ---
>  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..86ae27b05fc2 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_atomic32 vmci_num_guest_devices = COUNTER_ATOMIC_INIT(0);
>  
>  bool vmci_guest_code_active(void)
>  {
> -	return atomic_read(&vmci_num_guest_devices) != 0;
> +	return counter_atomic32_read(&vmci_num_guest_devices) != 0;

Shouldn't this be "> 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_atomic32_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_atomic32_dec(&vmci_num_guest_devices);

If there is a bug elsewhere and vmci_guest_remove_device() (or probe)
gets called too many times, shouldn't we protect the rest of this stack
from having vmci_num_guest_devices go negative (and therefore non-zero)?

This really seems like it should be refcount_t to me, though I have no
idea what the races between the dec() and the read() might mean in this
code generally.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 11/11] drivers/edac: convert pci counters to counter_atomic32
  2020-10-06 20:44 ` [PATCH v2 11/11] drivers/edac: convert pci counters " Shuah Khan
@ 2020-10-07 18:28   ` Kees Cook
  0 siblings, 0 replies; 36+ messages in thread
From: Kees Cook @ 2020-10-07 18:28 UTC (permalink / raw)
  To: Shuah Khan
  Cc: bp, mchehab, tony.luck, james.morse, rric, gregkh, linux-edac,
	linux-kernel, Borislav Petkov

On Tue, Oct 06, 2020 at 02:44:42PM -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 for pci counters keep track of pci parity and
> non-parity errors. Convert them to use counter_atomic32.
> 
> Overflow will wrap around and reset the counts as was the case prior to
> the conversion.
> 
> Acked-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

Looks like pure logging. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 00/11] Introduce Simple atomic counters
  2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
                   ` (10 preceding siblings ...)
  2020-10-06 20:44 ` [PATCH v2 11/11] drivers/edac: convert pci counters " Shuah Khan
@ 2020-10-07 18:30 ` Kees Cook
  11 siblings, 0 replies; 36+ messages in thread
From: Kees Cook @ 2020-10-07 18:30 UTC (permalink / raw)
  To: Shuah Khan
  Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
	tony.luck, bp, arve, tkjos, maco, joel, christian, hridya,
	surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
	linux-kselftest, linux-acpi, devel, openipmi-developer,
	linux-edac

On Tue, Oct 06, 2020 at 02:44:31PM -0600, Shuah Khan wrote:
> -- Addressed Kees's  and Joel's comments:
>    1. Removed dec_return interfaces (Patch 1/11)
>    2. Removed counter_simple interfaces to be added later with changes
>       to drivers that use them (if any) (Patch 1/11)
>    3. Comment and Changelogs updates to Patch 2/11

Thanks!

> Kees, if this series is good, would you like to take this through your
> tree or would you like to take this through mine?

I think it's very close! I've sent reviews. Why don't you take this tree
for now? (Originally I thought this was going through Greg's tree since
it was touching a lot of drivers.)

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 01/11] counters: Introduce counter_atomic* counters
  2020-10-07 18:11   ` Kees Cook
@ 2020-10-07 19:26     ` Shuah Khan
  2020-10-07 20:30       ` Kees Cook
  0 siblings, 1 reply; 36+ messages in thread
From: Shuah Khan @ 2020-10-07 19:26 UTC (permalink / raw)
  To: Kees Cook; +Cc: corbet, gregkh, linux-doc, linux-kernel, Shuah Khan

On 10/7/20 12:11 PM, Kees Cook wrote:
> On Tue, Oct 06, 2020 at 02:44:32PM -0600, Shuah Khan wrote:
>> Introduce Simple atomic counters.
>>
>> There are a number of atomic_t usages in the kernel where atomic_t api
>> is used strictly for counting and not for managing object lifetime. In
>> some cases, atomic_t might not even be needed.
>>
>> The purpose of these counters is to clearly differentiate atomic_t
>> counters from atomic_t usages that guard object lifetimes, hence prone
>> to overflow and underflow errors. It allows tools that scan for underflow
>> and overflow on atomic_t usages to detect overflow and underflows to scan
>> just the cases that are prone to errors.
>>
>> Simple atomic counters api provides interfaces for simple atomic counters
>> that just count, and don't guard resource lifetimes. Counter 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.
>>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>> ---
>>   Documentation/core-api/counters.rst | 103 +++++++++++++++++
>>   MAINTAINERS                         |   7 ++
>>   include/linux/counters.h            | 173 ++++++++++++++++++++++++++++
>>   lib/Kconfig                         |  10 ++
>>   lib/Makefile                        |   1 +
>>   lib/test_counters.c                 | 157 +++++++++++++++++++++++++
>>   6 files changed, 451 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..ba1ce325b639
>> --- /dev/null
>> +++ b/Documentation/core-api/counters.rst
>> @@ -0,0 +1,103 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +======================
>> +Simple 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 to clearly differentiate atomic_t counters
>> +from atomic_t usages that guard object lifetimes, hence prone to overflow
>> +and underflow errors. It allows tools that scan for underflow and overflow
>> +on atomic_t usages to detect overflow and underflows to scan just the cases
>> +that are prone to errors.
>> +
>> +Simple atomic counters api provides interfaces for simple atomic counters
>> +that just count, and don't guard resource lifetimes. Counter 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_atomic32_* to guard lifetimes could lead to use-after free
>> +when it overflows and undefined behavior when used to manage state
>> +changes and device usage/open states.
>> +
>> +Use refcount_t interfaces for guarding resources.
>> +
>> +.. warning::
>> +        Counter 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_atomic32 and counter_atomic64 types use atomic_t and atomic64_t
>> +underneath to leverage atomic_t api,  providing a small subset of atomic_t
>> +interfaces necessary to support simple counters. ::
>> +
>> +        struct counter_atomic32 { atomic_t cnt; };
>> +        struct counter_atomic64 { atomic64_t cnt; };
>> +
>> +Please see :ref:`Documentation/core-api/atomic_ops.rst <atomic_ops>` for
>> +information on the Semantics and Behavior of Atomic operations.
>> +
>> +.. warning::
>> +        It is important to keep the ops to a very small subset to ensure
>> +        that the Counter API will never be used for guarding resource
>> +        lifetimes and state management.
>> +
>> +        inc_return() is added to support current atomic_inc_return()
>> +        usages and avoid forcing the use of _inc() followed by _read().
>> +
>> +Initializers
>> +------------
>> +
>> +Interfaces for initializing counters are write operations which in turn
>> +invoke their ``ATOMIC_INIT() and atomic_set()`` counterparts ::
>> +
>> +        #define COUNTER_ATOMIC_INIT(i)    { .cnt = ATOMIC_INIT(i) }
>> +        counter_atomic32_set() --> atomic_set()
>> +
>> +        static struct counter_atomic32 acnt = COUNTER_ATOMIC_INIT(0);
>> +        counter_atomic32_set(0);
>> +
>> +        static struct counter_atomic64 acnt = COUNTER_ATOMIC_INIT(0);
>> +        counter_atomic64_set(0);
>> +
>> +Increment interface
>> +-------------------
>> +
>> +Increments counter and doesn't return the new counter value. ::
>> +
>> +        counter_atomic32_inc() --> atomic_inc()
>> +        counter_atomic64_inc() --> atomic64_inc()
>> +
>> +Increment and return new counter value interface
>> +------------------------------------------------
>> +
>> +Increments counter and returns the new counter value. ::
>> +
>> +        counter_atomic32_inc_return() --> atomic_inc_return()
>> +        counter_atomic64_inc_return() --> atomic64_inc_return()
>> +
>> +Decrement interface
>> +-------------------
>> +
>> +Decrements counter and doesn't return the new counter value. ::
>> +
>> +        counter_atomic32_dec() --> atomic_dec()
>> +        counter_atomic64_dec() --> atomic64_dec()
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 33b27e62ce19..4e82d0ffcab0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15839,6 +15839,13 @@ S:	Maintained
>>   F:	Documentation/fb/sm712fb.rst
>>   F:	drivers/video/fbdev/sm712*
>>   
>> +SIMPLE ATOMIC and NON-ATOMIC COUNTERS
>> +M:	Shuah Khan <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..c0c26a13f768
>> --- /dev/null
>> +++ b/include/linux/counters.h
>> @@ -0,0 +1,173 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Interface for simple atomic counters that just count.
>> + *
>> + * Counter 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 refcount_t interfaces for guarding resources.
>> + *
>> + * The interface provides:
>> + * atomic32 & atomic64 functions:
>> + *	increment and no return
>> + *	increment and return value
>> + *	decrement and no return
>> + *	read
>> + *	set
>> + *
>> + * counter_atomic32 unctions leverage/use atomic_t interfaces.
> 
> typo: functions

Thanks for the catch.

> 
>> + * counter_atomic64 functions leverage/use atomic64_t interfaces.
>> + * The counter will wrap around to 0 when it overflows.
>> + * These interfaces should not be used to guard resource lifetimes.
>> + *
>> + * Reference and API guide:
>> + *	Documentation/core-api/counters.rst for more information.
>> + *
>> + */
>> +
>> +#ifndef __LINUX_COUNTERS_H
>> +#define __LINUX_COUNTERS_H
>> +
>> +#include <linux/atomic.h>
>> +
>> +/**
>> + * struct counter_atomic32 - Simple atomic counter
>> + * @cnt: int
>> + *
>> + * The counter wraps around to 0, when it overflows. Should not
>> + * be used to guard object lifetimes.
>> + **/
>> +struct counter_atomic32 {
>> +	atomic_t cnt;
>> +};
>> +
>> +#define COUNTER_ATOMIC_INIT(i)		{ .cnt = ATOMIC_INIT(i) }
>> +
>> +/*
>> + * counter_atomic32_inc() - increment counter value
>> + * @cntr: struct counter_atomic32 pointer
>> + *
>> + */
>> +static inline void counter_atomic32_inc(struct counter_atomic32 *cntr)
>> +{
>> +	atomic_inc(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic32_inc_return() - increment counter value and return it
>> + * @cntr: struct counter_atomic32 pointer
>> + *
>> + * Return: returns the new counter value after incrementing it
>> + */
>> +static inline int counter_atomic32_inc_return(struct counter_atomic32 *cntr)
>> +{
>> +	return atomic_inc_return(&cntr->cnt);
>> +}
> 
> So, there's an issue here between the types and the documentation: while
> this will eventually wrap around to 0, it will first go through the
> negative value space (i.e. INT_MAX + 1 == INT_MIN, INT_MIN < 0).
> 

Right. It does go through INT_MIN state before it wraps around.

> Current users of atomic_t should already be expecting this, but does it
> make sense?
> 
> i.e. should the documentation be updated to "wraps around to negative
> values", or should the counter API be updated to force the unsigned
> value:
> 
> +static inline u32 counter_atomic32_inc_return(struct counter_atomic32 *cntr)
> +{
> +	return (u32)atomic_inc_return(&cntr->cnt);
> +}
> 
> I see many forcing the return type from atomic_*{read,return}*():
> 
> $ git grep -E '\((unsigned|unsigned int|u32)\).*\batomic.*(read|return)' | wc -l
> 67
> 
> My instinct is to say leave it "int" and adjust documentation, which is
> the least disruptive, but I am enticed by the desire to make sure a
> counter doesn't "misbehave" and go negative when the usage wants it
> always positive.
> 

I would recommend leaving it as "int". Changing the API to unsigned has
other ramifications and cascading changes.

My quick search shows me there are 612 atomic_inc_return usages and
14 out of them are forcing the return type from int to u32.

For atomic_read the numbers are 51 out of 5833 forcing u32. We have
couple of options:

1. Update the documentation since we have more cases where
    int is just fine.
2. Add counter_atomic32_inc_return_u32() variant to cover these few
    cases that are forcing the return.

I recommend going with option 1 with Documentation update and add
option 2 when we convert one of these 60+.

>> +static void test_counter_atomic32_overflow(void)
>> +{
>> +	static struct counter_atomic32 ucnt = COUNTER_ATOMIC_INIT(0);
>> +	static struct counter_atomic32 ocnt = COUNTER_ATOMIC_INIT(INT_MAX);
>> +	int start_val;
>> +	int end_val;
>> +
>> +	start_val = counter_atomic32_read(&ucnt);
>> +	counter_atomic32_dec(&ucnt);
>> +	end_val = counter_atomic32_read(&ucnt);
> 
> This is testing that counter operations match native int operations,
> which seems fine. I wonder if hard-coded values should be added too, to
> just more directly map the explicit expectations? E.g. adding a second
> test with each:
> 
> 	test_counter_result_print32("Test underflow (int)",
> 				    start_val, end_val, start_val-1);
> 	test_counter_result_print32("Test underflow (-1)",
> 				    start_val, end_val, -1);
> 
> 

Yeah. I can add that.

>> +
>> +	start_val = counter_atomic32_read(&ocnt);
>> +	end_val = counter_atomic32_inc_return(&ocnt);
> 
> and:
> 
> 	test_counter_result_print32("Test overflow (int)",
> 				    start_val, end_val, start_val+1);
> 	test_counter_result_print32("Test underflow (INT_MIN)",
> 				    start_val, end_val, INT_MIN);
> 

Sure.

> 
> Otherwise, yes, looks great; thank you!
> 

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count to counter_atomic32
  2020-10-07 18:15   ` Kees Cook
@ 2020-10-07 19:33     ` Shuah Khan
  2020-10-07 19:38       ` Johannes Berg
  0 siblings, 1 reply; 36+ messages in thread
From: Shuah Khan @ 2020-10-07 19:33 UTC (permalink / raw)
  To: Kees Cook, gregkh; +Cc: johannes, rafael, linux-kernel, Shuah Khan

On 10/7/20 12:15 PM, Kees Cook wrote:
> On Tue, Oct 06, 2020 at 02:44:35PM -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_atomic32.
>>
>> This conversion doesn't change the overflow wrap around behavior.
>>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> I actually wonder if this should use refcount_t just because it is
> designed to be an alway-unique value. It is hard to imagine ever causing
> this to overflow, but why not let it be protected?
> 

This is one of the cases where devcd_count doesn't guard lifetimes,
however if it ever overflows, refcount_t is a better choice.

If we decide refcount_t is a better choice, I can drop this patch
and send refcount_t conversion patch instead.

Greg! Any thoughts on refcount_t for this being a better choice?


thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count to counter_atomic32
  2020-10-07 19:33     ` Shuah Khan
@ 2020-10-07 19:38       ` Johannes Berg
  2020-10-07 19:59         ` Shuah Khan
  2020-10-07 20:43         ` Kees Cook
  0 siblings, 2 replies; 36+ messages in thread
From: Johannes Berg @ 2020-10-07 19:38 UTC (permalink / raw)
  To: Shuah Khan, Kees Cook, gregkh; +Cc: rafael, linux-kernel

On Wed, 2020-10-07 at 13:33 -0600, Shuah Khan wrote:
> On 10/7/20 12:15 PM, Kees Cook wrote:
> > On Tue, Oct 06, 2020 at 02:44:35PM -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_atomic32.
> > > 
> > > This conversion doesn't change the overflow wrap around behavior.
> > > 
> > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> > 
> > I actually wonder if this should use refcount_t just because it is
> > designed to be an alway-unique value. It is hard to imagine ever causing
> > this to overflow, but why not let it be protected?
> > 
> 
> This is one of the cases where devcd_count doesn't guard lifetimes,
> however if it ever overflows, refcount_t is a better choice.
> 
> If we decide refcount_t is a better choice, I can drop this patch
> and send refcount_t conversion patch instead.
> 
> Greg! Any thoughts on refcount_t for this being a better choice?

I'm not Greg, but ... there's a 5 minute timeout. So in order to cause a
clash you'd have to manage to overflow the counter within a 5 minute
interval, otherwise you can actually reuse the numbers starting again
from 0 without any ill effect.

And even if you *do* manage to overflow it quickly enough it'll just
fail device_add() and error out, and nothing happens.

So I think it's fairly much pointless to think about protecting against
some kind of overflows. It's just trying to get a "temporarily unique
ID" here, could be doing anything else instead, but most other things
would require bigger data structures and/or (higher level) locking.

OTOH, if you *do* somehow create that many core dumps (huge uptimes and
extremely frequent crashes?) it seems like refcount_t would be a bad
choice because it saturates, and then you can only do one more dump per
5 minutes? Or maybe that's a good thing in these ill cases ...

I don't think it'll really happen either way :)

johannes


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count to counter_atomic32
  2020-10-07 19:38       ` Johannes Berg
@ 2020-10-07 19:59         ` Shuah Khan
  2020-10-07 20:43         ` Kees Cook
  1 sibling, 0 replies; 36+ messages in thread
From: Shuah Khan @ 2020-10-07 19:59 UTC (permalink / raw)
  To: Johannes Berg, Kees Cook, gregkh; +Cc: rafael, linux-kernel, Shuah Khan

On 10/7/20 1:38 PM, Johannes Berg wrote:
> On Wed, 2020-10-07 at 13:33 -0600, Shuah Khan wrote:
>> On 10/7/20 12:15 PM, Kees Cook wrote:
>>> On Tue, Oct 06, 2020 at 02:44:35PM -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_atomic32.
>>>>
>>>> This conversion doesn't change the overflow wrap around behavior.
>>>>
>>>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>>
>>> I actually wonder if this should use refcount_t just because it is
>>> designed to be an alway-unique value. It is hard to imagine ever causing
>>> this to overflow, but why not let it be protected?
>>>
>>
>> This is one of the cases where devcd_count doesn't guard lifetimes,
>> however if it ever overflows, refcount_t is a better choice.
>>
>> If we decide refcount_t is a better choice, I can drop this patch
>> and send refcount_t conversion patch instead.
>>
>> Greg! Any thoughts on refcount_t for this being a better choice?
> 
> I'm not Greg, but ... there's a 5 minute timeout. So in order to cause a
> clash you'd have to manage to overflow the counter within a 5 minute
> interval, otherwise you can actually reuse the numbers starting again
> from 0 without any ill effect.
> 
> And even if you *do* manage to overflow it quickly enough it'll just
> fail device_add() and error out, and nothing happens.
> 
> So I think it's fairly much pointless to think about protecting against
> some kind of overflows. It's just trying to get a "temporarily unique
> ID" here, could be doing anything else instead, but most other things
> would require bigger data structures and/or (higher level) locking.
> 
> OTOH, if you *do* somehow create that many core dumps (huge uptimes and
> extremely frequent crashes?) it seems like refcount_t would be a bad
> choice because it saturates, and then you can only do one more dump per
> 5 minutes? Or maybe that's a good thing in these ill cases ...
> 
> I don't think it'll really happen either way :)
> 

I didn't think this could overflow and if it does we might have other
problems.

Thank you taking the time for this detailed analysis. This clarifies the
"very little chance of this counter overflowing and no ill effects".

thanks,
-- Shuah



^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 01/11] counters: Introduce counter_atomic* counters
  2020-10-07 19:26     ` Shuah Khan
@ 2020-10-07 20:30       ` Kees Cook
  0 siblings, 0 replies; 36+ messages in thread
From: Kees Cook @ 2020-10-07 20:30 UTC (permalink / raw)
  To: Shuah Khan; +Cc: corbet, gregkh, linux-doc, linux-kernel

On Wed, Oct 07, 2020 at 01:26:53PM -0600, Shuah Khan wrote:
> On 10/7/20 12:11 PM, Kees Cook wrote:
> > My instinct is to say leave it "int" and adjust documentation, which is
> > the least disruptive, but I am enticed by the desire to make sure a
> > counter doesn't "misbehave" and go negative when the usage wants it
> > always positive.
> > 
> 
> I would recommend leaving it as "int". Changing the API to unsigned has
> other ramifications and cascading changes.
> 
> My quick search shows me there are 612 atomic_inc_return usages and
> 14 out of them are forcing the return type from int to u32.
> 
> For atomic_read the numbers are 51 out of 5833 forcing u32. We have
> couple of options:
> 
> 1. Update the documentation since we have more cases where
>    int is just fine.
> 2. Add counter_atomic32_inc_return_u32() variant to cover these few
>    cases that are forcing the return.
> 
> I recommend going with option 1 with Documentation update and add
> option 2 when we convert one of these 60+.

Agreed: 1 seems best, and then later 2 if it feels justified. :)

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count to counter_atomic32
  2020-10-07 19:38       ` Johannes Berg
  2020-10-07 19:59         ` Shuah Khan
@ 2020-10-07 20:43         ` Kees Cook
  2020-10-08  6:42           ` Johannes Berg
  1 sibling, 1 reply; 36+ messages in thread
From: Kees Cook @ 2020-10-07 20:43 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Shuah Khan, gregkh, rafael, linux-kernel

On Wed, Oct 07, 2020 at 09:38:47PM +0200, Johannes Berg wrote:
> On Wed, 2020-10-07 at 13:33 -0600, Shuah Khan wrote:
> > On 10/7/20 12:15 PM, Kees Cook wrote:
> > > On Tue, Oct 06, 2020 at 02:44:35PM -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_atomic32.
> > > > 
> > > > This conversion doesn't change the overflow wrap around behavior.
> > > > 
> > > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> > > 
> > > I actually wonder if this should use refcount_t just because it is
> > > designed to be an alway-unique value. It is hard to imagine ever causing
> > > this to overflow, but why not let it be protected?
> > > 
> > 
> > This is one of the cases where devcd_count doesn't guard lifetimes,
> > however if it ever overflows, refcount_t is a better choice.
> > 
> > If we decide refcount_t is a better choice, I can drop this patch
> > and send refcount_t conversion patch instead.
> > 
> > Greg! Any thoughts on refcount_t for this being a better choice?
> 
> I'm not Greg, but ... there's a 5 minute timeout. So in order to cause a
> clash you'd have to manage to overflow the counter within a 5 minute
> interval, otherwise you can actually reuse the numbers starting again
> from 0 without any ill effect.

That's not true as far as I can see: there's no reset in here. It's a
global heap variable with function-level visibility (note the "static"),
so it is only ever initialized once:

void dev_coredumpm(struct device *dev, struct module *owner,
                   void *data, size_t datalen, gfp_t gfp,
                   ssize_t (*read)(char *buffer, loff_t offset, size_t count,
                                   void *data, size_t datalen),
                   void (*free)(void *data))
{
        static atomic_t devcd_count = ATOMIC_INIT(0);
	...
        dev_set_name(&devcd->devcd_dev, "devcd%d",
                     atomic_inc_return(&devcd_count));
	...
}

https://godbolt.org/z/T6Wfcj

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count to counter_atomic32
  2020-10-07 20:43         ` Kees Cook
@ 2020-10-08  6:42           ` Johannes Berg
  2020-10-08  7:37             ` Kees Cook
  0 siblings, 1 reply; 36+ messages in thread
From: Johannes Berg @ 2020-10-08  6:42 UTC (permalink / raw)
  To: Kees Cook; +Cc: Shuah Khan, gregkh, rafael, linux-kernel

On Wed, 2020-10-07 at 13:43 -0700, Kees Cook wrote:
> > > > I actually wonder if this should use refcount_t just because it is
> > > > designed to be an alway-unique value. It is hard to imagine ever causing
> > > > this to overflow, but why not let it be protected?
> > > > 
> > > 
> > > This is one of the cases where devcd_count doesn't guard lifetimes,
> > > however if it ever overflows, refcount_t is a better choice.
> > > 
> > > If we decide refcount_t is a better choice, I can drop this patch
> > > and send refcount_t conversion patch instead.
> > > 
> > > Greg! Any thoughts on refcount_t for this being a better choice?
> > 
> > I'm not Greg, but ... there's a 5 minute timeout. So in order to cause a
> > clash you'd have to manage to overflow the counter within a 5 minute
> > interval, otherwise you can actually reuse the numbers starting again
> > from 0 without any ill effect.
> 
> That's not true as far as I can see: there's no reset in here. It's a
> global heap variable with function-level visibility (note the "static"),
> so it is only ever initialized once:

Yes, obviously it is a static variable. You'll note that I also never
claimed anything regarding reset.

What I said was two things (perhaps with too many words :-) ):
 1) each value that we derive from this ever-incrementing (modulo 2^32)
    variable only get used for a limited amount of time (max. 5 minutes)
 2) if you manage to overflow within 5 minutes, then the following
    device_add() will just fail and nothing else/bad will happen

Therefore, there's no problem with wrapping, and IMHO it'd be *better*
than saturating because (1) means that the wrapping almost certainly
doesn't matter, and (2) means that even if you do manage to wrap and
cause a "clash" (what I wrote in the text you quoted) this is entirely
harmless.

OTOH, if you saturate, then - again under the premise of actually
getting there, however unlikely it may be - you are afterwards *always*
hitting (2), regardless of (1), which seems counter-productive given
that (1) means that (2) almost certainly won't happen.

IOW, I disagree with you, and think that counter_atomic_32 is more
appropriate here than refcount_t.

johannes


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count to counter_atomic32
  2020-10-08  6:42           ` Johannes Berg
@ 2020-10-08  7:37             ` Kees Cook
  0 siblings, 0 replies; 36+ messages in thread
From: Kees Cook @ 2020-10-08  7:37 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Shuah Khan, gregkh, rafael, linux-kernel

On Thu, Oct 08, 2020 at 08:42:22AM +0200, Johannes Berg wrote:
>  1) each value that we derive from this ever-incrementing (modulo 2^32)
>     variable only get used for a limited amount of time (max. 5 minutes)

Ah! That's what I misunderstood, the resulting value is only used for 5
minutes. Got it; thanks!

> IOW, I disagree with you, and think that counter_atomic_32 is more
> appropriate here than refcount_t.

I agree now! :)

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 10/11] drivers/misc/vmw_vmci: convert num guest devices counter to counter_atomic32
  2020-10-07 18:27   ` Kees Cook
@ 2020-10-08 17:12     ` Shuah Khan
  0 siblings, 0 replies; 36+ messages in thread
From: Shuah Khan @ 2020-10-08 17:12 UTC (permalink / raw)
  To: Kees Cook; +Cc: arnd, gregkh, linux-kernel, Shuah Khan

On 10/7/20 12:27 PM, Kees Cook wrote:
> On Tue, Oct 06, 2020 at 02:44:41PM -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_atomic32.
>>
>> This conversion doesn't change the overflow wrap around behavior.
>>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> I'm not convinced this isn't both managing lifetime and already buggy.
> Specifically, I'm looking at how vmci_guest_code_active() is used --
> it's being tested before making calls? Is this safe?
> 

It is being used as a flag in the sense that !=0 indicates that
there is a guest instance. This counter value isn't used in any
code paths.

>> ---
>>   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..86ae27b05fc2 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_atomic32 vmci_num_guest_devices = COUNTER_ATOMIC_INIT(0);
>>   
>>   bool vmci_guest_code_active(void)
>>   {
>> -	return atomic_read(&vmci_num_guest_devices) != 0;
>> +	return counter_atomic32_read(&vmci_num_guest_devices) != 0;
> 
> Shouldn't this be "> 0" ?
> 

Correct. > 0 would be the right check here in the context this
counter is being used, however, I don't think this value will
ever get close overflow. Please see below.

>>   }
>>   
>>   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_atomic32_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_atomic32_dec(&vmci_num_guest_devices);
> 
> If there is a bug elsewhere and vmci_guest_remove_device() (or probe)
> gets called too many times, shouldn't we protect the rest of this stack
> from having vmci_num_guest_devices go negative (and therefore non-zero)?
> 

vmci_num_guest_devices overflow/underflow causing stack corruption is
a minor problems compared to what could happen if _probe gets called
many times, considering the number of pci managed devices we would end
up with. In the sequence of things, we would run out of devm resources
to begin with.

vmci_num_guest_devices is used as bool flag really to determine if a
guest instance exists.

This driver has lots of problems that need to be addressed.

> This really seems like it should be refcount_t to me, though I have no
> idea what the races between the dec() and the read() might mean in this
> code generally.
> 

I don't believe so. vmci_num_guest_devices is used as a status as one
of the factors to make decisions. The way it is being used atomic32 is
just fine.

This patch doesn't solve these problems and add any new ones.

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 01/11] counters: Introduce counter_atomic* counters
  2020-10-07  9:04   ` Greg KH
@ 2020-10-08 17:18     ` Shuah Khan
  0 siblings, 0 replies; 36+ messages in thread
From: Shuah Khan @ 2020-10-08 17:18 UTC (permalink / raw)
  To: Greg KH; +Cc: corbet, keescook, linux-doc, linux-kernel, Shuah Khan

On 10/7/20 3:04 AM, Greg KH wrote:
> On Tue, Oct 06, 2020 at 02:44:32PM -0600, Shuah Khan wrote:
>> Introduce Simple atomic counters.
>>
>> There are a number of atomic_t usages in the kernel where atomic_t api
>> is used strictly for counting and not for managing object lifetime. In
>> some cases, atomic_t might not even be needed.
>>
>> The purpose of these counters is to clearly differentiate atomic_t
>> counters from atomic_t usages that guard object lifetimes, hence prone
>> to overflow and underflow errors. It allows tools that scan for underflow
>> and overflow on atomic_t usages to detect overflow and underflows to scan
>> just the cases that are prone to errors.
>>
>> Simple atomic counters api provides interfaces for simple atomic counters
>> that just count, and don't guard resource lifetimes. Counter 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.
>>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>> ---
>>   Documentation/core-api/counters.rst | 103 +++++++++++++++++
>>   MAINTAINERS                         |   7 ++
>>   include/linux/counters.h            | 173 ++++++++++++++++++++++++++++
>>   lib/Kconfig                         |  10 ++
>>   lib/Makefile                        |   1 +
>>   lib/test_counters.c                 | 157 +++++++++++++++++++++++++
>>   6 files changed, 451 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..ba1ce325b639
>> --- /dev/null
>> +++ b/Documentation/core-api/counters.rst
>> @@ -0,0 +1,103 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +======================
>> +Simple 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 to clearly differentiate atomic_t counters
>> +from atomic_t usages that guard object lifetimes, hence prone to overflow
>> +and underflow errors. It allows tools that scan for underflow and overflow
>> +on atomic_t usages to detect overflow and underflows to scan just the cases
>> +that are prone to errors.
>> +
>> +Simple atomic counters api provides interfaces for simple atomic counters
>> +that just count, and don't guard resource lifetimes. Counter 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_atomic32_* to guard lifetimes could lead to use-after free
>> +when it overflows and undefined behavior when used to manage state
>> +changes and device usage/open states.
>> +
>> +Use refcount_t interfaces for guarding resources.
>> +
>> +.. warning::
>> +        Counter 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_atomic32 and counter_atomic64 types use atomic_t and atomic64_t
>> +underneath to leverage atomic_t api,  providing a small subset of atomic_t
>> +interfaces necessary to support simple counters. ::
>> +
>> +        struct counter_atomic32 { atomic_t cnt; };
>> +        struct counter_atomic64 { atomic64_t cnt; };
>> +
>> +Please see :ref:`Documentation/core-api/atomic_ops.rst <atomic_ops>` for
>> +information on the Semantics and Behavior of Atomic operations.
>> +
>> +.. warning::
>> +        It is important to keep the ops to a very small subset to ensure
>> +        that the Counter API will never be used for guarding resource
>> +        lifetimes and state management.
>> +
>> +        inc_return() is added to support current atomic_inc_return()
>> +        usages and avoid forcing the use of _inc() followed by _read().
>> +
>> +Initializers
>> +------------
>> +
>> +Interfaces for initializing counters are write operations which in turn
>> +invoke their ``ATOMIC_INIT() and atomic_set()`` counterparts ::
>> +
>> +        #define COUNTER_ATOMIC_INIT(i)    { .cnt = ATOMIC_INIT(i) }
>> +        counter_atomic32_set() --> atomic_set()
>> +
>> +        static struct counter_atomic32 acnt = COUNTER_ATOMIC_INIT(0);
>> +        counter_atomic32_set(0);
>> +
>> +        static struct counter_atomic64 acnt = COUNTER_ATOMIC_INIT(0);
>> +        counter_atomic64_set(0);
>> +
>> +Increment interface
>> +-------------------
>> +
>> +Increments counter and doesn't return the new counter value. ::
>> +
>> +        counter_atomic32_inc() --> atomic_inc()
>> +        counter_atomic64_inc() --> atomic64_inc()
>> +
>> +Increment and return new counter value interface
>> +------------------------------------------------
>> +
>> +Increments counter and returns the new counter value. ::
>> +
>> +        counter_atomic32_inc_return() --> atomic_inc_return()
>> +        counter_atomic64_inc_return() --> atomic64_inc_return()
>> +
>> +Decrement interface
>> +-------------------
>> +
>> +Decrements counter and doesn't return the new counter value. ::
>> +
>> +        counter_atomic32_dec() --> atomic_dec()
>> +        counter_atomic64_dec() --> atomic64_dec()
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 33b27e62ce19..4e82d0ffcab0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15839,6 +15839,13 @@ S:	Maintained
>>   F:	Documentation/fb/sm712fb.rst
>>   F:	drivers/video/fbdev/sm712*
>>   
>> +SIMPLE ATOMIC and NON-ATOMIC COUNTERS
>> +M:	Shuah Khan <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..c0c26a13f768
>> --- /dev/null
>> +++ b/include/linux/counters.h
>> @@ -0,0 +1,173 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Interface for simple atomic counters that just count.
>> + *
>> + * Counter 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 refcount_t interfaces for guarding resources.
>> + *
>> + * The interface provides:
>> + * atomic32 & atomic64 functions:
>> + *	increment and no return
>> + *	increment and return value
>> + *	decrement and no return
>> + *	read
>> + *	set
>> + *
>> + * counter_atomic32 unctions leverage/use atomic_t interfaces.
>> + * counter_atomic64 functions leverage/use atomic64_t interfaces.
>> + * The counter will wrap around to 0 when it overflows.
>> + * These interfaces should not be used to guard resource lifetimes.
>> + *
>> + * Reference and API guide:
>> + *	Documentation/core-api/counters.rst for more information.
>> + *
>> + */
>> +
>> +#ifndef __LINUX_COUNTERS_H
>> +#define __LINUX_COUNTERS_H
>> +
>> +#include <linux/atomic.h>
>> +
>> +/**
>> + * struct counter_atomic32 - Simple atomic counter
>> + * @cnt: int
>> + *
>> + * The counter wraps around to 0, when it overflows. Should not
>> + * be used to guard object lifetimes.
>> + **/
>> +struct counter_atomic32 {
>> +	atomic_t cnt;
>> +};
>> +
>> +#define COUNTER_ATOMIC_INIT(i)		{ .cnt = ATOMIC_INIT(i) }
>> +
>> +/*
>> + * counter_atomic32_inc() - increment counter value
>> + * @cntr: struct counter_atomic32 pointer
>> + *
>> + */
>> +static inline void counter_atomic32_inc(struct counter_atomic32 *cntr)
>> +{
>> +	atomic_inc(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic32_inc_return() - increment counter value and return it
>> + * @cntr: struct counter_atomic32 pointer
>> + *
>> + * Return: returns the new counter value after incrementing it
>> + */
>> +static inline int counter_atomic32_inc_return(struct counter_atomic32 *cntr)
>> +{
>> +	return atomic_inc_return(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic32_dec() - decrement counter value
>> + * @cntr: struct counter_atomic32 pointer
>> + *
>> + */
>> +static inline void counter_atomic32_dec(struct counter_atomic32 *cntr)
>> +{
>> +	atomic_dec(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic32_read() - read counter value
>> + * @cntr: struct counter_atomic32 pointer
>> + *
>> + * Return: return the counter value
>> + */
>> +static inline int counter_atomic32_read(const struct counter_atomic32 *cntr)
>> +{
>> +	return atomic_read(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic32_set() - set counter value
>> + * @cntr: struct counter_atomic32 pointer
>> + * @val:  new counter value to set
>> + *
>> + */
>> +static inline void
>> +counter_atomic32_set(struct counter_atomic32 *cntr, int val)
>> +{
>> +	atomic_set(&cntr->cnt, val);
>> +}
>> +
>> +#ifdef CONFIG_64BIT
>> +/*
>> + * struct counter_atomic64 - Simple atomic counter
>> + * @cnt: atomic64_t
>> + *
>> + * The counter wraps around to 0, when it overflows. Should not
>> + * be used to guard object lifetimes.
>> + */
>> +struct counter_atomic64 {
>> +	atomic64_t cnt;
>> +};
>> +
>> +/*
>> + * counter_atomic64_inc() - increment counter value
>> + * @cntr: struct counter_atomic64 pointer
>> + *
>> + */
>> +static inline void counter_atomic64_inc(struct counter_atomic64 *cntr)
>> +{
>> +	atomic64_inc(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic64_inc_return() - increment counter value and return it
>> + * @cntr: struct counter_atomic64 pointer
>> + *
>> + * Return: return the new counter value after incrementing it
>> + */
>> +static inline s64
>> +counter_atomic64_inc_return(struct counter_atomic64 *cntr)
>> +{
>> +	return atomic64_inc_return(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic64_dec() - decrement counter value
>> + * @cntr: struct counter_atomic64 pointer
>> + *
>> + */
>> +static inline void counter_atomic64_dec(
>> +				struct counter_atomic64 *cntr)
>> +{
>> +	atomic64_dec(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic64_read() - read counter value
>> + * @cntr: struct counter_atomic64 pointer
>> + *
>> + * Return: return the counter value
>> + */
>> +static inline s64
>> +counter_atomic64_read(const struct counter_atomic64 *cntr)
>> +{
>> +	return atomic64_read(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic64_set() - set counter value
>> + * @cntr: struct counter_atomic64 pointer
>> + * &val:  new counter value to set
>> + *
>> + */
>> +static inline void
>> +counter_atomic64_set(struct counter_atomic64 *cntr, s64 val)
>> +{
>> +	atomic64_set(&cntr->cnt, val);
>> +}
>> +
>> +#endif /* CONFIG_64BIT */
>> +#endif /* __LINUX_COUNTERS_H */
>> diff --git a/lib/Kconfig b/lib/Kconfig
>> index b4b98a03ff98..00cb4264bd8b 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 counter functions"
>> +	default n
> 
> Nit, if you end up doing another version, this "default n" isn't needed,
> it's the default already :)
> 

Looks like I am generating v3 and will fix this one as well.

> Other than that tiny thing, still looks good to me, thanks for doing
> this work.
> 

thanks,
-- Shuah


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32
  2020-10-06 20:44 ` [PATCH v2 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32 Shuah Khan
  2020-10-07 18:18   ` Kees Cook
@ 2020-10-09 12:39   ` Christian Brauner
  1 sibling, 0 replies; 36+ messages in thread
From: Christian Brauner @ 2020-10-09 12:39 UTC (permalink / raw)
  To: Shuah Khan
  Cc: gregkh, arve, tkjos, maco, joel, christian, hridya, surenb,
	keescook, devel, linux-kernel

On Tue, Oct 06, 2020 at 02:44:38PM -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_atomic32.
> 
> This conversion doesn't change the overflow wrap around behavior.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> ---

Thanks!
Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2020-10-09 12:39 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
2020-10-06 20:44 ` [PATCH v2 01/11] counters: Introduce counter_atomic* counters Shuah Khan
2020-10-07  9:04   ` Greg KH
2020-10-08 17:18     ` Shuah Khan
2020-10-07 18:11   ` Kees Cook
2020-10-07 19:26     ` Shuah Khan
2020-10-07 20:30       ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 02/11] selftests:lib:test_counters: add new test for counters Shuah Khan
2020-10-07 18:12   ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 03/11] drivers/base: convert deferred_trigger_count and probe_count to counter_atomic32 Shuah Khan
2020-10-07 18:13   ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count " Shuah Khan
2020-10-07 18:15   ` Kees Cook
2020-10-07 19:33     ` Shuah Khan
2020-10-07 19:38       ` Johannes Berg
2020-10-07 19:59         ` Shuah Khan
2020-10-07 20:43         ` Kees Cook
2020-10-08  6:42           ` Johannes Berg
2020-10-08  7:37             ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 05/11] drivers/acpi: convert seqno counter_atomic32 Shuah Khan
2020-10-07 18:16   ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 06/11] drivers/acpi/apei: " Shuah Khan
2020-10-07 18:17   ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32 Shuah Khan
2020-10-07 18:18   ` Kees Cook
2020-10-09 12:39   ` Christian Brauner
2020-10-06 20:44 ` [PATCH v2 08/11] drivers/base/test/test_async_driver_probe: convert to use counter_atomic32 Shuah Khan
2020-10-07 18:20   ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 09/11] drivers/char/ipmi: convert stats " Shuah Khan
2020-10-07 18:21   ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 10/11] drivers/misc/vmw_vmci: convert num guest devices counter to counter_atomic32 Shuah Khan
2020-10-07 18:27   ` Kees Cook
2020-10-08 17:12     ` Shuah Khan
2020-10-06 20:44 ` [PATCH v2 11/11] drivers/edac: convert pci counters " Shuah Khan
2020-10-07 18:28   ` Kees Cook
2020-10-07 18:30 ` [PATCH v2 00/11] Introduce Simple atomic counters Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).