linux-kernel - Re: [RFC PATCH 08/11] drivers/base/test/test_async_driver_probe: convert to use counter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200923103329.GI3154647@kroah.com>
Date:   Wed, 23 Sep 2020 12:33:29 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Shuah Khan <skhan@...uxfoundation.org>
Cc:     rafael@...nel.org, keescook@...omium.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 08/11] drivers/base/test/test_async_driver_probe:
 convert to use counter_atomic

On Tue, Sep 22, 2020 at 07:43:37PM -0600, Shuah Khan wrote:
> counter_atomic is introduced to be used when a variable is used as
> a simple counter and doesn't guard object lifetimes. This clearly
> differentiates atomic_t usages that guard object lifetimes.
> 
> counter_atomic variables 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.
> 
> atomic_t variables used to count errors, warns, keep track of timeout,
> and async completion are counters.
> 
> Unsure overflow is a concern for timeout and async completion, and there
> are no checks for overflow to hold them to upper bounds. Overflow and
> wrap around doesn't impact errors, and warns.
> 
> Convert them to use counter_atomic.
> 
> This conversion doesn't change the oveflow wrap around behavior.
> 
> Signed-off-by: Shuah Khan <skhan@...uxfoundation.org>
> ---
>  drivers/base/test/test_async_driver_probe.c | 23 ++++++++++++---------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/base/test/test_async_driver_probe.c b/drivers/base/test/test_async_driver_probe.c
> index 3bb7beb127a9..def08cd03eb5 100644
> --- a/drivers/base/test/test_async_driver_probe.c
> +++ b/drivers/base/test/test_async_driver_probe.c
> @@ -14,11 +14,12 @@
>  #include <linux/numa.h>
>  #include <linux/nodemask.h>
>  #include <linux/topology.h>
> +#include <linux/counters.h>
>  
>  #define TEST_PROBE_DELAY	(5 * 1000)	/* 5 sec */
>  #define TEST_PROBE_THRESHOLD	(TEST_PROBE_DELAY / 2)
>  
> -static atomic_t warnings, errors, timeout, async_completed;
> +static struct counter_atomic warnings, errors, timeout, async_completed;

Having 3 atomic variables here feels like something is not right and we
should switch the code over to using a single lock, and 3 variables.

But that's not the fault of your conversion, it looks fine.

It is interesting that this is digging up all sorts of "odd, why is this
code written like that???" issues with the conversion, which means it's
a good thing to do :)

Reviewed-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>