[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

[PATCH 03/13] drivers/acpi: convert seqno seqnum_ops

[Shuah Khan]

seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.

seqnum32 variables 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.

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

Convert it to use seqnum_ops. This conversion replaces inc_return()
with _inc() followed by _read().

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/acpi/acpi_extlog.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 72f1fb77abcd..1e2b36aab9aa 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -12,6 +12,7 @@
 #include <linux/ratelimit.h>
 #include <linux/edac.h>
 #include <linux/ras.h>
+#include <linux/seqnum_ops.h>
 #include <asm/cpu.h>
 #include <asm/mce.h>
 
@@ -93,7 +94,7 @@ static struct acpi_hest_generic_status *extlog_elog_entry_check(int cpu, int ban
 static void __print_extlog_rcd(const char *pfx,
 			       struct acpi_hest_generic_status *estatus, int cpu)
 {
-	static atomic_t seqno;
+	static struct seqnum32 seqno;
 	unsigned int curr_seqno;
 	char pfx_seq[64];
 
@@ -103,7 +104,8 @@ static void __print_extlog_rcd(const char *pfx,
 		else
 			pfx = KERN_ERR;
 	}
-	curr_seqno = atomic_inc_return(&seqno);
+	seqnum32_inc(&seqno);
+	curr_seqno = seqnum32_read(&seqno);
 	snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}", pfx, curr_seqno);
 	printk("%s""Hardware error detected on CPU%d\n", pfx_seq, cpu);
 	cper_estatus_print(pfx_seq, estatus);
-- 
2.27.0

[PATCH 04/13] drivers/acpi/apei: convert seqno to seqnum_ops

[Shuah Khan]

seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.

seqnum32 variables 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.

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

Convert it to use seqnum_ops. This conversion replaces inc_return()
with _inc() followed by _read().

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/acpi/apei/ghes.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index fce7ade2aba9..aa91998ce6f4 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -41,6 +41,7 @@
 #include <linux/uuid.h>
 #include <linux/ras.h>
 #include <linux/task_work.h>
+#include <linux/seqnum_ops.h>
 
 #include <acpi/actbl1.h>
 #include <acpi/ghes.h>
@@ -625,7 +626,7 @@ static void __ghes_print_estatus(const char *pfx,
 				 const struct acpi_hest_generic *generic,
 				 const struct acpi_hest_generic_status *estatus)
 {
-	static atomic_t seqno;
+	static struct seqnum32 seqno = SEQNUM_INIT(0);
 	unsigned int curr_seqno;
 	char pfx_seq[64];
 
@@ -636,7 +637,8 @@ static void __ghes_print_estatus(const char *pfx,
 		else
 			pfx = KERN_ERR;
 	}
-	curr_seqno = atomic_inc_return(&seqno);
+	seqnum32_inc(&seqno);
+	curr_seqno = seqnum32_read(&seqno);
 	snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}" HW_ERR, pfx, curr_seqno);
 	printk("%s""Hardware error from APEI Generic Hardware Error Source: %d\n",
 	       pfx_seq, generic->header.source_id);
-- 
2.27.0

[PATCH 05/13] drivers/base/test/test_async_driver_probe: convert to use seqnum_ops

[Shuah Khan]

seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.

seqnum32 variables 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.

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

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

Convert them to use seqnum32 and init counters to 0. This conversion
doesn't change the overflow wrap around behavior.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/base/test/test_async_driver_probe.c | 26 +++++++++++++--------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/base/test/test_async_driver_probe.c b/drivers/base/test/test_async_driver_probe.c
index 3bb7beb127a9..1886f79ec252 100644
--- a/drivers/base/test/test_async_driver_probe.c
+++ b/drivers/base/test/test_async_driver_probe.c
@@ -14,11 +14,15 @@
 #include <linux/numa.h>
 #include <linux/nodemask.h>
 #include <linux/topology.h>
+#include <linux/seqnum_ops.h>
 
 #define TEST_PROBE_DELAY	(5 * 1000)	/* 5 sec */
 #define TEST_PROBE_THRESHOLD	(TEST_PROBE_DELAY / 2)
 
-static atomic_t warnings, errors, timeout, async_completed;
+static struct seqnum32 warnings = SEQNUM_INIT(0);
+static struct seqnum32 errors = SEQNUM_INIT(0);
+static struct seqnum32 timeout = SEQNUM_INIT(0);
+static struct seqnum32 async_completed = SEQNUM_INIT(0);
 
 static int test_probe(struct platform_device *pdev)
 {
@@ -29,9 +33,9 @@ static int test_probe(struct platform_device *pdev)
 	 * have then report it as an error, otherwise we wil sleep for the
 	 * required amount of time and then report completion.
 	 */
-	if (atomic_read(&timeout)) {
+	if (seqnum32_read(&timeout)) {
 		dev_err(dev, "async probe took too long\n");
-		atomic_inc(&errors);
+		seqnum32_inc(&errors);
 	} else {
 		dev_dbg(&pdev->dev, "sleeping for %d msecs in probe\n",
 			 TEST_PROBE_DELAY);
@@ -48,10 +52,10 @@ static int test_probe(struct platform_device *pdev)
 		    dev_to_node(dev) != numa_node_id()) {
 			dev_warn(dev, "NUMA node mismatch %d != %d\n",
 				 dev_to_node(dev), numa_node_id());
-			atomic_inc(&warnings);
+			seqnum32_inc(&warnings);
 		}
 
-		atomic_inc(&async_completed);
+		seqnum32_inc(&async_completed);
 	}
 
 	return 0;
@@ -244,11 +248,12 @@ static int __init test_async_probe_init(void)
 	 * Otherwise if they completed without errors or warnings then
 	 * report successful completion.
 	 */
-	if (atomic_read(&async_completed) != async_id) {
+	if (seqnum32_read(&async_completed) != async_id) {
 		pr_err("async events still pending, forcing timeout\n");
-		atomic_inc(&timeout);
+		seqnum32_inc(&timeout);
 		err = -ETIMEDOUT;
-	} else if (!atomic_read(&errors) && !atomic_read(&warnings)) {
+	} else if (!seqnum32_read(&errors) &&
+		   !seqnum32_read(&warnings)) {
 		pr_info("completed successfully\n");
 		return 0;
 	}
@@ -271,12 +276,13 @@ static int __init test_async_probe_init(void)
 	 * errors or warnings being reported by the probe routine.
 	 */
 	if (err)
-		atomic_inc(&errors);
+		seqnum32_inc(&errors);
 	else
 		err = -EINVAL;
 
 	pr_err("Test failed with %d errors and %d warnings\n",
-	       atomic_read(&errors), atomic_read(&warnings));
+	       seqnum32_read(&errors),
+	       seqnum32_read(&warnings));
 
 	return err;
 }
-- 
2.27.0

[PATCH 06/13] drivers/char/ipmi: convert stats to use seqnum_ops

[Shuah Khan]

seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.

seqnum32 variables 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.

atomic_t variables used for stats are atomic counters. Overflow will
wrap around and reset the stats and no change with the conversion.

Convert them to use seqnum_ops.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/char/ipmi/ipmi_msghandler.c | 9 +++++----
 drivers/char/ipmi/ipmi_si_intf.c    | 9 +++++----
 drivers/char/ipmi/ipmi_ssif.c       | 9 +++++----
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 8774a3b8ff95..a8f03b4dade9 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -35,6 +35,7 @@
 #include <linux/nospec.h>
 #include <linux/vmalloc.h>
 #include <linux/delay.h>
+#include <linux/seqnum_ops.h>
 
 #define IPMI_DRIVER_VERSION "39.2"
 
@@ -587,7 +588,7 @@ struct ipmi_smi {
 	struct ipmi_my_addrinfo addrinfo[IPMI_MAX_CHANNELS];
 	bool channels_ready;
 
-	atomic_t stats[IPMI_NUM_STATS];
+	struct seqnum32 stats[IPMI_NUM_STATS];
 
 	/*
 	 * run_to_completion duplicate of smb_info, smi_info
@@ -633,9 +634,9 @@ static LIST_HEAD(smi_watchers);
 static DEFINE_MUTEX(smi_watchers_mutex);
 
 #define ipmi_inc_stat(intf, stat) \
-	atomic_inc(&(intf)->stats[IPMI_STAT_ ## stat])
+	seqnum32_inc(&(intf)->stats[IPMI_STAT_ ## stat])
 #define ipmi_get_stat(intf, stat) \
-	((unsigned int) atomic_read(&(intf)->stats[IPMI_STAT_ ## stat]))
+	((unsigned int) seqnum32_read(&(intf)->stats[IPMI_STAT_ ## stat]))
 
 static const char * const addr_src_to_str[] = {
 	"invalid", "hotmod", "hardcoded", "SPMI", "ACPI", "SMBIOS", "PCI",
@@ -3468,7 +3469,7 @@ int ipmi_add_smi(struct module         *owner,
 	INIT_LIST_HEAD(&intf->cmd_rcvrs);
 	init_waitqueue_head(&intf->waitq);
 	for (i = 0; i < IPMI_NUM_STATS; i++)
-		atomic_set(&intf->stats[i], 0);
+		seqnum32_set(&intf->stats[i], 0);
 
 	mutex_lock(&ipmi_interfaces_mutex);
 	/* Look for a hole in the numbers. */
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 5eac94cf4ff8..e1354076a58a 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -43,6 +43,7 @@
 #include "ipmi_si_sm.h"
 #include <linux/string.h>
 #include <linux/ctype.h>
+#include <linux/seqnum_ops.h>
 
 /* Measure times between events in the driver. */
 #undef DEBUG_TIMING
@@ -237,7 +238,7 @@ struct smi_info {
 	bool dev_group_added;
 
 	/* Counters and things for the proc filesystem. */
-	atomic_t stats[SI_NUM_STATS];
+	struct seqnum32 stats[SI_NUM_STATS];
 
 	struct task_struct *thread;
 
@@ -245,9 +246,9 @@ struct smi_info {
 };
 
 #define smi_inc_stat(smi, stat) \
-	atomic_inc(&(smi)->stats[SI_STAT_ ## stat])
+	seqnum32_inc(&(smi)->stats[SI_STAT_ ## stat])
 #define smi_get_stat(smi, stat) \
-	((unsigned int) atomic_read(&(smi)->stats[SI_STAT_ ## stat]))
+	((unsigned int) seqnum32_read(&(smi)->stats[SI_STAT_ ## stat]))
 
 #define IPMI_MAX_INTFS 4
 static int force_kipmid[IPMI_MAX_INTFS];
@@ -2030,7 +2031,7 @@ static int try_smi_init(struct smi_info *new_smi)
 	atomic_set(&new_smi->req_events, 0);
 	new_smi->run_to_completion = false;
 	for (i = 0; i < SI_NUM_STATS; i++)
-		atomic_set(&new_smi->stats[i], 0);
+		seqnum32_set(&new_smi->stats[i], 0);
 
 	new_smi->interrupt_disabled = true;
 	atomic_set(&new_smi->need_watch, 0);
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 0416b9c9d410..0e61e072b213 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -47,6 +47,7 @@
 #include <linux/acpi.h>
 #include <linux/ctype.h>
 #include <linux/time64.h>
+#include <linux/seqnum_ops.h>
 #include "ipmi_dmi.h"
 
 #define DEVICE_NAME "ipmi_ssif"
@@ -286,13 +287,13 @@ struct ssif_info {
 	unsigned int  multi_len;
 	unsigned int  multi_pos;
 
-	atomic_t stats[SSIF_NUM_STATS];
+	struct seqnum32 stats[SSIF_NUM_STATS];
 };
 
 #define ssif_inc_stat(ssif, stat) \
-	atomic_inc(&(ssif)->stats[SSIF_STAT_ ## stat])
+	seqnum32_inc(&(ssif)->stats[SSIF_STAT_ ## stat])
 #define ssif_get_stat(ssif, stat) \
-	((unsigned int) atomic_read(&(ssif)->stats[SSIF_STAT_ ## stat]))
+	((unsigned int) seqnum32_read(&(ssif)->stats[SSIF_STAT_ ## stat]))
 
 static bool initialized;
 static bool platform_registered;
@@ -1864,7 +1865,7 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	timer_setup(&ssif_info->watch_timer, watch_timeout, 0);
 
 	for (i = 0; i < SSIF_NUM_STATS; i++)
-		atomic_set(&ssif_info->stats[i], 0);
+		seqnum32_set(&ssif_info->stats[i], 0);
 
 	if (ssif_info->supports_pec)
 		ssif_info->client->flags |= I2C_CLIENT_PEC;
-- 
2.27.0

[PATCH 07/13] drivers/edac: convert pci counters to seqnum_ops

[Shuah Khan]

seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.

seqnum32 variables 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.

atomic_t variables used for pci counters keep track of pci parity and
non-parity errors. Convert them to use seqnum_ops.

Overflow will wrap around and reset the counts as was the case prior to
the conversion.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/edac/edac_pci.h       |  5 +++--
 drivers/edac/edac_pci_sysfs.c | 28 ++++++++++++++--------------
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/edac/edac_pci.h b/drivers/edac/edac_pci.h
index 5175f5724cfa..33b33e62c37f 100644
--- a/drivers/edac/edac_pci.h
+++ b/drivers/edac/edac_pci.h
@@ -30,12 +30,13 @@
 #include <linux/pci.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
+#include <linux/seqnum_ops.h>
 
 #ifdef CONFIG_PCI
 
 struct edac_pci_counter {
-	atomic_t pe_count;
-	atomic_t npe_count;
+	struct seqnum32 pe_count;
+	struct seqnum32 npe_count;
 };
 
 /*
diff --git a/drivers/edac/edac_pci_sysfs.c b/drivers/edac/edac_pci_sysfs.c
index 53042af7262e..96f950fb39b0 100644
--- a/drivers/edac/edac_pci_sysfs.c
+++ b/drivers/edac/edac_pci_sysfs.c
@@ -23,8 +23,8 @@ static int edac_pci_log_pe = 1;		/* log PCI parity errors */
 static int edac_pci_log_npe = 1;	/* log PCI non-parity error errors */
 static int edac_pci_poll_msec = 1000;	/* one second workq period */
 
-static atomic_t pci_parity_count = ATOMIC_INIT(0);
-static atomic_t pci_nonparity_count = ATOMIC_INIT(0);
+static struct seqnum32 pci_parity_count = SEQNUM_INIT(0);
+static struct seqnum32 pci_nonparity_count = SEQNUM_INIT(0);
 
 static struct kobject *edac_pci_top_main_kobj;
 static atomic_t edac_pci_sysfs_refcount = ATOMIC_INIT(0);
@@ -58,13 +58,13 @@ int edac_pci_get_poll_msec(void)
 /**************************** EDAC PCI sysfs instance *******************/
 static ssize_t instance_pe_count_show(struct edac_pci_ctl_info *pci, char *data)
 {
-	return sprintf(data, "%u\n", atomic_read(&pci->counters.pe_count));
+	return sprintf(data, "%u\n", seqnum32_read(&pci->counters.pe_count));
 }
 
 static ssize_t instance_npe_count_show(struct edac_pci_ctl_info *pci,
 				char *data)
 {
-	return sprintf(data, "%u\n", atomic_read(&pci->counters.npe_count));
+	return sprintf(data, "%u\n", seqnum32_read(&pci->counters.npe_count));
 }
 
 #define to_instance(k) container_of(k, struct edac_pci_ctl_info, kobj)
@@ -553,7 +553,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 			edac_printk(KERN_CRIT, EDAC_PCI,
 				"Signaled System Error on %s\n",
 				pci_name(dev));
-			atomic_inc(&pci_nonparity_count);
+			seqnum32_inc(&pci_nonparity_count);
 		}
 
 		if (status & (PCI_STATUS_PARITY)) {
@@ -561,7 +561,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 				"Master Data Parity Error on %s\n",
 				pci_name(dev));
 
-			atomic_inc(&pci_parity_count);
+			seqnum32_inc(&pci_parity_count);
 		}
 
 		if (status & (PCI_STATUS_DETECTED_PARITY)) {
@@ -569,7 +569,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 				"Detected Parity Error on %s\n",
 				pci_name(dev));
 
-			atomic_inc(&pci_parity_count);
+			seqnum32_inc(&pci_parity_count);
 		}
 	}
 
@@ -592,7 +592,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 				edac_printk(KERN_CRIT, EDAC_PCI, "Bridge "
 					"Signaled System Error on %s\n",
 					pci_name(dev));
-				atomic_inc(&pci_nonparity_count);
+				seqnum32_inc(&pci_nonparity_count);
 			}
 
 			if (status & (PCI_STATUS_PARITY)) {
@@ -600,7 +600,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 					"Master Data Parity Error on "
 					"%s\n", pci_name(dev));
 
-				atomic_inc(&pci_parity_count);
+				seqnum32_inc(&pci_parity_count);
 			}
 
 			if (status & (PCI_STATUS_DETECTED_PARITY)) {
@@ -608,7 +608,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 					"Detected Parity Error on %s\n",
 					pci_name(dev));
 
-				atomic_inc(&pci_parity_count);
+				seqnum32_inc(&pci_parity_count);
 			}
 		}
 	}
@@ -646,7 +646,7 @@ void edac_pci_do_parity_check(void)
 	if (!check_pci_errors)
 		return;
 
-	before_count = atomic_read(&pci_parity_count);
+	before_count = seqnum32_read(&pci_parity_count);
 
 	/* scan all PCI devices looking for a Parity Error on devices and
 	 * bridges.
@@ -658,7 +658,7 @@ void edac_pci_do_parity_check(void)
 	/* Only if operator has selected panic on PCI Error */
 	if (edac_pci_get_panic_on_pe()) {
 		/* If the count is different 'after' from 'before' */
-		if (before_count != atomic_read(&pci_parity_count))
+		if (before_count != seqnum32_read(&pci_parity_count))
 			panic("EDAC: PCI Parity Error");
 	}
 }
@@ -686,7 +686,7 @@ void edac_pci_handle_pe(struct edac_pci_ctl_info *pci, const char *msg)
 {
 
 	/* global PE counter incremented by edac_pci_do_parity_check() */
-	atomic_inc(&pci->counters.pe_count);
+	seqnum32_inc(&pci->counters.pe_count);
 
 	if (edac_pci_get_log_pe())
 		edac_pci_printk(pci, KERN_WARNING,
@@ -711,7 +711,7 @@ void edac_pci_handle_npe(struct edac_pci_ctl_info *pci, const char *msg)
 {
 
 	/* global NPE counter incremented by edac_pci_do_parity_check() */
-	atomic_inc(&pci->counters.npe_count);
+	seqnum32_inc(&pci->counters.npe_count);
 
 	if (edac_pci_get_log_npe())
 		edac_pci_printk(pci, KERN_WARNING,
-- 
2.27.0

[PATCH 08/13] drivers/oprofile: convert stats to use seqnum_ops

[Shuah Khan]

seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.

seqnum32 variables 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.

atomic_t variables used for stats are atomic counters. Overflow will
wrap around and reset the stats and no change with the conversion.

Convert them to use seqnum_ops.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 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 ++-
 include/linux/oprofile.h          |  3 ++-
 7 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/oprofile/buffer_sync.c b/drivers/oprofile/buffer_sync.c
index cc917865f13a..0503d467429b 100644
--- a/drivers/oprofile/buffer_sync.c
+++ b/drivers/oprofile/buffer_sync.c
@@ -34,6 +34,7 @@
 #include <linux/sched/mm.h>
 #include <linux/sched/task.h>
 #include <linux/gfp.h>
+#include <linux/seqnum_ops.h>
 
 #include "oprofile_stats.h"
 #include "event_buffer.h"
@@ -347,7 +348,7 @@ static void add_data(struct op_entry *entry, struct mm_struct *mm)
 		if (cookie == NO_COOKIE)
 			offset = pc;
 		if (cookie == INVALID_COOKIE) {
-			atomic_inc(&oprofile_stats.sample_lost_no_mapping);
+			seqnum32_inc(&oprofile_stats.sample_lost_no_mapping);
 			offset = pc;
 		}
 		if (cookie != last_cookie) {
@@ -391,14 +392,14 @@ add_sample(struct mm_struct *mm, struct op_sample *s, int in_kernel)
 	/* add userspace sample */
 
 	if (!mm) {
-		atomic_inc(&oprofile_stats.sample_lost_no_mm);
+		seqnum32_inc(&oprofile_stats.sample_lost_no_mm);
 		return 0;
 	}
 
 	cookie = lookup_dcookie(mm, s->eip, &offset);
 
 	if (cookie == INVALID_COOKIE) {
-		atomic_inc(&oprofile_stats.sample_lost_no_mapping);
+		seqnum32_inc(&oprofile_stats.sample_lost_no_mapping);
 		return 0;
 	}
 
@@ -556,7 +557,7 @@ void sync_buffer(int cpu)
 		/* ignore backtraces if failed to add a sample */
 		if (state == sb_bt_start) {
 			state = sb_bt_ignore;
-			atomic_inc(&oprofile_stats.bt_lost_no_mapping);
+			seqnum32_inc(&oprofile_stats.bt_lost_no_mapping);
 		}
 	}
 	release_mm(mm);
diff --git a/drivers/oprofile/event_buffer.c b/drivers/oprofile/event_buffer.c
index 6c9edc8bbc95..6ec2f1ed8d94 100644
--- a/drivers/oprofile/event_buffer.c
+++ b/drivers/oprofile/event_buffer.c
@@ -19,6 +19,7 @@
 #include <linux/dcookies.h>
 #include <linux/fs.h>
 #include <linux/uaccess.h>
+#include <linux/seqnum_ops.h>
 
 #include "oprof.h"
 #include "event_buffer.h"
@@ -53,7 +54,7 @@ void add_event_entry(unsigned long value)
 	}
 
 	if (buffer_pos == buffer_size) {
-		atomic_inc(&oprofile_stats.event_lost_overflow);
+		seqnum32_inc(&oprofile_stats.event_lost_overflow);
 		return;
 	}
 
diff --git a/drivers/oprofile/oprof.c b/drivers/oprofile/oprof.c
index ed2c3ec07024..f3183ef0f607 100644
--- a/drivers/oprofile/oprof.c
+++ b/drivers/oprofile/oprof.c
@@ -15,6 +15,7 @@
 #include <linux/workqueue.h>
 #include <linux/time.h>
 #include <linux/mutex.h>
+#include <linux/seqnum_ops.h>
 
 #include "oprof.h"
 #include "event_buffer.h"
@@ -110,7 +111,7 @@ static void switch_worker(struct work_struct *work)
 	if (oprofile_ops.switch_events())
 		return;
 
-	atomic_inc(&oprofile_stats.multiplex_counter);
+	seqnum32_inc(&oprofile_stats.multiplex_counter);
 	start_switch_worker();
 }
 
diff --git a/drivers/oprofile/oprofile_stats.c b/drivers/oprofile/oprofile_stats.c
index 59659cea4582..04e9b2a0d35a 100644
--- a/drivers/oprofile/oprofile_stats.c
+++ b/drivers/oprofile/oprofile_stats.c
@@ -11,6 +11,7 @@
 #include <linux/smp.h>
 #include <linux/cpumask.h>
 #include <linux/threads.h>
+#include <linux/seqnum_ops.h>
 
 #include "oprofile_stats.h"
 #include "cpu_buffer.h"
@@ -30,11 +31,11 @@ void oprofile_reset_stats(void)
 		cpu_buf->sample_invalid_eip = 0;
 	}
 
-	atomic_set(&oprofile_stats.sample_lost_no_mm, 0);
-	atomic_set(&oprofile_stats.sample_lost_no_mapping, 0);
-	atomic_set(&oprofile_stats.event_lost_overflow, 0);
-	atomic_set(&oprofile_stats.bt_lost_no_mapping, 0);
-	atomic_set(&oprofile_stats.multiplex_counter, 0);
+	seqnum32_set(&oprofile_stats.sample_lost_no_mm, 0);
+	seqnum32_set(&oprofile_stats.sample_lost_no_mapping, 0);
+	seqnum32_set(&oprofile_stats.event_lost_overflow, 0);
+	seqnum32_set(&oprofile_stats.bt_lost_no_mapping, 0);
+	seqnum32_set(&oprofile_stats.multiplex_counter, 0);
 }
 
 
diff --git a/drivers/oprofile/oprofile_stats.h b/drivers/oprofile/oprofile_stats.h
index 1fc622bd1834..229bcbb16527 100644
--- a/drivers/oprofile/oprofile_stats.h
+++ b/drivers/oprofile/oprofile_stats.h
@@ -11,13 +11,14 @@
 #define OPROFILE_STATS_H
 
 #include <linux/atomic.h>
+#include <linux/seqnum_ops.h>
 
 struct oprofile_stat_struct {
-	atomic_t sample_lost_no_mm;
-	atomic_t sample_lost_no_mapping;
-	atomic_t bt_lost_no_mapping;
-	atomic_t event_lost_overflow;
-	atomic_t multiplex_counter;
+	struct seqnum32 sample_lost_no_mm;
+	struct seqnum32 sample_lost_no_mapping;
+	struct seqnum32 bt_lost_no_mapping;
+	struct seqnum32 event_lost_overflow;
+	struct seqnum32 multiplex_counter;
 };
 
 extern struct oprofile_stat_struct oprofile_stats;
diff --git a/drivers/oprofile/oprofilefs.c b/drivers/oprofile/oprofilefs.c
index 0875f2f122b3..c5749b9aca11 100644
--- a/drivers/oprofile/oprofilefs.c
+++ b/drivers/oprofile/oprofilefs.c
@@ -17,6 +17,7 @@
 #include <linux/fs_context.h>
 #include <linux/pagemap.h>
 #include <linux/uaccess.h>
+#include <linux/seqnum_ops.h>
 
 #include "oprof.h"
 
@@ -193,7 +194,7 @@ static const struct file_operations atomic_ro_fops = {
 
 
 int oprofilefs_create_ro_atomic(struct dentry *root,
-	char const *name, atomic_t *val)
+	char const *name, struct seqnum32 *val)
 {
 	return __oprofilefs_create_file(root, name,
 					&atomic_ro_fops, 0444, val);
diff --git a/include/linux/oprofile.h b/include/linux/oprofile.h
index b2a0f15f11fe..f770254a0c8a 100644
--- a/include/linux/oprofile.h
+++ b/include/linux/oprofile.h
@@ -19,6 +19,7 @@
 #include <linux/errno.h>
 #include <linux/printk.h>
 #include <linux/atomic.h>
+#include <linux/seqnum_ops.h>
  
 /* Each escaped entry is prefixed by ESCAPE_CODE
  * then one of the following codes, then the
@@ -140,7 +141,7 @@ int oprofilefs_create_ro_ulong(struct dentry * root,
  
 /** Create a file for read-only access to an atomic_t. */
 int oprofilefs_create_ro_atomic(struct dentry * root,
-	char const * name, atomic_t * val);
+	char const *name, struct seqnum32 *val);
  
 /** create a directory */
 struct dentry *oprofilefs_mkdir(struct dentry *parent, char const *name);
-- 
2.27.0

[PATCH 09/13] drivers/staging/rtl8723bs: convert stats to use seqnum_ops

[Shuah Khan]

seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.

seqnum32 variables 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.

atomic_t variables used for stats are atomic counters. Overflow will
wrap around and reset the stats and no change with the conversion.

Convert them to use seqnum_ops. This conversion replaces inc_return()
with _inc() followed by _read().

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 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 +-
 4 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index 2abe205e3453..c3350f97816d 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -10,6 +10,7 @@
 #include <rtw_debug.h>
 #include <hal_btcoex.h>
 #include <linux/jiffies.h>
+#include <linux/seqnum_ops.h>
 
 static struct _cmd_callback rtw_cmd_callback[] = {
 	{GEN_CMD_CODE(_Read_MACREG), NULL}, /*0*/
@@ -207,7 +208,7 @@ static void c2h_wk_callback(_workitem * work);
 int rtw_init_evt_priv(struct evt_priv *pevtpriv)
 {
 	/* allocate DMA-able/Non-Page memory for cmd_buf and rsp_buf */
-	atomic_set(&pevtpriv->event_seq, 0);
+	seqnum32_set(&pevtpriv->event_seq, 0);
 	pevtpriv->evt_done_cnt = 0;
 
 	_init_workitem(&pevtpriv->c2h_wk, c2h_wk_callback, NULL);
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index b912ad2f4b72..addcd11b153b 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -11,6 +11,7 @@
 #include <rtw_wifi_regd.h>
 #include <hal_btcoex.h>
 #include <linux/kernel.h>
+#include <linux/seqnum_ops.h>
 #include <asm/unaligned.h>
 
 static struct mlme_handler mlme_sta_tbl[] = {
@@ -281,7 +282,7 @@ static void init_mlme_ext_priv_value(struct adapter *padapter)
 	struct mlme_ext_priv *pmlmeext = &padapter->mlmeextpriv;
 	struct mlme_ext_info *pmlmeinfo = &pmlmeext->mlmext_info;
 
-	atomic_set(&pmlmeext->event_seq, 0);
+	seqnum32_set(&pmlmeext->event_seq, 0);
 	pmlmeext->mgnt_seq = 0;/* reset to zero when disconnect at client mode */
 	pmlmeext->sa_query_seq = 0;
 	pmlmeext->mgnt_80211w_IPN = 0;
@@ -5051,7 +5052,9 @@ void report_survey_event(struct adapter *padapter, union recv_frame *precv_frame
 	pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
 	pc2h_evt_hdr->len = sizeof(struct survey_event);
 	pc2h_evt_hdr->ID = GEN_EVT_CODE(_Survey);
-	pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+	seqnum32_inc(&pmlmeext->event_seq);
+	pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
 
 	psurvey_evt = (struct survey_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
 
@@ -5104,7 +5107,9 @@ void report_surveydone_event(struct adapter *padapter)
 	pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
 	pc2h_evt_hdr->len = sizeof(struct surveydone_event);
 	pc2h_evt_hdr->ID = GEN_EVT_CODE(_SurveyDone);
-	pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+	seqnum32_inc(&pmlmeext->event_seq);
+	pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
 
 	psurveydone_evt = (struct surveydone_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
 	psurveydone_evt->bss_cnt = pmlmeext->sitesurvey_res.bss_cnt;
@@ -5151,7 +5156,9 @@ void report_join_res(struct adapter *padapter, int res)
 	pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
 	pc2h_evt_hdr->len = sizeof(struct joinbss_event);
 	pc2h_evt_hdr->ID = GEN_EVT_CODE(_JoinBss);
-	pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+	seqnum32_inc(&pmlmeext->event_seq);
+	pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
 
 	pjoinbss_evt = (struct joinbss_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
 	memcpy((unsigned char *)(&(pjoinbss_evt->network.network)), &(pmlmeinfo->network), sizeof(struct wlan_bssid_ex));
@@ -5202,7 +5209,9 @@ void report_wmm_edca_update(struct adapter *padapter)
 	pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
 	pc2h_evt_hdr->len = sizeof(struct wmm_event);
 	pc2h_evt_hdr->ID = GEN_EVT_CODE(_WMM);
-	pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+	seqnum32_inc(&pmlmeext->event_seq);
+	pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
 
 	pwmm_event = (struct wmm_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
 	pwmm_event->wmm = 0;
@@ -5249,7 +5258,9 @@ void report_del_sta_event(struct adapter *padapter, unsigned char *MacAddr, unsi
 	pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
 	pc2h_evt_hdr->len = sizeof(struct stadel_event);
 	pc2h_evt_hdr->ID = GEN_EVT_CODE(_DelSTA);
-	pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+	seqnum32_inc(&pmlmeext->event_seq);
+	pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
 
 	pdel_sta_evt = (struct stadel_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
 	memcpy((unsigned char *)(&(pdel_sta_evt->macaddr)), MacAddr, ETH_ALEN);
@@ -5302,7 +5313,9 @@ void report_add_sta_event(struct adapter *padapter, unsigned char *MacAddr, int
 	pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
 	pc2h_evt_hdr->len = sizeof(struct stassoc_event);
 	pc2h_evt_hdr->ID = GEN_EVT_CODE(_AddSTA);
-	pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+	seqnum32_inc(&pmlmeext->event_seq);
+	pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
 
 	padd_sta_evt = (struct stassoc_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
 	memcpy((unsigned char *)(&(padd_sta_evt->macaddr)), MacAddr, ETH_ALEN);
@@ -6620,10 +6633,10 @@ u8 mlme_evt_hdl(struct adapter *padapter, unsigned char *pbuf)
 
 	#ifdef CHECK_EVENT_SEQ
 	/*  checking event sequence... */
-	if (evt_seq != (atomic_read(&pevt_priv->event_seq) & 0x7f)) {
+	if (evt_seq != (seqnum32_read(&pevt_priv->event_seq) & 0x7f)) {
 		RT_TRACE(_module_rtl871x_cmd_c_, _drv_info_,
 			 ("Event Seq Error! %d vs %d\n", (evt_seq & 0x7f),
-			  (atomic_read(&pevt_priv->event_seq) & 0x7f)));
+			  (seqnum32_read(&pevt_priv->event_seq) & 0x7f)));
 
 		pevt_priv->event_seq = (evt_seq+1)&0x7f;
 
@@ -6647,7 +6660,7 @@ u8 mlme_evt_hdl(struct adapter *padapter, unsigned char *pbuf)
 
 	}
 
-	atomic_inc(&pevt_priv->event_seq);
+	seqnum32_inc(&pevt_priv->event_seq);
 
 	peventbuf += 2;
 
diff --git a/drivers/staging/rtl8723bs/include/rtw_cmd.h b/drivers/staging/rtl8723bs/include/rtw_cmd.h
index 56c77bc7ca81..cc0ea649388b 100644
--- a/drivers/staging/rtl8723bs/include/rtw_cmd.h
+++ b/drivers/staging/rtl8723bs/include/rtw_cmd.h
@@ -8,6 +8,7 @@
 #define __RTW_CMD_H_
 
 #include <linux/completion.h>
+#include <linux/seqnum_ops.h>
 
 #define C2H_MEM_SZ (16*1024)
 
@@ -62,7 +63,7 @@
 		struct rtw_cbuf *c2h_queue;
 		#define C2H_QUEUE_MAX_LEN 10
 
-		atomic_t event_seq;
+		struct seqnum32 event_seq;
 		u8 *evt_buf;	/* shall be non-paged, and 4 bytes aligned */
 		u8 *evt_allocated_buf;
 		u32 evt_done_cnt;
diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
index 1567831caf91..ebb050253bcf 100644
--- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
+++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
@@ -7,6 +7,7 @@
 #ifndef __RTW_MLME_EXT_H_
 #define __RTW_MLME_EXT_H_
 
+#include <linux/seqnum_ops.h>
 
 /* 	Commented by Albert 20101105 */
 /* 	Increase the SURVEY_TO value from 100 to 150  (100ms to 150ms) */
@@ -461,7 +462,7 @@ struct p2p_oper_class_map {
 struct mlme_ext_priv {
 	struct adapter	*padapter;
 	u8 mlmeext_init;
-	atomic_t		event_seq;
+	struct seqnum32		event_seq;
 	u16 mgnt_seq;
 	u16 sa_query_seq;
 	u64 mgnt_80211w_IPN;
-- 
2.27.0

[PATCH 10/13] usb: usbip/vhci: convert seqno to seqnum_ops

[Shuah Khan]

seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.

seqnum32 variables 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.

Convert it to use seqnum_ops. This conversion replaces inc_return()
with _inc() followed by _read().

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/usb/usbip/vhci.h     | 3 ++-
 drivers/usb/usbip/vhci_hcd.c | 9 ++++++---
 drivers/usb/usbip/vhci_rx.c  | 3 ++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/usbip/vhci.h b/drivers/usb/usbip/vhci.h
index 5659dce1526e..cb76747f423b 100644
--- a/drivers/usb/usbip/vhci.h
+++ b/drivers/usb/usbip/vhci.h
@@ -15,6 +15,7 @@
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
 #include <linux/wait.h>
+#include <linux/seqnum_ops.h>
 
 struct vhci_device {
 	struct usb_device *udev;
@@ -108,7 +109,7 @@ struct vhci_hcd {
 	unsigned resuming:1;
 	unsigned long re_timeout;
 
-	atomic_t seqnum;
+	struct seqnum32 seqnum;
 
 	/*
 	 * NOTE:
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 66cde5e5f796..0d854d9707e5 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/seqnum_ops.h>
 
 #include "usbip_common.h"
 #include "vhci.h"
@@ -665,7 +666,8 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev)
 
 	spin_lock_irqsave(&vdev->priv_lock, flags);
 
-	priv->seqnum = atomic_inc_return(&vhci_hcd->seqnum);
+	seqnum32_inc(&vhci_hcd->seqnum);
+	priv->seqnum = seqnum32_read(&vhci_hcd->seqnum);
 	if (priv->seqnum == 0xffff)
 		dev_info(&urb->dev->dev, "seqnum max\n");
 
@@ -921,7 +923,8 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 			return -ENOMEM;
 		}
 
-		unlink->seqnum = atomic_inc_return(&vhci_hcd->seqnum);
+		seqnum32_inc(&vhci_hcd->seqnum);
+		unlink->seqnum = seqnum32_read(&vhci_hcd->seqnum);
 		if (unlink->seqnum == 0xffff)
 			pr_info("seqnum max\n");
 
@@ -1181,7 +1184,7 @@ static int vhci_start(struct usb_hcd *hcd)
 		vdev->rhport = rhport;
 	}
 
-	atomic_set(&vhci_hcd->seqnum, 0);
+	seqnum32_set(&vhci_hcd->seqnum, 0);
 
 	hcd->power_budget = 0; /* no limit */
 	hcd->uses_new_polling = 1;
diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
index 266024cbb64f..0d0def7ccfee 100644
--- a/drivers/usb/usbip/vhci_rx.c
+++ b/drivers/usb/usbip/vhci_rx.c
@@ -5,6 +5,7 @@
 
 #include <linux/kthread.h>
 #include <linux/slab.h>
+#include <linux/seqnum_ops.h>
 
 #include "usbip_common.h"
 #include "vhci.h"
@@ -68,7 +69,7 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
 	if (!urb) {
 		pr_err("cannot find a urb of seqnum %u max seqnum %d\n",
 			pdu->base.seqnum,
-			atomic_read(&vhci_hcd->seqnum));
+			seqnum32_read(&vhci_hcd->seqnum));
 		usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
 		return;
 	}
-- 
2.27.0

[PATCH 11/13] drivers/staging/rtl8188eu: convert stats to use seqnum_ops

[Shuah Khan]

seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.

seqnum32 variables 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.

atomic_t variables used for stats are atomic counters. Overflow will
wrap around and reset the stats and no change with the conversion.

Convert them to use seqnum_ops. This conversion replaces inc_return()
with _inc() followed by _read().

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 23 ++++++++++++++-----
 .../staging/rtl8188eu/include/rtw_mlme_ext.h  |  3 ++-
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
index b3eddcb83cd0..31dd9d31dd9a 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
@@ -7,6 +7,7 @@
 #define _RTW_MLME_EXT_C_
 
 #include <linux/ieee80211.h>
+#include <linux/seqnum_ops.h>
 #include <asm/unaligned.h>
 
 #include <osdep_service.h>
@@ -3860,7 +3861,7 @@ static void init_mlme_ext_priv_value(struct adapter *padapter)
 		_12M_RATE_, _24M_RATE_, 0xff,
 	};
 
-	atomic_set(&pmlmeext->event_seq, 0);
+	seqnum32_set(&pmlmeext->event_seq, 0);
 	pmlmeext->mgnt_seq = 0;/* reset to zero when disconnect at client mode */
 
 	pmlmeext->cur_channel = padapter->registrypriv.channel;
@@ -4189,7 +4190,9 @@ void report_survey_event(struct adapter *padapter,
 	pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
 	pc2h_evt_hdr->len = sizeof(struct survey_event);
 	pc2h_evt_hdr->ID = _Survey_EVT_;
-	pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+	seqnum32_inc(&pmlmeext->event_seq);
+	pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
 
 	psurvey_evt = (struct survey_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
 
@@ -4239,7 +4242,9 @@ void report_surveydone_event(struct adapter *padapter)
 	pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
 	pc2h_evt_hdr->len = sizeof(struct surveydone_event);
 	pc2h_evt_hdr->ID = _SurveyDone_EVT_;
-	pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+	seqnum32_inc(&pmlmeext->event_seq);
+	pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
 
 	psurveydone_evt = (struct surveydone_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
 	psurveydone_evt->bss_cnt = pmlmeext->sitesurvey_res.bss_cnt;
@@ -4283,7 +4288,9 @@ void report_join_res(struct adapter *padapter, int res)
 	pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
 	pc2h_evt_hdr->len = sizeof(struct joinbss_event);
 	pc2h_evt_hdr->ID = _JoinBss_EVT_;
-	pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+	seqnum32_inc(&pmlmeext->event_seq);
+	pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
 
 	pjoinbss_evt = (struct joinbss_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
 	memcpy((unsigned char *)(&pjoinbss_evt->network.network), &pmlmeinfo->network, sizeof(struct wlan_bssid_ex));
@@ -4333,7 +4340,9 @@ void report_del_sta_event(struct adapter *padapter, unsigned char *MacAddr,
 	pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
 	pc2h_evt_hdr->len = sizeof(struct stadel_event);
 	pc2h_evt_hdr->ID = _DelSTA_EVT_;
-	pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+	seqnum32_inc(&pmlmeext->event_seq);
+	pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
 
 	pdel_sta_evt = (struct stadel_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
 	ether_addr_copy((unsigned char *)(&pdel_sta_evt->macaddr), MacAddr);
@@ -4386,7 +4395,9 @@ void report_add_sta_event(struct adapter *padapter, unsigned char *MacAddr,
 	pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
 	pc2h_evt_hdr->len = sizeof(struct stassoc_event);
 	pc2h_evt_hdr->ID = _AddSTA_EVT_;
-	pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+	seqnum32_inc(&pmlmeext->event_seq);
+	pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
 
 	padd_sta_evt = (struct stassoc_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
 	ether_addr_copy((unsigned char *)(&padd_sta_evt->macaddr), MacAddr);
diff --git a/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h b/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
index b11a6886a083..09b7e3bb2738 100644
--- a/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
+++ b/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
@@ -7,6 +7,7 @@
 #ifndef __RTW_MLME_EXT_H_
 #define __RTW_MLME_EXT_H_
 
+#include <linux/seqnum_ops.h>
 #include <osdep_service.h>
 #include <drv_types.h>
 #include <wlan_bssdef.h>
@@ -393,7 +394,7 @@ struct p2p_oper_class_map {
 struct mlme_ext_priv {
 	struct adapter	*padapter;
 	u8	mlmeext_init;
-	atomic_t	event_seq;
+	struct	seqnum32 event_seq;
 	u16	mgnt_seq;
 
 	unsigned char	cur_channel;
-- 
2.27.0

[PATCH 12/13] drivers/staging/unisys/visorhba: convert stats to use seqnum_ops

[Shuah Khan]

seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.

seqnum32 variables 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.

atomic_t variables used for error_count and ios_threshold are atomic
counters and guarded by max. values. No change to the behavior with
this change.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 .../staging/unisys/visorhba/visorhba_main.c   | 37 ++++++++++---------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
index 7ae5306b92fe..3209958b8aaa 100644
--- a/drivers/staging/unisys/visorhba/visorhba_main.c
+++ b/drivers/staging/unisys/visorhba/visorhba_main.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/seq_file.h>
 #include <linux/visorbus.h>
+#include <linux/seqnum_ops.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_cmnd.h>
@@ -41,8 +42,8 @@ MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_GUID_STR);
 struct visordisk_info {
 	struct scsi_device *sdev;
 	u32 valid;
-	atomic_t ios_threshold;
-	atomic_t error_count;
+	struct seqnum32 ios_threshold;
+	struct seqnum32 error_count;
 	struct visordisk_info *next;
 };
 
@@ -374,10 +375,10 @@ static int visorhba_abort_handler(struct scsi_cmnd *scsicmd)
 
 	scsidev = scsicmd->device;
 	vdisk = scsidev->hostdata;
-	if (atomic_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
-		atomic_inc(&vdisk->error_count);
+	if (seqnum32_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
+		seqnum32_inc(&vdisk->error_count);
 	else
-		atomic_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
+		seqnum32_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
 	rtn = forward_taskmgmt_command(TASK_MGMT_ABORT_TASK, scsidev);
 	if (rtn == SUCCESS) {
 		scsicmd->result = DID_ABORT << 16;
@@ -401,10 +402,10 @@ static int visorhba_device_reset_handler(struct scsi_cmnd *scsicmd)
 
 	scsidev = scsicmd->device;
 	vdisk = scsidev->hostdata;
-	if (atomic_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
-		atomic_inc(&vdisk->error_count);
+	if (seqnum32_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
+		seqnum32_inc(&vdisk->error_count);
 	else
-		atomic_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
+		seqnum32_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
 	rtn = forward_taskmgmt_command(TASK_MGMT_LUN_RESET, scsidev);
 	if (rtn == SUCCESS) {
 		scsicmd->result = DID_RESET << 16;
@@ -429,10 +430,10 @@ static int visorhba_bus_reset_handler(struct scsi_cmnd *scsicmd)
 	scsidev = scsicmd->device;
 	shost_for_each_device(scsidev, scsidev->host) {
 		vdisk = scsidev->hostdata;
-		if (atomic_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
-			atomic_inc(&vdisk->error_count);
+		if (seqnum32_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
+			seqnum32_inc(&vdisk->error_count);
 		else
-			atomic_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
+			seqnum32_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
 	}
 	rtn = forward_taskmgmt_command(TASK_MGMT_BUS_RESET, scsidev);
 	if (rtn == SUCCESS) {
@@ -803,9 +804,9 @@ static void do_scsi_linuxstat(struct uiscmdrsp *cmdrsp,
 		return;
 	/* Okay see what our error_count is here.... */
 	vdisk = scsidev->hostdata;
-	if (atomic_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT) {
-		atomic_inc(&vdisk->error_count);
-		atomic_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
+	if (seqnum32_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT) {
+		seqnum32_inc(&vdisk->error_count);
+		seqnum32_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
 	}
 }
 
@@ -881,10 +882,10 @@ static void do_scsi_nolinuxstat(struct uiscmdrsp *cmdrsp,
 		kfree(buf);
 	} else {
 		vdisk = scsidev->hostdata;
-		if (atomic_read(&vdisk->ios_threshold) > 0) {
-			atomic_dec(&vdisk->ios_threshold);
-			if (atomic_read(&vdisk->ios_threshold) == 0)
-				atomic_set(&vdisk->error_count, 0);
+		if (seqnum32_read(&vdisk->ios_threshold) > 0) {
+			seqnum32_dec(&vdisk->ios_threshold);
+			if (seqnum32_read(&vdisk->ios_threshold) == 0)
+				seqnum32_set(&vdisk->error_count, 0);
 		}
 	}
 }
-- 
2.27.0

[PATCH 13/13] security/integrity/ima: converts stats to seqnum_ops

[Shuah Khan]

seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.

seqnum32 variables 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.

atomic_t variables used for eima_htable.violations and number of stored
measurements and ios_threshold are atomic counters, and violations is
only an idicator and can overflow. No chane to the behavior with this
change.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 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 ++++---
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 6ebefec616e4..55fe1d14c67a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -21,6 +21,7 @@
 #include <linux/tpm.h>
 #include <linux/audit.h>
 #include <crypto/hash_info.h>
+#include <linux/seqnum_ops.h>
 
 #include "../integrity.h"
 
@@ -174,8 +175,8 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 extern spinlock_t ima_queue_lock;
 
 struct ima_h_table {
-	atomic_long_t len;	/* number of stored measurements in the list */
-	atomic_long_t violations;
+	struct seqnum64 len;	/* number of stored measurements in the list */
+	struct seqnum64 violations;
 	struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE];
 };
 extern struct ima_h_table ima_htable;
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 4f39fb93f278..b1a203435698 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -144,7 +144,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
 	int result;
 
 	/* can overflow, only indicator */
-	atomic_long_inc(&ima_htable.violations);
+	seqnum64_inc(&ima_htable.violations);
 
 	result = ima_alloc_init_template(&event_data, &entry, NULL);
 	if (result < 0) {
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index ea8ff8a07b36..03a78b445052 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -39,12 +39,12 @@ __setup("ima_canonical_fmt", default_canonical_fmt_setup);
 static int valid_policy = 1;
 
 static ssize_t ima_show_htable_value(char __user *buf, size_t count,
-				     loff_t *ppos, atomic_long_t *val)
+				     loff_t *ppos, struct seqnum64 *val)
 {
 	char tmpbuf[32];	/* greater than largest 'long' string value */
 	ssize_t len;
 
-	len = scnprintf(tmpbuf, sizeof(tmpbuf), "%li\n", atomic_long_read(val));
+	len = scnprintf(tmpbuf, sizeof(tmpbuf), "%lli\n", seqnum64_read(val));
 	return simple_read_from_buffer(buf, count, ppos, tmpbuf, len);
 }
 
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index c096ef8945c7..87db50dd1721 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -17,6 +17,7 @@
 
 #include <linux/rculist.h>
 #include <linux/slab.h>
+#include <linux/seqnum_ops.h>
 #include "ima.h"
 
 #define AUDIT_CAUSE_LEN_MAX 32
@@ -33,8 +34,8 @@ static unsigned long binary_runtime_size = ULONG_MAX;
 
 /* key: inode (before secure-hashing a file) */
 struct ima_h_table ima_htable = {
-	.len = ATOMIC_LONG_INIT(0),
-	.violations = ATOMIC_LONG_INIT(0),
+	.len = SEQNUM_INIT(0),
+	.violations = SEQNUM_INIT(0),
 	.queue[0 ... IMA_MEASURE_HTABLE_SIZE - 1] = HLIST_HEAD_INIT
 };
 
@@ -106,7 +107,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry,
 	INIT_LIST_HEAD(&qe->later);
 	list_add_tail_rcu(&qe->later, &ima_measurements);
 
-	atomic_long_inc(&ima_htable.len);
+	seqnum64_inc(&ima_htable.len);
 	if (update_htable) {
 		key = ima_hash_key(entry->digests[ima_hash_algo_idx].digest);
 		hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
-- 
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 12/13] drivers/staging/unisys/visorhba: convert stats to use seqnum_ops

[Greg KH]

On Tue, Nov 10, 2020 at 12:53:38PM -0700, Shuah Khan wrote:
> seqnum_ops api is introduced to be used when a variable is used as
> a sequence/stat counter and doesn't guard object lifetimes. This
> clearly differentiates atomic_t usages that guard object lifetimes.
> 
> seqnum32 variables 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.
> 
> atomic_t variables used for error_count and ios_threshold are atomic
> counters and guarded by max. values. No change to the behavior with
> this change.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  .../staging/unisys/visorhba/visorhba_main.c   | 37 ++++++++++---------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
> index 7ae5306b92fe..3209958b8aaa 100644
> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
> @@ -10,6 +10,7 @@
>  #include <linux/module.h>
>  #include <linux/seq_file.h>
>  #include <linux/visorbus.h>
> +#include <linux/seqnum_ops.h>
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsi_cmnd.h>
> @@ -41,8 +42,8 @@ MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_GUID_STR);
>  struct visordisk_info {
>  	struct scsi_device *sdev;
>  	u32 valid;
> -	atomic_t ios_threshold;
> -	atomic_t error_count;
> +	struct seqnum32 ios_threshold;
> +	struct seqnum32 error_count;

Are you sure the threshold variable is a sequence number?

It goes up and down, not just up and up and up.

An error count just goes up :)

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 12/13] drivers/staging/unisys/visorhba: convert stats to use seqnum_ops

[Shuah Khan]

On 11/10/20 1:42 PM, Greg KH wrote:
> On Tue, Nov 10, 2020 at 12:53:38PM -0700, Shuah Khan wrote:
>> seqnum_ops api is introduced to be used when a variable is used as
>> a sequence/stat counter and doesn't guard object lifetimes. This
>> clearly differentiates atomic_t usages that guard object lifetimes.
>>
>> seqnum32 variables 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.
>>
>> atomic_t variables used for error_count and ios_threshold are atomic
>> counters and guarded by max. values. No change to the behavior with
>> this change.
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>> ---
>>   .../staging/unisys/visorhba/visorhba_main.c   | 37 ++++++++++---------
>>   1 file changed, 19 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
>> index 7ae5306b92fe..3209958b8aaa 100644
>> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
>> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/module.h>
>>   #include <linux/seq_file.h>
>>   #include <linux/visorbus.h>
>> +#include <linux/seqnum_ops.h>
>>   #include <scsi/scsi.h>
>>   #include <scsi/scsi_host.h>
>>   #include <scsi/scsi_cmnd.h>
>> @@ -41,8 +42,8 @@ MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_GUID_STR);
>>   struct visordisk_info {
>>   	struct scsi_device *sdev;
>>   	u32 valid;
>> -	atomic_t ios_threshold;
>> -	atomic_t error_count;
>> +	struct seqnum32 ios_threshold;
>> +	struct seqnum32 error_count;
> 
> Are you sure the threshold variable is a sequence number >
> It goes up and down, not just up and up and up.

Right. I does go down. Turns out this is the only place seqnum32_dec()
is used. :)

I will fix. I noticed you made a comment about _dec() interfaces on
1/13. I can drop those as well. This way seqnum_ops can be just used
for up counters.

thanks,
-- Shuah

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.