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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux