Re: [RFC PATCH 01/11] counters: Introduce counter and counter_atomic
From: Shuah Khan
Date: Wed Sep 23 2020 - 16:48:27 EST
On 9/23/20 1:04 PM, Kees Cook wrote:
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...
Thanks for the review.
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.
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.
I would say we will need at least inc_return() for being able to convert
all counter atomic_t usages.
+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.
+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)
Thanks for the catch. Will fit it.
+
+.. 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 */
Will do.
--- /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.
Sounds good.
+ 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.
I knew you would ask for helpers. :)
Yeah will do.
thanks,
-- Shuah