[bug report] rcutorture: Perform more frequent testing of ->gpwrap

[bug report] rcutorture: Perform more frequent testing of ->gpwrap

[Dan Carpenter]

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();
    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?

    4587 
    4588         return 0;
    4589 
    4590 unwind:
    4591         torture_init_end();
    4592         rcu_torture_cleanup();
    4593         if (shutdown_secs) {
    4594                 WARN_ON(!IS_MODULE(CONFIG_RCU_TORTURE_TEST));
    4595                 kernel_power_off();
    4596         }
    4597         return firsterr;
    4598 }

regards,
dan carpenter

Re: [bug report] rcutorture: Perform more frequent testing of ->gpwrap

[Dan Carpenter]

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)


>     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?
> 
>     4587 
>     4588         return 0;
>     4589 
>     4590 unwind:
>     4591         torture_init_end();
                   ^^^^^^^^^^^^^^^^^^^

regards,
dan carpenter

>     4592         rcu_torture_cleanup();
>     4593         if (shutdown_secs) {
>     4594                 WARN_ON(!IS_MODULE(CONFIG_RCU_TORTURE_TEST));
>     4595                 kernel_power_off();
>     4596         }
>     4597         return firsterr;
>     4598 }
> 
> regards,
> dan carpenter

Re: [bug report] rcutorture: Perform more frequent testing of ->gpwrap

[Paul E. McKenney]

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!  ;-)

							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?
> > 
> >     4587 
> >     4588         return 0;
> >     4589 
> >     4590 unwind:
> >     4591         torture_init_end();
>                    ^^^^^^^^^^^^^^^^^^^
> 
> regards,
> dan carpenter
> 
> >     4592         rcu_torture_cleanup();
> >     4593         if (shutdown_secs) {
> >     4594                 WARN_ON(!IS_MODULE(CONFIG_RCU_TORTURE_TEST));
> >     4595                 kernel_power_off();
> >     4596         }
> >     4597         return firsterr;
> >     4598 }
> > 
> > regards,
> > dan carpenter
> 

Re: [bug report] rcutorture: Perform more frequent testing of ->gpwrap

[Dan Carpenter]

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().

regards,
dan carpenter

Re: [bug report] rcutorture: Perform more frequent testing of ->gpwrap

[Paul E. McKenney]

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!

							Thanx, Paul

Re: [bug report] rcutorture: Perform more frequent testing of ->gpwrap

[Joel Fernandes]

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:

Re: [bug report] rcutorture: Perform more frequent testing of ->gpwrap

[Joel Fernandes]

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