Re: [RFC PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic

[Kees Cook]

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