Re: [RFC PATCH 01/11] counters: Introduce counter and counter_atomic

[Shuah Khan]

On 9/23/20 2:58 PM, Kees Cook wrote:
> On Wed, Sep 23, 2020 at 02:48:22PM -0600, Shuah Khan wrote:
> > On 9/23/20 1:04 PM, Kees Cook wrote:
> > > On Tue, Sep 22, 2020 at 07:43:30PM -0600, Shuah Khan wrote:
> > > I would really like these APIs to be _impossible_ to use for object
> > > lifetime management. To that end, I would like to have all of the
> > > *_return() functions removed. It should be strictly init, inc, dec,
> > > read.
> >
> > Yes. I am with you on making this API as small as possible so it won't
> > be used for lifetime mgmt. That means no support for:
> >
> > *_test, add_negative etc.
> >
> > I started out with just init, inc, dec, read. As I started looking
> > for candidates that can be converted to counters, I found inc_return()
> > usages. I think we need inc_return() for sure. I haven't come across
> > atomic_dec_return() yet.
>
> What are the inc_return() cases? If they're not "safe" to use inc() and
> then read(), then those likely need a closer look at what they're doing.

3 in this series I sent. I would say I barely scratched the surface
when it comes to finding candidates for converting.

drivers/android/binder.c
drivers/acpi/acpi_extlog.c
drivers/acpi/apei/ghes.c

These uses look reasonable to me. Having this inc_return() will save
making _inc() followed by _read()

> > > > +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.
> > >
> > > Why even force the distinction? I think all the counters should be
> > > atomic and then there is no chance they will get accidentally used in
> > > places where someone *thinks* it's safe to use a non-atomic. So,
> > > "_atomic" can be removed from the name and the non-atomic implementation
> > > can get removed. Anyone already using non-atomic counters is just using
> > > "int" and "long" anyway. Let's please only create APIs that are always
> > > safe to use, and provide some benefit over a native time.
> >
> > I am with Greg on this. I think we will find several atomic_t usages
> > that don't need atomicity.
>
> If you want to distinguish from atomic and create a wrapping "int", how
> about making "counter" be the atomic and name the other "counter_unsafe"
> (or "counter_best_effort", "counter_simple", ...) etc?

I will change counter to counter_simple and add a warning that this
should only be used when atomic isn't needed. I can outline some
tips for choosing the right one.

thanks,
-- Shuah