Re: [RFC PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic
On Wed, Sep 23, 2020 at 09:31:34PM +0200, Greg KH wrote:
> On Wed, Sep 23, 2020 at 12:04:58PM -0700, Kees Cook wrote:
> > On Wed, Sep 23, 2020 at 07:10:27AM +0200, Greg KH wrote:
> > > On Tue, Sep 22, 2020 at 07:43:36PM -0600, Shuah Khan wrote:
> > > > 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_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
> > > > + struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
> > > > + struct counter_atomic obj_created[BINDER_STAT_COUNT];
> > > > + struct counter_atomic obj_deleted[BINDER_STAT_COUNT];
> > >
> > > These are just debugging statistics, no reason they have to be atomic
> > > variables at all and they should be able to just be "struct counter"
> > > variables instead.
> >
> > But there's no reason for them _not_ to be atomic. Please let's keep
> > this API as always safe. Why even provide a new foot-gun here?
>
> These are debugging things, how can you shoot yourself in the foot with
> that???
Because suddenly you might be trying to use these values for debugging
only to dig and dig to discover that because they were non-atomic, some
parallel race cause a counter to get dropped, etc.
Since we can design this API robustly, let's take the opportunity to do
so.
--
Kees Cook
Re: [RFC PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic
On Wed, Sep 23, 2020 at 12:04:58PM -0700, Kees Cook wrote:
> On Wed, Sep 23, 2020 at 07:10:27AM +0200, Greg KH wrote:
> > On Tue, Sep 22, 2020 at 07:43:36PM -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_atomic.
> > >
> > > This conversion doesn't change the oveflow wrap around behavior.
> > >
> > > Signed-off-by: Shuah Khan
> > > ---
> > > 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..11a0407c46df 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -66,6 +66,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > >
> > > #include
> > > #include
> > > @@ -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_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
> > > + struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
> > > + struct counter_atomic obj_created[BINDER_STAT_COUNT];
> > > + struct counter_atomic obj_deleted[BINDER_STAT_COUNT];
> >
> > These are just debugging statistics, no reason they have to be atomic
> > variables at all and they should be able to just be "struct counter"
> > variables instead.
>
> But there's no reason for them _not_ to be atomic. Please let's keep
> this API as always safe. Why even provide a new foot-gun here?
These are debugging things, how can you shoot yourself in the foot with
that???
thanks,
greg k-h
Re: [RFC PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic
On Wed, Sep 23, 2020 at 07:10:27AM +0200, Greg KH wrote:
> On Tue, Sep 22, 2020 at 07:43:36PM -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_atomic.
> >
> > This conversion doesn't change the oveflow wrap around behavior.
> >
> > Signed-off-by: Shuah Khan
> > ---
> > 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..11a0407c46df 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -66,6 +66,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -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_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
> > + struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
> > + struct counter_atomic obj_created[BINDER_STAT_COUNT];
> > + struct counter_atomic obj_deleted[BINDER_STAT_COUNT];
>
> These are just debugging statistics, no reason they have to be atomic
> variables at all and they should be able to just be "struct counter"
> variables instead.
But there's no reason for them _not_ to be atomic. Please let's keep
this API as always safe. Why even provide a new foot-gun here?
--
Kees Cook
Re: [RFC PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic
On Tue, Sep 22, 2020 at 07:43:36PM -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_atomic.
>
> This conversion doesn't change the oveflow wrap around behavior.
>
> Signed-off-by: Shuah Khan
> ---
> 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..11a0407c46df 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -66,6 +66,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -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_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
> + struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
> + struct counter_atomic obj_created[BINDER_STAT_COUNT];
> + struct counter_atomic obj_deleted[BINDER_STAT_COUNT];
These are just debugging statistics, no reason they have to be atomic
variables at all and they should be able to just be "struct counter"
variables instead.
thanks for looking into all of these!
greg k-h
[RFC PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic
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_atomic.
This conversion doesn't change the oveflow wrap around behavior.
Signed-off-by: Shuah Khan
---
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..11a0407c46df 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -66,6 +66,7 @@
#include
#include
#include
+#include
#include
#include
@@ -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_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
+ struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
+ struct counter_atomic obj_created[BINDER_STAT_COUNT];
+ struct counter_atomic 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_atomic_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_atomic_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_atomic_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_atomic_inc(&binder_stats.bc[_IOC_NR(cmd)]);
+ counter_atomic_inc(&proc->stats.bc[_IOC_NR(cmd)]);
+ counter_atomic_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_atomic_inc(&binder_stats.br[_IOC_NR(cmd)]);
+ counter_atomic_inc(&proc->stats.br[_IOC_NR(cmd)]);
+ counter_atomic_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_atomic_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_atomic_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_SI
