From: Shuah Khan <skhan@linuxfoundation.org>
To: Kees Cook <keescook@chromium.org>
Cc: corbet@lwn.net, gregkh@linuxfoundation.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
Shuah Khan <skhan@linuxfoundation.org>
Subject: Re: [PATCH v2 01/11] counters: Introduce counter_atomic* counters
Date: Wed, 7 Oct 2020 13:26:53 -0600 [thread overview]
Message-ID: <f0939f9d-d60f-7a97-31bd-e527c79ce284@linuxfoundation.org> (raw)
In-Reply-To: <202010071056.E4804235E@keescook>
On 10/7/20 12:11 PM, Kees Cook wrote:
> On Tue, Oct 06, 2020 at 02:44:32PM -0600, Shuah Khan wrote:
>> Introduce Simple 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.
>>
>> The purpose of these counters is to 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.
>>
>> Simple atomic counters api provides interfaces for simple atomic counters
>> that just count, and don't guard resource lifetimes. Counter 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.
>>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>> ---
>> Documentation/core-api/counters.rst | 103 +++++++++++++++++
>> MAINTAINERS | 7 ++
>> include/linux/counters.h | 173 ++++++++++++++++++++++++++++
>> lib/Kconfig | 10 ++
>> lib/Makefile | 1 +
>> lib/test_counters.c | 157 +++++++++++++++++++++++++
>> 6 files changed, 451 insertions(+)
>> create mode 100644 Documentation/core-api/counters.rst
>> create mode 100644 include/linux/counters.h
>> create mode 100644 lib/test_counters.c
>>
>> diff --git a/Documentation/core-api/counters.rst b/Documentation/core-api/counters.rst
>> new file mode 100644
>> index 000000000000..ba1ce325b639
>> --- /dev/null
>> +++ b/Documentation/core-api/counters.rst
>> @@ -0,0 +1,103 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +======================
>> +Simple atomic counters
>> +======================
>> +
>> +:Author: Shuah Khan
>> +
>> +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.
>> +
>> +The purpose of these counters is to 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.
>> +
>> +Simple atomic counters api provides interfaces for simple atomic counters
>> +that just count, and don't guard resource lifetimes. Counter 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_atomic32_* 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 refcount_t interfaces for guarding resources.
>> +
>> +.. 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_atomic32 and counter_atomic64 types use atomic_t and atomic64_t
>> +underneath to leverage atomic_t api, providing a small subset of atomic_t
>> +interfaces necessary to support simple counters. ::
>> +
>> + struct counter_atomic32 { atomic_t cnt; };
>> + struct counter_atomic64 { atomic64_t cnt; };
>> +
>> +Please see :ref:`Documentation/core-api/atomic_ops.rst <atomic_ops>` for
>> +information on the Semantics and Behavior of Atomic operations.
>> +
>> +.. warning::
>> + It is important to keep the ops to a very small subset to ensure
>> + that the Counter API will never be used for guarding resource
>> + lifetimes and state management.
>> +
>> + inc_return() is added to support current atomic_inc_return()
>> + usages and avoid forcing the use of _inc() followed by _read().
>> +
>> +Initializers
>> +------------
>> +
>> +Interfaces for initializing counters are write operations which in turn
>> +invoke their ``ATOMIC_INIT() and atomic_set()`` counterparts ::
>> +
>> + #define COUNTER_ATOMIC_INIT(i) { .cnt = ATOMIC_INIT(i) }
>> + counter_atomic32_set() --> atomic_set()
>> +
>> + static struct counter_atomic32 acnt = COUNTER_ATOMIC_INIT(0);
>> + counter_atomic32_set(0);
>> +
>> + static struct counter_atomic64 acnt = COUNTER_ATOMIC_INIT(0);
>> + counter_atomic64_set(0);
>> +
>> +Increment interface
>> +-------------------
>> +
>> +Increments counter and doesn't return the new counter value. ::
>> +
>> + counter_atomic32_inc() --> atomic_inc()
>> + counter_atomic64_inc() --> atomic64_inc()
>> +
>> +Increment and return new counter value interface
>> +------------------------------------------------
>> +
>> +Increments counter and returns the new counter value. ::
>> +
>> + counter_atomic32_inc_return() --> atomic_inc_return()
>> + counter_atomic64_inc_return() --> atomic64_inc_return()
>> +
>> +Decrement interface
>> +-------------------
>> +
>> +Decrements counter and doesn't return the new counter value. ::
>> +
>> + counter_atomic32_dec() --> atomic_dec()
>> + counter_atomic64_dec() --> atomic64_dec()
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 33b27e62ce19..4e82d0ffcab0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15839,6 +15839,13 @@ S: Maintained
>> F: Documentation/fb/sm712fb.rst
>> F: drivers/video/fbdev/sm712*
>>
>> +SIMPLE ATOMIC and NON-ATOMIC COUNTERS
>> +M: Shuah Khan <skhan@linuxfoundation.org>
>> +L: linux-kernel@vger.kernel.org
>> +S: Maintained
>> +F: include/linux/counters.h
>> +F: lib/test_counters.c
>> +
>> SIMPLE FIRMWARE INTERFACE (SFI)
>> S: Obsolete
>> W: http://simplefirmware.org/
>> diff --git a/include/linux/counters.h b/include/linux/counters.h
>> new file mode 100644
>> index 000000000000..c0c26a13f768
>> --- /dev/null
>> +++ b/include/linux/counters.h
>> @@ -0,0 +1,173 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Interface for simple atomic counters that just count.
>> + *
>> + * Counter 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 refcount_t interfaces for guarding resources.
>> + *
>> + * The interface provides:
>> + * atomic32 & atomic64 functions:
>> + * increment and no return
>> + * increment and return value
>> + * decrement and no return
>> + * read
>> + * set
>> + *
>> + * counter_atomic32 unctions leverage/use atomic_t interfaces.
>
> typo: functions
Thanks for the catch.
>
>> + * counter_atomic64 functions leverage/use atomic64_t interfaces.
>> + * The counter will wrap around to 0 when it overflows.
>> + * These interfaces should not be used to guard resource lifetimes.
>> + *
>> + * Reference and API guide:
>> + * Documentation/core-api/counters.rst for more information.
>> + *
>> + */
>> +
>> +#ifndef __LINUX_COUNTERS_H
>> +#define __LINUX_COUNTERS_H
>> +
>> +#include <linux/atomic.h>
>> +
>> +/**
>> + * struct counter_atomic32 - Simple atomic counter
>> + * @cnt: int
>> + *
>> + * The counter wraps around to 0, when it overflows. Should not
>> + * be used to guard object lifetimes.
>> + **/
>> +struct counter_atomic32 {
>> + atomic_t cnt;
>> +};
>> +
>> +#define COUNTER_ATOMIC_INIT(i) { .cnt = ATOMIC_INIT(i) }
>> +
>> +/*
>> + * counter_atomic32_inc() - increment counter value
>> + * @cntr: struct counter_atomic32 pointer
>> + *
>> + */
>> +static inline void counter_atomic32_inc(struct counter_atomic32 *cntr)
>> +{
>> + atomic_inc(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic32_inc_return() - increment counter value and return it
>> + * @cntr: struct counter_atomic32 pointer
>> + *
>> + * Return: returns the new counter value after incrementing it
>> + */
>> +static inline int counter_atomic32_inc_return(struct counter_atomic32 *cntr)
>> +{
>> + return atomic_inc_return(&cntr->cnt);
>> +}
>
> So, there's an issue here between the types and the documentation: while
> this will eventually wrap around to 0, it will first go through the
> negative value space (i.e. INT_MAX + 1 == INT_MIN, INT_MIN < 0).
>
Right. It does go through INT_MIN state before it wraps around.
> Current users of atomic_t should already be expecting this, but does it
> make sense?
>
> i.e. should the documentation be updated to "wraps around to negative
> values", or should the counter API be updated to force the unsigned
> value:
>
> +static inline u32 counter_atomic32_inc_return(struct counter_atomic32 *cntr)
> +{
> + return (u32)atomic_inc_return(&cntr->cnt);
> +}
>
> I see many forcing the return type from atomic_*{read,return}*():
>
> $ git grep -E '\((unsigned|unsigned int|u32)\).*\batomic.*(read|return)' | wc -l
> 67
>
> My instinct is to say leave it "int" and adjust documentation, which is
> the least disruptive, but I am enticed by the desire to make sure a
> counter doesn't "misbehave" and go negative when the usage wants it
> always positive.
>
I would recommend leaving it as "int". Changing the API to unsigned has
other ramifications and cascading changes.
My quick search shows me there are 612 atomic_inc_return usages and
14 out of them are forcing the return type from int to u32.
For atomic_read the numbers are 51 out of 5833 forcing u32. We have
couple of options:
1. Update the documentation since we have more cases where
int is just fine.
2. Add counter_atomic32_inc_return_u32() variant to cover these few
cases that are forcing the return.
I recommend going with option 1 with Documentation update and add
option 2 when we convert one of these 60+.
>> +static void test_counter_atomic32_overflow(void)
>> +{
>> + static struct counter_atomic32 ucnt = COUNTER_ATOMIC_INIT(0);
>> + static struct counter_atomic32 ocnt = COUNTER_ATOMIC_INIT(INT_MAX);
>> + int start_val;
>> + int end_val;
>> +
>> + start_val = counter_atomic32_read(&ucnt);
>> + counter_atomic32_dec(&ucnt);
>> + end_val = counter_atomic32_read(&ucnt);
>
> This is testing that counter operations match native int operations,
> which seems fine. I wonder if hard-coded values should be added too, to
> just more directly map the explicit expectations? E.g. adding a second
> test with each:
>
> test_counter_result_print32("Test underflow (int)",
> start_val, end_val, start_val-1);
> test_counter_result_print32("Test underflow (-1)",
> start_val, end_val, -1);
>
>
Yeah. I can add that.
>> +
>> + start_val = counter_atomic32_read(&ocnt);
>> + end_val = counter_atomic32_inc_return(&ocnt);
>
> and:
>
> test_counter_result_print32("Test overflow (int)",
> start_val, end_val, start_val+1);
> test_counter_result_print32("Test underflow (INT_MIN)",
> start_val, end_val, INT_MIN);
>
Sure.
>
> Otherwise, yes, looks great; thank you!
>
thanks,
-- Shuah
next prev parent reply other threads:[~2020-10-07 19:26 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-06 20:44 [PATCH v2 00/11] Introduce Simple atomic counters Shuah Khan
2020-10-06 20:44 ` [PATCH v2 01/11] counters: Introduce counter_atomic* counters Shuah Khan
2020-10-07 9:04 ` Greg KH
2020-10-08 17:18 ` Shuah Khan
2020-10-07 18:11 ` Kees Cook
2020-10-07 19:26 ` Shuah Khan [this message]
2020-10-07 20:30 ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 02/11] selftests:lib:test_counters: add new test for counters Shuah Khan
2020-10-07 18:12 ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 03/11] drivers/base: convert deferred_trigger_count and probe_count to counter_atomic32 Shuah Khan
2020-10-07 18:13 ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count " Shuah Khan
2020-10-07 18:15 ` Kees Cook
2020-10-07 19:33 ` Shuah Khan
2020-10-07 19:38 ` Johannes Berg
2020-10-07 19:59 ` Shuah Khan
2020-10-07 20:43 ` Kees Cook
2020-10-08 6:42 ` Johannes Berg
2020-10-08 7:37 ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 05/11] drivers/acpi: convert seqno counter_atomic32 Shuah Khan
2020-10-07 18:16 ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 06/11] drivers/acpi/apei: " Shuah Khan
2020-10-07 18:17 ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32 Shuah Khan
2020-10-07 18:18 ` Kees Cook
2020-10-09 12:39 ` Christian Brauner
2020-10-06 20:44 ` [PATCH v2 08/11] drivers/base/test/test_async_driver_probe: convert to use counter_atomic32 Shuah Khan
2020-10-07 18:20 ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 09/11] drivers/char/ipmi: convert stats " Shuah Khan
2020-10-07 18:21 ` Kees Cook
2020-10-06 20:44 ` [PATCH v2 10/11] drivers/misc/vmw_vmci: convert num guest devices counter to counter_atomic32 Shuah Khan
2020-10-07 18:27 ` Kees Cook
2020-10-08 17:12 ` Shuah Khan
2020-10-06 20:44 ` [PATCH v2 11/11] drivers/edac: convert pci counters " Shuah Khan
2020-10-07 18:28 ` Kees Cook
2020-10-07 18:30 ` [PATCH v2 00/11] Introduce Simple atomic counters Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f0939f9d-d60f-7a97-31bd-e527c79ce284@linuxfoundation.org \
--to=skhan@linuxfoundation.org \
--cc=corbet@lwn.net \
--cc=gregkh@linuxfoundation.org \
--cc=keescook@chromium.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).