Introduce seqnum_ops
Subsystems (15)
Shuah Khan
maintainercover Introduce seqnum_ops
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/[email protected]/
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
Signature · 1 line
2.27.0
Shuah Khan
maintainerPATCH 1/13 seqnum_ops: Introduce Sequence Number Ops
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.
--- 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
Documentation/core-api/atomic_ops.rst +4 -0
@@ -1,3 +1,7 @@ +.. SPDX-License-Identifier: GPL-2.0 + +.. _atomic_ops: + ======================================================= Semantics and Behavior of Atomic and Bitmask Operations =======================================================
Documentation/core-api/index.rst +1 -0
@@ -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
Documentation/core-api/seqnum_ops.rst new file +117 -0
117 lines not on this page · raw message
MAINTAINERS +7 -0
@@ -15977,6 +15977,13 @@ S: Maintained F: Documentation/fb/sm712fb.rst F: drivers/video/fbdev/sm712* +SEQNUM OPS +M: Shuah Khan <[email protected]> +L: [email protected] +S: Maintained +F: include/linux/seqnum_ops.h +F: lib/test_seqnum_ops.c + SIMPLE FIRMWARE INTERFACE (SFI) S: Obsolete W: http://simplefirmware.org/
include/linux/seqnum_ops.h new file +152 -0
152 lines not on this page · raw message
lib/Kconfig +9 -0
@@ -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
lib/Makefile +1 -0
@@ -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
lib/test_seqnum_ops.c new file +154 -0
154 lines not on this page · raw message
Signature · 1 line
2.27.0
Greg KH
maintainerRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
Documentation/core-api/seqnum_ops.rst line 117
Quoted 7 lines · Shuah Khan
Documentation/core-api/seqnum_ops.rst new file +7
-0+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
Greg KH
maintainerRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 12 lines · Greg KH
Documentation/core-api/seqnum_ops.rst line 117
Quoted 7 lines · Shuah Khan
Documentation/core-api/seqnum_ops.rst new file +7
-0+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?
Quoted 2 lines
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 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 13 lines · Greg KH
Quoted 12 lines · Greg KH
Documentation/core-api/seqnum_ops.rst line 117
Quoted 7 lines · Shuah Khan
Documentation/core-api/seqnum_ops.rst new file +7
-0+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?
Quoted 2 lines
Quoted 2 lines
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?).
Quoted 3 lines
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?)
Signature · 1 line
Kees Cook
Shuah Khan
maintainerRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 14 lines · Kees Cook
Quoted 13 lines · Greg KH
Quoted 12 lines · Greg KH
Documentation/core-api/seqnum_ops.rst line 117
Quoted 7 lines · Shuah Khan
Documentation/core-api/seqnum_ops.rst new file +7
-0+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?
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
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
Quoted 4 lines
Quoted 3 lines
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.
Quoted 4 lines
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 itwrap_u32andwrap_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
Quoted 7 lines · Shuah Khan
Quoted 3 lines
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 itwrap_u32andwrap_u64?)Still like
seqnum_ops.There is
seqcount_tinseqlock.hwhich 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.
Shuah Khan
maintainerRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 11 lines · Matthew Wilcox
Quoted 7 lines · Shuah Khan
Quoted 3 lines
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 itwrap_u32andwrap_u64?)Still like
seqnum_ops.There is
seqcount_tinseqlock.hwhich 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?
Quoted 2 lines
"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
Greg KH
maintainerRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 8 lines · Shuah Khan
Quoted 7 lines · Matthew Wilcox
Quoted 6 lines · Shuah Khan
Quoted 3 lines
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 itwrap_u32andwrap_u64?)Still like
seqnum_ops.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
There is
seqcount_tinseqlock.hwhich 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
Shuah Khan
maintainerRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 11 lines · Greg KH
Quoted 10 lines · Shuah Khan
Quoted 9 lines · Matthew Wilcox
Quoted 7 lines · Shuah Khan
Quoted 3 lines
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 itwrap_u32andwrap_u64?)Still like
seqnum_ops.There is
seqcount_tinseqlock.hwhich is a totally different feature.
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
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.
Quoted 5 lines
And I don't understand the hesitation about "sequence" in a name, as that's exactly what this is. seqlock is different, yes.
Quoted 1 line
How about "
seqnum_t"? That's what we call the sequence number that weexport to uevents, a "SEQNUM".
Good point.
This is what we have currently in patch v1 and let's just go with it.
thanks,
-- Shuah
Shuah Khan
maintainerRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
In reply to Shuah KhanQuoted 15 lines · Shuah Khan
Quoted 14 lines · Kees Cook
Quoted 13 lines · Greg KH
Quoted 12 lines · Greg KH
Documentation/core-api/seqnum_ops.rst line 117
Quoted 7 lines · Shuah Khan
Documentation/core-api/seqnum_ops.rst new file +7
-0+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?
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
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
Quoted 9 lines · Shuah Khan
atomic64_tdepends onCONFIG_64BIT
include/linux/types.h#ifdef
CONFIG_64BITtypedef 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.
Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
In reply to Shuah KhanQuoted 1 line · Shuah Khan
Sequence Numbers wrap around to
INT_MINwhen 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.
Quoted 9 lines
+++
b/Documentation/core-api/seqnum_ops.rstDocumentation/core-api/seqnum_ops.rst new file +7
-0@@ -0,0 +1,117 @@ +.. SPDX-License-Identifier: GPL-2.0 + +.. include:: <isonum.txt> + +.. _seqnum_ops: + +==========================
Documentation/core-api/seqnum_ops.rst line 17
Quoted 10 lines
Documentation/core-api/seqnum_ops.rst new file +10
-0+Sequence Number Operations +========================== + +:Author: Shuah Khan +:Copyright: |copy| 2020, The Linux Foundation +:Copyright: |copy| 2020, Shuah Khan <[email protected]> + +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.
Quoted 5 lines
Documentation/core-api/seqnum_ops.rst new file +5
-0+Read interface +-------------- + +Reads and returns the current value. :: +
Documentation/core-api/seqnum_ops.rst line 109
Quoted 10 lines
Documentation/core-api/seqnum_ops.rst new file +10
-0+ 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.
include/linux/seqnum_ops.h line 90
Quoted 13 lines
include/linux/seqnum_ops.h new file +13
-0+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.
include/linux/seqnum_ops.h line 123
Quoted 2 lines
include/linux/seqnum_ops.h new file +2
-0+static inline void seqnum64_dec ( +struct seqnum64 *seq)
That one is particularly weird.
Shuah Khan
maintainerRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 5 lines · Matthew Wilcox
Quoted 1 line · Shuah Khan
Sequence Numbers wrap around to
INT_MINwhen it overflows and should notWhy 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.
Quoted 65 lines
Quoted 10 lines
+++
b/Documentation/core-api/seqnum_ops.rstDocumentation/core-api/seqnum_ops.rst new file +7
-0@@ -0,0 +1,117 @@ +.. SPDX-License-Identifier: GPL-2.0 + +.. include:: <isonum.txt> + +.. _seqnum_ops: + +==========================Documentation/core-api/seqnum_ops.rst line 17
Quoted 10 lines
Documentation/core-api/seqnum_ops.rst new file +10
-0+Sequence Number Operations +========================== + +:Author: Shuah Khan +:Copyright: |copy| 2020, The Linux Foundation +:Copyright: |copy| 2020, Shuah Khan <[email protected]> + +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_tlater. 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.Quoted 5 lines
Documentation/core-api/seqnum_ops.rst new file +5
-0+Read interface +-------------- + +Reads and returns the current value. :: +Documentation/core-api/seqnum_ops.rst line 109
Quoted 10 lines
Documentation/core-api/seqnum_ops.rst new file +10
-0+ 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 byseqnum_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.
Quoted 14 lines
include/linux/seqnum_ops.h new file +13
-0+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.
Quoted 5 lines
include/linux/seqnum_ops.h line 123
Quoted 2 lines
include/linux/seqnum_ops.h new file +2
-0+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
Quoted 3 lines · Shuah Khan
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_tusages that are up counters.
As in, existing callers expect the "read" value to be int?
Signature · 1 line
Kees Cook
Shuah Khan
maintainerRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 7 lines · Kees Cook
Quoted 3 lines · Shuah Khan
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_tusages 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
Peter Zijlstra
NAKRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
In reply to Shuah KhanQuoted 7 lines · Shuah Khan
+ * 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.
Shuah Khan
maintainerRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 11 lines · Peter Zijlstra
Quoted 7 lines · Shuah Khan
+ * The interface provides:
+ * seqnum32 & seqnum64 functions:
+ * initialization
+ * set
+ * read
+ * increment and no return
+ * decrement and no returnNAK, 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
Quoted 4 lines
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 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 1 line · Shuah Khan
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.
Shuah Khan
maintainerRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 7 lines · Peter Zijlstra
Quoted 1 line · Shuah Khan
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
Quoted 3 lines · Shuah Khan
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.
Shuah Khan
maintainerRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 10 lines · Peter Zijlstra
Quoted 3 lines · Shuah Khan
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.
Quoted 3 lines
All I'm saying is that if you want a sequence number,
inc_return(orfetch_inc) is the only sane option.
Cool.
thanks,
-- Shuah
Re: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 7 lines · Shuah Khan
Quoted 6 lines · Peter Zijlstra
Quoted 3 lines · Shuah Khan
Not sure what to make of the 6080 atomic_read()s and 3413 atomic_inc()s, some of which might be assuming uniqueness guarantee.
Quoted 8 lines
Quoted 4 lines
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.
Greg KH
maintainerRe: [PATCH 01/13] seqnum_ops: Introduce Sequence Number Ops
Quoted 23 lines · Peter Zijlstra
Quoted 7 lines · Shuah Khan
Quoted 6 lines · Peter Zijlstra
Quoted 3 lines · Shuah Khan
Not sure what to make of the 6080 atomic_read()s and 3413 atomic_inc()s, some of which might be assuming uniqueness guarantee.
Quoted 8 lines
Quoted 4 lines
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_tor whatever else bring any actual differences. They're pure wrappers without change in semantics.
Quoted 7 lines
refcount_tis 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
Shuah Khan
maintainerPATCH 2/13 selftests:lib:test_seqnum_ops: add new test for seqnum_ops
In reply to Shuah KhanAdd 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.
--- 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
Documentation/core-api/seqnum_ops.rst +9 -0
@@ -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.
MAINTAINERS +1 -0
@@ -15983,6 +15983,7 @@ L: [email protected] 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
include/linux/seqnum_ops.h +2 -0
@@ -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 * */
tools/testing/selftests/lib/Makefile +1 -0
@@ -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
tools/testing/selftests/lib/config +1 -0
@@ -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
tools/testing/selftests/lib/test_seqnum_ops.sh new file +10 -0
@@ -0,0 +1,10 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0 +# +# Copyright (c) 2020 Shuah Khan <[email protected]> +# 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
Signature · 1 line
2.27.0
Shuah Khan
maintainerPATCH 3/13 drivers/acpi: convert seqno seqnum_ops
In reply to Shuah Khanseqnum_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().
--- drivers/acpi/acpi_extlog.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/acpi/acpi_extlog.c +4 -2
@@ -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 banstatic 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);
Signature · 1 line
2.27.0
Shuah Khan
maintainerPATCH 4/13 drivers/acpi/apei: convert seqno to seqnum_ops
In reply to Shuah Khanseqnum_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().
--- drivers/acpi/apei/ghes.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/acpi/apei/ghes.c +4 -2
@@ -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);
Signature · 1 line
2.27.0
Shuah Khan
maintainerPATCH 5/13 drivers/base/test/test_async_driver_probe: convert to use seqnum_ops
In reply to Shuah Khanseqnum_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.
--- drivers/base/test/test_async_driver_probe.c | 26 +++++++++++++-------- 1 file changed, 16 insertions(+), 10 deletions(-)
drivers/base/test/test_async_driver_probe.c +16 -10
@@ -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; }
Signature · 1 line
2.27.0
Shuah Khan
maintainerPATCH 6/13 drivers/char/ipmi: convert stats to use seqnum_ops
In reply to Shuah Khanseqnum_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.
--- 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(-)
drivers/char/ipmi/ipmi_msghandler.c +5 -4
@@ -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. */
drivers/char/ipmi/ipmi_si_intf.c +5 -4
@@ -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 4static 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 );
drivers/char/ipmi/ipmi_ssif.c +5 -4
@@ -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 ;
Signature · 1 line
2.27.0
Shuah Khan
maintainerPATCH 7/13 drivers/edac: convert pci counters to seqnum_ops
In reply to Shuah Khanseqnum_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.
--- drivers/edac/edac_pci.h | 5 +++-- drivers/edac/edac_pci_sysfs.c | 28 ++++++++++++++-------------- 2 files changed, 17 insertions(+), 16 deletions(-)
drivers/edac/edac_pci.h +3 -2
@@ -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; };/*
drivers/edac/edac_pci_sysfs.c +14 -14
@@ -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,
Signature · 1 line
2.27.0
Shuah Khan
maintainerPATCH 8/13 drivers/oprofile: convert stats to use seqnum_ops
In reply to Shuah Khanseqnum_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.
--- 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(-)
drivers/oprofile/buffer_sync.c +5 -4
@@ -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);
drivers/oprofile/event_buffer.c +2 -1
@@ -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 ; }
drivers/oprofile/oprof.c +2 -1
@@ -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 (); }
drivers/oprofile/oprofile_stats.c +6 -5
@@ -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 ); }
drivers/oprofile/oprofile_stats.h +6 -5
@@ -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;
drivers/oprofile/oprofilefs.c +2 -1
@@ -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);
include/linux/oprofile.h +2 -1
@@ -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);
Signature · 1 line
2.27.0
Shuah Khan
maintainerPATCH 9/13 drivers/staging/rtl8723bs: convert stats to use seqnum_ops
In reply to Shuah Khanseqnum_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().
--- 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(-)
drivers/staging/rtl8723bs/core/rtw_cmd.c +2 -1
@@ -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 );
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c +23 -10
@@ -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 ;
drivers/staging/rtl8723bs/include/rtw_cmd.h +2 -1
@@ -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;
drivers/staging/rtl8723bs/include/rtw_mlme_ext.h +2 -1
@@ -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;
Signature · 1 line
2.27.0
Shuah Khan
maintainerPATCH 10/13 usb: usbip/vhci: convert seqno to seqnum_ops
In reply to Shuah Khanseqnum_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().
--- 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(-)
drivers/usb/usbip/vhci.h +2 -1
@@ -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:
drivers/usb/usbip/vhci_hcd.c +6 -3
@@ -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 ;
drivers/usb/usbip/vhci_rx.c +2 -1
@@ -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 ; }
Signature · 1 line
2.27.0
Shuah Khan
maintainerPATCH 11/13 drivers/staging/rtl8188eu: convert stats to use seqnum_ops
In reply to Shuah Khanseqnum_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().
--- drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 23 ++++++++++++++----- .../staging/rtl8188eu/include/rtw_mlme_ext.h | 3 ++- 2 files changed, 19 insertions(+), 7 deletions(-)
drivers/staging/rtl8188eu/core/rtw_mlme_ext.c +17 -6
@@ -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);
drivers/staging/rtl8188eu/include/rtw_mlme_ext.h +2 -1
@@ -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;
Signature · 1 line
2.27.0
Shuah Khan
maintainerPATCH 12/13 drivers/staging/unisys/visorhba: convert stats to use seqnum_ops
In reply to Shuah Khanseqnum_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.
--- .../staging/unisys/visorhba/visorhba_main.c | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-)
drivers/staging/unisys/visorhba/visorhba_main.c +19 -18
@@ -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 ); } } }
Signature · 1 line
2.27.0
Greg KH
maintainerRe: [PATCH 12/13] drivers/staging/unisys/visorhba: convert stats to use seqnum_ops
Quoted 30 lines · Shuah Khan
seqnum_opsapi is introduced to be used when a variable is used as a sequence/stat counter and doesn't guard object lifetimes. This clearly differentiatesatomic_tusages that guard object lifetimes.seqnum32 variables wrap around to
INT_MINwhen it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.
atomic_tvariables used forerror_countandios_thresholdare atomic counters and guarded by max. values. No change to the behavior with this change.Signed-off-by Shuah Khan <[email protected]>--- .../staging/unisys/visorhba/visorhba_main.c | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-)drivers/staging/unisys/visorhba/visorhba_main.c +1
-0@@ -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>
drivers/staging/unisys/visorhba/visorhba_main.c MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_GUID_STR); · line 46
Quoted 7 lines
drivers/staging/unisys/visorhba/visorhba_main.c +2
-2@@ -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
Shuah Khan
maintainerRe: [PATCH 12/13] drivers/staging/unisys/visorhba: convert stats to use seqnum_ops
Quoted 33 lines · Greg KH
Quoted 32 lines · Shuah Khan
seqnum_opsapi is introduced to be used when a variable is used as a sequence/stat counter and doesn't guard object lifetimes. This clearly differentiatesatomic_tusages that guard object lifetimes.seqnum32 variables wrap around to
INT_MINwhen it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.
atomic_tvariables used forerror_countandios_thresholdare atomic counters and guarded by max. values. No change to the behavior with this change.Signed-off-by Shuah Khan <[email protected]>--- .../staging/unisys/visorhba/visorhba_main.c | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-)drivers/staging/unisys/visorhba/visorhba_main.c +1
-0@@ -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;
Quoted 8 lines
drivers/staging/unisys/visorhba/visorhba_main.c MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_GUID_STR); · line 46
Quoted 5 lines
drivers/staging/unisys/visorhba/visorhba_main.c +2
-2u32 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
Shuah Khan
maintainerPATCH 13/13 security/integrity/ima: converts stats to seqnum_ops
In reply to Shuah Khanseqnum_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.
--- 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(-)
security/integrity/ima/ima.h +3 -2
@@ -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;
security/integrity/ima/ima_api.c +1 -1
@@ -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 ) {
security/integrity/ima/ima_fs.c +2 -2
@@ -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); }
security/integrity/ima/ima_queue.c +4 -3
@@ -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]);
Signature · 1 line
2.27.0
kernel test robot
build botRe: [PATCH 13/13] security/integrity/ima: converts stats to seqnum_ops
I love your patch! Perhaps something to improve:
Hi Shuah,
I love your patch! Perhaps something to improve:
[auto build test WARNING on staging/staging-testing]
[also build test WARNING on integrity/next-integrity char-misc/char-misc-testing usb/usb-testing linus/master v5.10-rc3 next-20201110] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Shuah-Khan/Introduce-seqnum_ops/20201111-035753 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git f4acd33c446b2ba97f1552a4da90050109d01ca7 config: sh-allmodconfig (attached as .config) compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/4124aef613b0e30b7da08aaec750983854e1ca5a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Shuah-Khan/Introduce-seqnum_ops/20201111-035753 git checkout 4124aef613b0e30b7da08aaec750983854e1ca5a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag as appropriate
All warnings (new ones prefixed by >>):
In file included from security/integrity/ima/ima_fs.c:25:
security/integrity/ima/ima.h:178:18: error: field 'len' has incomplete type
178 | struct seqnum64 len; /* number of stored measurements in the list */
| ^~~
security/integrity/ima/ima.h:179:18: error: field 'violations' has incomplete type
179 | struct seqnum64 violations;
| ^~~~~~~~~~
security/integrity/ima/ima_fs.c: In function 'ima_show_htable_value':
security/integrity/ima/ima_fs.c:47:52: error: implicit declaration of function 'seqnum64_read'; did you mean 'seqnum32_read'? [-Werror=implicit-function-declaration]
47 | len = scnprintf(tmpbuf, sizeof(tmpbuf), "%lli\n", seqnum64_read(val));
| ^~~~~~~~~~~~~
| seqnum32_read
>> security/integrity/ima/ima_fs.c:47:46: warning: format '%lli' expects argument of type 'long long int', but argument 4 has type 'int' [-Wformat=]
47 | len = scnprintf(tmpbuf, sizeof(tmpbuf), "%lli\n", seqnum64_read(val));
| ~~~^ ~~~~~~~~~~~~~~~~~~
| | |
| | int
| long long int
| %i
security/integrity/ima/ima_fs.c: In function 'ima_show_htable_violations':
security/integrity/ima/ima_fs.c:56:1: error: control reaches end of non-void function [-Werror=return-type]
56 | }
| ^
security/integrity/ima/ima_fs.c: In function 'ima_show_measurements_count':
security/integrity/ima/ima_fs.c:69:1: error: control reaches end of non-void function [-Werror=return-type]
69 | }
| ^cc1: some warnings being treated as errors
vim +47 security/integrity/ima/ima_fs.c
40
41 static ssize_t ima_show_htable_value(char __user *buf, size_t count,
42 loff_t *ppos, struct seqnum64 *val)
43 {
44 char tmpbuf[32]; /* greater than largest 'long' string value */
45 ssize_t len;
46
> 47 len = scnprintf(tmpbuf, sizeof(tmpbuf), "%lli\n", seqnum64_read(val));
48 return simple_read_from_buffer(buf, count, ppos, tmpbuf, len);
49 }
50 ---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
Quoted 29 lines · Shuah Khan
There are a number of
atomic_tusages in the kernel whereatomic_tapi 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_tcounter usages fromatomic_tusages that guard object lifetimes, hence prone to overflow and underflow errors.The
atomic_tapi 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. Therefcount_tapi was introduced to address resource lifetime problems related toatomic_twrapping. There is a large overlap between theatomic_tapi used for resource lifetimes and just counters, stats, and sequence numbers. It has become difficult to differentiate between theatomic_tusages that should be converted torefcount_tand the ones that can be left alone. Introducingseqnum_opsto wrap the usages that are stats, counters, sequence numbers makes it easier for tools that scan for underflow and overflow onatomic_tusages to detect overflow and underflows to scan just the cases that are prone to errors.Sequence Number api provides interfaces for simple
atomic_tcounter usages that just count, and don't guard resource lifetimes. Theseqnum_opsare built on top ofatomic_tapi, providing a smaller subset ofatomic_tinterfaces necessary to supportatomic_tusages as simple counters. This api hasinit/set/inc/dec/readand doesn't support any otheratomic_tops with the intent to restrict the use of these interfaces as simple counting usages.
Quoted 4 lines
Sequence Numbers wrap around to
INT_MINwhen it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states. Overflowing toINT_MINis consistent with theatomic_tapi, 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
Shuah Khan
maintainerQuoted 30 lines · Alan Stern
Quoted 29 lines · Shuah Khan
There are a number of
atomic_tusages in the kernel whereatomic_tapi 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_tcounter usages fromatomic_tusages that guard object lifetimes, hence prone to overflow and underflow errors.The
atomic_tapi 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. Therefcount_tapi was introduced to address resource lifetime problems related toatomic_twrapping. There is a large overlap between theatomic_tapi used for resource lifetimes and just counters, stats, and sequence numbers. It has become difficult to differentiate between theatomic_tusages that should be converted torefcount_tand the ones that can be left alone. Introducingseqnum_opsto wrap the usages that are stats, counters, sequence numbers makes it easier for tools that scan for underflow and overflow onatomic_tusages to detect overflow and underflows to scan just the cases that are prone to errors.Sequence Number api provides interfaces for simple
atomic_tcounter usages that just count, and don't guard resource lifetimes. Theseqnum_opsare built on top ofatomic_tapi, providing a smaller subset ofatomic_tinterfaces necessary to supportatomic_tusages as simple counters. This api hasinit/set/inc/dec/readand doesn't support any otheratomic_tops with the intent to restrict the use of these interfaces as simple counting usages.
Quoted 9 lines
Quoted 4 lines
Sequence Numbers wrap around to
INT_MINwhen it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states. Overflowing toINT_MINis consistent with theatomic_tapi, 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_ONCEandWRITE_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
Quoted 3 lines · Shuah Khan
There are a number of
atomic_tusages in the kernel whereatomic_tapi 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?
Shuah Khan
maintainerQuoted 8 lines · Matthew Wilcox
Quoted 3 lines · Shuah Khan
There are a number of
atomic_tusages in the kernel whereatomic_tapi 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
Quoted 6 lines · Shuah Khan
Quoted 5 lines · Matthew Wilcox
Quoted 3 lines · Shuah Khan
There are a number of
atomic_tusages in the kernel whereatomic_tapi is used strictly for counting sequence numbers and other statistical counters and not for managing object lifetime.
Quoted 6 lines
Quoted 3 lines
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_torstat_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).