On 4/24/2025 10:44 AM, Paul E. McKenney wrote:
> On Thu, Apr 24, 2025 at 09:54:14AM +0300, Dan Carpenter wrote:
>> On Wed, Apr 23, 2025 at 05:41:37PM -0700, Paul E. McKenney wrote:
>>> On Wed, Apr 23, 2025 at 10:19:56AM +0300, Dan Carpenter wrote:
>>>> On Wed, Apr 23, 2025 at 10:17:16AM +0300, Dan Carpenter wrote:
>>>>> Hello Joel Fernandes,
>>>>>
>>>>> Commit bd57ec707441 ("rcutorture: Perform more frequent testing of
>>>>> ->gpwrap") from Feb 16, 2025 (linux-next), leads to the following
>>>>> Smatch static checker warning:
>>>>>
>>>>> kernel/rcu/rcutorture.c:4586 rcu_torture_init()
>>>>> warn: missing error code 'firsterr'
>>>>>
>>>>> kernel/rcu/rcutorture.c
>>>>> 4576 if (torture_init_error(firsterr))
>>>>> 4577 goto unwind;
>>>>> 4578 }
>>>>> 4579 if (object_debug)
>>>>> 4580 rcu_test_debug_objects();
>>>>> 4581 torture_init_end();
>>>> ^^^^^^^^^^^^^^^^^^^
>>>> Also:
>>>>
>>>> kernel/rcu/rcutorture.c:4591 rcu_torture_init() warn: double unlock
>>>> 'global &fullstop_mutex' (orig line 4581)
>>>
>>> You lost me on this one. This mutex is acquired by the earlier call to
>>> torture_init_begin(), but only if that function returns true. If that
>>> function returns false, yes, it releases fullstop_mutex, but in that case
>>> rcu_torture_init() returns immediately, thus avoiding this invocation
>>> of torture_init_end().
>>>
>>> Or am I missing something subtle here? Or missing something obvious,
>>> for that matter! ;-)
>>>
>>
>> I explained it badly. Joel's patch added a goto so now we call
>> torture_init_end() twice in a row.
>>
>>> Thanx, Paul
>>>
>>>>> 4582 if (cur_ops->gp_slow_register &&
>>>>> !WARN_ON_ONCE(!cur_ops->gp_slow_unregister))
>>>>> 4583 cur_ops->gp_slow_register(&rcu_fwd_cb_nodelay);
>>>>> 4584
>>>>> 4585 if (cur_ops->set_gpwrap_lag && rcu_gpwrap_lag_init())
>>>>> --> 4586 goto unwind;
>>>>>
>>>>> set an error code?
>>
>> Here is the goto.
>>
>>>>>
>>>>> 4587
>>>>> 4588 return 0;
>>>>> 4589
>>>>> 4590 unwind:
>>>>> 4591 torture_init_end();
>>>> ^^^^^^^^^^^^^^^^^^^
>>
>> Here is the second torture_init_end().
>
> Ah! Thank you for persisting. Over to you, Joel!
Oops, thanks for finding both the issues. I will move the torture_init_end()
further down and run some tests. At first look, that seems like the right thing
to do:
diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index 92e2686a4795..bb9a8e718533 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -4582,13 +4582,17 @@ rcu_torture_init(void)
}
if (object_debug)
rcu_test_debug_objects();
- torture_init_end();
+
if (cur_ops->gp_slow_register &&
!WARN_ON_ONCE(!cur_ops->gp_slow_unregister))
cur_ops->gp_slow_register(&rcu_fwd_cb_nodelay);
- if (gpwrap_lag && cur_ops->set_gpwrap_lag && rcu_gpwrap_lag_init())
- goto unwind;
+ if (gpwrap_lag && cur_ops->set_gpwrap_lag) {
+ firsterr = rcu_gpwrap_lag_init();
+ if (torture_init_error(firsterr))
+ goto unwind;
+ }
+ torture_init_end();
return 0;
unwind:
