* [PATCH 11/11] drivers/edac: convert pci counters to counter_atomic32
2020-09-25 23:47 [PATCH 00/11] Introduce Simple atomic and non-atomic counters Shuah Khan
@ 2020-09-25 23:47 ` Shuah Khan
2020-09-28 12:05 ` Borislav Petkov
2020-09-25 23:52 ` [PATCH 00/11] Introduce Simple atomic and non-atomic counters Kees Cook
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Shuah Khan @ 2020-09-25 23:47 UTC (permalink / raw)
To: bp, mchehab, tony.luck, james.morse, rric, gregkh, keescook
Cc: Shuah Khan, linux-edac, linux-kernel
counter_atomic* is introduced to be used when a variable is used as
a simple counter and doesn't guard object lifetimes. This clearly
differentiates atomic_t usages that guard object lifetimes.
counter_atomic* variables will wrap around to 0 when it overflows and
should not be used to guard resource lifetimes, device usage and
open counts that control state changes, and pm states.
atomic_t variables used for pci counters keep track of pci parity and
non-parity errors. Convert them to use counter_atomic32.
Overflow will wrap around and reset the counts as was the case prior to
the conversion.
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
drivers/edac/edac_pci.h | 5 +++--
drivers/edac/edac_pci_sysfs.c | 28 ++++++++++++++--------------
2 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/drivers/edac/edac_pci.h b/drivers/edac/edac_pci.h
index 5175f5724cfa..797b25a6afc0 100644
--- a/drivers/edac/edac_pci.h
+++ b/drivers/edac/edac_pci.h
@@ -30,12 +30,13 @@
#include <linux/pci.h>
#include <linux/types.h>
#include <linux/workqueue.h>
+#include <linux/counters.h>
#ifdef CONFIG_PCI
struct edac_pci_counter {
- atomic_t pe_count;
- atomic_t npe_count;
+ struct counter_atomic32 pe_count;
+ struct counter_atomic32 npe_count;
};
/*
diff --git a/drivers/edac/edac_pci_sysfs.c b/drivers/edac/edac_pci_sysfs.c
index 53042af7262e..d33a726234c0 100644
--- a/drivers/edac/edac_pci_sysfs.c
+++ b/drivers/edac/edac_pci_sysfs.c
@@ -23,8 +23,8 @@ static int edac_pci_log_pe = 1; /* log PCI parity errors */
static int edac_pci_log_npe = 1; /* log PCI non-parity error errors */
static int edac_pci_poll_msec = 1000; /* one second workq period */
-static atomic_t pci_parity_count = ATOMIC_INIT(0);
-static atomic_t pci_nonparity_count = ATOMIC_INIT(0);
+static struct counter_atomic32 pci_parity_count = COUNTER_ATOMIC_INIT(0);
+static struct counter_atomic32 pci_nonparity_count = COUNTER_ATOMIC_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", counter_atomic32_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", counter_atomic32_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);
+ counter_atomic32_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);
+ counter_atomic32_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);
+ counter_atomic32_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);
+ counter_atomic32_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);
+ counter_atomic32_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);
+ counter_atomic32_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 = counter_atomic32_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 != counter_atomic32_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);
+ counter_atomic32_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);
+ counter_atomic32_inc(&pci->counters.npe_count);
if (edac_pci_get_log_npe())
edac_pci_printk(pci, KERN_WARNING,
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 11/11] drivers/edac: convert pci counters to counter_atomic32
2020-09-25 23:47 ` [PATCH 11/11] drivers/edac: convert pci counters to counter_atomic32 Shuah Khan
@ 2020-09-28 12:05 ` Borislav Petkov
0 siblings, 0 replies; 17+ messages in thread
From: Borislav Petkov @ 2020-09-28 12:05 UTC (permalink / raw)
To: Shuah Khan
Cc: mchehab, tony.luck, james.morse, rric, gregkh, keescook,
linux-edac, linux-kernel
On Fri, Sep 25, 2020 at 05:47:25PM -0600, Shuah Khan wrote:
> counter_atomic* is introduced to be used when a variable is used as
> a simple counter and doesn't guard object lifetimes. This clearly
> differentiates atomic_t usages that guard object lifetimes.
>
> counter_atomic* variables will wrap around to 0 when it overflows and
> should not be used to guard resource lifetimes, device usage and
> open counts that control state changes, and pm states.
>
> atomic_t variables used for pci counters keep track of pci parity and
> non-parity errors. Convert them to use counter_atomic32.
>
> Overflow will wrap around and reset the counts as was the case prior to
> the conversion.
>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
> drivers/edac/edac_pci.h | 5 +++--
> drivers/edac/edac_pci_sysfs.c | 28 ++++++++++++++--------------
> 2 files changed, 17 insertions(+), 16 deletions(-)
The patches I was Cced - this one and the apei one, look ok to me.
Acked-by: Borislav Petkov <bp@suse.de>
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-25 23:47 [PATCH 00/11] Introduce Simple atomic and non-atomic counters Shuah Khan
2020-09-25 23:47 ` [PATCH 11/11] drivers/edac: convert pci counters to counter_atomic32 Shuah Khan
@ 2020-09-25 23:52 ` Kees Cook
2020-09-26 0:13 ` Shuah Khan
2020-09-26 16:22 ` Kees Cook
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2020-09-25 23:52 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> -- Addressed Kees's comments:
> 1. Non-atomic counters renamed to counter_simple32 and counter_simple64
> to clearly indicate size.
> 2. Added warning for counter_simple* usage and it should be used only
> when there is no need for atomicity.
> 3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
> 4. Renamed counter_atomic_long to counter_atomic64 and it now uses
> atomic64_t ops and indicates size.
> 5. Test updated for the API renames.
> 6. Added helper functions for test results printing
> 7. Verified that the test module compiles in kunit env. and test
> module can be loaded to run the test.
Thanks for all of this!
> 8. Updated Documentation to reflect the intent to make the API
> restricted so it can never be used to guard object lifetimes
> and state management. I left _return ops for now, inc_return
> is necessary for now as per the discussion we had on this topic.
I still *really* do not want dec_return() to exist. That is asking for
trouble. I'd prefer inc_return() not exist either, but I can live with
it. ;)
--
Kees Cook
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-25 23:52 ` [PATCH 00/11] Introduce Simple atomic and non-atomic counters Kees Cook
@ 2020-09-26 0:13 ` Shuah Khan
2020-09-26 16:33 ` Kees Cook
0 siblings, 1 reply; 17+ messages in thread
From: Shuah Khan @ 2020-09-26 0:13 UTC (permalink / raw)
To: Kees Cook
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac, Shuah Khan
On 9/25/20 5:52 PM, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
>> -- Addressed Kees's comments:
>> 1. Non-atomic counters renamed to counter_simple32 and counter_simple64
>> to clearly indicate size.
>> 2. Added warning for counter_simple* usage and it should be used only
>> when there is no need for atomicity.
>> 3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
>> 4. Renamed counter_atomic_long to counter_atomic64 and it now uses
>> atomic64_t ops and indicates size.
>> 5. Test updated for the API renames.
>> 6. Added helper functions for test results printing
>> 7. Verified that the test module compiles in kunit env. and test
>> module can be loaded to run the test.
>
> Thanks for all of this!
>
>> 8. Updated Documentation to reflect the intent to make the API
>> restricted so it can never be used to guard object lifetimes
>> and state management. I left _return ops for now, inc_return
>> is necessary for now as per the discussion we had on this topic.
>
> I still *really* do not want dec_return() to exist. That is asking for
> trouble. I'd prefer inc_return() not exist either, but I can live with
> it. ;)
>
Thanks. I am equally concerned about adding anything that can be used to
guard object lifetimes. So I will make sure this set won't expand and
plan to remove dec_return() if we don't find any usages.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-26 0:13 ` Shuah Khan
@ 2020-09-26 16:33 ` Kees Cook
2020-09-28 22:52 ` Shuah Khan
0 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2020-09-26 16:33 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Fri, Sep 25, 2020 at 06:13:37PM -0600, Shuah Khan wrote:
> On 9/25/20 5:52 PM, Kees Cook wrote:
> > On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> > > -- Addressed Kees's comments:
> > > 1. Non-atomic counters renamed to counter_simple32 and counter_simple64
> > > to clearly indicate size.
> > > 2. Added warning for counter_simple* usage and it should be used only
> > > when there is no need for atomicity.
> > > 3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
> > > 4. Renamed counter_atomic_long to counter_atomic64 and it now uses
> > > atomic64_t ops and indicates size.
> > > 5. Test updated for the API renames.
> > > 6. Added helper functions for test results printing
> > > 7. Verified that the test module compiles in kunit env. and test
> > > module can be loaded to run the test.
> >
> > Thanks for all of this!
> >
> > > 8. Updated Documentation to reflect the intent to make the API
> > > restricted so it can never be used to guard object lifetimes
> > > and state management. I left _return ops for now, inc_return
> > > is necessary for now as per the discussion we had on this topic.
> >
> > I still *really* do not want dec_return() to exist. That is asking for
> > trouble. I'd prefer inc_return() not exist either, but I can live with
> > it. ;)
> >
>
> Thanks. I am equally concerned about adding anything that can be used to
> guard object lifetimes. So I will make sure this set won't expand and
> plan to remove dec_return() if we don't find any usages.
I would like it much stronger than "if". dec_return() needs to be just
dec() and read(). It will not be less efficient (since they're both
inlines), but it _will_ create a case where the atomicity cannot be used
for ref counting. My point is that anything that _requires_ dec_return()
(or, frankly, inc_return()) is _not_ "just" a statistical counter. It
may not be a refcounter, but it relies on the inc/dec atomicity for some
reason beyond counting in once place and reporting it in another.
--
Kees Cook
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-26 16:33 ` Kees Cook
@ 2020-09-28 22:52 ` Shuah Khan
0 siblings, 0 replies; 17+ messages in thread
From: Shuah Khan @ 2020-09-28 22:52 UTC (permalink / raw)
To: Kees Cook
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac, Shuah Khan
On 9/26/20 10:33 AM, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 06:13:37PM -0600, Shuah Khan wrote:
>> On 9/25/20 5:52 PM, Kees Cook wrote:
>>> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
>>>> -- Addressed Kees's comments:
>>>> 1. Non-atomic counters renamed to counter_simple32 and counter_simple64
>>>> to clearly indicate size.
>>>> 2. Added warning for counter_simple* usage and it should be used only
>>>> when there is no need for atomicity.
>>>> 3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
>>>> 4. Renamed counter_atomic_long to counter_atomic64 and it now uses
>>>> atomic64_t ops and indicates size.
>>>> 5. Test updated for the API renames.
>>>> 6. Added helper functions for test results printing
>>>> 7. Verified that the test module compiles in kunit env. and test
>>>> module can be loaded to run the test.
>>>
>>> Thanks for all of this!
>>>
>>>> 8. Updated Documentation to reflect the intent to make the API
>>>> restricted so it can never be used to guard object lifetimes
>>>> and state management. I left _return ops for now, inc_return
>>>> is necessary for now as per the discussion we had on this topic.
>>>
>>> I still *really* do not want dec_return() to exist. That is asking for
>>> trouble. I'd prefer inc_return() not exist either, but I can live with
>>> it. ;)
>>>
>>
I didn't read this correctly the first time around.
>> Thanks. I am equally concerned about adding anything that can be used to
>> guard object lifetimes. So I will make sure this set won't expand and
>> plan to remove dec_return() if we don't find any usages.
>
> I would like it much stronger than "if". dec_return() needs to be just
> dec() and read(). It will not be less efficient (since they're both
> inlines), but it _will_ create a case where the atomicity cannot be used
> for ref counting. My point is that anything that _requires_ dec_return()
> (or, frankly, inc_return()) is _not_ "just" a statistical counter. It
> may not be a refcounter, but it relies on the inc/dec atomicity for some
> reason beyond counting in once place and reporting it in another.
>
I am not thinking about efficiency rather two calls instead of one if
an decrement needs to followed by return. In any case, I agree with you
that there is no need to add dec_return now without any use-cases.
I will update the patch series to remove it.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-25 23:47 [PATCH 00/11] Introduce Simple atomic and non-atomic counters Shuah Khan
2020-09-25 23:47 ` [PATCH 11/11] drivers/edac: convert pci counters to counter_atomic32 Shuah Khan
2020-09-25 23:52 ` [PATCH 00/11] Introduce Simple atomic and non-atomic counters Kees Cook
@ 2020-09-26 16:22 ` Kees Cook
2020-09-28 22:42 ` Shuah Khan
2020-09-26 16:29 ` Kees Cook
2020-09-27 23:35 ` Joel Fernandes
4 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2020-09-26 16:22 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> This patch series is a result of discussion at the refcount_t BOF
> the Linux Plumbers Conference. In this discussion, we identified
> a need for looking closely and investigating atomic_t usages in
> the kernel when it is used strictly as a counter without it
> controlling object lifetimes and state changes.
BTW, I realized the KSPP issue tracker hadn't broken this task out of
the refcount_t conversion issue[1] into a separate issue, so I've created
it now: https://github.com/KSPP/linux/issues/106
-Kees
[1] https://github.com/KSPP/linux/issues/104
--
Kees Cook
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-26 16:22 ` Kees Cook
@ 2020-09-28 22:42 ` Shuah Khan
0 siblings, 0 replies; 17+ messages in thread
From: Shuah Khan @ 2020-09-28 22:42 UTC (permalink / raw)
To: Kees Cook
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac, Shuah Khan
On 9/26/20 10:22 AM, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
>> This patch series is a result of discussion at the refcount_t BOF
>> the Linux Plumbers Conference. In this discussion, we identified
>> a need for looking closely and investigating atomic_t usages in
>> the kernel when it is used strictly as a counter without it
>> controlling object lifetimes and state changes.
>
> BTW, I realized the KSPP issue tracker hadn't broken this task out of
> the refcount_t conversion issue[1] into a separate issue, so I've created
> it now: https://github.com/KSPP/linux/issues/106
>
Cool. Thanks.
-- Shuah
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-25 23:47 [PATCH 00/11] Introduce Simple atomic and non-atomic counters Shuah Khan
` (2 preceding siblings ...)
2020-09-26 16:22 ` Kees Cook
@ 2020-09-26 16:29 ` Kees Cook
2020-09-28 22:41 ` Shuah Khan
2020-09-27 23:35 ` Joel Fernandes
4 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2020-09-26 16:29 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> 7. Verified that the test module compiles in kunit env. and test
> module can be loaded to run the test.
I meant write it using KUnit interfaces (e.g. KUNIT_EXPECT*(),
kunit_test_suite(), etc):
https://www.kernel.org/doc/html/latest/dev-tools/kunit/
Though I see the docs are still not updated[1] to reflect the Kconfig
(CONFIG_foo_KUNIT_TEST) and file naming conventions (foo_kunit.c).
-Kees
[1] https://lore.kernel.org/lkml/20200911042404.3598910-1-davidgow@google.com/
--
Kees Cook
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-26 16:29 ` Kees Cook
@ 2020-09-28 22:41 ` Shuah Khan
2020-09-28 23:13 ` Kees Cook
0 siblings, 1 reply; 17+ messages in thread
From: Shuah Khan @ 2020-09-28 22:41 UTC (permalink / raw)
To: Kees Cook
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac, Shuah Khan
On 9/26/20 10:29 AM, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
>> 7. Verified that the test module compiles in kunit env. and test
>> module can be loaded to run the test.
>
> I meant write it using KUnit interfaces (e.g. KUNIT_EXPECT*(),
> kunit_test_suite(), etc):
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/
>
> Though I see the docs are still not updated[1] to reflect the Kconfig
> (CONFIG_foo_KUNIT_TEST) and file naming conventions (foo_kunit.c).
>
I would like to be able to run this test outside Kunit env., hence the
choice to go with a module and kselftest script. It makes it easier to
test as part of my workflow as opposed to doing a kunit and build and
running it that way.
I don't mind adding TEST_COUNTERS to kunit default configs though.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-28 22:41 ` Shuah Khan
@ 2020-09-28 23:13 ` Kees Cook
2020-10-06 15:21 ` Shuah Khan
0 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2020-09-28 23:13 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Mon, Sep 28, 2020 at 04:41:47PM -0600, Shuah Khan wrote:
> On 9/26/20 10:29 AM, Kees Cook wrote:
> > On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> > > 7. Verified that the test module compiles in kunit env. and test
> > > module can be loaded to run the test.
> >
> > I meant write it using KUnit interfaces (e.g. KUNIT_EXPECT*(),
> > kunit_test_suite(), etc):
> > https://www.kernel.org/doc/html/latest/dev-tools/kunit/
> >
> > Though I see the docs are still not updated[1] to reflect the Kconfig
> > (CONFIG_foo_KUNIT_TEST) and file naming conventions (foo_kunit.c).
> >
>
> I would like to be able to run this test outside Kunit env., hence the
> choice to go with a module and kselftest script. It makes it easier to
> test as part of my workflow as opposed to doing a kunit and build and
> running it that way.
It does -- you just load it normally like before and it prints out
everything just fine. This is how I use the lib/test_user_copy.c and
lib/test_overflow.c before/after their conversions.
--
Kees Cook
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-28 23:13 ` Kees Cook
@ 2020-10-06 15:21 ` Shuah Khan
0 siblings, 0 replies; 17+ messages in thread
From: Shuah Khan @ 2020-10-06 15:21 UTC (permalink / raw)
To: Kees Cook
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, joel, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac, Shuah Khan
On 9/28/20 5:13 PM, Kees Cook wrote:
> On Mon, Sep 28, 2020 at 04:41:47PM -0600, Shuah Khan wrote:
>> On 9/26/20 10:29 AM, Kees Cook wrote:
>>> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
>>>> 7. Verified that the test module compiles in kunit env. and test
>>>> module can be loaded to run the test.
>>>
>>> I meant write it using KUnit interfaces (e.g. KUNIT_EXPECT*(),
>>> kunit_test_suite(), etc):
>>> https://www.kernel.org/doc/html/latest/dev-tools/kunit/
>>>
>>> Though I see the docs are still not updated[1] to reflect the Kconfig
>>> (CONFIG_foo_KUNIT_TEST) and file naming conventions (foo_kunit.c).
>>>
>>
>> I would like to be able to run this test outside Kunit env., hence the
>> choice to go with a module and kselftest script. It makes it easier to
>> test as part of my workflow as opposed to doing a kunit and build and
>> running it that way.
>
> It does -- you just load it normally like before and it prints out
> everything just fine. This is how I use the lib/test_user_copy.c and
> lib/test_overflow.c before/after their conversions.
>
I am not seeing any kunit links to either of these tests. I find the
lib/test_overflow.c very hard to read.
I am going to stick with what I have for now and handle conversion
later.
I think it might be a good idea to add tests for atomic_t and refcount_t
APIS as well at some point.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-25 23:47 [PATCH 00/11] Introduce Simple atomic and non-atomic counters Shuah Khan
` (3 preceding siblings ...)
2020-09-26 16:29 ` Kees Cook
@ 2020-09-27 23:35 ` Joel Fernandes
2020-09-28 20:34 ` Kees Cook
4 siblings, 1 reply; 17+ messages in thread
From: Joel Fernandes @ 2020-09-27 23:35 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, keescook, gregkh, shuah, rafael, johannes, lenb,
james.morse, tony.luck, bp, arve, tkjos, maco, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> This patch series is a result of discussion at the refcount_t BOF
> the Linux Plumbers Conference. In this discussion, we identified
> a need for looking closely and investigating atomic_t usages in
> the kernel when it is used strictly as a counter without it
> controlling object lifetimes and state changes.
>
> There are a number of atomic_t usages in the kernel where atomic_t api
> is used strictly for counting and not for managing object lifetime. In
> some cases, atomic_t might not even be needed.
>
> The purpose of these counters is twofold: 1. clearly differentiate
> atomic_t counters from atomic_t usages that guard object lifetimes,
> hence prone to overflow and underflow errors. It allows tools that scan
> for underflow and overflow on atomic_t usages to detect overflow and
> underflows to scan just the cases that are prone to errors. 2. provides
> non-atomic counters for cases where atomic isn't necessary.
Nice series :)
It appears there is no user of counter_simple in this series other than the
selftest. Would you be planning to add any conversions in the series itself,
for illustration of use? Sorry if I missed a usage.
Also how do we guard against atomicity of counter_simple RMW operations? Is
the implication that it should be guarded using other synchronization to
prevent lost-update problem?
Some more comments:
1. atomic RMW operations that have a return value are fully ordered. Would
you be adding support to counter_simple for such ordering as well, for
consistency?
2. I felt counter_atomic and counter_atomic64 would be nice equivalents to
the atomic and atomic64 naming currently used (i.e. dropping the '32').
However that is just my opinion and I am ok with either naming.
thanks!
- Joel
>
> Simple atomic and non-atomic counters api provides interfaces for simple
> atomic and non-atomic counters that just count, and don't guard resource
> lifetimes. Counters will wrap around to 0 when it overflows and should
> not be used to guard resource lifetimes, device usage and open counts
> that control state changes, and pm states.
>
> Using counter_atomic to guard lifetimes could lead to use-after free
> when it overflows and undefined behavior when used to manage state
> changes and device usage/open states.
>
> This patch series introduces Simple atomic and non-atomic counters.
> Counter atomic ops leverage atomic_t and provide a sub-set of atomic_t
> ops.
>
> In addition this patch series converts a few drivers to use the new api.
> The following criteria is used for select variables for conversion:
>
> 1. Variable doesn't guard object lifetimes, manage state changes e.g:
> device usage counts, device open counts, and pm states.
> 2. Variable is used for stats and counters.
> 3. The conversion doesn't change the overflow behavior.
>
> Changes since RFC:
> -- Thanks for reviews and reviewed-by, and Acked-by tags. Updated
> the patches with the tags.
> -- Addressed Kees's comments:
> 1. Non-atomic counters renamed to counter_simple32 and counter_simple64
> to clearly indicate size.
> 2. Added warning for counter_simple* usage and it should be used only
> when there is no need for atomicity.
> 3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
> 4. Renamed counter_atomic_long to counter_atomic64 and it now uses
> atomic64_t ops and indicates size.
> 5. Test updated for the API renames.
> 6. Added helper functions for test results printing
> 7. Verified that the test module compiles in kunit env. and test
> module can be loaded to run the test.
> 8. Updated Documentation to reflect the intent to make the API
> restricted so it can never be used to guard object lifetimes
> and state management. I left _return ops for now, inc_return
> is necessary for now as per the discussion we had on this topic.
> -- Updated driver patches with API name changes.
> -- We discussed if binder counters can be non-atomic. For now I left
> them the same as the RFC patch - using counter_atomic32
> -- Unrelated to this patch series:
> The patch series review uncovered improvements could be made to
> test_async_driver_probe and vmw_vmci/vmci_guest. I will track
> these for fixing later.
>
> Shuah Khan (11):
> counters: Introduce counter_simple* and counter_atomic* counters
> selftests:lib:test_counters: add new test for counters
> drivers/base: convert deferred_trigger_count and probe_count to
> counter_atomic32
> drivers/base/devcoredump: convert devcd_count to counter_atomic32
> drivers/acpi: convert seqno counter_atomic32
> drivers/acpi/apei: convert seqno counter_atomic32
> drivers/android/binder: convert stats, transaction_log to
> counter_atomic32
> drivers/base/test/test_async_driver_probe: convert to use
> counter_atomic32
> drivers/char/ipmi: convert stats to use counter_atomic32
> drivers/misc/vmw_vmci: convert num guest devices counter to
> counter_atomic32
> drivers/edac: convert pci counters to counter_atomic32
>
> Documentation/core-api/counters.rst | 174 +++++++++
> MAINTAINERS | 8 +
> drivers/acpi/acpi_extlog.c | 5 +-
> drivers/acpi/apei/ghes.c | 5 +-
> drivers/android/binder.c | 41 +--
> drivers/android/binder_internal.h | 3 +-
> drivers/base/dd.c | 19 +-
> drivers/base/devcoredump.c | 5 +-
> drivers/base/test/test_async_driver_probe.c | 23 +-
> drivers/char/ipmi/ipmi_msghandler.c | 9 +-
> drivers/char/ipmi/ipmi_si_intf.c | 9 +-
> drivers/edac/edac_pci.h | 5 +-
> drivers/edac/edac_pci_sysfs.c | 28 +-
> drivers/misc/vmw_vmci/vmci_guest.c | 9 +-
> include/linux/counters.h | 350 +++++++++++++++++++
> lib/Kconfig | 10 +
> lib/Makefile | 1 +
> lib/test_counters.c | 276 +++++++++++++++
> tools/testing/selftests/lib/Makefile | 1 +
> tools/testing/selftests/lib/config | 1 +
> tools/testing/selftests/lib/test_counters.sh | 5 +
> 21 files changed, 913 insertions(+), 74 deletions(-)
> create mode 100644 Documentation/core-api/counters.rst
> create mode 100644 include/linux/counters.h
> create mode 100644 lib/test_counters.c
> create mode 100755 tools/testing/selftests/lib/test_counters.sh
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-27 23:35 ` Joel Fernandes
@ 2020-09-28 20:34 ` Kees Cook
2020-09-28 21:17 ` Joel Fernandes
0 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2020-09-28 20:34 UTC (permalink / raw)
To: Joel Fernandes
Cc: Shuah Khan, corbet, gregkh, shuah, rafael, johannes, lenb,
james.morse, tony.luck, bp, arve, tkjos, maco, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Sun, Sep 27, 2020 at 07:35:26PM -0400, Joel Fernandes wrote:
> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> > This patch series is a result of discussion at the refcount_t BOF
> > the Linux Plumbers Conference. In this discussion, we identified
> > a need for looking closely and investigating atomic_t usages in
> > the kernel when it is used strictly as a counter without it
> > controlling object lifetimes and state changes.
> >
> > There are a number of atomic_t usages in the kernel where atomic_t api
> > is used strictly for counting and not for managing object lifetime. In
> > some cases, atomic_t might not even be needed.
> >
> > The purpose of these counters is twofold: 1. clearly differentiate
> > atomic_t counters from atomic_t usages that guard object lifetimes,
> > hence prone to overflow and underflow errors. It allows tools that scan
> > for underflow and overflow on atomic_t usages to detect overflow and
> > underflows to scan just the cases that are prone to errors. 2. provides
> > non-atomic counters for cases where atomic isn't necessary.
>
> Nice series :)
>
> It appears there is no user of counter_simple in this series other than the
> selftest. Would you be planning to add any conversions in the series itself,
> for illustration of use? Sorry if I missed a usage.
>
> Also how do we guard against atomicity of counter_simple RMW operations? Is
> the implication that it should be guarded using other synchronization to
> prevent lost-update problem?
>
> Some more comments:
>
> 1. atomic RMW operations that have a return value are fully ordered. Would
> you be adding support to counter_simple for such ordering as well, for
> consistency?
No -- there is no atomicity guarantee for counter_simple. I would prefer
counter_simple not exist at all, specifically for this reason.
> 2. I felt counter_atomic and counter_atomic64 would be nice equivalents to
> the atomic and atomic64 naming currently used (i.e. dropping the '32').
> However that is just my opinion and I am ok with either naming.
I had asked that they be size-named to avoid any confusion (i.e. we're
making a new API).
--
Kees Cook
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-28 20:34 ` Kees Cook
@ 2020-09-28 21:17 ` Joel Fernandes
2020-09-28 23:01 ` Shuah Khan
0 siblings, 1 reply; 17+ messages in thread
From: Joel Fernandes @ 2020-09-28 21:17 UTC (permalink / raw)
To: Kees Cook
Cc: Shuah Khan, corbet, gregkh, shuah, rafael, johannes, lenb,
james.morse, tony.luck, bp, arve, tkjos, maco, christian, hridya,
surenb, minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac
On Mon, Sep 28, 2020 at 01:34:31PM -0700, Kees Cook wrote:
> On Sun, Sep 27, 2020 at 07:35:26PM -0400, Joel Fernandes wrote:
> > On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
> > > This patch series is a result of discussion at the refcount_t BOF
> > > the Linux Plumbers Conference. In this discussion, we identified
> > > a need for looking closely and investigating atomic_t usages in
> > > the kernel when it is used strictly as a counter without it
> > > controlling object lifetimes and state changes.
> > >
> > > There are a number of atomic_t usages in the kernel where atomic_t api
> > > is used strictly for counting and not for managing object lifetime. In
> > > some cases, atomic_t might not even be needed.
> > >
> > > The purpose of these counters is twofold: 1. clearly differentiate
> > > atomic_t counters from atomic_t usages that guard object lifetimes,
> > > hence prone to overflow and underflow errors. It allows tools that scan
> > > for underflow and overflow on atomic_t usages to detect overflow and
> > > underflows to scan just the cases that are prone to errors. 2. provides
> > > non-atomic counters for cases where atomic isn't necessary.
> >
> > Nice series :)
> >
> > It appears there is no user of counter_simple in this series other than the
> > selftest. Would you be planning to add any conversions in the series itself,
> > for illustration of use? Sorry if I missed a usage.
> >
> > Also how do we guard against atomicity of counter_simple RMW operations? Is
> > the implication that it should be guarded using other synchronization to
> > prevent lost-update problem?
> >
> > Some more comments:
> >
> > 1. atomic RMW operations that have a return value are fully ordered. Would
> > you be adding support to counter_simple for such ordering as well, for
> > consistency?
>
> No -- there is no atomicity guarantee for counter_simple. I would prefer
> counter_simple not exist at all, specifically for this reason.
Yeah I am ok with it not existing, especially also as there are no examples
of its conversion/usage in the series.
> > 2. I felt counter_atomic and counter_atomic64 would be nice equivalents to
> > the atomic and atomic64 naming currently used (i.e. dropping the '32').
> > However that is just my opinion and I am ok with either naming.
>
> I had asked that they be size-named to avoid any confusion (i.e. we're
> making a new API).
Works for me.
Cheers,
- Joel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/11] Introduce Simple atomic and non-atomic counters
2020-09-28 21:17 ` Joel Fernandes
@ 2020-09-28 23:01 ` Shuah Khan
0 siblings, 0 replies; 17+ messages in thread
From: Shuah Khan @ 2020-09-28 23:01 UTC (permalink / raw)
To: Joel Fernandes, Kees Cook
Cc: corbet, gregkh, shuah, rafael, johannes, lenb, james.morse,
tony.luck, bp, arve, tkjos, maco, christian, hridya, surenb,
minyard, arnd, mchehab, rric, linux-doc, linux-kernel,
linux-kselftest, linux-acpi, devel, openipmi-developer,
linux-edac, Shuah Khan
On 9/28/20 3:17 PM, Joel Fernandes wrote:
> On Mon, Sep 28, 2020 at 01:34:31PM -0700, Kees Cook wrote:
>> On Sun, Sep 27, 2020 at 07:35:26PM -0400, Joel Fernandes wrote:
>>> On Fri, Sep 25, 2020 at 05:47:14PM -0600, Shuah Khan wrote:
>>>> This patch series is a result of discussion at the refcount_t BOF
>>>> the Linux Plumbers Conference. In this discussion, we identified
>>>> a need for looking closely and investigating atomic_t usages in
>>>> the kernel when it is used strictly as a counter without it
>>>> controlling object lifetimes and state changes.
>>>>
>>>> There are a number of atomic_t usages in the kernel where atomic_t api
>>>> is used strictly for counting and not for managing object lifetime. In
>>>> some cases, atomic_t might not even be needed.
>>>>
>>>> The purpose of these counters is twofold: 1. clearly differentiate
>>>> atomic_t counters from atomic_t usages that guard object lifetimes,
>>>> hence prone to overflow and underflow errors. It allows tools that scan
>>>> for underflow and overflow on atomic_t usages to detect overflow and
>>>> underflows to scan just the cases that are prone to errors. 2. provides
>>>> non-atomic counters for cases where atomic isn't necessary.
>>>
>>> Nice series :)
>>>
Thanks.
>>> It appears there is no user of counter_simple in this series other than the
>>> selftest. Would you be planning to add any conversions in the series itself,
>>> for illustration of use? Sorry if I missed a usage.
>>>
>>> Also how do we guard against atomicity of counter_simple RMW operations? Is
>>> the implication that it should be guarded using other synchronization to
>>> prevent lost-update problem?
>>>
>>> Some more comments:
>>>
>>> 1. atomic RMW operations that have a return value are fully ordered. Would
>>> you be adding support to counter_simple for such ordering as well, for
>>> consistency?
>>
>> No -- there is no atomicity guarantee for counter_simple. I would prefer
>> counter_simple not exist at all, specifically for this reason.
>
> Yeah I am ok with it not existing, especially also as there are no examples
> of its conversion/usage in the series.
>
No. counter_simple is just for counting when there is no need for
atomicity with the premise that there might be some use-cases. You
are right that this patch series doesn't use these. My hunch is though
that atomic_t is overused and it isn't needed in all cases.
I will do some research to look for any places that can use
counter_simple before I spin v2. If I don't find any, I can drop them.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 17+ messages in thread