Message-ID: <20201014091720.GC2628@hirez.programming.kicks-ass.net>
Date:   Wed, 14 Oct 2020 11:17:20 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Shuah Khan <skhan@...uxfoundation.org>
Cc:     Kees Cook <keescook@...omium.org>, corbet@....net,
        gregkh@...uxfoundation.org, shuah@...nel.org, rafael@...nel.org,
        johannes@...solutions.net, lenb@...nel.org, james.morse@....com,
        tony.luck@...el.com, bp@...en8.de, arve@...roid.com,
        tkjos@...roid.com, maco@...roid.com, joel@...lfernandes.org,
        christian@...uner.io, hridya@...gle.com, surenb@...gle.com,
        minyard@....org, arnd@...db.de, mchehab@...nel.org,
        rric@...nel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-acpi@...r.kernel.org, devel@...verdev.osuosl.org,
        openipmi-developer@...ts.sourceforge.net,
        linux-edac@...r.kernel.org, Will Deacon <will@...nel.org>
Subject: Re: [PATCH v3 00/11] Introduce Simple atomic counters

On Tue, Oct 13, 2020 at 08:12:20PM -0600, Shuah Khan wrote:

> They don't add any new behavior, As Kees mentioned they do give us a
> way to clearly differentiate atomic usages that can wrap.

No it doesn't! atomic_t can wrap, this thing can wrap, no distinction.

All it does is fragment the API and sow confusion. FOR NO BENEFIT.

> > Worse, it mixes 2 unrelated cases into one type, which just makes a
> > mockery of things (all the inc_return users are not statistics, some
> > might even mis-behave if they wrap).
> > 
> 
> You are right that all inc_return usages aren't statistics. There are
> 3 distinct usages:
> 
> 1. Stats
> 2. Cases where wrapping is fine
> 3. Cases where wrapping could be a problem. In which case, this API
>    shouldn't be used.

And yet, afaict patch 4 is case 3...

> There is no need to keep inc_return in this API as such. I included it
> so it can be used for above cases 1 and 2, so the users don't have to
> call inc() followed by read(). It can be left out of the API.
> 
> The atomic_t usages in the kernel fall into the following categories:
> 
> 1. Stats (tolerance for accuracy determines whether they need to be
>    atomic or not). RFC version included non-atomic API for cases
>    when lossiness is acceptable. All these cases use/need just init
>    and inc. There are two variations in this case:
> 
>    a. No checks for wrapping. Use signed value.
>    b. No checks for wrapping, but return unsigned.
> 
> 2. Reference counters that release resource and rapping could result
>    in use-after-free type problems. There are two variations in this
>    case:
> 
>    a. Increments and decrements aren't bounded.
>    b. Increments and decrements are bounded.
> 
>    Currently tools that flag unsafe atomic_t usages that are candidates
>    for refcount_t conversions don't make a distinction between the two.
> 
>    The second case, since increments and decrements are bounded, it is
>    safe to continue to use it. At the moment there is no good way to
>    tell them apart other than looking at each of these cases.
> 
> 3. Reference counters that manage/control states. Wrapping is a problem
>    in this case, as it could lead to undefined behavior. These cases
>    don't use test and free, use inc/dec. At the moment there is no good
>    way to tell them apart other than looking at each of these cases.
>    This is addressed by REFCOUNT_SATURATED case.

Wrong! The atomic usage in mutex doesn't fall in any of those
categories.


The only thing you're all saying that makes sense is that unintentional
wrapping can have bad consequences, the rest is pure confusion.

Focus on the non-wrapping cases, _everything_ else is not going
anywhere.

So audit the kernel, find the cases that should not wrap, categorize and
create APIs for them that trap the wrapping. But don't go around
confusing things that don't need confusion.

Powered by blists - more mailing lists