Introduce Simple atomic and non-atomic counters

Introduce Simple atomic and non-atomic counters.

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

The purpose of these counters is twofold: 1. clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows tools that scan for underflow and overflow on atomic_t usages to detect overflow and underflows to scan just the cases that are prone to errors. 2. provides non-atomic counters for cases where atomic isn't necessary.

Simple atomic and non-atomic counters api provides interfaces for simple atomic and non-atomic counters that just count, and don't guard resource lifetimes. Counters will wrap around to 0 when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.

Using counter_atomic to guard lifetimes could lead to use-after free when it overflows and undefined behavior when used to manage state changes and device usage/open states.

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

---
 Documentation/core-api/counters.rst | 158 +++++++++++++
 MAINTAINERS                         |   7 +
 include/linux/counters.h            | 343 ++++++++++++++++++++++++++++
 lib/Kconfig                         |  10 +
 lib/Makefile                        |   1 +
 lib/test_counters.c                 | 283 +++++++++++++++++++++++

Introduce Simple atomic and non-atomic counters.

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

The purpose of these counters is twofold: 1. clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows tools that scan for underflow and overflow on atomic_t usages to detect overflow and underflows to scan just the cases that are prone to errors. 2. provides non-atomic counters for cases where atomic isn't necessary.

Simple atomic and non-atomic counters api provides interfaces for simple atomic and non-atomic counters that just count, and don't guard resource lifetimes. Counters will wrap around to 0 when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.

Using counter_atomic to guard lifetimes could lead to use-after free when it overflows and undefined behavior when used to manage state changes and device usage/open states.

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

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

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

lib/test_counters.c new file +8 -0

@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Kernel module for testing Counters
+ *
+ * Authors:
+ *	Shuah Khan	<[email protected]>
+ */
+

Quoted 5 lines · Shuah Khan

Introduce Simple atomic and non-atomic counters.

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

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

Quoted 14 lines

The purpose of these counters is twofold: 1. clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows tools that scan for underflow and overflow on atomic_t usages to detect overflow and underflows to scan just the cases that are prone to errors. 2. provides non-atomic counters for cases where atomic isn't necessary.

Simple atomic and non-atomic counters api provides interfaces for simple atomic and non-atomic counters that just count, and don't guard resource lifetimes. Counters will wrap around to 0 when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.

Quoted 5 lines

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

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

Documentation/core-api/counters.rst line 11

Quoted 3 lines

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

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

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

Quoted 35 lines · Kees Cook

Quoted 5 lines · Shuah Khan

Introduce Simple atomic and non-atomic counters.

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

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

Quoted 14 lines

The purpose of these counters is twofold: 1. clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows tools that scan for underflow and overflow on atomic_t usages to detect overflow and underflows to scan just the cases that are prone to errors. 2. provides non-atomic counters for cases where atomic isn't necessary.

Simple atomic and non-atomic counters api provides interfaces for simple atomic and non-atomic counters that just count, and don't guard resource lifetimes. Counters will wrap around to 0 when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.

Quoted 5 lines

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

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

Quoted 11 lines

Documentation/core-api/counters.rst line 11

Quoted 3 lines

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

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

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

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

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

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

Quoted 5 lines · Shuah Khan

Introduce Simple atomic and non-atomic counters.

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

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

Quoted 18 lines

The purpose of these counters is twofold: 1. clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows tools that scan for underflow and overflow on atomic_t usages to detect overflow and underflows to scan just the cases that are prone to errors. 2. provides non-atomic counters for cases where atomic isn't necessary.

Simple atomic and non-atomic counters api provides interfaces for simple atomic and non-atomic counters that just count, and don't guard resource lifetimes. Counters will wrap around to 0 when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.

Using counter_atomic to guard lifetimes could lead to use-after free when it overflows and undefined behavior when used to manage state changes and device usage/open states.

Quoted 1 line

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

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

Documentation/core-api/counters.rst line 11

Quoted 3 lines

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

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

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

Quoted 4 lines

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

+Simple atomic and non-atomic counters api provides interfaces for simple
+atomic and non-atomic counters that just count, and don't guard resource
+lifetimes. Counters will wrap around to 0 when it overflows and should
+not be used to guard resource lifetimes, device usage and open counts

Quoted 8 lines

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

+that control state changes, and pm states.
+
+Using counter_atomic to guard lifetimes could lead to use-after free
+when it overflows and undefined behavior when used to manage state
+changes and device usage/open states.
+
+Use refcnt_t interfaces for guarding resources.

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

Quoted 14 lines

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

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

Documentation/core-api/counters.rst line 54

Quoted 10 lines

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

+
+Atomic counter interfaces
+=========================
+
+counter_atomic and counter_atomic_long types use atomic_t and atomic_long_t
+underneath to leverage atomic_t api,  providing a small subset of atomic_t
+interfaces necessary to support simple counters. ::
+
+        struct counter_atomic { atomic_t cnt; };
+        struct counter_atomic_long { atomic_long_t cnt; };

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

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

or

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

Quoted 15 lines

lib/test_counters.c new file +12 -0

@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Kernel module for testing Counters
+ *
+ * Authors:
+ *	Shuah Khan	<[email protected]>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/counters.h>

lib/test_counters.c line 18

Quoted 6 lines

lib/test_counters.c new file +6 -0

+
+void test_counter_atomic(void)
+{
+	static struct counter_atomic acnt = COUNTER_ATOMIC_INIT(0);
+	int start_val = counter_atomic_read(&acnt);
+	int end_val;

Please build this test using KUnit.

lib/test_counters.c line 218

Quoted 5 lines

lib/test_counters.c new file +5 -0

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

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

Quoted 6 lines · Kees Cook

On Tue, Sep 22, 2020 at 07:43:30PM -0600, Shuah Khan wrote:
I would really like these APIs to be _impossible_ to use for object lifetime management. To that end, I would like to have all of the *_return() functions removed. It should be strictly init, inc, dec, read.

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

*_test, add_negative etc.

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

Quoted 4 lines

Documentation/core-api/counters.rst line 11

Quoted 3 lines

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

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

Quoted 8 lines

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

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

Quoted 7 lines

lib/test_counters.c line 218

Quoted 4 lines

lib/test_counters.c new file +4 -0

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

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

I knew you would ask for helpers. :)

Yeah will do.

Quoted 13 lines · Shuah Khan

Quoted 6 lines · Kees Cook

On Tue, Sep 22, 2020 at 07:43:30PM -0600, Shuah Khan wrote:
I would really like these APIs to be _impossible_ to use for object lifetime management. To that end, I would like to have all of the *_return() functions removed. It should be strictly init, inc, dec, read.

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

*_test, add_negative etc.

Quoted 4 lines

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

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

Quoted 13 lines

Quoted 12 lines

Documentation/core-api/counters.rst line 11

Quoted 3 lines

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

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

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

Quoted 2 lines

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

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

Quoted 14 lines · Kees Cook

Quoted 13 lines · Shuah Khan

Quoted 6 lines · Kees Cook

On Tue, Sep 22, 2020 at 07:43:30PM -0600, Shuah Khan wrote:
I would really like these APIs to be _impossible_ to use for object lifetime management. To that end, I would like to have all of the *_return() functions removed. It should be strictly init, inc, dec, read.

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

*_test, add_negative etc.

Quoted 8 lines

Quoted 4 lines

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

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

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

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

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

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

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

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

deferred_trigger_count gets incremented and read. It doesn't guard object lifetimes, device usage counts, device open counts, and pm states. There is very little chance of this counter overflowing. Convert it to use counter_atomic.

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

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

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

---
 drivers/base/dd.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

drivers/base/dd.c +10 -9

index 857b0a928e8d..f49fe45960ac 100644
@@ -28,6 +28,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
 #include <linux/slab.h>
+#include <linux/counters.h>
 
 #include "base.h"
 #include "power/power.h"

@@ -54,7 +55,7 @@
 static DEFINE_MUTEX(deferred_probe_mutex);
 static LIST_HEAD(deferred_probe_pending_list);
 static LIST_HEAD(deferred_probe_active_list);
-static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
+static struct counter_atomic deferred_trigger_count = COUNTER_ATOMIC_INIT(0);
 static struct dentry *deferred_devices;
 static bool initcalls_done;
 

@@ -173,7 +174,7 @@ static void driver_deferred_probe_trigger(void)
 	 * into the active list so they can be retried by the workqueue
 	 */
 	mutex_lock(&deferred_probe_mutex);
-	atomic_inc(&deferred_trigger_count);
+	counter_atomic_inc(&deferred_trigger_count);
 	list_splice_tail_init(&deferred_probe_pending_list,
 			      &deferred_probe_active_list);
 	mutex_unlock(&deferred_probe_mutex);

@@ -466,7 +467,7 @@ int device_bind_driver(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(device_bind_driver);
 
-static atomic_t probe_count = ATOMIC_INIT(0);
+static struct counter_atomic probe_count = COUNTER_ATOMIC_INIT(0);
 static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
 
 static void driver_deferred_probe_add_trigger(struct device *dev,

@@ -474,7 +475,7 @@ static void driver_deferred_probe_add_trigger(struct device *dev,
 {
 	driver_deferred_probe_add(dev);
 	/* Did a trigger occur while probing? Need to re-trigger if yes */
-	if (local_trigger_count != atomic_read(&deferred_trigger_count))
+	if (local_trigger_count != counter_atomic_read(&deferred_trigger_count))
 		driver_deferred_probe_trigger();
 }
 

@@ -493,7 +494,7 @@ static DEVICE_ATTR_RO(state_synced);
 static int really_probe(struct device *dev, struct device_driver *drv)
 {
 	int ret = -EPROBE_DEFER;
-	int local_trigger_count = atomic_read(&deferred_trigger_count);
+	int local_trigger_count = counter_atomic_read(&deferred_trigger_count);
 	bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
 			   !drv->suppress_bind_attrs;
 

@@ -514,7 +515,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	if (ret)
 		return ret;
 
-	atomic_inc(&probe_count);
+	counter_atomic_inc(&probe_count);
 	pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
 		 drv->bus->name, __func__, drv->name, dev_name(dev));
 	if (!list_empty(&dev->devres_head)) {

@@ -648,7 +649,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	 */
 	ret = 0;
 done:
-	atomic_dec(&probe_count);
+	counter_atomic_dec(&probe_count);
 	wake_up_all(&probe_waitqueue);
 	return ret;
 }

@@ -678,7 +679,7 @@ static int really_probe_debug(struct device *dev, struct device_driver *drv)
  */
 int driver_probe_done(void)
 {
-	int local_probe_count = atomic_read(&probe_count);
+	int local_probe_count = counter_atomic_read(&probe_count);
 
 	pr_debug("%s: probe_count = %d\n", __func__, local_probe_count);
 	if (local_probe_count)

@@ -699,7 +700,7 @@ void wait_for_device_probe(void)
 	flush_work(&deferred_probe_work);
 
 	/* wait for the known devices to complete their probing */
-	wait_event(probe_waitqueue, atomic_read(&probe_count) == 0);
+	wait_event(probe_waitqueue, counter_atomic_read(&probe_count) == 0);
 	async_synchronize_full();
 }
 EXPORT_SYMBOL_GPL(wait_for_device_probe);

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.

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

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

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

---
 drivers/base/devcoredump.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/base/devcoredump.c +3 -2

index e42d0b514384..d719424d1e0b 100644
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/fs.h>
 #include <linux/workqueue.h>
+#include <linux/counters.h>
 
 static struct class devcd_class;
 

@@ -255,7 +256,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
 				   void *data, size_t datalen),
 		   void (*free)(void *data))
 {
-	static atomic_t devcd_count = ATOMIC_INIT(0);
+	static struct counter_atomic devcd_count = COUNTER_ATOMIC_INIT(0);
 	struct devcd_entry *devcd;
 	struct device *existing;
 

@@ -286,7 +287,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
 	device_initialize(&devcd->devcd_dev);
 
 	dev_set_name(&devcd->devcd_dev, "devcd%d",
-		     atomic_inc_return(&devcd_count));
+		     counter_atomic_inc_return(&devcd_count));
 	devcd->devcd_dev.class = &devcd_class;
 
 	if (device_add(&devcd->devcd_dev))

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.

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

Convert it to use counter_atomic.

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

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

---
 drivers/acpi/acpi_extlog.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/acpi/acpi_extlog.c +3 -3

index f138e12b7b82..23b696b7eb14 100644
@@ -12,6 +12,7 @@
 #include <linux/ratelimit.h>
 #include <linux/edac.h>
 #include <linux/ras.h>
+#include <linux/counters.h>
 #include <asm/cpu.h>
 #include <asm/mce.h>
 

@@ -93,7 +94,7 @@ static struct acpi_hest_generic_status *extlog_elog_entry_check(int cpu, int ban
 static void __print_extlog_rcd(const char *pfx,
                               struct acpi_hest_generic_status *estatus, int cpu)
 {
-       static atomic_t seqno;
+       static struct counter_atomic seqno;
        unsigned int curr_seqno;
        char pfx_seq[64];
 

@@ -103,7 +104,7 @@ static void __print_extlog_rcd(const char *pfx,
                else
                        pfx = KERN_ERR;
        }
-       curr_seqno = atomic_inc_return(&seqno);
+       curr_seqno = counter_atomic_inc_return(&seqno);
        snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}", pfx, curr_seqno);
        printk("%s""Hardware error detected on CPU%d\n", pfx_seq, cpu);
        cper_estatus_print(pfx_seq, estatus);
--

2.25.1

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

Quoted 8 lines

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

Convert it to use counter_atomic.

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

Quoted 2 lines

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

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

Quoted 10 lines · Rafael J. Wysocki

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

Quoted 8 lines

Quoted 8 lines

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

Convert it to use counter_atomic.

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

I see typo here. Will fix it.

Quoted 1 line

Quoted 1 line

Quoted 4 lines

Quoted 1 line

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

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

Thanks Rafael. Okay to add your Acked-by?

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

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

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

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

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

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

---
 drivers/android/binder.c          | 41 ++++++++++++++++---------------
 drivers/android/binder_internal.h |  3 ++-
 2 files changed, 23 insertions(+), 21 deletions(-)

drivers/android/binder.c +1 -0

index f936530a19b0..11a0407c46df 100644
@@ -66,6 +66,7 @@
 #include <linux/syscalls.h>
 #include <linux/task_work.h>
 #include <linux/sizes.h>
+#include <linux/counters.h>
 
 #include <uapi/linux/android/binder.h>
 #include <uapi/linux/android/binderfs.h>

@@ -172,22 +173,22 @@ enum binder_stat_types {
 };

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

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

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

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

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

---
 drivers/android/binder.c          | 41 ++++++++++++++++---------------
 drivers/android/binder_internal.h |  3 ++-
 2 files changed, 23 insertions(+), 21 deletions(-)

drivers/android/binder.c +1 -0

index f936530a19b0..11a0407c46df 100644
@@ -66,6 +66,7 @@
 #include <linux/syscalls.h>
 #include <linux/task_work.h>
 #include <linux/sizes.h>
+#include <linux/counters.h>
 
 #include <uapi/linux/android/binder.h>
 #include <uapi/linux/android/binderfs.h>

@@ -172,22 +173,22 @@ enum binder_stat_types {
 };
 

drivers/android/binder.c enum binder_stat_types { · line 179

Quoted 9 lines

drivers/android/binder.c +4 -4

 struct binder_stats {
-	atomic_t br[_IOC_NR(BR_FAILED_REPLY) + 1];
-	atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
-	atomic_t obj_created[BINDER_STAT_COUNT];
-	atomic_t obj_deleted[BINDER_STAT_COUNT];
+	struct counter_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
+	struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
+	struct counter_atomic obj_created[BINDER_STAT_COUNT];
+	struct counter_atomic obj_deleted[BINDER_STAT_COUNT];

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

Quoted 40 lines · Greg KH

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

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

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

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

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

---
 drivers/android/binder.c          | 41 ++++++++++++++++---------------
 drivers/android/binder_internal.h |  3 ++-
 2 files changed, 23 insertions(+), 21 deletions(-)

drivers/android/binder.c +1 -0

index f936530a19b0..11a0407c46df 100644
@@ -66,6 +66,7 @@
 #include <linux/syscalls.h>
 #include <linux/task_work.h>
 #include <linux/sizes.h>
+#include <linux/counters.h>
 
 #include <uapi/linux/android/binder.h>
 #include <uapi/linux/android/binderfs.h>

@@ -172,22 +173,22 @@ enum binder_stat_types {
 };
 

Quoted 13 lines

drivers/android/binder.c enum binder_stat_types { · line 179

Quoted 9 lines

drivers/android/binder.c +4 -4

 struct binder_stats {
-	atomic_t br[_IOC_NR(BR_FAILED_REPLY) + 1];
-	atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
-	atomic_t obj_created[BINDER_STAT_COUNT];
-	atomic_t obj_deleted[BINDER_STAT_COUNT];
+	struct counter_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
+	struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
+	struct counter_atomic obj_created[BINDER_STAT_COUNT];
+	struct counter_atomic obj_deleted[BINDER_STAT_COUNT];

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

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

Quoted 16 lines · Kees Cook

Quoted 14 lines · Greg KH

drivers/android/binder.c enum binder_stat_types { · line 179

Quoted 9 lines · Shuah Khan

struct binder_stats {

drivers/android/binder.c +4 -4

-	atomic_t br[_IOC_NR(BR_FAILED_REPLY) + 1];
-	atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
-	atomic_t obj_created[BINDER_STAT_COUNT];
-	atomic_t obj_deleted[BINDER_STAT_COUNT];
+	struct counter_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
+	struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
+	struct counter_atomic obj_created[BINDER_STAT_COUNT];
+	struct counter_atomic obj_deleted[BINDER_STAT_COUNT];

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

Quoted 2 lines

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

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

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

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

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

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

Convert them to use counter_atomic.

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

Signed-off-by Shuah Khan <[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..def08cd03eb5 100644

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

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

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

Convert it to use counter_atomic.

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

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

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

drivers/misc/vmw_vmci/vmci_guest.c +4 -3

index cc8eeb361fcd..2c21448af730 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_atomic vmci_num_guest_devices = COUNTER_ATOMIC_INIT(0);
 
 bool vmci_guest_code_active(void)
 {
-	return atomic_read(&vmci_num_guest_devices) != 0;
+	return counter_atomic_read(&vmci_num_guest_devices) != 0;
 }
 
 u32 vmci_get_vm_context_id(void)

@@ -624,7 +625,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
 
 	dev_dbg(&pdev->dev, "Registered device\n");
 
-	atomic_inc(&vmci_num_guest_devices);
+	counter_atomic_inc(&vmci_num_guest_devices);
 
 	/* Enable specific interrupt bits. */
 	cmd = VMCI_IMR_DATAGRAM;