Introduce Simple atomic counters

Introduce Simple atomic counters.

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

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

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

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

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

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

new file mode 100644
index 000000000000..ba1ce325b639
@@ -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()

MAINTAINERS +7 -0

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

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

new file mode 100644
index 000000000000..c0c26a13f768
@@ -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 */

lib/Kconfig +0 -0

index b4b98a03ff98..00cb4264bd8b 100644

Quoted 350 lines · 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 <[email protected]>

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

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

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

new file mode 100644
index 000000000000..ba1ce325b639
@@ -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()

MAINTAINERS +7 -0

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

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

new file mode 100644
index 000000000000..c0c26a13f768
@@ -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 */

lib/Kconfig +0 -0

index b4b98a03ff98..00cb4264bd8b 100644
@@ -658,6 +658,16 @@ config OBJAGG
  config STRING_SELFTEST
  	tristate "Test string functions"
 

lib/Kconfig config OBJAGG · line 663

Quoted 3 lines

lib/Kconfig +3 -0

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

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 <[email protected]>

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

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

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

new file mode 100644
index 000000000000..ba1ce325b639
@@ -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()

MAINTAINERS +7 -0

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

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

new file mode 100644
index 000000000000..c0c26a13f768
@@ -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.

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

+ * 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);
+}
+

Quoted 184 lines · 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 <[email protected]>

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

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

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

new file mode 100644
index 000000000000..ba1ce325b639
@@ -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()

MAINTAINERS +7 -0

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

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

new file mode 100644
index 000000000000..c0c26a13f768
@@ -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

include/linux/counters.h line 21

Quoted 6 lines

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

+ *	increment and return value
+ *	decrement and no return
+ *	read
+ *	set
+ *
+ * counter_atomic32 unctions leverage/use atomic_t interfaces.

typo: functions

Quoted 44 lines

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

+ * 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
+ */

include/linux/counters.h line 68

Quoted 4 lines

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

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

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

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

include/linux/counters.h line 78

Quoted 10 lines

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

+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);

include/linux/counters.h line 81

Quoted 3 lines

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

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

Quoted 5 lines · Kees Cook

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

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 <[email protected]>

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

counter_atomic* variables 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 <[email protected]>

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

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

counter_atomic* variables 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 <[email protected]>

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

Quoted 13 lines · Shuah Khan

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

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

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

Quoted 4 lines

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

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

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

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?

Quoted 14 lines · Kees Cook

Quoted 13 lines · Shuah Khan

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

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

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

Quoted 9 lines

Quoted 4 lines

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

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

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

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?

Quoted 25 lines · Shuah Khan

Quoted 14 lines · Kees Cook

Quoted 13 lines · Shuah Khan

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

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

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

Quoted 9 lines

Quoted 4 lines

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

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

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

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?

Quoted 7 lines

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 :)

Quoted 31 lines · Shuah Khan

Quoted 14 lines · Kees Cook

Quoted 13 lines · Shuah Khan

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

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

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

Quoted 9 lines

Quoted 4 lines

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

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

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

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.

Quoted 1 line

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.

Quoted 13 lines

Quoted 12 lines

Quoted 4 lines

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?

Quoted 4 lines

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:

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)

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

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 <[email protected]>

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

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

counter_atomic* variables 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 <[email protected]>

Acked-by Borislav Petkov <[email protected]>

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

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

counter_atomic* variables 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) <[email protected]>

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

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

counter_atomic* variables 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 <[email protected]>

Reviewed-by Joel Fernandes (Google) <[email protected]>

Reviewed-by Kees Cook <[email protected]>

---

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

counter_atomic* variables 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 <[email protected]>

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

---
 drivers/base/test/test_async_driver_probe.c | 23 ++++++++++++---------
 1 file changed, 13 insertions(+), 10 deletions(-)

drivers/base/test/test_async_driver_probe.c +0 -0

index 3bb7beb127a9..e969c1b09426 100644

drivers/base/test/test_async_driver_probe.c +11 -9

 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;
 }

Signature · 1 line

2.25.1

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 <[email protected]>

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

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

counter_atomic* variables 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 <[email protected]>

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

---
 drivers/misc/vmw_vmci/vmci_guest.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/misc/vmw_vmci/vmci_guest.c +1 -0

index cc8eeb361fcd..86ae27b05fc2 100644
@@ -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"

drivers/misc/vmw_vmci/vmci_guest.c +1 -1

 }
 
 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;

Quoted 14 lines · Shuah Khan

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

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

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

Convert it to use counter_atomic32.

Quoted 4 lines

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

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

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

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?

Quoted 23 lines

---
  drivers/misc/vmw_vmci/vmci_guest.c | 9 +++++----
  1 file changed, 5 insertions(+), 4 deletions(-)

drivers/misc/vmw_vmci/vmci_guest.c +2 -1

index cc8eeb361fcd..86ae27b05fc2 100644
@@ -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);
 

drivers/misc/vmw_vmci/vmci_guest.c struct pci_dev *vmci_pdev; · line 76

Quoted 4 lines

drivers/misc/vmw_vmci/vmci_guest.c +1 -1

  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" ?

Quoted 14 lines

drivers/misc/vmw_vmci/vmci_guest.c +1 -1

  }
 
  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)
 

drivers/misc/vmw_vmci/vmci_guest.c static void vmci_guest_remove_device(struct pci_dev *pdev) · line 688

Quoted 4 lines

drivers/misc/vmw_vmci/vmci_guest.c +1 -1

  	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.

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 <[email protected]>

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

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