On 4/24/2025 11:17 AM, Joel Fernandes wrote:
>
>
> 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;
Passes basic testing, fixed and pushed for more testing with attribution:
git://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git (next.2025.04.24b)
Thanks!
- Joel
