* [PATCH 07/11] drivers/android/binder: convert stats, transaction_log 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-27 23:39 ` Joel Fernandes
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: gregkh, arve, tkjos, maco, joel, christian, hridya, surenb, keescook
Cc: devel, linux-kernel, Shuah Khan
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.
stats tracks per-process binder statistics. Unsure if there is a chance
of this overflowing, other than stats getting reset to 0. Convert it to
use counter_atomic.
binder_transaction_log:cur is used to keep track of the current log entry
location. Overflow is handled in the code. Since it is used as a
counter, convert it to use counter_atomic32.
This conversion doesn't change the overflow wrap around behavior.
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
drivers/android/binder.c | 41 ++++++++++++++++---------------
drivers/android/binder_internal.h | 3 ++-
2 files changed, 23 insertions(+), 21 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f936530a19b0..52175cd6a62b 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -66,6 +66,7 @@
#include <linux/syscalls.h>
#include <linux/task_work.h>
#include <linux/sizes.h>
+#include <linux/counters.h>
#include <uapi/linux/android/binder.h>
#include <uapi/linux/android/binderfs.h>
@@ -172,22 +173,22 @@ enum binder_stat_types {
};
struct binder_stats {
- atomic_t br[_IOC_NR(BR_FAILED_REPLY) + 1];
- atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
- atomic_t obj_created[BINDER_STAT_COUNT];
- atomic_t obj_deleted[BINDER_STAT_COUNT];
+ struct counter_atomic32 br[_IOC_NR(BR_FAILED_REPLY) + 1];
+ struct counter_atomic32 bc[_IOC_NR(BC_REPLY_SG) + 1];
+ struct counter_atomic32 obj_created[BINDER_STAT_COUNT];
+ struct counter_atomic32 obj_deleted[BINDER_STAT_COUNT];
};
static struct binder_stats binder_stats;
static inline void binder_stats_deleted(enum binder_stat_types type)
{
- atomic_inc(&binder_stats.obj_deleted[type]);
+ counter_atomic32_inc(&binder_stats.obj_deleted[type]);
}
static inline void binder_stats_created(enum binder_stat_types type)
{
- atomic_inc(&binder_stats.obj_created[type]);
+ counter_atomic32_inc(&binder_stats.obj_created[type]);
}
struct binder_transaction_log binder_transaction_log;
@@ -197,7 +198,7 @@ static struct binder_transaction_log_entry *binder_transaction_log_add(
struct binder_transaction_log *log)
{
struct binder_transaction_log_entry *e;
- unsigned int cur = atomic_inc_return(&log->cur);
+ unsigned int cur = counter_atomic32_inc_return(&log->cur);
if (cur >= ARRAY_SIZE(log->entry))
log->full = true;
@@ -3615,9 +3616,9 @@ static int binder_thread_write(struct binder_proc *proc,
ptr += sizeof(uint32_t);
trace_binder_command(cmd);
if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.bc)) {
- atomic_inc(&binder_stats.bc[_IOC_NR(cmd)]);
- atomic_inc(&proc->stats.bc[_IOC_NR(cmd)]);
- atomic_inc(&thread->stats.bc[_IOC_NR(cmd)]);
+ counter_atomic32_inc(&binder_stats.bc[_IOC_NR(cmd)]);
+ counter_atomic32_inc(&proc->stats.bc[_IOC_NR(cmd)]);
+ counter_atomic32_inc(&thread->stats.bc[_IOC_NR(cmd)]);
}
switch (cmd) {
case BC_INCREFS:
@@ -4047,9 +4048,9 @@ static void binder_stat_br(struct binder_proc *proc,
{
trace_binder_return(cmd);
if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.br)) {
- atomic_inc(&binder_stats.br[_IOC_NR(cmd)]);
- atomic_inc(&proc->stats.br[_IOC_NR(cmd)]);
- atomic_inc(&thread->stats.br[_IOC_NR(cmd)]);
+ counter_atomic32_inc(&binder_stats.br[_IOC_NR(cmd)]);
+ counter_atomic32_inc(&proc->stats.br[_IOC_NR(cmd)]);
+ counter_atomic32_inc(&thread->stats.br[_IOC_NR(cmd)]);
}
}
@@ -5841,7 +5842,7 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
BUILD_BUG_ON(ARRAY_SIZE(stats->bc) !=
ARRAY_SIZE(binder_command_strings));
for (i = 0; i < ARRAY_SIZE(stats->bc); i++) {
- int temp = atomic_read(&stats->bc[i]);
+ int temp = counter_atomic32_read(&stats->bc[i]);
if (temp)
seq_printf(m, "%s%s: %d\n", prefix,
@@ -5851,7 +5852,7 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
BUILD_BUG_ON(ARRAY_SIZE(stats->br) !=
ARRAY_SIZE(binder_return_strings));
for (i = 0; i < ARRAY_SIZE(stats->br); i++) {
- int temp = atomic_read(&stats->br[i]);
+ int temp = counter_atomic32_read(&stats->br[i]);
if (temp)
seq_printf(m, "%s%s: %d\n", prefix,
@@ -5863,8 +5864,8 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
BUILD_BUG_ON(ARRAY_SIZE(stats->obj_created) !=
ARRAY_SIZE(stats->obj_deleted));
for (i = 0; i < ARRAY_SIZE(stats->obj_created); i++) {
- int created = atomic_read(&stats->obj_created[i]);
- int deleted = atomic_read(&stats->obj_deleted[i]);
+ int created = counter_atomic32_read(&stats->obj_created[i]);
+ int deleted = counter_atomic32_read(&stats->obj_deleted[i]);
if (created || deleted)
seq_printf(m, "%s%s: active %d total %d\n",
@@ -6054,7 +6055,7 @@ static void print_binder_transaction_log_entry(struct seq_file *m,
int binder_transaction_log_show(struct seq_file *m, void *unused)
{
struct binder_transaction_log *log = m->private;
- unsigned int log_cur = atomic_read(&log->cur);
+ unsigned int log_cur = counter_atomic32_read(&log->cur);
unsigned int count;
unsigned int cur;
int i;
@@ -6124,8 +6125,8 @@ static int __init binder_init(void)
if (ret)
return ret;
- atomic_set(&binder_transaction_log.cur, ~0U);
- atomic_set(&binder_transaction_log_failed.cur, ~0U);
+ counter_atomic32_set(&binder_transaction_log.cur, ~0U);
+ counter_atomic32_set(&binder_transaction_log_failed.cur, ~0U);
binder_debugfs_dir_entry_root = debugfs_create_dir("binder", NULL);
if (binder_debugfs_dir_entry_root)
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 283d3cb9c16e..c77960c01430 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -12,6 +12,7 @@
#include <linux/stddef.h>
#include <linux/types.h>
#include <linux/uidgid.h>
+#include <linux/counters.h>
struct binder_context {
struct binder_node *binder_context_mgr_node;
@@ -136,7 +137,7 @@ struct binder_transaction_log_entry {
};
struct binder_transaction_log {
- atomic_t cur;
+ struct counter_atomic32 cur;
bool full;
struct binder_transaction_log_entry entry[32];
};
--
2.25.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32
2020-09-25 23:47 ` [PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32 Shuah Khan
@ 2020-09-27 23:39 ` Joel Fernandes
0 siblings, 0 replies; 17+ messages in thread
From: Joel Fernandes @ 2020-09-27 23:39 UTC (permalink / raw)
To: Shuah Khan
Cc: devel, tkjos, keescook, gregkh, linux-kernel, surenb, arve,
hridya, maco, christian
On Fri, Sep 25, 2020 at 05:47:21PM -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.
>
> stats tracks per-process binder statistics. Unsure if there is a chance
> of this overflowing, other than stats getting reset to 0. Convert it to
> use counter_atomic.
>
> binder_transaction_log:cur is used to keep track of the current log entry
> location. Overflow is handled in the code. Since it is used as a
> counter, convert it to use counter_atomic32.
>
> This conversion doesn't change the overflow wrap around behavior.
>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
thanks,
- Joel
> ---
> drivers/android/binder.c | 41 ++++++++++++++++---------------
> drivers/android/binder_internal.h | 3 ++-
> 2 files changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index f936530a19b0..52175cd6a62b 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -66,6 +66,7 @@
> #include <linux/syscalls.h>
> #include <linux/task_work.h>
> #include <linux/sizes.h>
> +#include <linux/counters.h>
>
> #include <uapi/linux/android/binder.h>
> #include <uapi/linux/android/binderfs.h>
> @@ -172,22 +173,22 @@ enum binder_stat_types {
> };
>
> struct binder_stats {
> - atomic_t br[_IOC_NR(BR_FAILED_REPLY) + 1];
> - atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
> - atomic_t obj_created[BINDER_STAT_COUNT];
> - atomic_t obj_deleted[BINDER_STAT_COUNT];
> + struct counter_atomic32 br[_IOC_NR(BR_FAILED_REPLY) + 1];
> + struct counter_atomic32 bc[_IOC_NR(BC_REPLY_SG) + 1];
> + struct counter_atomic32 obj_created[BINDER_STAT_COUNT];
> + struct counter_atomic32 obj_deleted[BINDER_STAT_COUNT];
> };
>
> static struct binder_stats binder_stats;
>
> static inline void binder_stats_deleted(enum binder_stat_types type)
> {
> - atomic_inc(&binder_stats.obj_deleted[type]);
> + counter_atomic32_inc(&binder_stats.obj_deleted[type]);
> }
>
> static inline void binder_stats_created(enum binder_stat_types type)
> {
> - atomic_inc(&binder_stats.obj_created[type]);
> + counter_atomic32_inc(&binder_stats.obj_created[type]);
> }
>
> struct binder_transaction_log binder_transaction_log;
> @@ -197,7 +198,7 @@ static struct binder_transaction_log_entry *binder_transaction_log_add(
> struct binder_transaction_log *log)
> {
> struct binder_transaction_log_entry *e;
> - unsigned int cur = atomic_inc_return(&log->cur);
> + unsigned int cur = counter_atomic32_inc_return(&log->cur);
>
> if (cur >= ARRAY_SIZE(log->entry))
> log->full = true;
> @@ -3615,9 +3616,9 @@ static int binder_thread_write(struct binder_proc *proc,
> ptr += sizeof(uint32_t);
> trace_binder_command(cmd);
> if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.bc)) {
> - atomic_inc(&binder_stats.bc[_IOC_NR(cmd)]);
> - atomic_inc(&proc->stats.bc[_IOC_NR(cmd)]);
> - atomic_inc(&thread->stats.bc[_IOC_NR(cmd)]);
> + counter_atomic32_inc(&binder_stats.bc[_IOC_NR(cmd)]);
> + counter_atomic32_inc(&proc->stats.bc[_IOC_NR(cmd)]);
> + counter_atomic32_inc(&thread->stats.bc[_IOC_NR(cmd)]);
> }
> switch (cmd) {
> case BC_INCREFS:
> @@ -4047,9 +4048,9 @@ static void binder_stat_br(struct binder_proc *proc,
> {
> trace_binder_return(cmd);
> if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.br)) {
> - atomic_inc(&binder_stats.br[_IOC_NR(cmd)]);
> - atomic_inc(&proc->stats.br[_IOC_NR(cmd)]);
> - atomic_inc(&thread->stats.br[_IOC_NR(cmd)]);
> + counter_atomic32_inc(&binder_stats.br[_IOC_NR(cmd)]);
> + counter_atomic32_inc(&proc->stats.br[_IOC_NR(cmd)]);
> + counter_atomic32_inc(&thread->stats.br[_IOC_NR(cmd)]);
> }
> }
>
> @@ -5841,7 +5842,7 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
> BUILD_BUG_ON(ARRAY_SIZE(stats->bc) !=
> ARRAY_SIZE(binder_command_strings));
> for (i = 0; i < ARRAY_SIZE(stats->bc); i++) {
> - int temp = atomic_read(&stats->bc[i]);
> + int temp = counter_atomic32_read(&stats->bc[i]);
>
> if (temp)
> seq_printf(m, "%s%s: %d\n", prefix,
> @@ -5851,7 +5852,7 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
> BUILD_BUG_ON(ARRAY_SIZE(stats->br) !=
> ARRAY_SIZE(binder_return_strings));
> for (i = 0; i < ARRAY_SIZE(stats->br); i++) {
> - int temp = atomic_read(&stats->br[i]);
> + int temp = counter_atomic32_read(&stats->br[i]);
>
> if (temp)
> seq_printf(m, "%s%s: %d\n", prefix,
> @@ -5863,8 +5864,8 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
> BUILD_BUG_ON(ARRAY_SIZE(stats->obj_created) !=
> ARRAY_SIZE(stats->obj_deleted));
> for (i = 0; i < ARRAY_SIZE(stats->obj_created); i++) {
> - int created = atomic_read(&stats->obj_created[i]);
> - int deleted = atomic_read(&stats->obj_deleted[i]);
> + int created = counter_atomic32_read(&stats->obj_created[i]);
> + int deleted = counter_atomic32_read(&stats->obj_deleted[i]);
>
> if (created || deleted)
> seq_printf(m, "%s%s: active %d total %d\n",
> @@ -6054,7 +6055,7 @@ static void print_binder_transaction_log_entry(struct seq_file *m,
> int binder_transaction_log_show(struct seq_file *m, void *unused)
> {
> struct binder_transaction_log *log = m->private;
> - unsigned int log_cur = atomic_read(&log->cur);
> + unsigned int log_cur = counter_atomic32_read(&log->cur);
> unsigned int count;
> unsigned int cur;
> int i;
> @@ -6124,8 +6125,8 @@ static int __init binder_init(void)
> if (ret)
> return ret;
>
> - atomic_set(&binder_transaction_log.cur, ~0U);
> - atomic_set(&binder_transaction_log_failed.cur, ~0U);
> + counter_atomic32_set(&binder_transaction_log.cur, ~0U);
> + counter_atomic32_set(&binder_transaction_log_failed.cur, ~0U);
>
> binder_debugfs_dir_entry_root = debugfs_create_dir("binder", NULL);
> if (binder_debugfs_dir_entry_root)
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index 283d3cb9c16e..c77960c01430 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -12,6 +12,7 @@
> #include <linux/stddef.h>
> #include <linux/types.h>
> #include <linux/uidgid.h>
> +#include <linux/counters.h>
>
> struct binder_context {
> struct binder_node *binder_context_mgr_node;
> @@ -136,7 +137,7 @@ struct binder_transaction_log_entry {
> };
>
> struct binder_transaction_log {
> - atomic_t cur;
> + struct counter_atomic32 cur;
> bool full;
> struct binder_transaction_log_entry entry[32];
> };
> --
> 2.25.1
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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 07/11] drivers/android/binder: convert stats, transaction_log 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: rafael, linux-kselftest, joel, rric, shuah, devel, minyard,
corbet, surenb, linux-doc, linux-acpi, lenb, tkjos, arnd, bp,
openipmi-developer, mchehab, maco, christian, linux-edac,
tony.luck, gregkh, linux-kernel, arve, james.morse, hridya,
johannes
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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: rafael, linux-kselftest, joel, rric, shuah, devel, minyard,
corbet, surenb, linux-doc, linux-acpi, lenb, tkjos, arnd, bp,
Shuah Khan, openipmi-developer, mchehab, maco, christian,
linux-edac, tony.luck, gregkh, linux-kernel, arve, james.morse,
hridya, johannes
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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: rafael, linux-kselftest, joel, rric, shuah, devel, minyard,
corbet, surenb, linux-doc, linux-acpi, lenb, tkjos, arnd, bp,
openipmi-developer, mchehab, maco, christian, linux-edac,
tony.luck, gregkh, linux-kernel, arve, james.morse, hridya,
johannes
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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: rafael, linux-kselftest, joel, rric, shuah, devel, minyard,
corbet, surenb, linux-doc, linux-acpi, lenb, tkjos, arnd, bp,
Shuah Khan, openipmi-developer, mchehab, maco, christian,
linux-edac, tony.luck, gregkh, linux-kernel, arve, james.morse,
hridya, johannes
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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 07/11] drivers/android/binder: convert stats, transaction_log 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: rafael, linux-kselftest, joel, rric, shuah, devel, minyard,
corbet, surenb, linux-doc, linux-acpi, lenb, tkjos, arnd, bp,
openipmi-developer, mchehab, maco, christian, linux-edac,
tony.luck, gregkh, linux-kernel, arve, james.morse, hridya,
johannes
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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: rafael, linux-kselftest, joel, rric, shuah, devel, minyard,
corbet, surenb, linux-doc, linux-acpi, lenb, tkjos, arnd, bp,
Shuah Khan, openipmi-developer, mchehab, maco, christian,
linux-edac, tony.luck, gregkh, linux-kernel, arve, james.morse,
hridya, johannes
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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: rafael, linux-kselftest, joel, rric, shuah, devel, minyard,
corbet, surenb, linux-doc, linux-acpi, lenb, tkjos, arnd, bp,
openipmi-developer, mchehab, maco, christian, linux-edac,
tony.luck, gregkh, linux-kernel, arve, james.morse, hridya,
johannes
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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: rafael, linux-kselftest, joel, rric, shuah, devel, minyard,
corbet, surenb, linux-doc, linux-acpi, lenb, tkjos, arnd, bp,
Shuah Khan, openipmi-developer, mchehab, maco, christian,
linux-edac, tony.luck, gregkh, linux-kernel, arve, james.morse,
hridya, johannes
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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: rafael, linux-kselftest, joel, rric, shuah, devel, minyard,
corbet, surenb, linux-doc, linux-acpi, lenb, tkjos, arnd, bp,
openipmi-developer, mchehab, maco, christian, linux-edac,
tony.luck, gregkh, linux-kernel, arve, james.morse, hridya,
johannes
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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: rafael, linux-kselftest, joel, rric, shuah, devel, minyard,
corbet, surenb, linux-doc, linux-acpi, lenb, tkjos, arnd, bp,
Shuah Khan, openipmi-developer, mchehab, maco, christian,
linux-edac, tony.luck, gregkh, linux-kernel, arve, james.morse,
hridya, johannes
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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: rafael, linux-kselftest, rric, shuah, devel, arnd, corbet,
surenb, linux-doc, linux-acpi, lenb, tkjos, keescook, minyard,
bp, openipmi-developer, mchehab, maco, christian, linux-edac,
tony.luck, gregkh, linux-kernel, arve, james.morse, hridya,
johannes
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
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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: rafael, linux-kselftest, rric, shuah, devel, minyard, corbet,
surenb, linux-doc, linux-acpi, lenb, tkjos, arnd, bp, Shuah Khan,
openipmi-developer, mchehab, maco, christian, linux-edac,
tony.luck, gregkh, linux-kernel, arve, james.morse, hridya,
johannes
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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: rafael, linux-kselftest, rric, shuah, devel, minyard, corbet,
surenb, linux-doc, linux-acpi, lenb, tkjos, arnd, bp, Shuah Khan,
openipmi-developer, mchehab, maco, christian, linux-edac,
tony.luck, gregkh, linux-kernel, arve, james.morse, hridya,
johannes
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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: rafael, linux-kselftest, rric, shuah, devel, minyard, corbet,
surenb, linux-doc, linux-acpi, lenb, tkjos, arnd, bp, Shuah Khan,
openipmi-developer, mchehab, maco, christian, linux-edac,
tony.luck, gregkh, linux-kernel, arve, james.morse, hridya,
johannes
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 17+ messages in thread