linux-kernel - Re: [PATCH] rcutorture: remove unneeded check

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]

Date:   Fri, 9 Oct 2020 13:18:25 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     trix@...hat.com
Cc:     dave@...olabs.net, josh@...htriplett.org, rostedt@...dmis.org,
        mathieu.desnoyers@...icios.com, jiangshanlai@...il.com,
        joel@...lfernandes.org, natechancellor@...il.com,
        ndesaulniers@...gle.com, linux-kernel@...r.kernel.org,
        rcu@...r.kernel.org, clang-built-linux@...glegroups.com
Subject: Re: [PATCH] rcutorture: remove unneeded check

On Fri, Oct 09, 2020 at 12:47:36PM -0700, trix@...hat.com wrote:
> From: Tom Rix <trix@...hat.com>
> 
> clang static analysis reports this problem:
> 
> rcutorture.c:1999:2: warning: Called function pointer
>   is null (null dereference)
>         cur_ops->sync(); /* Later readers see above write. */
>         ^~~~~~~~~~~~~~~
> 
> This is a false positive triggered by an earlier, later ignored
> NULL check of sync() op.  By inspection of the rcu_torture_ops,
> the sync() op is never uninitialized.  So this earlier check is
> not needed.

You lost me on this one.  This check is at the very beginning of
rcu_torture_fwd_prog_nr().  Or are you saying that clang is seeing an
earlier check in one of rcu_torture_fwd_prog_nr()'s callers?  If so,
where exactly is this check?

In any case, the check is needed because all three functions are invoked
if there is a self-propagating RCU callback that ensures that there is
always an RCU grace period outstanding.

Ah.  Is clang doing local analysis and assuming that because there was
a NULL check earlier, then the pointer might be NULL later?  That does
not seem to me to be a sound check.

So please let me know exactly what is causing clang to emit this
diagnostic.  It might or might not be worth fixing this, but either way
I need to understand the situation so as to be able to understand the
set of feasible fixes.

						Thanx, Paul

> Signed-off-by: Tom Rix <trix@...hat.com>
> ---
>  kernel/rcu/rcutorture.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> index beba9e7963c8..6efc03a1d623 100644
> --- a/kernel/rcu/rcutorture.c
> +++ b/kernel/rcu/rcutorture.c
> @@ -1989,7 +1989,7 @@ static void rcu_torture_fwd_prog_nr(struct rcu_fwd *rfp,
>  	unsigned long stopat;
>  	static DEFINE_TORTURE_RANDOM(trs);
>  
> -	if  (cur_ops->call && cur_ops->sync && cur_ops->cb_barrier) {
> +	if  (cur_ops->call && cur_ops->cb_barrier) {
>  		init_rcu_head_on_stack(&fcs.rh);
>  		selfpropcb = true;
>  	}
> -- 
> 2.18.1
>