* [PATCH 05/11] drivers/acpi: convert seqno 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-25 23:47 ` [PATCH 06/11] drivers/acpi/apei: " Shuah Khan
` (4 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Shuah Khan @ 2020-09-25 23:47 UTC (permalink / raw)
To: rafael, lenb, gregkh, keescook; +Cc: Shuah Khan, linux-acpi, 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.
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.
Convert it to use counter_atomic32.
This conversion doesn't change the overflow wrap around behavior.
Acked-by: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
drivers/acpi/acpi_extlog.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index f138e12b7b82..d1e733f15cf5 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -12,6 +12,7 @@
#include <linux/ratelimit.h>
#include <linux/edac.h>
#include <linux/ras.h>
+#include <linux/counters.h>
#include <asm/cpu.h>
#include <asm/mce.h>
@@ -93,7 +94,7 @@ static struct acpi_hest_generic_status *extlog_elog_entry_check(int cpu, int ban
static void __print_extlog_rcd(const char *pfx,
struct acpi_hest_generic_status *estatus, int cpu)
{
- static atomic_t seqno;
+ static struct counter_atomic32 seqno;
unsigned int curr_seqno;
char pfx_seq[64];
@@ -103,7 +104,7 @@ static void __print_extlog_rcd(const char *pfx,
else
pfx = KERN_ERR;
}
- curr_seqno = atomic_inc_return(&seqno);
+ curr_seqno = counter_atomic32_inc_return(&seqno);
snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}", pfx, curr_seqno);
printk("%s""Hardware error detected on CPU%d\n", pfx_seq, cpu);
cper_estatus_print(pfx_seq, estatus);
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 06/11] drivers/acpi/apei: convert seqno counter_atomic32
2020-09-25 23:47 [PATCH 00/11] Introduce Simple atomic and non-atomic counters Shuah Khan
2020-09-25 23:47 ` [PATCH 05/11] drivers/acpi: convert seqno counter_atomic32 Shuah Khan
@ 2020-09-25 23:47 ` Shuah Khan
2020-09-25 23:52 ` [PATCH 00/11] Introduce Simple atomic and non-atomic counters Kees Cook
` (3 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Shuah Khan @ 2020-09-25 23:47 UTC (permalink / raw)
To: rafael, james.morse, tony.luck, bp, gregkh, keescook
Cc: Shuah Khan, linux-acpi, 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.
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.
Convert it to use counter_atomic32.
This conversion doesn't change the overflow wrap around behavior.
Acked-by: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
drivers/acpi/apei/ghes.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 81bf71b10d44..92169436be18 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -41,6 +41,7 @@
#include <linux/uuid.h>
#include <linux/ras.h>
#include <linux/task_work.h>
+#include <linux/counters.h>
#include <acpi/actbl1.h>
#include <acpi/ghes.h>
@@ -562,7 +563,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 counter_atomic32 seqno = COUNTER_ATOMIC_INIT(0);
unsigned int curr_seqno;
char pfx_seq[64];
@@ -573,7 +574,7 @@ static void __ghes_print_estatus(const char *pfx,
else
pfx = KERN_ERR;
}
- curr_seqno = atomic_inc_return(&seqno);
+ curr_seqno = counter_atomic32_inc_return(&seqno);
snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}" HW_ERR, pfx, curr_seqno);
printk("%s""Hardware error from APEI Generic Hardware Error Source: %d\n",
pfx_seq, generic->header.source_id);
--
2.25.1
^ permalink raw reply related [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 05/11] drivers/acpi: convert seqno counter_atomic32 Shuah Khan
2020-09-25 23:47 ` [PATCH 06/11] drivers/acpi/apei: " 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)
5 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
` (2 preceding siblings ...)
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
5 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
` (3 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
5 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
` (4 preceding siblings ...)
2020-09-26 16:29 ` Kees Cook
@ 2020-09-27 23:35 ` Joel Fernandes
2020-09-28 20:34 ` Kees Cook
5 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