linux-kernel - Re: [RFC PATCH 01/11] counters: Introduce counter and counter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202009231354.3456CB141@keescook>
Date:   Wed, 23 Sep 2020 13:58:34 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Shuah Khan <skhan@...uxfoundation.org>
Cc:     corbet@....net, gregkh@...uxfoundation.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 01/11] counters: Introduce counter and counter_atomic

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.

> > > +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?

> > > +	end_val = counter_long_dec_return(&acnt);
> > > +	pr_info("Test read decrement and return: %ld to %ld - %s\n",
> > > +		start_val, end_val,
> > > +		((start_val-1 == end_val) ? "PASS" : "FAIL"));
> > 
> > I also see a lot of copy/paste patterns here. These should all use a
> > common helper.
> 
> I knew you would ask for helpers. :)

Heh. inlines for everyone! ;)

> Yeah will do.

Awesome!

-- 
Kees Cook