[PATCH 00/13] Introduce seqnum_ops

[PATCH 00/13] Introduce seqnum_ops

[Shuah Khan]

There are a number of atomic_t usages in the kernel where atomic_t api
is used strictly for counting sequence numbers and other statistical
counters and not for managing object lifetime.

The purpose of these Sequence Number Ops is to clearly differentiate
atomic_t counter usages from atomic_t usages that guard object lifetimes,
hence prone to overflow and underflow errors.

The atomic_t api provides a wide range of atomic operations as a base
api to implement atomic counters, bitops, spinlock interfaces. The usages
also evolved into being used for resource lifetimes and state management.
The refcount_t api was introduced to address resource lifetime problems
related to atomic_t wrapping. There is a large overlap between the
atomic_t api used for resource lifetimes and just counters, stats, and
sequence numbers. It has become difficult to differentiate between the
atomic_t usages that should be converted to refcount_t and the ones that
can be left alone. Introducing seqnum_ops to wrap the usages that are
stats, counters, sequence numbers makes it easier for 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.

Sequence Number api provides interfaces for simple atomic_t counter usages
that just count, and don't guard resource lifetimes. The seqnum_ops are
built on top of atomic_t api, providing a smaller subset of atomic_t
interfaces necessary to support atomic_t usages as simple counters.
This api has init/set/inc/dec/read and doesn't support any other atomic_t
ops with the intent to restrict the use of these interfaces as simple
counting usages.

Sequence Numbers wrap around to INT_MIN when it overflows and should not
be used to guard resource lifetimes, device usage and open counts that
control state changes, and pm states. Overflowing to INT_MIN is consistent
with the atomic_t api, which it is built on top of.

Using seqnum 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.

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.
4. Note: inc_return() usages are changed to _inc() followed by _read()
   Patches: 03/13, 04/13, 09/13, 10/13, 11/13
5. drivers/acpi and drivers/acpi/apei patches have been reviewed
   before the rename, however in addition to rename, inc_return()
   usages are changed to _inc() followed by _read()
6. test_async_driver_probe, char/ipmi, and edac patches have been
   reviewed and no changes other than the rename to seqnum_ops.
7. security/integrity/ima: Okay to depend on CONFIG_64BIT? 

The work for this is a follow-on to the discussion and review of
Introduce Simple atomic counters patch series:

//lore.kernel.org/lkml/cover.1602209970.git.skhan@linuxfoundation.org/

Based on the feedback to restrict and limit the scope:
- dropped inc_return()
- renamed interfaces to match the intent and also shorten the
  interface names.

Shuah Khan (13):
  seqnum_ops: Introduce Sequence Number Ops
  selftests: lib:test_seqnum_ops: add new test for seqnum_ops
  drivers/acpi: convert seqno seqnum_ops
  drivers/acpi/apei: convert seqno to seqnum_ops
  drivers/base/test/test_async_driver_probe: convert to use seqnum_ops
  drivers/char/ipmi: convert stats to use seqnum_ops
  drivers/edac: convert pci counters to seqnum_ops
  drivers/oprofile: convert stats to use seqnum_ops
  drivers/staging/rtl8723bs: convert stats to use seqnum_ops
  usb: usbip/vhci: convert seqno to seqnum_ops
  drivers/staging/rtl8188eu: convert stats to use seqnum_ops
  drivers/staging/unisys/visorhba: convert stats to use seqnum_ops
  security/integrity/ima: converts stats to seqnum_ops

 Documentation/core-api/atomic_ops.rst         |   4 +
 Documentation/core-api/index.rst              |   1 +
 Documentation/core-api/seqnum_ops.rst         | 126 ++++++++++++++
 MAINTAINERS                                   |   8 +
 drivers/acpi/acpi_extlog.c                    |   6 +-
 drivers/acpi/apei/ghes.c                      |   6 +-
 drivers/base/test/test_async_driver_probe.c   |  26 +--
 drivers/char/ipmi/ipmi_msghandler.c           |   9 +-
 drivers/char/ipmi/ipmi_si_intf.c              |   9 +-
 drivers/char/ipmi/ipmi_ssif.c                 |   9 +-
 drivers/edac/edac_pci.h                       |   5 +-
 drivers/edac/edac_pci_sysfs.c                 |  28 ++--
 drivers/oprofile/buffer_sync.c                |   9 +-
 drivers/oprofile/event_buffer.c               |   3 +-
 drivers/oprofile/oprof.c                      |   3 +-
 drivers/oprofile/oprofile_stats.c             |  11 +-
 drivers/oprofile/oprofile_stats.h             |  11 +-
 drivers/oprofile/oprofilefs.c                 |   3 +-
 drivers/staging/rtl8188eu/core/rtw_mlme_ext.c |  23 ++-
 .../staging/rtl8188eu/include/rtw_mlme_ext.h  |   3 +-
 drivers/staging/rtl8723bs/core/rtw_cmd.c      |   3 +-
 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c |  33 ++--
 drivers/staging/rtl8723bs/include/rtw_cmd.h   |   3 +-
 .../staging/rtl8723bs/include/rtw_mlme_ext.h  |   3 +-
 .../staging/unisys/visorhba/visorhba_main.c   |  37 +++--
 drivers/usb/usbip/vhci.h                      |   3 +-
 drivers/usb/usbip/vhci_hcd.c                  |   9 +-
 drivers/usb/usbip/vhci_rx.c                   |   3 +-
 include/linux/oprofile.h                      |   3 +-
 include/linux/seqnum_ops.h                    | 154 ++++++++++++++++++
 lib/Kconfig                                   |   9 +
 lib/Makefile                                  |   1 +
 lib/test_seqnum_ops.c                         | 154 ++++++++++++++++++
 security/integrity/ima/ima.h                  |   5 +-
 security/integrity/ima/ima_api.c              |   2 +-
 security/integrity/ima/ima_fs.c               |   4 +-
 security/integrity/ima/ima_queue.c            |   7 +-
 tools/testing/selftests/lib/Makefile          |   1 +
 tools/testing/selftests/lib/config            |   1 +
 .../testing/selftests/lib/test_seqnum_ops.sh  |  10 ++
 40 files changed, 637 insertions(+), 111 deletions(-)
 create mode 100644 Documentation/core-api/seqnum_ops.rst
 create mode 100644 include/linux/seqnum_ops.h
 create mode 100644 lib/test_seqnum_ops.c
 create mode 100755 tools/testing/selftests/lib/test_seqnum_ops.sh

-- 
2.27.0

[PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Shuah Khan]

There are a number of atomic_t usages in the kernel where atomic_t api
is used strictly for counting sequence numbers and other statistical
counters and not for managing object lifetime.

The purpose of these Sequence Number Ops is to clearly differentiate
atomic_t counter usages from atomic_t usages that guard object lifetimes,
hence prone to overflow and underflow errors.

The atomic_t api provides a wide range of atomic operations as a base
api to implement atomic counters, bitops, spinlock interfaces. The usages
also evolved into being used for resource lifetimes and state management.
The refcount_t api was introduced to address resource lifetime problems
related to atomic_t wrapping. There is a large overlap between the
atomic_t api used for resource lifetimes and just counters, stats, and
sequence numbers. It has become difficult to differentiate between the
atomic_t usages that should be converted to refcount_t and the ones that
can be left alone. Introducing seqnum_ops to wrap the usages that are
stats, counters, sequence numbers makes it easier for 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.

Sequence Number api provides interfaces for simple atomic_t counter usages
that just count, and don't guard resource lifetimes. The seqnum_ops are
built on top of atomic_t api, providing a smaller subset of atomic_t
interfaces necessary to support atomic_t usages as simple counters.
This api has init/set/inc/dec/read and doesn't support any other atomic_t
ops with the intent to restrict the use of these interfaces as simple
counting usages.

Sequence Numbers wrap around to INT_MIN when it overflows and should not
be used to guard resource lifetimes, device usage and open counts that
control state changes, and pm states. Overflowing to INT_MIN is consistent
with the atomic_t api, which it is built on top of.

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

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 Documentation/core-api/atomic_ops.rst |   4 +
 Documentation/core-api/index.rst      |   1 +
 Documentation/core-api/seqnum_ops.rst | 117 +++++++++++++++++++
 MAINTAINERS                           |   7 ++
 include/linux/seqnum_ops.h            | 152 +++++++++++++++++++++++++
 lib/Kconfig                           |   9 ++
 lib/Makefile                          |   1 +
 lib/test_seqnum_ops.c                 | 154 ++++++++++++++++++++++++++
 8 files changed, 445 insertions(+)
 create mode 100644 Documentation/core-api/seqnum_ops.rst
 create mode 100644 include/linux/seqnum_ops.h
 create mode 100644 lib/test_seqnum_ops.c

diff --git a/Documentation/core-api/atomic_ops.rst b/Documentation/core-api/atomic_ops.rst
index 724583453e1f..762cbc0947e7 100644
--- a/Documentation/core-api/atomic_ops.rst
+++ b/Documentation/core-api/atomic_ops.rst
@@ -1,3 +1,7 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+.. _atomic_ops:
+
 =======================================================
 Semantics and Behavior of Atomic and Bitmask Operations
 =======================================================
diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
index 69171b1799f2..be958afe757c 100644
--- a/Documentation/core-api/index.rst
+++ b/Documentation/core-api/index.rst
@@ -55,6 +55,7 @@ How Linux keeps everything from happening at the same time.  See
 
    atomic_ops
    refcount-vs-atomic
+   seqnum_ops
    irq/index
    local_ops
    padata
diff --git a/Documentation/core-api/seqnum_ops.rst b/Documentation/core-api/seqnum_ops.rst
new file mode 100644
index 000000000000..7a396c2cda19
--- /dev/null
+++ b/Documentation/core-api/seqnum_ops.rst
@@ -0,0 +1,117 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+.. include:: <isonum.txt>
+
+.. _seqnum_ops:
+
+==========================
+Sequence Number Operations
+==========================
+
+:Author: Shuah Khan
+:Copyright: |copy| 2020, The Linux Foundation
+:Copyright: |copy| 2020, Shuah Khan <skhan@linuxfoundation.org>
+
+There are a number of atomic_t usages in the kernel where atomic_t api
+is used strictly for counting sequence numbers and other statistical
+counters and not for managing object lifetime.
+
+The purpose of these Sequence Number Ops is to clearly differentiate
+atomic_t counter usages 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.
+
+The atomic_t api provides a wide range of atomic operations as a base
+api to implement atomic counters, bitops, spinlock interfaces. The usages
+also evolved into being used for resource lifetimes and state management.
+The refcount_t api was introduced to address resource lifetime problems
+related to atomic_t wrapping. There is a large overlap between the
+atomic_t api used for resource lifetimes and just counters, stats, and
+sequence numbers. It has become difficult to differentiate between the
+atomic_t usages that should be converted to refcount_t and the ones that
+can be left alone. Introducing seqnum_ops to wrap the usages that are
+stats, counters, sequence numbers makes it easier for 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.
+
+Sequence Number api provides interfaces for simple atomic_t counter usages
+that just count, and don't guard resource lifetimes. The seqnum_ops are
+built on top of atomic_t api, providing a smaller subset of atomic_t
+interfaces necessary to support atomic_t usages as simple counters.
+This api has init/set/inc/dec/read and doesn't support any other atomic_t
+ops with the intent to restrict the use of these interfaces as simple
+counting usages.
+
+Sequence Numbers wrap around to INT_MIN when it overflows and should not
+be used to guard resource lifetimes, device usage and open counts that
+control state changes, and pm states. Overflowing to INT_MIN is consistent
+with the atomic_t api, which it is built on top of.
+
+Using seqnum 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::
+        seqnum wraps around to INT_MIN when it overflows.
+        Should not be used to guard resource lifetimes.
+        Should not be used to manage device state and pm state.
+
+Sequence Number Ops
+===================
+
+seqnum32 and seqnum64 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 seqnum32 { atomic_t seqnum; };
+        struct seqnum64 { atomic64_t seqnum; };
+
+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 Seqnum API will never be used for guarding resource
+        lifetimes and state management.
+
+Initializers
+------------
+
+Interfaces for initializing sequence numbers are write operations which
+in turn invoke their ``ATOMIC_INIT() and atomic_set()`` counterparts ::
+
+        #define SEQNUM_INIT(i)    { .seqnum = ATOMIC_INIT(i) }
+        seqnum32_set() --> atomic_set()
+
+        static struct seqnum32 aseq = SEQNUM_INIT(0);
+        seqnum32_set(0);
+
+        static struct seqnum aseq = SEQNUM_INIT(0);
+        seqnum64_set(0);
+
+Read interface
+--------------
+
+Reads and returns the current value. ::
+
+        seqnum32_read() --> atomic_read()
+        seqnum64_read() --> atomic64_read()
+
+Increment interface
+-------------------
+
+Increments sequence number and doesn't return the new value. ::
+
+        seqnum32_inc() --> atomic_inc()
+        seqnum64_inc() --> atomic64_inc()
+
+Decrement interface
+-------------------
+
+Decrements sequence number and doesn't return the new value. ::
+
+        seqnum32_dec() --> atomic_dec()
+        seqnum64_dec() --> atomic64_dec()
diff --git a/MAINTAINERS b/MAINTAINERS
index b516bb34a8d5..c83a6f05610b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15977,6 +15977,13 @@ S:	Maintained
 F:	Documentation/fb/sm712fb.rst
 F:	drivers/video/fbdev/sm712*
 
+SEQNUM OPS
+M:	Shuah Khan <skhan@linuxfoundation.org>
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	include/linux/seqnum_ops.h
+F:	lib/test_seqnum_ops.c
+
 SIMPLE FIRMWARE INTERFACE (SFI)
 S:	Obsolete
 W:	http://simplefirmware.org/
diff --git a/include/linux/seqnum_ops.h b/include/linux/seqnum_ops.h
new file mode 100644
index 000000000000..b97c7f310beb
--- /dev/null
+++ b/include/linux/seqnum_ops.h
@@ -0,0 +1,152 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * seqnum_ops.h - Interfaces for sequential and statistical counters.
+ *
+ * Copyright (c) 2020 Shuah Khan <skhan@linuxfoundation.org>
+ * Copyright (c) 2020 The Linux Foundation
+ *
+ * Sequence Numbers wrap around to INT_MIN 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 seqnum32 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:
+ * seqnum32 & seqnum64 functions:
+ *	initialization
+ *	set
+ *	read
+ *	increment and no return
+ *	decrement and no return
+ *
+ * seqnum32 functions leverage/use atomic_t interfaces.
+ * seqnum64 functions leverage/use atomic64_t interfaces.
+ * The seqnum32 wraps around to INT_MIN when it overflows.
+ * These interfaces should not be used to guard resource lifetimes.
+ *
+ * Reference and API guide:
+ *	Documentation/core-api/seqnum_ops.rst for more information.
+ *
+ */
+
+#ifndef __LINUX_SEQNUM_OPS_H
+#define __LINUX_SEQNUM_OPS_H
+
+#include <linux/atomic.h>
+
+/**
+ * struct seqnum32 - Sequential/Statistical atomic counter
+ * @seqnum: int
+ *
+ * The seqnum wraps around to INT_MIN, when it overflows. Should not
+ * be used to guard object lifetimes.
+ **/
+struct seqnum32 {
+	atomic_t seqnum;
+};
+
+#define SEQNUM_INIT(i)		{ .seqnum = ATOMIC_INIT(i) }
+
+/*
+ * seqnum32_inc() - increment seqnum value
+ * @seq: struct seqnum32 pointer
+ *
+ */
+static inline void seqnum32_inc(struct seqnum32 *seq)
+{
+	atomic_inc(&seq->seqnum);
+}
+
+/*
+ * seqnum32_dec() - decrement seqnum value
+ * @seq: struct seqnum32 pointer
+ *
+ */
+static inline void seqnum32_dec(struct seqnum32 *seq)
+{
+	atomic_dec(&seq->seqnum);
+}
+
+/*
+ * seqnum32_read() - read seqnum value
+ * @seq: struct seqnum32 pointer
+ *
+ * Return: return the current value
+ */
+static inline int seqnum32_read(const struct seqnum32 *seq)
+{
+	return atomic_read(&seq->seqnum);
+}
+
+/*
+ * seqnum32_set() - set seqnum value
+ * @seq: struct seqnum32 pointer
+ * @val: new value to set
+ *
+ */
+static inline void
+seqnum32_set(struct seqnum32 *seq, int val)
+{
+	atomic_set(&seq->seqnum, val);
+}
+
+#ifdef CONFIG_64BIT
+/*
+ * struct seqnum64 - Sequential/Statistical atomic counter
+ * @seq: atomic64_t
+ *
+ * The seqnum wraps around to INT_MIN, when it overflows. Should not
+ * be used to guard object lifetimes.
+ */
+struct seqnum64 {
+	atomic64_t seqnum;
+};
+
+/*
+ * seqnum64_inc() - increment seqnum value
+ * @seq: struct seqnum64 pointer
+ *
+ */
+static inline void seqnum64_inc(struct seqnum64 *seq)
+{
+	atomic64_inc(&seq->seqnum);
+}
+
+/*
+ * seqnum64_dec() - decrement seqnum value
+ * @seq: struct seqnum64 pointer
+ *
+ */
+static inline void seqnum64_dec(
+				struct seqnum64 *seq)
+{
+	atomic64_dec(&seq->seqnum);
+}
+
+/*
+ * seqnum64_read() - read seqnum value
+ * @seq: struct seqnum64 pointer
+ *
+ * Return: return the seqnum value
+ */
+static inline s64
+seqnum64_read(const struct seqnum64 *seq)
+{
+	return atomic64_read(&seq->seqnum);
+}
+
+/*
+ * seqnum64_set() - set seqnum value
+ * @seq: struct seqnum64 pointer
+ * &val:  new seqnum value to set
+ *
+ */
+static inline void seqnum64_set(struct seqnum64 *seq, s64 val)
+{
+	atomic64_set(&seq->seqnum, val);
+}
+
+#endif /* CONFIG_64BIT */
+#endif /* __LINUX_COUNTERS_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index b46a9fd122c8..c362c2713e11 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -663,6 +663,15 @@ config OBJAGG
 config STRING_SELFTEST
 	tristate "Test string functions"
 
+config TEST_SEQNUM_OPS
+	tristate "Test Sequence Number Ops API"
+	help
+	   A test module for Sequence Number Ops API. A corresponding
+	   selftest can be used to test the Seqnum Ops API. Select this
+	   for testing Sequence Number Ops API.
+
+	   See Documentation/core-api/seqnum_ops.rst
+
 endmenu
 
 config GENERIC_IOREMAP
diff --git a/lib/Makefile b/lib/Makefile
index ce45af50983a..7d17c25e4d73 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o
 obj-$(CONFIG_TEST_LOCKUP) += test_lockup.o
 obj-$(CONFIG_TEST_HMM) += test_hmm.o
 obj-$(CONFIG_TEST_FREE_PAGES) += test_free_pages.o
+obj-$(CONFIG_TEST_SEQNUM_OPS) += test_seqnum_ops.o
 
 #
 # CFLAGS for compiling floating point code inside the kernel. x86/Makefile turns
diff --git a/lib/test_seqnum_ops.c b/lib/test_seqnum_ops.c
new file mode 100644
index 000000000000..12bd5e3123a4
--- /dev/null
+++ b/lib/test_seqnum_ops.c
@@ -0,0 +1,154 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * test_seqnum_ops.c - Kernel module for testing Seqnum API
+ *
+ * Copyright (c) 2020 Shuah Khan <skhan@linuxfoundation.org>
+ * Copyright (c) 2020 The Linux Foundation
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/seqnum_ops.h>
+
+static inline void
+test_seqnum_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_seqnum32(void)
+{
+	static struct seqnum32 seq = SEQNUM_INIT(0);
+	int start_val = seqnum32_read(&seq);
+	int end_val;
+
+	seqnum32_inc(&seq);
+	end_val = seqnum32_read(&seq);
+	test_seqnum_result_print32("Test read and increment",
+				    start_val, end_val, start_val+1);
+
+	start_val = seqnum32_read(&seq);
+	seqnum32_dec(&seq);
+	end_val = seqnum32_read(&seq);
+	test_seqnum_result_print32("Test read and decrement",
+				    start_val, end_val, start_val-1);
+
+	start_val = seqnum32_read(&seq);
+	seqnum32_set(&seq, INT_MAX);
+	end_val = seqnum32_read(&seq);
+	test_seqnum_result_print32("Test set", start_val, end_val, INT_MAX);
+}
+
+static void test_seqnum32_overflow(void)
+{
+	static struct seqnum32 useq = SEQNUM_INIT(0);
+	static struct seqnum32 oseq = SEQNUM_INIT(INT_MAX);
+	int start_val;
+	int end_val;
+
+	start_val = seqnum32_read(&useq);
+	seqnum32_dec(&useq);
+	end_val = seqnum32_read(&useq);
+	test_seqnum_result_print32("Test underflow (int)",
+				    start_val, end_val, start_val-1);
+	test_seqnum_result_print32("Test underflow (-1)",
+				    start_val, end_val, -1);
+
+	start_val = seqnum32_read(&oseq);
+	seqnum32_inc(&oseq);
+	end_val = seqnum32_read(&oseq);
+	test_seqnum_result_print32("Test overflow (int)",
+				    start_val, end_val, start_val+1);
+	test_seqnum_result_print32("Test overflow (INT_MIN)",
+				    start_val, end_val, INT_MIN);
+}
+
+#ifdef CONFIG_64BIT
+
+static inline void
+test_seqnum_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_seqnum64(void)
+{
+	static struct seqnum64 seq = SEQNUM_INIT(0);
+	s64 start_val = seqnum64_read(&seq);
+	s64 end_val;
+
+	seqnum64_inc(&seq);
+	end_val = seqnum64_read(&seq);
+	test_seqnum_result_print64("Test read and increment",
+				    start_val, end_val, start_val+1);
+
+	start_val = seqnum64_read(&seq);
+	seqnum64_dec(&seq);
+	end_val = seqnum64_read(&seq);
+	test_seqnum_result_print64("Test read and decrement",
+				    start_val, end_val, start_val-1);
+
+	start_val = seqnum64_read(&seq);
+	seqnum64_set(&seq, INT_MAX);
+	end_val = seqnum64_read(&seq);
+	test_seqnum_result_print64("Test set", start_val, end_val, INT_MAX);
+}
+
+static void test_seqnum64_overflow(void)
+{
+	static struct seqnum64 useq = SEQNUM_INIT(0);
+	static struct seqnum64 oseq = SEQNUM_INIT(INT_MAX);
+	s64 start_val;
+	s64 end_val;
+
+	start_val = seqnum64_read(&useq);
+	seqnum64_dec(&useq);
+	end_val = seqnum64_read(&useq);
+	test_seqnum_result_print64("Test underflow",
+				    start_val, end_val, start_val-1);
+
+	start_val = seqnum64_read(&oseq);
+	seqnum64_inc(&oseq);
+	end_val = seqnum64_read(&oseq);
+	test_seqnum_result_print64("Test overflow",
+				    start_val, end_val, start_val+1);
+}
+
+#endif /* CONFIG_64BIT */
+
+static int __init test_seqnum_ops_init(void)
+{
+	pr_info("Start seqnum32_*() interfaces test\n");
+	test_seqnum32();
+	test_seqnum32_overflow();
+	pr_info("End seqnum32_*() interfaces test\n\n");
+
+#ifdef CONFIG_64BIT
+	pr_info("Start seqnum64_*() interfaces test\n");
+	test_seqnum64();
+	test_seqnum64_overflow();
+	pr_info("End seqnum64_*() interfaces test\n\n");
+
+#endif /* CONFIG_64BIT */
+
+	return 0;
+}
+
+module_init(test_seqnum_ops_init);
+
+static void __exit test_seqnum_ops_exit(void)
+{
+	pr_info("exiting.\n");
+}
+
+module_exit(test_seqnum_ops_exit);
+
+MODULE_AUTHOR("Shuah Khan <skhan@linuxfoundation.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.27.0

[PATCH 02/13] selftests:lib:test_seqnum_ops: add new test for seqnum_ops

[Shuah Khan]

Add a new selftest for testing seqnum_ops. This test loads test_seqnum_ops
test module and unloads it. The test module runs tests and prints results
to dmesg.

There are a number of atomic_t usages in the kernel where atomic_t api
is used strictly for counting sequence numbers and other statistical
counters and not for managing object lifetime.

The purpose of these Sequence Number Ops is to clearly differentiate
atomic_t counter usages from atomic_t usages that guard object lifetimes,
hence prone to overflow and underflow errors.

The atomic_t api provides a wide range of atomic operations as a base
api to implement atomic counters, bitops, spinlock interfaces. The usages
also evolved into being used for resource lifetimes and state management.
The refcount_t api was introduced to address resource lifetime problems
related to atomic_t wrapping. There is a large overlap between the
atomic_t api used for resource lifetimes and just counters, stats, and
sequence numbers. It has become difficult to differentiate between the
atomic_t usages that should be converted to refcount_t and the ones that
can be left alone. Introducing seqnum_ops to wrap the usages that are
stats, counters, sequence numbers makes it easier for 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.

Sequence Number api provides interfaces for simple atomic_t counter usages
that just count, and don't guard resource lifetimes. The seqnum_ops are
built on top of atomic_t api, providing a smaller subset of atomic_t
interfaces necessary to support atomic_t usages as simple counters.
This api has init/set/inc/dec/read and doesn't support other atomic_t
ops with the intent to restrict the use of these interfaces as simple
counting usages.

Sequence Numbers wrap around to INT_MIN when it overflows and should not
be used to guard resource lifetimes, device usage and open counts that
control state changes, and pm states. Overflowing to INT_MIN is consistent
with the atomic_t api, which it is built on top of.

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

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 Documentation/core-api/seqnum_ops.rst          |  9 +++++++++
 MAINTAINERS                                    |  1 +
 include/linux/seqnum_ops.h                     |  2 ++
 tools/testing/selftests/lib/Makefile           |  1 +
 tools/testing/selftests/lib/config             |  1 +
 tools/testing/selftests/lib/test_seqnum_ops.sh | 10 ++++++++++
 6 files changed, 24 insertions(+)
 create mode 100755 tools/testing/selftests/lib/test_seqnum_ops.sh

diff --git a/Documentation/core-api/seqnum_ops.rst b/Documentation/core-api/seqnum_ops.rst
index 7a396c2cda19..3a9ddba985f2 100644
--- a/Documentation/core-api/seqnum_ops.rst
+++ b/Documentation/core-api/seqnum_ops.rst
@@ -115,3 +115,12 @@ Decrements sequence number and doesn't return the new value. ::
 
         seqnum32_dec() --> atomic_dec()
         seqnum64_dec() --> atomic64_dec()
+
+Where are the seqnum_ops and how to use and test them?
+------------------------------------------------------
+
+.. kernel-doc:: include/linux/seqnum_ops.h
+
+Please see lib/test_seqnum_ops.c for examples usages.
+Please find selftest: testing/selftests/lib/test_seqnum_ops.sh
+Please check dmesg for results after running test_seqnum_ops.sh.
diff --git a/MAINTAINERS b/MAINTAINERS
index c83a6f05610b..e6ae131836a5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15983,6 +15983,7 @@ L:	linux-kernel@vger.kernel.org
 S:	Maintained
 F:	include/linux/seqnum_ops.h
 F:	lib/test_seqnum_ops.c
+F:	tools/testing/selftests/lib/test_seqnum_ops.sh
 
 SIMPLE FIRMWARE INTERFACE (SFI)
 S:	Obsolete
diff --git a/include/linux/seqnum_ops.h b/include/linux/seqnum_ops.h
index b97c7f310beb..a1def2ad5bc2 100644
--- a/include/linux/seqnum_ops.h
+++ b/include/linux/seqnum_ops.h
@@ -28,6 +28,8 @@
  *
  * Reference and API guide:
  *	Documentation/core-api/seqnum_ops.rst for more information.
+ *	lib/test_seqnum_ops.c - example usages
+ *	tools/testing/selftests/lib/test_seqnum_ops.sh
  *
  */
 
diff --git a/tools/testing/selftests/lib/Makefile b/tools/testing/selftests/lib/Makefile
index a105f094676e..1818444f0e97 100644
--- a/tools/testing/selftests/lib/Makefile
+++ b/tools/testing/selftests/lib/Makefile
@@ -5,5 +5,6 @@
 all:
 
 TEST_PROGS := printf.sh bitmap.sh prime_numbers.sh strscpy.sh
+TEST_PROGS += test_seqnum_ops.sh
 
 include ../lib.mk
diff --git a/tools/testing/selftests/lib/config b/tools/testing/selftests/lib/config
index b80ee3f6e265..674ed2a2ac82 100644
--- a/tools/testing/selftests/lib/config
+++ b/tools/testing/selftests/lib/config
@@ -3,3 +3,4 @@ CONFIG_TEST_BITMAP=m
 CONFIG_PRIME_NUMBERS=m
 CONFIG_TEST_STRSCPY=m
 CONFIG_TEST_BITOPS=m
+CONFIG_TEST_SEQNUM_OPS=m
diff --git a/tools/testing/selftests/lib/test_seqnum_ops.sh b/tools/testing/selftests/lib/test_seqnum_ops.sh
new file mode 100755
index 000000000000..fdce16b220ba
--- /dev/null
+++ b/tools/testing/selftests/lib/test_seqnum_ops.sh
@@ -0,0 +1,10 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (c) 2020 Shuah Khan <skhan@linuxfoundation.org>
+# Copyright (c) 2020 The Linux Foundation
+#
+# Tests the Sequence Number Ops interfaces using test_seqnum_ops
+# kernel module
+#
+$(dirname $0)/../kselftest/module.sh "test_seqnum_ops" test_seqnum_ops
-- 
2.27.0

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Greg KH]

On Tue, Nov 10, 2020 at 12:53:27PM -0700, Shuah Khan wrote:
> +Decrement interface
> +-------------------
> +
> +Decrements sequence number and doesn't return the new value. ::
> +
> +        seqnum32_dec() --> atomic_dec()
> +        seqnum64_dec() --> atomic64_dec()

Why would you need to decrement a sequence number?  Shouldn't they just
always go up?

I see you use them in your patch 12/13, but I don't think that really is
a sequence number there, but rather just some other odd value :)

thanks,

greg k-h

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Greg KH]

On Tue, Nov 10, 2020 at 09:41:40PM +0100, Greg KH wrote:
> On Tue, Nov 10, 2020 at 12:53:27PM -0700, Shuah Khan wrote:
> > +Decrement interface
> > +-------------------
> > +
> > +Decrements sequence number and doesn't return the new value. ::
> > +
> > +        seqnum32_dec() --> atomic_dec()
> > +        seqnum64_dec() --> atomic64_dec()
> 
> Why would you need to decrement a sequence number?  Shouldn't they just
> always go up?
> 
> I see you use them in your patch 12/13, but I don't think that really is
> a sequence number there, but rather just some other odd value :)

Note, other than this, I like the idea.  It makes it obvious what these
atomic variables are being used for, and they can't be abused for other
things.  Nice work.

thanks,

greg k-h

Re: [PATCH 00/13] Introduce seqnum_ops

[Alan Stern]

On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
> There are a number of atomic_t usages in the kernel where atomic_t api
> is used strictly for counting sequence numbers and other statistical
> counters and not for managing object lifetime.
> 
> The purpose of these Sequence Number Ops is to clearly differentiate
> atomic_t counter usages from atomic_t usages that guard object lifetimes,
> hence prone to overflow and underflow errors.
> 
> The atomic_t api provides a wide range of atomic operations as a base
> api to implement atomic counters, bitops, spinlock interfaces. The usages
> also evolved into being used for resource lifetimes and state management.
> The refcount_t api was introduced to address resource lifetime problems
> related to atomic_t wrapping. There is a large overlap between the
> atomic_t api used for resource lifetimes and just counters, stats, and
> sequence numbers. It has become difficult to differentiate between the
> atomic_t usages that should be converted to refcount_t and the ones that
> can be left alone. Introducing seqnum_ops to wrap the usages that are
> stats, counters, sequence numbers makes it easier for 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.
> 
> Sequence Number api provides interfaces for simple atomic_t counter usages
> that just count, and don't guard resource lifetimes. The seqnum_ops are
> built on top of atomic_t api, providing a smaller subset of atomic_t
> interfaces necessary to support atomic_t usages as simple counters.
> This api has init/set/inc/dec/read and doesn't support any other atomic_t
> ops with the intent to restrict the use of these interfaces as simple
> counting usages.
> 
> Sequence Numbers wrap around to INT_MIN when it overflows and should not
> be used to guard resource lifetimes, device usage and open counts that
> control state changes, and pm states. Overflowing to INT_MIN is consistent
> with the atomic_t api, which it is built on top of.

If Sequence Numbers are subject to wraparound then they aren't reliable.  
Given that they aren't reliable, why use atomic instructions at all?  
Why not just use plain regular integers with READ_ONCE and WRITE_ONCE?

Alan Stern

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Matthew Wilcox]

On Tue, Nov 10, 2020 at 12:53:27PM -0700, Shuah Khan wrote:
> Sequence Numbers wrap around to INT_MIN when it overflows and should not

Why would sequence numbers be signed?  I know they're built on top of
atomic_t, which is signed, but conceptually a sequence number is unsigned.

> +++ b/Documentation/core-api/seqnum_ops.rst
> @@ -0,0 +1,117 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +.. include:: <isonum.txt>
> +
> +.. _seqnum_ops:
> +
> +==========================
> +Sequence Number Operations
> +==========================
> +
> +:Author: Shuah Khan
> +:Copyright: |copy| 2020, The Linux Foundation
> +:Copyright: |copy| 2020, Shuah Khan <skhan@linuxfoundation.org>
> +
> +There are a number of atomic_t usages in the kernel where atomic_t api
> +is used strictly for counting sequence numbers and other statistical
> +counters and not for managing object lifetime.

You start by describing why this was introduced.  I think rather, you
should start by describing what this is.  You can compare and contrast
it with atomic_t later.  Also, I don't think it's necessary to describe
its implementation in this document.  This document should explain to
someone why they want to use this.

> +Read interface
> +--------------
> +
> +Reads and returns the current value. ::
> +
> +        seqnum32_read() --> atomic_read()
> +        seqnum64_read() --> atomic64_read()
> +
> +Increment interface
> +-------------------
> +
> +Increments sequence number and doesn't return the new value. ::
> +
> +        seqnum32_inc() --> atomic_inc()
> +        seqnum64_inc() --> atomic64_inc()

That seems odd to me.  For many things, I want to know what the
sequence number was incremented to.  Obviously seqnum_inc(); followed
by seqnum_read(); is racy.

Do we really want to be explicit about seqnum32 being 32-bit?
I'd be inclined to have seqnum/seqnum64 instead of seqnum32/seqnum64.

> +static inline int seqnum32_read(const struct seqnum32 *seq)
> +{
> +	return atomic_read(&seq->seqnum);
> +}
> +
> +/*
> + * seqnum32_set() - set seqnum value
> + * @seq: struct seqnum32 pointer
> + * @val: new value to set
> + *
> + */
> +static inline void
> +seqnum32_set(struct seqnum32 *seq, int val)

You have some odd formatting like the above line split.

> +static inline void seqnum64_dec(
> +				struct seqnum64 *seq)

That one is particularly weird.

Re: [PATCH 00/13] Introduce seqnum_ops

[Shuah Khan]

On 11/10/20 1:44 PM, Alan Stern wrote:
> On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
>> There are a number of atomic_t usages in the kernel where atomic_t api
>> is used strictly for counting sequence numbers and other statistical
>> counters and not for managing object lifetime.
>>
>> The purpose of these Sequence Number Ops is to clearly differentiate
>> atomic_t counter usages from atomic_t usages that guard object lifetimes,
>> hence prone to overflow and underflow errors.
>>
>> The atomic_t api provides a wide range of atomic operations as a base
>> api to implement atomic counters, bitops, spinlock interfaces. The usages
>> also evolved into being used for resource lifetimes and state management.
>> The refcount_t api was introduced to address resource lifetime problems
>> related to atomic_t wrapping. There is a large overlap between the
>> atomic_t api used for resource lifetimes and just counters, stats, and
>> sequence numbers. It has become difficult to differentiate between the
>> atomic_t usages that should be converted to refcount_t and the ones that
>> can be left alone. Introducing seqnum_ops to wrap the usages that are
>> stats, counters, sequence numbers makes it easier for 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.
>>
>> Sequence Number api provides interfaces for simple atomic_t counter usages
>> that just count, and don't guard resource lifetimes. The seqnum_ops are
>> built on top of atomic_t api, providing a smaller subset of atomic_t
>> interfaces necessary to support atomic_t usages as simple counters.
>> This api has init/set/inc/dec/read and doesn't support any other atomic_t
>> ops with the intent to restrict the use of these interfaces as simple
>> counting usages.
>>
>> Sequence Numbers wrap around to INT_MIN when it overflows and should not
>> be used to guard resource lifetimes, device usage and open counts that
>> control state changes, and pm states. Overflowing to INT_MIN is consistent
>> with the atomic_t api, which it is built on top of.
> 
> If Sequence Numbers are subject to wraparound then they aren't reliable.
> Given that they aren't reliable, why use atomic instructions at all?
> Why not just use plain regular integers with READ_ONCE and WRITE_ONCE?
> 

You still need atomic update for these numbers. The intent is to provide
atomic api for cases where the variable doesn't guard lifetimes and yet
needs atomic instructions.

Several such usages where atomic_t is used for up counting, also use
upper bounds. It is also an option to switch to seqnum64 to avoid
wrap around in case there is a concern.

thanks,
-- Shuah

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Shuah Khan]

On 11/10/20 2:03 PM, Matthew Wilcox wrote:
> On Tue, Nov 10, 2020 at 12:53:27PM -0700, Shuah Khan wrote:
>> Sequence Numbers wrap around to INT_MIN when it overflows and should not
> 
> Why would sequence numbers be signed?  I know they're built on top of
> atomic_t, which is signed, but conceptually a sequence number is unsigned.

Yes we have some instances where unsigned is being used. I considered
going to unsigned. Changing the API to unsigned has other ramifications
and cascading changes to current atomic_t usages that are up counters.

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

A total of 53 out of 6080 atomic_read() usages force return type to
unsigned.

git grep -E '\((unsigned|unsigned int|u32)\).*\batomic.*(inc_return)' | 
wc -l
11

A total of 11 out of 620 atomic_inc_return() usages force return type
to unsigned.

Changing the API to unsigned has other ramifications and cascading
changes to current atomic_t usages that are up counters.

We could add unsigned to seqnum_ops though.

> 
>> +++ b/Documentation/core-api/seqnum_ops.rst
>> @@ -0,0 +1,117 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +.. include:: <isonum.txt>
>> +
>> +.. _seqnum_ops:
>> +
>> +==========================
>> +Sequence Number Operations
>> +==========================
>> +
>> +:Author: Shuah Khan
>> +:Copyright: |copy| 2020, The Linux Foundation
>> +:Copyright: |copy| 2020, Shuah Khan <skhan@linuxfoundation.org>
>> +
>> +There are a number of atomic_t usages in the kernel where atomic_t api
>> +is used strictly for counting sequence numbers and other statistical
>> +counters and not for managing object lifetime.
> 
> You start by describing why this was introduced.  I think rather, you
> should start by describing what this is.  You can compare and contrast
> it with atomic_t later.  Also, I don't think it's necessary to describe
> its implementation in this document.  This document should explain to
> someone why they want to use this.
> 
>> +Read interface
>> +--------------
>> +
>> +Reads and returns the current value. ::
>> +
>> +        seqnum32_read() --> atomic_read()
>> +        seqnum64_read() --> atomic64_read()
>> +
>> +Increment interface
>> +-------------------
>> +
>> +Increments sequence number and doesn't return the new value. ::
>> +
>> +        seqnum32_inc() --> atomic_inc()
>> +        seqnum64_inc() --> atomic64_inc()
> 
> That seems odd to me.  For many things, I want to know what the
> sequence number was incremented to.  Obviously seqnum_inc(); followed
> by seqnum_read(); is racy.
> 
> Do we really want to be explicit about seqnum32 being 32-bit?
> I'd be inclined to have seqnum/seqnum64 instead of seqnum32/seqnum64.
> 
>> +static inline int seqnum32_read(const struct seqnum32 *seq)
>> +{
>> +	return atomic_read(&seq->seqnum);
>> +}
>> +
>> +/*
>> + * seqnum32_set() - set seqnum value
>> + * @seq: struct seqnum32 pointer
>> + * @val: new value to set
>> + *
>> + */
>> +static inline void
>> +seqnum32_set(struct seqnum32 *seq, int val)
>  > You have some odd formatting like the above line split.
> 
>> +static inline void seqnum64_dec(
>> +				struct seqnum64 *seq)
> 
> That one is particularly weird.
> 

Thanks for catching these. This code needed cleanup after the
rename from a looong names from previous version.

thanks,
-- Shuah

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Kees Cook]

On Tue, Nov 10, 2020 at 09:43:02PM +0100, Greg KH wrote:
> On Tue, Nov 10, 2020 at 09:41:40PM +0100, Greg KH wrote:
> > On Tue, Nov 10, 2020 at 12:53:27PM -0700, Shuah Khan wrote:
> > > +Decrement interface
> > > +-------------------
> > > +
> > > +Decrements sequence number and doesn't return the new value. ::
> > > +
> > > +        seqnum32_dec() --> atomic_dec()
> > > +        seqnum64_dec() --> atomic64_dec()
> > 
> > Why would you need to decrement a sequence number?  Shouldn't they just
> > always go up?
> > 
> > I see you use them in your patch 12/13, but I don't think that really is
> > a sequence number there, but rather just some other odd value :)

To that end, they should likely be internally cast to u32 and u64 (and
why is seqnum64 ifdef on CONFIG_64BIT?).

> Note, other than this, I like the idea.  It makes it obvious what these
> atomic variables are being used for, and they can't be abused for other
> things.  Nice work.

Agreed: this is a clear wrapping sequence counter. It's only abuse would
be using it in a place where wrapping actually is _not_ safe. (bikeshed:
can we call it wrap_u32 and wrap_u64?)

-- 
Kees Cook

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Kees Cook]

On Tue, Nov 10, 2020 at 03:58:48PM -0700, Shuah Khan wrote:
> Yes we have some instances where unsigned is being used. I considered
> going to unsigned. Changing the API to unsigned has other ramifications
> and cascading changes to current atomic_t usages that are up counters.

As in, existing callers expect the "read" value to be int?

-- 
Kees Cook

Re: [PATCH 00/13] Introduce seqnum_ops

[Matthew Wilcox]

On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
> There are a number of atomic_t usages in the kernel where atomic_t api
> is used strictly for counting sequence numbers and other statistical
> counters and not for managing object lifetime.

We already have something in Linux called a sequence counter, and it's
different from this.  ID counter?  instance number?  monotonic_t?  stat_t?

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Peter Zijlstra]

On Tue, Nov 10, 2020 at 12:53:27PM -0700, Shuah Khan wrote:

> + * The interface provides:
> + * seqnum32 & seqnum64 functions:
> + *	initialization
> + *	set
> + *	read
> + *	increment and no return
> + *	decrement and no return

NAK, this is batshit insane again. If you want a sequence number, the
one and _ONLY_ primitive you want to expose is inc_return.

No set, no read, no inc, and most certainly, not dec.

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Shuah Khan]

On 11/10/20 5:20 PM, Kees Cook wrote:
> On Tue, Nov 10, 2020 at 03:58:48PM -0700, Shuah Khan wrote:
>> Yes we have some instances where unsigned is being used. I considered
>> going to unsigned. Changing the API to unsigned has other ramifications
>> and cascading changes to current atomic_t usages that are up counters.
> 
> As in, existing callers expect the "read" value to be int?
> 

Correct. In addition, these values are returned by other functions as
signed and it will result in lot of churn to use unsigned.

thanks,
-- Shuah

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Shuah Khan]

On 11/11/20 1:23 AM, Peter Zijlstra wrote:
> On Tue, Nov 10, 2020 at 12:53:27PM -0700, Shuah Khan wrote:
> 
>> + * The interface provides:
>> + * seqnum32 & seqnum64 functions:
>> + *	initialization
>> + *	set
>> + *	read
>> + *	increment and no return
>> + *	decrement and no return
> 
> NAK, this is batshit insane again.

Gosh that is a bit much. Definitely will never be part of my kernel
review/response vocabulary.

If you want a sequence number, the
> one and _ONLY_ primitive you want to expose is inc_return.
> 
> No set, no read, no inc, and most certainly, not dec.
> 

Agree with you on removing dec(). It isn't needed or up counting.
set() can go and use just init instead of set.

read and inc are needed for sure though. The reason being numbers
could be incremented in one place and read in other places. In some
cases inc_return is used.

Why would you say no to read and inc?

init, read, inc, and inc_return are necessary to be able to implement
up counters.

thanks,
-- Shuah

Re: [PATCH 00/13] Introduce seqnum_ops

[Shuah Khan]

On 11/10/20 9:33 PM, Matthew Wilcox wrote:
> On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
>> There are a number of atomic_t usages in the kernel where atomic_t api
>> is used strictly for counting sequence numbers and other statistical
>> counters and not for managing object lifetime.
> 
> We already have something in Linux called a sequence counter, and it's
> different from this.  ID counter?  instance number?  monotonic_t?  stat_t?
> 

No results for monotonic_t or stat_t. Can you give me a pointer to what
your referring to.

thanks,
-- Shuah

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Peter Zijlstra]

On Wed, Nov 11, 2020 at 08:56:49AM -0700, Shuah Khan wrote:

> Why would you say no to read and inc?

Because they don't guarantee uniqueness (bar wrapping), which is the
only reason to use an atomic to begin with.

Re: [PATCH 00/13] Introduce seqnum_ops

[Matthew Wilcox]

On Wed, Nov 11, 2020 at 09:03:20AM -0700, Shuah Khan wrote:
> On 11/10/20 9:33 PM, Matthew Wilcox wrote:
> > On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
> > > There are a number of atomic_t usages in the kernel where atomic_t api
> > > is used strictly for counting sequence numbers and other statistical
> > > counters and not for managing object lifetime.
> > 
> > We already have something in Linux called a sequence counter, and it's
> > different from this.  ID counter?  instance number?  monotonic_t?  stat_t?
> > 
> 
> No results for monotonic_t or stat_t. Can you give me a pointer to what
> your referring to.

We have a seqcount_t.  We need to call this something different.
maybe we should call it stat_t (and for that usage, stat_add() as well
as stat_inc() is a legitimate API to have).

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Shuah Khan]

On 11/11/20 9:04 AM, Peter Zijlstra wrote:
> On Wed, Nov 11, 2020 at 08:56:49AM -0700, Shuah Khan wrote:
> 
>> Why would you say no to read and inc?
> 
> Because they don't guarantee uniqueness (bar wrapping), which is the
> only reason to use an atomic to begin with.
> 

Thanks for the explanation. I see what you are saying.

Not sure what to make of the 6080 atomic_read()s and 3413
atomic_inc()s, some of which might be assuming uniqueness
guarantee.

As far as the sequence number api is concerned, I am with you on
not exposing read() and inc().

inc()s can just map to inc_return().

For read():
In the context of up counters, there is a definitely a need for get
current value type interface that guarantees uniqueness - similar to
inc_return without actually incrementing.

I will work on v2 based on the discussion.

thanks,
-- Shuah

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Peter Zijlstra]

On Wed, Nov 11, 2020 at 10:34:05AM -0700, Shuah Khan wrote:

> Not sure what to make of the 6080 atomic_read()s and 3413
> atomic_inc()s, some of which might be assuming uniqueness
> guarantee.

Well, clearly you just did: git grep atimic_{read,inc}() | wc -l and
didn't look at the usage. Equally clearly there can be bugs. Also
evidently much of those are not in fact sequence numbers.

All I'm saying is that if you want a sequence number, inc_return (or
fetch_inc) is the only sane option.

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Shuah Khan]

On 11/11/20 10:50 AM, Peter Zijlstra wrote:
> On Wed, Nov 11, 2020 at 10:34:05AM -0700, Shuah Khan wrote:
> 
>> Not sure what to make of the 6080 atomic_read()s and 3413
>> atomic_inc()s, some of which might be assuming uniqueness
>> guarantee.
> 
> Well, clearly you just did: git grep atimic_{read,inc}() | wc -l and
> didn't look at the usage. Equally clearly there can be bugs. Also
> evidently much of those are not in fact sequence numbers.
> 

Looking at the usage and classifying which usages are sequence
numbers is part of may audit and we are covered. Your explanation
and this discussion helps with do a better audit of these usages.

> All I'm saying is that if you want a sequence number, inc_return (or
> fetch_inc) is the only sane option.
> 

Cool.

thanks,
-- Shuah

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Shuah Khan]

On 11/10/20 5:18 PM, Kees Cook wrote:
> On Tue, Nov 10, 2020 at 09:43:02PM +0100, Greg KH wrote:
>> On Tue, Nov 10, 2020 at 09:41:40PM +0100, Greg KH wrote:
>>> On Tue, Nov 10, 2020 at 12:53:27PM -0700, Shuah Khan wrote:
>>>> +Decrement interface
>>>> +-------------------
>>>> +
>>>> +Decrements sequence number and doesn't return the new value. ::
>>>> +
>>>> +        seqnum32_dec() --> atomic_dec()
>>>> +        seqnum64_dec() --> atomic64_dec()
>>>
>>> Why would you need to decrement a sequence number?  Shouldn't they just
>>> always go up?
>>>
>>> I see you use them in your patch 12/13, but I don't think that really is
>>> a sequence number there, but rather just some other odd value :)
> 
> To that end, they should likely be internally cast to u32 and u64 (and
> why is seqnum64 ifdef on CONFIG_64BIT?).
> 

I had a reason for doing this, can't recall. I will revisit and remove
it which is ideal. I have to look at CONFIG_GENERIC_ATOMIC64 as well.

Not seeing any errors with my config which has CONFIG_64BIT=y


>> Note, other than this, I like the idea.  It makes it obvious what these
>> atomic variables are being used for, and they can't be abused for other
>> things.  Nice work.
> 

Thanks.

> Agreed: this is a clear wrapping sequence counter. It's only abuse would
> be using it in a place where wrapping actually is _not_ safe. (bikeshed:
> can we call it wrap_u32 and wrap_u64?)
> 

Still like seqnum_ops.

There is seqcount_t in seqlock.h which is a totally different feature.

thanks,
-- Shuah

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Peter Zijlstra]

On Wed, Nov 11, 2020 at 11:28:13AM -0700, Shuah Khan wrote:
> On 11/11/20 10:50 AM, Peter Zijlstra wrote:
> > On Wed, Nov 11, 2020 at 10:34:05AM -0700, Shuah Khan wrote:
> > 
> > > Not sure what to make of the 6080 atomic_read()s and 3413
> > > atomic_inc()s, some of which might be assuming uniqueness
> > > guarantee.
> > 
> > Well, clearly you just did: git grep atimic_{read,inc}() | wc -l and
> > didn't look at the usage. Equally clearly there can be bugs. Also
> > evidently much of those are not in fact sequence numbers.
> > 
> 
> Looking at the usage and classifying which usages are sequence
> numbers is part of may audit and we are covered. Your explanation
> and this discussion helps with do a better audit of these usages.

Auditing is fine, but I still don't see any point in actually having
these wrapping types. It's all a waste of space and compile-time IMO.

Neither this sequence counter, nor stat_t or whatever else bring any
actual differences. They're pure wrappers without change in semantics.

refcount_t is useful because it brought different semantics, it raises
exceptions on invalid usage (wraps). But this is just pointless NOPs.

So do your audit, but only introduce new types for things that actually
have different semantics. If you do a patch and the generated code is
100% identical but you have many more lines of code, you've only made it
worse.

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Matthew Wilcox]

On Wed, Nov 11, 2020 at 12:23:03PM -0700, Shuah Khan wrote:
> > Agreed: this is a clear wrapping sequence counter. It's only abuse would
> > be using it in a place where wrapping actually is _not_ safe. (bikeshed:
> > can we call it wrap_u32 and wrap_u64?)
> 
> Still like seqnum_ops.
> 
> There is seqcount_t in seqlock.h which is a totally different feature.

Yes, and that's why this new thing, whatever it is called should not
have the word "sequence" in it.  People will get it confused.  Also,
"ops" in Linux means "vector of methods", like a_ops, f_op, i_op, fl_ops.

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Greg KH]

On Wed, Nov 11, 2020 at 09:15:55PM +0100, Peter Zijlstra wrote:
> On Wed, Nov 11, 2020 at 11:28:13AM -0700, Shuah Khan wrote:
> > On 11/11/20 10:50 AM, Peter Zijlstra wrote:
> > > On Wed, Nov 11, 2020 at 10:34:05AM -0700, Shuah Khan wrote:
> > > 
> > > > Not sure what to make of the 6080 atomic_read()s and 3413
> > > > atomic_inc()s, some of which might be assuming uniqueness
> > > > guarantee.
> > > 
> > > Well, clearly you just did: git grep atimic_{read,inc}() | wc -l and
> > > didn't look at the usage. Equally clearly there can be bugs. Also
> > > evidently much of those are not in fact sequence numbers.
> > > 
> > 
> > Looking at the usage and classifying which usages are sequence
> > numbers is part of may audit and we are covered. Your explanation
> > and this discussion helps with do a better audit of these usages.
> 
> Auditing is fine, but I still don't see any point in actually having
> these wrapping types. It's all a waste of space and compile-time IMO.
> 
> Neither this sequence counter, nor stat_t or whatever else bring any
> actual differences. They're pure wrappers without change in semantics.
> 
> refcount_t is useful because it brought different semantics, it raises
> exceptions on invalid usage (wraps). But this is just pointless NOPs.
> 
> So do your audit, but only introduce new types for things that actually
> have different semantics. If you do a patch and the generated code is
> 100% identical but you have many more lines of code, you've only made it
> worse.

I'm sorry, but as someone who reviews the second-most code in the
kernel, I have to disagree.  If I see a "raw" atomic_t being used in a
driver, I then have to look up all instances of where that variable is
being used, to verify what they are using it for, why they are using,
and if all of the means they are really using it in the correct way.

Always remember that atomic_t is way down there on the "Rusty scale of
designing an API you can use properly" scale:
	https://ozlabs.org/~rusty/ols-2003-keynote/img46.html

If I see a sequence_t variable (or whatever we end up calling it), then
I instantly KNOW what this is for, and that is is impossible to get it
wrong when using it as the API for that variable prevents it from being
misused in horrible ways (like setting it to a value and decrementing
it.)

If me, as a kernel developer, wants to add a sequence number to my
driver, yes, I can "open code" one using an atomic_t and get it right
(or just use a u64 like we do for uevents), but then when I go back and
look at the code in 5 years, I have to try to remember exactly what I
did and where it is used and try to ensure that no one changed it
incorrectly.  Again, if this is a sequence_t, all of that goes away.

So this doesn't save codespace, or generated code, it saves mental
energy which is the most limited resource we have.  We write code for
the developers first, the compiler and cpu second, in order to create
something that us developers can maintain for long periods of time.
Kernel code is not like perl (write once, modify never), but like laws
(write once, modify constantly).

Remember us poor maintainers, who are doing the reviewing, and the
junior developers, creating new drivers where they have to implement
common features/patterns and the people that come after us and curse our
name as they try to understand exactly what a specific atomic_t was
supposed to be doing.  We want to make all of our lives easier, and this
type of api does just that.

thanks,

greg k-h

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Shuah Khan]

On 11/12/20 5:36 AM, Matthew Wilcox wrote:
> On Wed, Nov 11, 2020 at 12:23:03PM -0700, Shuah Khan wrote:
>>> Agreed: this is a clear wrapping sequence counter. It's only abuse would
>>> be using it in a place where wrapping actually is _not_ safe. (bikeshed:
>>> can we call it wrap_u32 and wrap_u64?)
>>
>> Still like seqnum_ops.
>>
>> There is seqcount_t in seqlock.h which is a totally different feature.
> 
> Yes, and that's why this new thing, whatever it is called should not
> have the word "sequence" in it.  People will get it confused. 

Any suggestions for name. I am bad with coming up with names. How does
Statcnt API and struct statcnt along the lines of your name suggestions
in your previous email?

> "ops" in Linux means "vector of methods", like a_ops, f_op, i_op, fl_ops.
> 

We also have "this_cpu_ops, atomic_ops, local_ops" etc. core-api.

The ops in the name is to keep with that nomenclature since these
are atomic ops.

thanks,
-- Shuah

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Greg KH]

On Thu, Nov 12, 2020 at 09:17:27AM -0700, Shuah Khan wrote:
> On 11/12/20 5:36 AM, Matthew Wilcox wrote:
> > On Wed, Nov 11, 2020 at 12:23:03PM -0700, Shuah Khan wrote:
> > > > Agreed: this is a clear wrapping sequence counter. It's only abuse would
> > > > be using it in a place where wrapping actually is _not_ safe. (bikeshed:
> > > > can we call it wrap_u32 and wrap_u64?)
> > > 
> > > Still like seqnum_ops.
> > > 
> > > There is seqcount_t in seqlock.h which is a totally different feature.
> > 
> > Yes, and that's why this new thing, whatever it is called should not
> > have the word "sequence" in it.  People will get it confused.
> 
> Any suggestions for name. I am bad with coming up with names. How does
> Statcnt API and struct statcnt along the lines of your name suggestions
> in your previous email?

What does "stat" mean here?

And I don't understand the hesitation about "sequence" in a name, as
that's exactly what this is.  seqlock is different, yes.

How about "seqnum_t"?  That's what we call the sequence number that we
export to uevents, a "SEQNUM".

thanks,

greg k-h

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Shuah Khan]

On 11/12/20 9:45 AM, Greg KH wrote:
> On Thu, Nov 12, 2020 at 09:17:27AM -0700, Shuah Khan wrote:
>> On 11/12/20 5:36 AM, Matthew Wilcox wrote:
>>> On Wed, Nov 11, 2020 at 12:23:03PM -0700, Shuah Khan wrote:
>>>>> Agreed: this is a clear wrapping sequence counter. It's only abuse would
>>>>> be using it in a place where wrapping actually is _not_ safe. (bikeshed:
>>>>> can we call it wrap_u32 and wrap_u64?)
>>>>
>>>> Still like seqnum_ops.
>>>>
>>>> There is seqcount_t in seqlock.h which is a totally different feature.
>>>
>>> Yes, and that's why this new thing, whatever it is called should not
>>> have the word "sequence" in it.  People will get it confused.
>>
>> Any suggestions for name. I am bad with coming up with names. How does
>> Statcnt API and struct statcnt along the lines of your name suggestions
>> in your previous email?
> 
> What does "stat" mean here?
> 

Stat doesn't really reflect what we are trying to do here and sequence
does. I am just looking to address confusion if any and make a call.

> And I don't understand the hesitation about "sequence" in a name, as
> that's exactly what this is.  seqlock is different, yes.
>  > How about "seqnum_t"?  That's what we call the sequence number that we
> export to uevents, a "SEQNUM".
> 

Good point.
This is what we have currently in patch v1 and let's just go with it.

thanks,
-- Shuah

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Shuah Khan]

On 11/11/20 12:23 PM, Shuah Khan wrote:
> On 11/10/20 5:18 PM, Kees Cook wrote:
>> On Tue, Nov 10, 2020 at 09:43:02PM +0100, Greg KH wrote:
>>> On Tue, Nov 10, 2020 at 09:41:40PM +0100, Greg KH wrote:
>>>> On Tue, Nov 10, 2020 at 12:53:27PM -0700, Shuah Khan wrote:
>>>>> +Decrement interface
>>>>> +-------------------
>>>>> +
>>>>> +Decrements sequence number and doesn't return the new value. ::
>>>>> +
>>>>> +        seqnum32_dec() --> atomic_dec()
>>>>> +        seqnum64_dec() --> atomic64_dec()
>>>>
>>>> Why would you need to decrement a sequence number?  Shouldn't they just
>>>> always go up?
>>>>
>>>> I see you use them in your patch 12/13, but I don't think that 
>>>> really is
>>>> a sequence number there, but rather just some other odd value :)
>>
>> To that end, they should likely be internally cast to u32 and u64 (and
>> why is seqnum64 ifdef on CONFIG_64BIT?).
>>
> 

atomic64_t depends on CONFIG_64BIT

include/linux/types.h

#ifdef CONFIG_64BIT
typedef struct {
         s64 counter;
} atomic64_t;
#endif

thanks,
-- Shuah

Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops

[Peter Zijlstra]

On Thu, Nov 12, 2020 at 02:27:49PM -0700, Shuah Khan wrote:

> atomic64_t depends on CONFIG_64BIT
> 
> include/linux/types.h
> 
> #ifdef CONFIG_64BIT
> typedef struct {
>         s64 counter;
> } atomic64_t;
> #endif

That's because some 32bit archs need to override the type definition.
atomic64_t is available on 32bit, although sometimes it is atrocious
crap.