[PATCH v2 00/11] Introduce Simple atomic counters

[PATCH v2 00/11] Introduce Simple atomic counters

[Shuah Khan]

This patch series is a result of discussion at the refcount_t BOF
the Linux Plumbers Conference. In this discussion, we 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

[PATCH v2 01/11] counters: Introduce counter_atomic* counters

[Shuah Khan]

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

Re: [PATCH v2 01/11] counters: Introduce counter_atomic* counters

[Greg KH]

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

Re: [PATCH v2 01/11] counters: Introduce counter_atomic* counters

[Kees Cook]

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

Re: [PATCH v2 00/11] Introduce Simple atomic counters

[Kees Cook]

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

Re: [PATCH v2 01/11] counters: Introduce counter_atomic* counters

[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

Re: [PATCH v2 01/11] counters: Introduce counter_atomic* counters

[Kees Cook]

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

Re: [PATCH v2 01/11] counters: Introduce counter_atomic* counters

[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