* [PATCH 01/11] counters: Introduce counter_simple* and counter_atomic* counters
2020-09-25 23:47 [PATCH 00/11] Introduce Simple atomic and non-atomic counters Shuah Khan
@ 2020-09-25 23:47 ` Shuah Khan
2020-09-25 23:52 ` [PATCH 00/11] Introduce Simple atomic and non-atomic counters Kees Cook
` (3 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Shuah Khan @ 2020-09-25 23:47 UTC (permalink / raw)
To: corbet, keescook, gregkh; +Cc: Shuah Khan, linux-doc, linux-kernel
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.
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
Documentation/core-api/counters.rst | 174 ++++++++++++++
MAINTAINERS | 7 +
include/linux/counters.h | 350 ++++++++++++++++++++++++++++
lib/Kconfig | 10 +
lib/Makefile | 1 +
lib/test_counters.c | 276 ++++++++++++++++++++++
6 files changed, 818 insertions(+)
create mode 100644 Documentation/core-api/counters.rst
create mode 100644 include/linux/counters.h
create mode 100644 lib/test_counters.c
diff --git a/Documentation/core-api/counters.rst b/Documentation/core-api/counters.rst
new file mode 100644
index 000000000000..384ad8cc25c6
--- /dev/null
+++ b/Documentation/core-api/counters.rst
@@ -0,0 +1,174 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=====================================
+Simple atomic and non-atomic counters
+=====================================
+
+:Author: Shuah Khan
+
+There are a number of atomic_t usages in the kernel where atomic_t api
+is used strictly for counting and not for managing object lifetime. In
+some cases, atomic_t might not even be needed.
+
+The purpose of these counters is twofold: 1. clearly differentiate
+atomic_t counters from atomic_t usages that guard object lifetimes,
+hence prone to overflow and underflow errors. It allows tools that scan
+for underflow and overflow on atomic_t usages to detect overflow and
+underflows to scan just the cases that are prone to errors. 2. provides
+non-atomic counters for cases where atomic isn't necessary.
+
+Simple atomic and non-atomic counters api provides interfaces for simple
+atomic and non-atomic counters that just count, and don't guard resource
+lifetimes. Counters will wrap around to 0 when it overflows and should
+not be used to guard resource lifetimes, device usage and open counts
+that control state changes, and pm states.
+
+Using counter_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::
+ Counters 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.
+
+.. warning::
+ Using non-atomic counters when atomicity is necessary can
+ result in undefined behavior. If a variable has multiple
+ CPU and thread scope, don't use non-atomic.
+
+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().
+
+ dec_return() is added anticipating a similar need and will be
+ removed if it isn't needed.
+
+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()
+
+Decrement and return new counter value interface
+------------------------------------------------
+
+Decrements counter and returns the new counter value. ::
+
+ counter_atomic32_dec_return() --> atomic_dec_return()
+ counter_atomic64_dec_return() --> atomic64_dec_return()
+
+Non-atomic counter operations
+=============================
+
+counter32 and counter64 types are non-atomic types. ::
+
+ struct counter_simple32 { int cnt; };
+ struct counter_simple64 { s64 cnt; };
+
+Initializers
+------------
+
+Interfaces for initializing counters ::
+
+ #define COUNTER_SIMPLE_INIT(i) { (i) }
+ counter_simple32_set();
+
+ static struct counter_simple32 acnt = COUNTER_SIMPLE_INIT(0);
+ counter_simple32_set(0);
+
+ static struct counter_simple64 acnt = COUNTER_SIMPLE_INIT(0);
+ counter_simple64_set(0);
+
+Increment interface
+-------------------
+
+Increments counter and doesn't return the new counter value. ::
+
+ counter_simple32_inc()
+ counter_simple64_inc()
+
+Increment and return new counter value interface
+------------------------------------------------
+
+Increments counter and returns the new counter value. ::
+
+ counter_simple32_inc_return()
+ counter_simple64_inc_return()
+
+Decrement interface
+-------------------
+
+Decrements counter and doesn't return the new counter value. ::
+
+ counter_simple32_dec()
+ counter_simple64_dec()
+
+Decrement and return new counter value interface
+------------------------------------------------
+
+Decrements counter and returns the new counter value. ::
+
+ counter_simple32_dec_return()
+ counter_simple64_dec_return()
diff --git a/MAINTAINERS b/MAINTAINERS
index d746519253c3..886ee9b5f164 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15841,6 +15841,13 @@ S: Maintained
F: Documentation/fb/sm712fb.rst
F: drivers/video/fbdev/sm712*
+SIMPLE ATOMIC and NON-ATOMIC COUNTERS
+M: Shuah Khan <skhan@linuxfoundation.org>
+L: linux-kernel@vger.kernel.org
+S: Maintained
+F: include/linux/counters.h
+F: lib/test_counters.c
+
SIMPLE FIRMWARE INTERFACE (SFI)
S: Obsolete
W: http://simplefirmware.org/
diff --git a/include/linux/counters.h b/include/linux/counters.h
new file mode 100644
index 000000000000..e8f046242c87
--- /dev/null
+++ b/include/linux/counters.h
@@ -0,0 +1,350 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Interface for simple atomic and non-atomic counters that just count,
+ * and should not be used guard resource lifetimes and device states.
+ * Counters will wrap around to 0 when it overflows and should not be
+ * used to guard resource lifetimes, device usage and open counts that
+ * control state changes, and pm states. Using counter_atomic to guard
+ * lifetimes could lead to use-after free when it overflows and undefined
+ * behavior when used to manage state changes and device usage/open states.
+ *
+ * Use refcount_t interfaces for guarding resources.
+ *
+ * The interface provides:
+ * atomic32 & atomic64 and
+ * non-atomic32 & non-atomic64
+ * increment and no return
+ * increment and return value
+ * decrement and no return
+ * decrement and return value
+ * read
+ * set
+ * functions.
+ *
+ * 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_dec_return() - decrement counter value and return it
+ * @cntr: struct counter_atomic32 pointer
+ *
+ * Return: return the new counter value after decrementing it
+ */
+static inline int counter_atomic32_dec_return(struct counter_atomic32 *cntr)
+{
+ return atomic_dec_return(&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);
+}
+
+/**
+ * struct counter_simple32 - Simple counter
+ * @cnt: int
+ *
+ * The counter wraps around to 0, when it overflows. Should not
+ * be used to guard object lifetimes.
+ */
+struct counter_simple32 {
+ int cnt;
+};
+
+#define COUNTER_SIMPLE_INIT(i) { (i) }
+
+/*
+ * counter_simple32_inc() - increment counter value
+ * @cntr: struct counter_simple32 pointer
+ *
+ */
+static inline void counter_simple32_inc(struct counter_simple32 *cntr)
+{
+ cntr->cnt++;
+}
+
+/*
+ * counter_simple32_inc_return() - increment counter value and return it
+ * @cntr: struct counter_simple32 pointer
+ *
+ * Return: return the new counter value after incrementing it
+ */
+static inline int counter_simple32_inc_return(struct counter_simple32 *cntr)
+{
+ return ++cntr->cnt;
+}
+
+/*
+ * counter_simple32_dec() - decrement counter value
+ * @cntr: struct counter_simple32 pointer
+ *
+ */
+static inline void counter_simple32_dec(struct counter_simple32 *cntr)
+{
+ cntr->cnt--;
+}
+
+/*
+ * counter_simple32_dec_return() - decrement counter value and return it
+ * @cntr: struct counter_simple32 pointer
+ *
+ * Return: return the new counter value after decrementing it
+ */
+static inline int counter_simple32_dec_return(struct counter_simple32 *cntr)
+{
+ return --cntr->cnt;
+}
+
+/*
+ * counter_simple32_read() - read counter value
+ * @cntr: struct counter_simple32 pointer
+ *
+ * Return: return the counter value
+ */
+static inline int counter_simple32_read(const struct counter_simple32 *cntr)
+{
+ return cntr->cnt;
+}
+
+/*
+ * counter_simple32_set() - set counter value
+ * @cntr: struct counter_simple32 pointer
+ * @val: new counter value to set
+ *
+ */
+static inline void counter_simple32_set(struct counter_simple32 *cntr, int val)
+{
+ 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_dec_return() - decrement counter value and return it
+ * @cntr: struct counter_atomic64 pointer
+ *
+ * Return: return the new counter value after decrementing it
+ */
+static inline s64
+counter_atomic64_dec_return(struct counter_atomic64 *cntr)
+{
+ return atomic64_dec_return(&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);
+}
+
+/*
+ * struct counter_simple64 - Simple counter
+ * @cnt: s64
+ *
+ * The counter wraps around to 0, when it overflows. Should not
+ * be used to guard object lifetimes.
+ */
+struct counter_simple64 {
+ s64 cnt;
+};
+
+/*
+ * counter_simple64_inc() - increment counter value
+ * @cntr: struct counter_simple64 pointer
+ *
+ */
+static inline void counter_simple64_inc(struct counter_simple64 *cntr)
+{
+ cntr->cnt++;
+}
+
+/*
+ * counter_simple64_inc_return() - increment counter value and return it
+ * @cntr: struct counter_simple64 pointer
+ *
+ * Return: return the counter value after incrementing it
+ */
+static inline s64 counter_simple64_inc_return(struct counter_simple64 *cntr)
+{
+ return ++cntr->cnt;
+}
+
+/*
+ * counter_simple64_dec() - decrement counter value
+ * @cntr: struct counter64 pointer
+ *
+ */
+static inline void counter_simple64_dec(struct counter_simple64 *cntr)
+{
+ cntr->cnt--;
+}
+
+/*
+ * counter_simple64_dec_return() - decrement counter value
+ * @cntr: counter64 pointer
+ *
+ */
+static inline s64 counter_simple64_dec_return(struct counter_simple64 *cntr)
+{
+ return --cntr->cnt;
+}
+
+/*
+ * counter_simple64_read() - read counter value
+ * @cntr: struct counter_simple64 pointer
+ *
+ * Return: return the new counter value
+ */
+static inline s64 counter_simple64_read(const struct counter_simple64 *cntr)
+{
+ return cntr->cnt;
+}
+
+/*
+ * counter_simple64_set() - set counter value
+ * @cntr: struct counter_simple64 pointer
+ * @val: new counter value to set
+ *
+ */
+static inline void counter_simple64_set(struct counter_simple64 *cntr, s64 val)
+{
+ cntr->cnt = val;
+}
+
+#endif /* CONFIG_64BIT */
+#endif /* __LINUX_COUNTERS_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index b4b98a03ff98..956063ea7aa8 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -658,6 +658,16 @@ config OBJAGG
config STRING_SELFTEST
tristate "Test string functions"
+config TEST_COUNTERS
+ tristate "Test Simple Atomic and Non-atomic counter functions"
+ default n
+ help
+ A test module for Simple Atomic and Non-atomic counter
+ functions. A corresponding selftest can be used to test
+ the counter functions.
+
+ Select this if you would like to test counters.
+
endmenu
config GENERIC_IOREMAP
diff --git a/lib/Makefile b/lib/Makefile
index a4a4c6864f51..95b357bb5f3c 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -99,6 +99,7 @@ obj-$(CONFIG_TEST_BLACKHOLE_DEV) += test_blackhole_dev.o
obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o
obj-$(CONFIG_TEST_LOCKUP) += test_lockup.o
obj-$(CONFIG_TEST_HMM) += test_hmm.o
+obj-$(CONFIG_TEST_COUNTERS) += test_counters.o
#
# CFLAGS for compiling floating point code inside the kernel. x86/Makefile turns
diff --git a/lib/test_counters.c b/lib/test_counters.c
new file mode 100644
index 000000000000..d074130d3872
--- /dev/null
+++ b/lib/test_counters.c
@@ -0,0 +1,276 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Kernel module for testing Counters
+ *
+ * Authors:
+ * Shuah Khan <skhan@linuxfoundation.org>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/counters.h>
+
+static inline void
+test_counter_result_print32(char *msg, int start, int end, int expected)
+{
+ pr_info("%s: %d to %d - %s\n",
+ msg, start, end,
+ ((expected == end) ? "PASS" : "FAIL"));
+}
+
+
+static void test_counter_atomic32(void)
+{
+ static struct counter_atomic32 acnt = COUNTER_ATOMIC_INIT(0);
+ int start_val = counter_atomic32_read(&acnt);
+ int end_val;
+
+ counter_atomic32_inc(&acnt);
+ end_val = counter_atomic32_read(&acnt);
+ test_counter_result_print32("Test read and increment",
+ start_val, end_val, start_val+1);
+
+ start_val = counter_atomic32_read(&acnt);
+ end_val = counter_atomic32_inc_return(&acnt);
+ test_counter_result_print32("Test read increment and return",
+ start_val, end_val, start_val+1);
+
+ start_val = counter_atomic32_read(&acnt);
+ counter_atomic32_dec(&acnt);
+ end_val = counter_atomic32_read(&acnt);
+ test_counter_result_print32("Test read and decrement",
+ start_val, end_val, start_val-1);
+
+ start_val = counter_atomic32_read(&acnt);
+ end_val = counter_atomic32_dec_return(&acnt);
+ test_counter_result_print32("Test read decrement and return",
+ start_val, end_val, start_val-1);
+
+ start_val = counter_atomic32_read(&acnt);
+ counter_atomic32_set(&acnt, INT_MAX);
+ end_val = counter_atomic32_read(&acnt);
+ test_counter_result_print32("Test set", start_val, end_val, INT_MAX);
+}
+
+static void test_counter_atomic32_overflow(void)
+{
+ static struct counter_atomic32 ucnt = COUNTER_ATOMIC_INIT(0);
+ static struct counter_atomic32 ocnt = COUNTER_ATOMIC_INIT(INT_MAX);
+ int start_val;
+ int end_val;
+
+ start_val = counter_atomic32_read(&ucnt);
+ end_val = counter_atomic32_dec_return(&ucnt);
+ test_counter_result_print32("Test underflow",
+ start_val, end_val, start_val-1);
+
+ start_val = counter_atomic32_read(&ocnt);
+ end_val = counter_atomic32_inc_return(&ocnt);
+ test_counter_result_print32("Test overflow",
+ start_val, end_val, start_val+1);
+}
+
+static void test_counter_simple32(void)
+{
+ static struct counter_simple32 acnt = COUNTER_SIMPLE_INIT(0);
+ int start_val = counter_simple32_read(&acnt);
+ int end_val;
+
+ counter_simple32_inc(&acnt);
+ end_val = counter_simple32_read(&acnt);
+ test_counter_result_print32("Test read and increment",
+ start_val, end_val, start_val+1);
+
+ start_val = counter_simple32_read(&acnt);
+ end_val = counter_simple32_inc_return(&acnt);
+ test_counter_result_print32("Test read increment and return",
+ start_val, end_val, start_val+1);
+
+ start_val = counter_simple32_read(&acnt);
+ counter_simple32_dec(&acnt);
+ end_val = counter_simple32_read(&acnt);
+ test_counter_result_print32("Test read and decrement",
+ start_val, end_val, start_val-1);
+
+ start_val = counter_simple32_read(&acnt);
+ end_val = counter_simple32_dec_return(&acnt);
+ test_counter_result_print32("Test read decrement and return",
+ start_val, end_val, start_val-1);
+
+ start_val = counter_simple32_read(&acnt);
+ counter_simple32_set(&acnt, INT_MAX);
+ end_val = counter_simple32_read(&acnt);
+ test_counter_result_print32("Test set", start_val, end_val, INT_MAX);
+}
+
+static void test_counter_simple32_overflow(void)
+{
+ static struct counter_simple32 ucnt = COUNTER_SIMPLE_INIT(0);
+ static struct counter_simple32 ocnt = COUNTER_SIMPLE_INIT(INT_MAX);
+ int start_val;
+ int end_val;
+
+ start_val = counter_simple32_read(&ucnt);
+ end_val = counter_simple32_dec_return(&ucnt);
+ test_counter_result_print32("Test underflow",
+ start_val, end_val, start_val-1);
+
+ start_val = counter_simple32_read(&ocnt);
+ end_val = counter_simple32_inc_return(&ocnt);
+ test_counter_result_print32("Test overflow",
+ start_val, end_val, start_val+1);
+}
+
+#ifdef CONFIG_64BIT
+
+static inline void
+test_counter_result_print64(char *msg, s64 start, s64 end, s64 expected)
+{
+ pr_info("%s: %lld to %lld - %s\n",
+ msg, start, end,
+ ((expected == end) ? "PASS" : "FAIL"));
+}
+
+static void test_counter_atomic64(void)
+{
+ static struct counter_atomic64 acnt = COUNTER_ATOMIC_INIT(0);
+ s64 start_val = counter_atomic64_read(&acnt);
+ s64 end_val;
+
+ counter_atomic64_inc(&acnt);
+ end_val = counter_atomic64_read(&acnt);
+ test_counter_result_print64("Test read and increment",
+ start_val, end_val, start_val+1);
+
+ start_val = counter_atomic64_read(&acnt);
+ end_val = counter_atomic64_inc_return(&acnt);
+ test_counter_result_print64("Test read increment and return",
+ start_val, end_val, start_val+1);
+
+ start_val = counter_atomic64_read(&acnt);
+ counter_atomic64_dec(&acnt);
+ end_val = counter_atomic64_read(&acnt);
+ test_counter_result_print64("Test read and decrement",
+ start_val, end_val, start_val-1);
+
+ start_val = counter_atomic64_read(&acnt);
+ end_val = counter_atomic64_dec_return(&acnt);
+ test_counter_result_print64("Test read decrement and return",
+ start_val, end_val, start_val-1);
+
+ start_val = counter_atomic64_read(&acnt);
+ counter_atomic64_set(&acnt, INT_MAX);
+ end_val = counter_atomic64_read(&acnt);
+ test_counter_result_print64("Test set", start_val, end_val, INT_MAX);
+}
+
+static void test_counter_atomic64_overflow(void)
+{
+ static struct counter_atomic64 ucnt = COUNTER_ATOMIC_INIT(0);
+ static struct counter_atomic64 ocnt = COUNTER_ATOMIC_INIT(INT_MAX);
+ s64 start_val;
+ s64 end_val;
+
+ start_val = counter_atomic64_read(&ucnt);
+ end_val = counter_atomic64_dec_return(&ucnt);
+ test_counter_result_print64("Test underflow",
+ start_val, end_val, start_val-1);
+
+ start_val = counter_atomic64_read(&ocnt);
+ end_val = counter_atomic64_inc_return(&ocnt);
+ test_counter_result_print64("Test overflow",
+ start_val, end_val, start_val+1);
+}
+
+static void test_counter_simple64(void)
+{
+ static struct counter_simple64 acnt = COUNTER_SIMPLE_INIT(0);
+ s64 start_val = counter_simple64_read(&acnt);
+ s64 end_val;
+
+ counter_simple64_inc(&acnt);
+ end_val = counter_simple64_read(&acnt);
+ test_counter_result_print64("Test read and increment",
+ start_val, end_val, start_val+1);
+
+ start_val = counter_simple64_read(&acnt);
+ end_val = counter_simple64_inc_return(&acnt);
+ test_counter_result_print64("Test read increment and return",
+ start_val, end_val, start_val+1);
+
+ start_val = counter_simple64_read(&acnt);
+ counter_simple64_dec(&acnt);
+ end_val = counter_simple64_read(&acnt);
+ test_counter_result_print64("Test read and decrement",
+ start_val, end_val, start_val-1);
+
+ start_val = counter_simple64_read(&acnt);
+ end_val = counter_simple64_dec_return(&acnt);
+ test_counter_result_print64("Test read decrement and return",
+ start_val, end_val, start_val-1);
+
+ start_val = counter_simple64_read(&acnt);
+ counter_simple64_set(&acnt, INT_MAX);
+ end_val = counter_simple64_read(&acnt);
+ test_counter_result_print64("Test set", start_val, end_val, INT_MAX);
+}
+
+static void test_counter_simple64_overflow(void)
+{
+ static struct counter_simple64 ucnt = COUNTER_SIMPLE_INIT(0);
+ static struct counter_simple64 ocnt = COUNTER_SIMPLE_INIT(INT_MAX);
+ s64 start_val;
+ s64 end_val;
+
+ start_val = counter_simple64_read(&ucnt);
+ end_val = counter_simple64_dec_return(&ucnt);
+ test_counter_result_print64("Test underflow",
+ start_val, end_val, start_val-1);
+
+ start_val = counter_simple64_read(&ocnt);
+ end_val = counter_simple64_inc_return(&ocnt);
+ test_counter_result_print64("Test overflow",
+ start_val, end_val, start_val+1);
+}
+
+#endif /* CONFIG_64BIT */
+
+static int __init test_counters_init(void)
+{
+ pr_info("Start counter_atomic32_*() interfaces test\n");
+ test_counter_atomic32();
+ test_counter_atomic32_overflow();
+ pr_info("End counter_atomic32_*() interfaces test\n\n");
+
+ pr_info("Start counter_simple32*() interfaces test\n");
+ test_counter_simple32();
+ test_counter_simple32_overflow();
+ pr_info("End counter_simple32*() interfaces test\n\n");
+
+#ifdef CONFIG_64BIT
+ pr_info("Start counter_atomic64_*() interfaces test\n");
+ test_counter_atomic64();
+ test_counter_atomic64_overflow();
+ pr_info("End counter_atomic64_*() interfaces test\n\n");
+
+ pr_info("Start counter_simple64_*() interfaces test\n");
+ test_counter_simple64();
+ test_counter_simple64_overflow();
+ pr_info("End counter_simple64_*() interfaces test\n\n");
+#endif /* CONFIG_64BIT */
+
+ return 0;
+}
+
+module_init(test_counters_init);
+
+static void __exit test_counters_exit(void)
+{
+ pr_info("exiting.\n");
+}
+
+module_exit(test_counters_exit);
+
+MODULE_AUTHOR("Shuah Khan <skhan@linuxfoundation.org>");
+MODULE_LICENSE("GPL v2");
--
2.25.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-25 23:47 [PATCH 00/11] Introduce Simple atomic and non-atomic counters Shuah Khan
2020-09-25 23:47 ` [PATCH 01/11] counters: Introduce counter_simple* and counter_atomic* counters Shuah Khan
@ 2020-09-25 23:52 ` Kees Cook
2020-09-26 0:13 ` Shuah Khan
2020-09-26 16:22 ` Kees Cook
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Kees Cook @ 2020-09-25 23:52 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya, surenb,
minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> -- Addressed Kees's comments:
> 1. Non-atomic counters renamed to counter_simple32 and counter_simple64
> to clearly indicate size.
> 2. Added warning for counter_simple* usage and it should be used only
> when there is no need for atomicity.
> 3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
> 4. Renamed counter_atomic_long to counter_atomic64 and it now uses
> atomic64_t ops and indicates size.
> 5. Test updated for the API renames.
> 6. Added helper functions for test results printing
> 7. Verified that the test module compiles in kunit env. and test
> module can be loaded to run the test.
Thanks for all of this!
> 8. Updated Documentation to reflect the intent to make the API
> restricted so it can never be used to guard object lifetimes
> and state management. I left _return ops for now, inc_return
> is necessary for now as per the discussion we had on this topic.
I still *really* do not want dec_return() to exist. That is asking for
trouble. I'd prefer inc_return() not exist either, but I can live with
it. ;)
--
Kees Cook
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-25 23:52 ` [PATCH 00/11] Introduce Simple atomic and non-atomic counters Kees Cook
@ 2020-09-26 0:13 ` Shuah Khan
2020-09-26 16:33 ` Kees Cook
0 siblings, 1 reply; 16+ messages in thread
From: Shuah Khan @ 2020-09-26 0:13 UTC (permalink / raw)
To: Kees Cook
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya, surenb,
minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac, Shuah Khan
On 9/25/20 5:52 PM, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
>> -- Addressed Kees's comments:
>> 1. Non-atomic counters renamed to counter_simple32 and counter_simple64
>> to clearly indicate size.
>> 2. Added warning for counter_simple* usage and it should be used only
>> when there is no need for atomicity.
>> 3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
>> 4. Renamed counter_atomic_long to counter_atomic64 and it now uses
>> atomic64_t ops and indicates size.
>> 5. Test updated for the API renames.
>> 6. Added helper functions for test results printing
>> 7. Verified that the test module compiles in kunit env. and test
>> module can be loaded to run the test.
>
> Thanks for all of this!
>
>> 8. Updated Documentation to reflect the intent to make the API
>> restricted so it can never be used to guard object lifetimes
>> and state management. I left _return ops for now, inc_return
>> is necessary for now as per the discussion we had on this topic.
>
> I still *really* do not want dec_return() to exist. That is asking for
> trouble. I'd prefer inc_return() not exist either, but I can live with
> it. ;)
>
Thanks. I am equally concerned about adding anything that can be used to
guard object lifetimes. So I will make sure this set won't expand and
plan to remove dec_return() if we don't find any usages.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-26 0:13 ` Shuah Khan
@ 2020-09-26 16:33 ` Kees Cook
2020-09-28 22:52 ` Shuah Khan
0 siblings, 1 reply; 16+ messages in thread
From: Kees Cook @ 2020-09-26 16:33 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya, surenb,
minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Fri, Sep 25, 2020 at 06:13:37PM -0600, Shuah Khan wrote:
> On 9/25/20 5:52 PM, Kees Cook wrote:
> > On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> > > -- Addressed Kees's comments:
> > > 1. Non-atomic counters renamed to counter_simple32 and counter_simple64
> > > to clearly indicate size.
> > > 2. Added warning for counter_simple* usage and it should be used only
> > > when there is no need for atomicity.
> > > 3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
> > > 4. Renamed counter_atomic_long to counter_atomic64 and it now uses
> > > atomic64_t ops and indicates size.
> > > 5. Test updated for the API renames.
> > > 6. Added helper functions for test results printing
> > > 7. Verified that the test module compiles in kunit env. and test
> > > module can be loaded to run the test.
> >
> > Thanks for all of this!
> >
> > > 8. Updated Documentation to reflect the intent to make the API
> > > restricted so it can never be used to guard object lifetimes
> > > and state management. I left _return ops for now, inc_return
> > > is necessary for now as per the discussion we had on this topic.
> >
> > I still *really* do not want dec_return() to exist. That is asking for
> > trouble. I'd prefer inc_return() not exist either, but I can live with
> > it. ;)
> >
>
> Thanks. I am equally concerned about adding anything that can be used to
> guard object lifetimes. So I will make sure this set won't expand and
> plan to remove dec_return() if we don't find any usages.
I would like it much stronger than "if". dec_return() needs to be just
dec() and read(). It will not be less efficient (since they're both
inlines), but it _will_ create a case where the atomicity cannot be used
for ref counting. My point is that anything that _requires_ dec_return()
(or, frankly, inc_return()) is _not_ "just" a statistical counter. It
may not be a refcounter, but it relies on the inc/dec atomicity for some
reason beyond counting in once place and reporting it in another.
--
Kees Cook
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-26 16:33 ` Kees Cook
@ 2020-09-28 22:52 ` Shuah Khan
0 siblings, 0 replies; 16+ messages in thread
From: Shuah Khan @ 2020-09-28 22:52 UTC (permalink / raw)
To: Kees Cook
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya, surenb,
minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac, Shuah Khan
On 9/26/20 10:33 AM, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 06:13:37PM -0600, Shuah Khan wrote:
>> On 9/25/20 5:52 PM, Kees Cook wrote:
>>> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
>>>> -- Addressed Kees's comments:
>>>> 1. Non-atomic counters renamed to counter_simple32 and counter_simple64
>>>> to clearly indicate size.
>>>> 2. Added warning for counter_simple* usage and it should be used only
>>>> when there is no need for atomicity.
>>>> 3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
>>>> 4. Renamed counter_atomic_long to counter_atomic64 and it now uses
>>>> atomic64_t ops and indicates size.
>>>> 5. Test updated for the API renames.
>>>> 6. Added helper functions for test results printing
>>>> 7. Verified that the test module compiles in kunit env. and test
>>>> module can be loaded to run the test.
>>>
>>> Thanks for all of this!
>>>
>>>> 8. Updated Documentation to reflect the intent to make the API
>>>> restricted so it can never be used to guard object lifetimes
>>>> and state management. I left _return ops for now, inc_return
>>>> is necessary for now as per the discussion we had on this topic.
>>>
>>> I still *really* do not want dec_return() to exist. That is asking for
>>> trouble. I'd prefer inc_return() not exist either, but I can live with
>>> it. ;)
>>>
>>
I didn't read this correctly the first time around.
>> Thanks. I am equally concerned about adding anything that can be used to
>> guard object lifetimes. So I will make sure this set won't expand and
>> plan to remove dec_return() if we don't find any usages.
>
> I would like it much stronger than "if". dec_return() needs to be just
> dec() and read(). It will not be less efficient (since they're both
> inlines), but it _will_ create a case where the atomicity cannot be used
> for ref counting. My point is that anything that _requires_ dec_return()
> (or, frankly, inc_return()) is _not_ "just" a statistical counter. It
> may not be a refcounter, but it relies on the inc/dec atomicity for some
> reason beyond counting in once place and reporting it in another.
>
I am not thinking about efficiency rather two calls instead of one if
an decrement needs to followed by return. In any case, I agree with you
that there is no need to add dec_return now without any use-cases.
I will update the patch series to remove it.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-25 23:47 [PATCH 00/11] Introduce Simple atomic and non-atomic counters Shuah Khan
2020-09-25 23:47 ` [PATCH 01/11] counters: Introduce counter_simple* and counter_atomic* counters Shuah Khan
2020-09-25 23:52 ` [PATCH 00/11] Introduce Simple atomic and non-atomic counters Kees Cook
@ 2020-09-26 16:22 ` Kees Cook
2020-09-28 22:42 ` Shuah Khan
2020-09-26 16:29 ` Kees Cook
2020-09-27 23:35 ` Joel Fernandes
4 siblings, 1 reply; 16+ messages in thread
From: Kees Cook @ 2020-09-26 16:22 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya, surenb,
minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> This patch series is a result of discussion at the refcount_t BOF
> the Linux Plumbers Conference. In this discussion, we identified
> a need for looking closely and investigating atomic_t usages in
> the kernel when it is used strictly as a counter without it
> controlling object lifetimes and state changes.
BTW, I realized the KSPP issue tracker hadn't broken this task out of
the refcount_t conversion issue[1] into a separate issue, so I've created
it now: https://github.com/KSPP/linux/issues/106
-Kees
[1] https://github.com/KSPP/linux/issues/104
--
Kees Cook
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-26 16:22 ` Kees Cook
@ 2020-09-28 22:42 ` Shuah Khan
0 siblings, 0 replies; 16+ messages in thread
From: Shuah Khan @ 2020-09-28 22:42 UTC (permalink / raw)
To: Kees Cook
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya, surenb,
minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac, Shuah Khan
On 9/26/20 10:22 AM, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
>> This patch series is a result of discussion at the refcount_t BOF
>> the Linux Plumbers Conference. In this discussion, we identified
>> a need for looking closely and investigating atomic_t usages in
>> the kernel when it is used strictly as a counter without it
>> controlling object lifetimes and state changes.
>
> BTW, I realized the KSPP issue tracker hadn't broken this task out of
> the refcount_t conversion issue[1] into a separate issue, so I've created
> it now: https://github.com/KSPP/linux/issues/106
>
Cool. Thanks.
-- Shuah
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-25 23:47 [PATCH 00/11] Introduce Simple atomic and non-atomic counters Shuah Khan
` (2 preceding siblings ...)
2020-09-26 16:22 ` Kees Cook
@ 2020-09-26 16:29 ` Kees Cook
2020-09-28 22:41 ` Shuah Khan
2020-09-27 23:35 ` Joel Fernandes
4 siblings, 1 reply; 16+ messages in thread
From: Kees Cook @ 2020-09-26 16:29 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya, surenb,
minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> 7. Verified that the test module compiles in kunit env. and test
> module can be loaded to run the test.
I meant write it using KUnit interfaces (e.g. KUNIT_EXPECT*(),
kunit_test_suite(), etc):
https://www.kernel.org/doc/html/latest/dev-tools/kunit/
Though I see the docs are still not updated[1] to reflect the Kconfig
(CONFIG_foo_KUNIT_TEST) and file naming conventions (foo_kunit.c).
-Kees
[1] https://lore.kernel.org/lkml/20200911042404.3598910-1-davidgow@google.com/
--
Kees Cook
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-26 16:29 ` Kees Cook
@ 2020-09-28 22:41 ` Shuah Khan
2020-09-28 23:13 ` Kees Cook
0 siblings, 1 reply; 16+ messages in thread
From: Shuah Khan @ 2020-09-28 22:41 UTC (permalink / raw)
To: Kees Cook
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya, surenb,
minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac, Shuah Khan
On 9/26/20 10:29 AM, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
>> 7. Verified that the test module compiles in kunit env. and test
>> module can be loaded to run the test.
>
> I meant write it using KUnit interfaces (e.g. KUNIT_EXPECT*(),
> kunit_test_suite(), etc):
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/
>
> Though I see the docs are still not updated[1] to reflect the Kconfig
> (CONFIG_foo_KUNIT_TEST) and file naming conventions (foo_kunit.c).
>
I would like to be able to run this test outside Kunit env., hence the
choice to go with a module and kselftest script. It makes it easier to
test as part of my workflow as opposed to doing a kunit and build and
running it that way.
I don't mind adding TEST_COUNTERS to kunit default configs though.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-28 22:41 ` Shuah Khan
@ 2020-09-28 23:13 ` Kees Cook
2020-10-06 15:21 ` Shuah Khan
0 siblings, 1 reply; 16+ messages in thread
From: Kees Cook @ 2020-09-28 23:13 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya, surenb,
minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Mon, Sep 28, 2020 at 04:41:47PM -0600, Shuah Khan wrote:
> On 9/26/20 10:29 AM, Kees Cook wrote:
> > On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> > > 7. Verified that the test module compiles in kunit env. and test
> > > module can be loaded to run the test.
> >
> > I meant write it using KUnit interfaces (e.g. KUNIT_EXPECT*(),
> > kunit_test_suite(), etc):
> > https://www.kernel.org/doc/html/latest/dev-tools/kunit/
> >
> > Though I see the docs are still not updated[1] to reflect the Kconfig
> > (CONFIG_foo_KUNIT_TEST) and file naming conventions (foo_kunit.c).
> >
>
> I would like to be able to run this test outside Kunit env., hence the
> choice to go with a module and kselftest script. It makes it easier to
> test as part of my workflow as opposed to doing a kunit and build and
> running it that way.
It does -- you just load it normally like before and it prints out
everything just fine. This is how I use the lib/test_user_copy.c and
lib/test_overflow.c before/after their conversions.
--
Kees Cook
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-28 23:13 ` Kees Cook
@ 2020-10-06 15:21 ` Shuah Khan
0 siblings, 0 replies; 16+ messages in thread
From: Shuah Khan @ 2020-10-06 15:21 UTC (permalink / raw)
To: Kees Cook
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya, surenb,
minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac, Shuah Khan
On 9/28/20 5:13 PM, Kees Cook wrote:
> On Mon, Sep 28, 2020 at 04:41:47PM -0600, Shuah Khan wrote:
>> On 9/26/20 10:29 AM, Kees Cook wrote:
>>> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
>>>> 7. Verified that the test module compiles in kunit env. and test
>>>> module can be loaded to run the test.
>>>
>>> I meant write it using KUnit interfaces (e.g. KUNIT_EXPECT*(),
>>> kunit_test_suite(), etc):
>>> https://www.kernel.org/doc/html/latest/dev-tools/kunit/
>>>
>>> Though I see the docs are still not updated[1] to reflect the Kconfig
>>> (CONFIG_foo_KUNIT_TEST) and file naming conventions (foo_kunit.c).
>>>
>>
>> I would like to be able to run this test outside Kunit env., hence the
>> choice to go with a module and kselftest script. It makes it easier to
>> test as part of my workflow as opposed to doing a kunit and build and
>> running it that way.
>
> It does -- you just load it normally like before and it prints out
> everything just fine. This is how I use the lib/test_user_copy.c and
> lib/test_overflow.c before/after their conversions.
>
I am not seeing any kunit links to either of these tests. I find the
lib/test_overflow.c very hard to read.
I am going to stick with what I have for now and handle conversion
later.
I think it might be a good idea to add tests for atomic_t and refcount_t
APIS as well at some point.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-25 23:47 [PATCH 00/11] Introduce Simple atomic and non-atomic counters Shuah Khan
` (3 preceding siblings ...)
2020-09-26 16:29 ` Kees Cook
@ 2020-09-27 23:35 ` Joel Fernandes
2020-09-28 20:34 ` Kees Cook
4 siblings, 1 reply; 16+ messages in thread
From: Joel Fernandes @ 2020-09-27 23:35 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, keescook, gregkh, shuah, rafael, johannes, lenb,
james.morse, tony.luck, bp, arve, tkjos, maco, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> This patch series is a result of discussion at the refcount_t BOF
> the Linux Plumbers Conference. In this discussion, we identified
> a need for looking closely and investigating atomic_t usages in
> the kernel when it is used strictly as a counter without it
> controlling object lifetimes and state changes.
>
> There are a number of atomic_t usages in the kernel where atomic_t api
> is used strictly for counting and not for managing object lifetime. In
> some cases, atomic_t might not even be needed.
>
> The purpose of these counters is 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.
Nice series :)
It appears there is no user of counter_simple in this series other than the
selftest. Would you be planning to add any conversions in the series itself,
for illustration of use? Sorry if I missed a usage.
Also how do we guard against atomicity of counter_simple RMW operations? Is
the implication that it should be guarded using other synchronization to
prevent lost-update problem?
Some more comments:
1. atomic RMW operations that have a return value are fully ordered. Would
you be adding support to counter_simple for such ordering as well, for
consistency?
2. I felt counter_atomic and counter_atomic64 would be nice equivalents to
the atomic and atomic64 naming currently used (i.e. dropping the '32').
However that is just my opinion and I am ok with either naming.
thanks!
- Joel
>
> Simple atomic and non-atomic counters api provides interfaces for simple
> atomic and non-atomic counters that just count, and don't guard resource
> lifetimes. Counters will wrap around to 0 when it overflows and should
> not be used to guard resource lifetimes, device usage and open counts
> that control state changes, and pm states.
>
> Using counter_atomic to guard lifetimes could lead to use-after free
> when it overflows and undefined behavior when used to manage state
> changes and device usage/open states.
>
> This patch series introduces Simple atomic and non-atomic counters.
> Counter atomic ops leverage atomic_t and provide a sub-set of atomic_t
> ops.
>
> In addition this patch series converts a few drivers to use the new api.
> The following criteria is used for select variables for conversion:
>
> 1. Variable doesn't guard object lifetimes, manage state changes e.g:
> device usage counts, device open counts, and pm states.
> 2. Variable is used for stats and counters.
> 3. The conversion doesn't change the overflow behavior.
>
> Changes since RFC:
> -- Thanks for reviews and reviewed-by, and Acked-by tags. Updated
> the patches with the tags.
> -- Addressed Kees's comments:
> 1. Non-atomic counters renamed to counter_simple32 and counter_simple64
> to clearly indicate size.
> 2. Added warning for counter_simple* usage and it should be used only
> when there is no need for atomicity.
> 3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
> 4. Renamed counter_atomic_long to counter_atomic64 and it now uses
> atomic64_t ops and indicates size.
> 5. Test updated for the API renames.
> 6. Added helper functions for test results printing
> 7. Verified that the test module compiles in kunit env. and test
> module can be loaded to run the test.
> 8. Updated Documentation to reflect the intent to make the API
> restricted so it can never be used to guard object lifetimes
> and state management. I left _return ops for now, inc_return
> is necessary for now as per the discussion we had on this topic.
> -- Updated driver patches with API name changes.
> -- We discussed if binder counters can be non-atomic. For now I left
> them the same as the RFC patch - using counter_atomic32
> -- Unrelated to this patch series:
> The patch series review uncovered improvements could be made to
> test_async_driver_probe and vmw_vmci/vmci_guest. I will track
> these for fixing later.
>
> Shuah Khan (11):
> counters: Introduce counter_simple* and counter_atomic* counters
> selftests:lib:test_counters: add new test for counters
> drivers/base: convert deferred_trigger_count and probe_count to
> counter_atomic32
> drivers/base/devcoredump: convert devcd_count to counter_atomic32
> drivers/acpi: convert seqno counter_atomic32
> drivers/acpi/apei: convert seqno counter_atomic32
> drivers/android/binder: convert stats, transaction_log to
> counter_atomic32
> drivers/base/test/test_async_driver_probe: convert to use
> counter_atomic32
> drivers/char/ipmi: convert stats to use counter_atomic32
> drivers/misc/vmw_vmci: convert num guest devices counter to
> counter_atomic32
> drivers/edac: convert pci counters to counter_atomic32
>
> Documentation/core-api/counters.rst | 174 +++++++++
> MAINTAINERS | 8 +
> drivers/acpi/acpi_extlog.c | 5 +-
> drivers/acpi/apei/ghes.c | 5 +-
> drivers/android/binder.c | 41 +--
> drivers/android/binder_internal.h | 3 +-
> drivers/base/dd.c | 19 +-
> drivers/base/devcoredump.c | 5 +-
> drivers/base/test/test_async_driver_probe.c | 23 +-
> drivers/char/ipmi/ipmi_msghandler.c | 9 +-
> drivers/char/ipmi/ipmi_si_intf.c | 9 +-
> drivers/edac/edac_pci.h | 5 +-
> drivers/edac/edac_pci_sysfs.c | 28 +-
> drivers/misc/vmw_vmci/vmci_guest.c | 9 +-
> include/linux/counters.h | 350 +++++++++++++++++++
> lib/Kconfig | 10 +
> lib/Makefile | 1 +
> lib/test_counters.c | 276 +++++++++++++++
> tools/testing/selftests/lib/Makefile | 1 +
> tools/testing/selftests/lib/config | 1 +
> tools/testing/selftests/lib/test_counters.sh | 5 +
> 21 files changed, 913 insertions(+), 74 deletions(-)
> create mode 100644 Documentation/core-api/counters.rst
> create mode 100644 include/linux/counters.h
> create mode 100644 lib/test_counters.c
> create mode 100755 tools/testing/selftests/lib/test_counters.sh
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-27 23:35 ` Joel Fernandes
@ 2020-09-28 20:34 ` Kees Cook
2020-09-28 21:17 ` Joel Fernandes
0 siblings, 1 reply; 16+ messages in thread
From: Kees Cook @ 2020-09-28 20:34 UTC (permalink / raw)
To: Joel Fernandes
Cc: Shuah Khan, corbet, gregkh, shuah, rafael, johannes, lenb,
james.morse, tony.luck, bp, arve, tkjos, maco, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Sun, Sep 27, 2020 at 07:35:26PM -0400, Joel Fernandes wrote:
> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> > This patch series is a result of discussion at the refcount_t BOF
> > the Linux Plumbers Conference. In this discussion, we identified
> > a need for looking closely and investigating atomic_t usages in
> > the kernel when it is used strictly as a counter without it
> > controlling object lifetimes and state changes.
> >
> > There are a number of atomic_t usages in the kernel where atomic_t api
> > is used strictly for counting and not for managing object lifetime. In
> > some cases, atomic_t might not even be needed.
> >
> > The purpose of these counters is 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.
>
> Nice series :)
>
> It appears there is no user of counter_simple in this series other than the
> selftest. Would you be planning to add any conversions in the series itself,
> for illustration of use? Sorry if I missed a usage.
>
> Also how do we guard against atomicity of counter_simple RMW operations? Is
> the implication that it should be guarded using other synchronization to
> prevent lost-update problem?
>
> Some more comments:
>
> 1. atomic RMW operations that have a return value are fully ordered. Would
> you be adding support to counter_simple for such ordering as well, for
> consistency?
No -- there is no atomicity guarantee for counter_simple. I would prefer
counter_simple not exist at all, specifically for this reason.
> 2. I felt counter_atomic and counter_atomic64 would be nice equivalents to
> the atomic and atomic64 naming currently used (i.e. dropping the '32').
> However that is just my opinion and I am ok with either naming.
I had asked that they be size-named to avoid any confusion (i.e. we're
making a new API).
--
Kees Cook
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-28 20:34 ` Kees Cook
@ 2020-09-28 21:17 ` Joel Fernandes
2020-09-28 23:01 ` Shuah Khan
0 siblings, 1 reply; 16+ messages in thread
From: Joel Fernandes @ 2020-09-28 21:17 UTC (permalink / raw)
To: Kees Cook
Cc: Shuah Khan, corbet, gregkh, shuah, rafael, johannes, lenb,
james.morse, tony.luck, bp, arve, tkjos, maco, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Mon, Sep 28, 2020 at 01:34:31PM -0700, Kees Cook wrote:
> On Sun, Sep 27, 2020 at 07:35:26PM -0400, Joel Fernandes wrote:
> > On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> > > This patch series is a result of discussion at the refcount_t BOF
> > > the Linux Plumbers Conference. In this discussion, we identified
> > > a need for looking closely and investigating atomic_t usages in
> > > the kernel when it is used strictly as a counter without it
> > > controlling object lifetimes and state changes.
> > >
> > > There are a number of atomic_t usages in the kernel where atomic_t api
> > > is used strictly for counting and not for managing object lifetime. In
> > > some cases, atomic_t might not even be needed.
> > >
> > > The purpose of these counters is 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.
> >
> > Nice series :)
> >
> > It appears there is no user of counter_simple in this series other than the
> > selftest. Would you be planning to add any conversions in the series itself,
> > for illustration of use? Sorry if I missed a usage.
> >
> > Also how do we guard against atomicity of counter_simple RMW operations? Is
> > the implication that it should be guarded using other synchronization to
> > prevent lost-update problem?
> >
> > Some more comments:
> >
> > 1. atomic RMW operations that have a return value are fully ordered. Would
> > you be adding support to counter_simple for such ordering as well, for
> > consistency?
>
> No -- there is no atomicity guarantee for counter_simple. I would prefer
> counter_simple not exist at all, specifically for this reason.
Yeah I am ok with it not existing, especially also as there are no examples
of its conversion/usage in the series.
> > 2. I felt counter_atomic and counter_atomic64 would be nice equivalents to
> > the atomic and atomic64 naming currently used (i.e. dropping the '32').
> > However that is just my opinion and I am ok with either naming.
>
> I had asked that they be size-named to avoid any confusion (i.e. we're
> making a new API).
Works for me.
Cheers,
- Joel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-28 21:17 ` Joel Fernandes
@ 2020-09-28 23:01 ` Shuah Khan
0 siblings, 0 replies; 16+ messages in thread
From: Shuah Khan @ 2020-09-28 23:01 UTC (permalink / raw)
To: Joel Fernandes, Kees Cook
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, christian, hridya, surenb,
minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac, Shuah Khan
On 9/28/20 3:17 PM, Joel Fernandes wrote:
> On Mon, Sep 28, 2020 at 01:34:31PM -0700, Kees Cook wrote:
>> On Sun, Sep 27, 2020 at 07:35:26PM -0400, Joel Fernandes wrote:
>>> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
>>>> This patch series is a result of discussion at the refcount_t BOF
>>>> the Linux Plumbers Conference. In this discussion, we identified
>>>> a need for looking closely and investigating atomic_t usages in
>>>> the kernel when it is used strictly as a counter without it
>>>> controlling object lifetimes and state changes.
>>>>
>>>> There are a number of atomic_t usages in the kernel where atomic_t api
>>>> is used strictly for counting and not for managing object lifetime. In
>>>> some cases, atomic_t might not even be needed.
>>>>
>>>> The purpose of these counters is 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.
>>>
>>> Nice series :)
>>>
Thanks.
>>> It appears there is no user of counter_simple in this series other than the
>>> selftest. Would you be planning to add any conversions in the series itself,
>>> for illustration of use? Sorry if I missed a usage.
>>>
>>> Also how do we guard against atomicity of counter_simple RMW operations? Is
>>> the implication that it should be guarded using other synchronization to
>>> prevent lost-update problem?
>>>
>>> Some more comments:
>>>
>>> 1. atomic RMW operations that have a return value are fully ordered. Would
>>> you be adding support to counter_simple for such ordering as well, for
>>> consistency?
>>
>> No -- there is no atomicity guarantee for counter_simple. I would prefer
>> counter_simple not exist at all, specifically for this reason.
>
> Yeah I am ok with it not existing, especially also as there are no examples
> of its conversion/usage in the series.
>
No. counter_simple is just for counting when there is no need for
atomicity with the premise that there might be some use-cases. You
are right that this patch series doesn't use these. My hunch is though
that atomic_t is overused and it isn't needed in all cases.
I will do some research to look for any places that can use
counter_simple before I spin v2. If I don't find any, I can drop them.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 16+ messages in thread