On Tue, Sep 22, 2020 at 07:43:30PM -0600, Shuah Khan wrote:
> Introduce Simple atomic and non-atomic counters.
>
> 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.
Thank you for working on a counter API! I'm glad to see work here,
though I have some pretty significant changes to request; see below...
>
> The purpose of these counters is twofold: 1. clearly differentiate
> atomic_t counters from atomic_t usages that guard object lifetimes,
> hence prone to overflow and underflow errors. It allows tools that scan
> for underflow and overflow on atomic_t usages to detect overflow and
> underflows to scan just the cases that are prone to errors. 2. provides
> non-atomic counters for cases where atomic isn't necessary.
>
> Simple atomic and non-atomic counters api provides interfaces for simple
> atomic and non-atomic counters that just count, and don't guard resource
> lifetimes. Counters 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.
>
> Using counter_atomic to guard lifetimes could lead to use-after free
> when it overflows and undefined behavior when used to manage state
> changes and device usage/open states.
>
> Signed-off-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
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.
> +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.
> +Simple atomic and non-atomic counters api provides interfaces for simple
> +atomic and non-atomic counters that just count, and don't guard resource
> +lifetimes. Counters 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.
> +
> +Using counter_atomic to guard lifetimes could lead to use-after free
> +when it overflows and undefined behavior when used to manage state
> +changes and device usage/open states.
> +
> +Use refcnt_t interfaces for guarding resources.
typo: refcount_t (this typo is repeated in a few places)
> +
> +.. warning::
> + Counter will wrap around to 0 when it overflows.
> + Should not be used to guard resource lifetimes.
> + Should not be used to manage device state and pm state.
> +
> +Test Counters Module and selftest
> +---------------------------------
> +
> +Please see :ref:`lib/test_counters.c <Test Counters Module>` for how to
> +use these interfaces and also test them.
> +
> +Selftest for testing:
> +:ref:`testing/selftests/lib/test_counters.sh <selftest for counters>`
> +
> +Atomic counter interfaces
> +=========================
> +
> +counter_atomic and counter_atomic_long types use atomic_t and atomic_long_t
> +underneath to leverage atomic_t api, providing a small subset of atomic_t
> +interfaces necessary to support simple counters. ::
> +
> + struct counter_atomic { atomic_t cnt; };
> + struct counter_atomic_long { atomic_long_t cnt; };
"Unsized" and "Long" are both unhelpful here. If it's unsized, that
tells nothing about the counter size. And "long" changes with word size.
I think counters should either _all_ be 64-bit, or they should be
explicitly sized in their name. Either:
struct counter; /* unsigned 64-bit, wraps back around to 0 */
or
struct counter32; /* unsigned 32-bit, wraps back around to 0 */
struct counter64; /* unsigned 64-bit, wraps back around to 0 */
> --- /dev/null
> +++ b/lib/test_counters.c
> @@ -0,0 +1,283 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Kernel module for testing Counters
> + *
> + * Authors:
> + * Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/counters.h>
> +
> +void test_counter_atomic(void)
> +{
> + static struct counter_atomic acnt = COUNTER_ATOMIC_INIT(0);
> + int start_val = counter_atomic_read(&acnt);
> + int end_val;
Please build this test using KUnit.
> + start_val = counter_long_read(&acnt);
> + 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.
--
Kees Cook
