[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4a5b08be-2693-9d4e-729f-931205ff4f0c@linuxfoundation.org>
Date: Thu, 8 Oct 2020 11:18:33 -0600
From: Shuah Khan <skhan@...uxfoundation.org>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: corbet@....net, keescook@...omium.org, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org,
Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v2 01/11] counters: Introduce counter_atomic* counters
On 10/7/20 3:04 AM, Greg KH 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@...uxfoundation.org>
>> Signed-off-by: Shuah Khan <skhan@...uxfoundation.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@...uxfoundation.org>
>> +L: linux-kernel@...r.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.
>> + * 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);
>> +}
>> +
>> +/*
>> + * counter_atomic32_dec() - decrement counter value
>> + * @cntr: struct counter_atomic32 pointer
>> + *
>> + */
>> +static inline void counter_atomic32_dec(struct counter_atomic32 *cntr)
>> +{
>> + atomic_dec(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic32_read() - read counter value
>> + * @cntr: struct counter_atomic32 pointer
>> + *
>> + * Return: return the counter value
>> + */
>> +static inline int counter_atomic32_read(const struct counter_atomic32 *cntr)
>> +{
>> + return atomic_read(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic32_set() - set counter value
>> + * @cntr: struct counter_atomic32 pointer
>> + * @val: new counter value to set
>> + *
>> + */
>> +static inline void
>> +counter_atomic32_set(struct counter_atomic32 *cntr, int val)
>> +{
>> + atomic_set(&cntr->cnt, val);
>> +}
>> +
>> +#ifdef CONFIG_64BIT
>> +/*
>> + * struct counter_atomic64 - Simple atomic counter
>> + * @cnt: atomic64_t
>> + *
>> + * The counter wraps around to 0, when it overflows. Should not
>> + * be used to guard object lifetimes.
>> + */
>> +struct counter_atomic64 {
>> + atomic64_t cnt;
>> +};
>> +
>> +/*
>> + * counter_atomic64_inc() - increment counter value
>> + * @cntr: struct counter_atomic64 pointer
>> + *
>> + */
>> +static inline void counter_atomic64_inc(struct counter_atomic64 *cntr)
>> +{
>> + atomic64_inc(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic64_inc_return() - increment counter value and return it
>> + * @cntr: struct counter_atomic64 pointer
>> + *
>> + * Return: return the new counter value after incrementing it
>> + */
>> +static inline s64
>> +counter_atomic64_inc_return(struct counter_atomic64 *cntr)
>> +{
>> + return atomic64_inc_return(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic64_dec() - decrement counter value
>> + * @cntr: struct counter_atomic64 pointer
>> + *
>> + */
>> +static inline void counter_atomic64_dec(
>> + struct counter_atomic64 *cntr)
>> +{
>> + atomic64_dec(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic64_read() - read counter value
>> + * @cntr: struct counter_atomic64 pointer
>> + *
>> + * Return: return the counter value
>> + */
>> +static inline s64
>> +counter_atomic64_read(const struct counter_atomic64 *cntr)
>> +{
>> + return atomic64_read(&cntr->cnt);
>> +}
>> +
>> +/*
>> + * counter_atomic64_set() - set counter value
>> + * @cntr: struct counter_atomic64 pointer
>> + * &val: new counter value to set
>> + *
>> + */
>> +static inline void
>> +counter_atomic64_set(struct counter_atomic64 *cntr, s64 val)
>> +{
>> + atomic64_set(&cntr->cnt, val);
>> +}
>> +
>> +#endif /* CONFIG_64BIT */
>> +#endif /* __LINUX_COUNTERS_H */
>> diff --git a/lib/Kconfig b/lib/Kconfig
>> index b4b98a03ff98..00cb4264bd8b 100644
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -658,6 +658,16 @@ config OBJAGG
>> config STRING_SELFTEST
>> tristate "Test string functions"
>>
>> +config TEST_COUNTERS
>> + tristate "Test Simple Atomic counter functions"
>> + default n
>
> Nit, if you end up doing another version, this "default n" isn't needed,
> it's the default already :)
>
Looks like I am generating v3 and will fix this one as well.
> Other than that tiny thing, still looks good to me, thanks for doing
> this work.
>
thanks,
-- Shuah
Powered by blists - more mailing lists