Re: [PATCH] rcutorture: remove unneeded check

[Tom Rix <trix@redhat.com>]

On 10/9/20 1:18 PM, Paul E. McKenney wrote:
> On Fri, Oct 09, 2020 at 12:47:36PM -0700, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.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

In rcu_prog_nr() there is check for for sync.

if ( ... cur_op->sync ...

   do something

This flags in clang's static analyzer as 'could be null'

later in the function, in a reachable block it is used

cur_ops->sync()

I agree this is not a good check that's why i said is was a false positive.

However when looking closer at how cur_ops is set, it is never uninitialized.

So the check is not needed.

This is not a fix, the code works fine.  It is a small optimization.

Tom

>
>> Signed-off-by: Tom Rix <trix@redhat.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
>>