linux-kernel - Re: [PATCH RFC] rcu: change return type to bool

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: <20150523185820.13b5ad82@gandalf.local.home>
Date:	Sat, 23 May 2015 18:58:20 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Nicholas Mc Guire <hofrat@...dl.org>
Cc:	Lai Jiangshan <laijs@...fujitsu.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Josh Triplett <josh@...htriplett.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] rcu: change return type to bool

On Sat, 23 May 2015 16:47:52 +0200
Nicholas Mc Guire <hofrat@...dl.org> wrote:

> Type-checking coccinelle spatches are being used to locate type mismatches
> between function signatures and return values in this case this produced:
> ./kernel/rcu/srcu.c:271 WARNING: return of wrong type
>         int != unsigned long,
> 
> srcu_readers_active() returns an int that is the sum of per_cpu unsigned
> long but the only user is cleanup_srcu_struct() which is using it as a
> boolean (condition) to see if there is any readers rather than actually
> using the approximate number of readers. The theoretically possible
> unsigned long overflow case does not need to be handled explicitly - if
> we had 4G++ readers then something else went wrong a long time ago.
> 
> proposal: change the return type to boolean. The function name is left
>           unchanged as it fits the naming expectation for a boolean.
> 
> patch was compile teseted for x86_64_defconfig (implies CONFIG_SRCU=y)
> 
> patch is against 4.1-rc4 (localversion-next is -next-20150522)
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@...dl.org>
> ---
>  kernel/rcu/srcu.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/srcu.c b/kernel/rcu/srcu.c
> index fb33d35..7c4fbeb 100644
> --- a/kernel/rcu/srcu.c
> +++ b/kernel/rcu/srcu.c
> @@ -252,14 +252,15 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *sp, int idx)
>  }
>  
>  /**
> - * srcu_readers_active - returns approximate number of readers.
> + * srcu_readers_active - returns true if there are readers. and false
> + *                       otherwise
>   * @sp: which srcu_struct to count active readers (holding srcu_read_lock).
>   *
>   * Note that this is not an atomic primitive, and can therefore suffer
>   * severe errors when invoked on an active srcu_struct.  That said, it
>   * can be useful as an error check at cleanup time.
>   */
> -static int srcu_readers_active(struct srcu_struct *sp)
> +static bool srcu_readers_active(struct srcu_struct *sp)
>  {
>  	int cpu;
>  	unsigned long sum = 0;
> @@ -268,7 +269,7 @@ static int srcu_readers_active(struct srcu_struct *sp)
>  		sum += READ_ONCE(per_cpu_ptr(sp->per_cpu_ref, cpu)->c[0]);
>  		sum += READ_ONCE(per_cpu_ptr(sp->per_cpu_ref, cpu)->c[1]);
>  	}
> -	return sum;
> +	return !!sum;

Hmm I wonder if gcc is smart enough to do the above without the need
for !!? That is, will it turn to !! because the return of the function
is bool, or does gcc complain about it not being bool without the !!?
Not a criticism of the patch, just a curiosity.

-- Steve


>  }
>  
>  /**

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/