On Tue, Nov 07, 2023 at 06:19:37PM +0300, Dan Carpenter wrote:
> Hello Paul E. McKenney,
>
> The patch 7939f0d7b28c: "rcu: Restrict access to RCU CPU stall
> notifiers" from Nov 1, 2023 (linux-next), leads to the following
> Smatch static checker warning:
>
> kernel/rcu/rcutorture.c:2502 rcu_torture_stall()
> error: uninitialized symbol 'ret'.
>
> kernel/rcu/rcutorture.c
> 2446 static int rcu_torture_stall(void *args)
> 2447 {
> 2448 int idx;
> 2449 int ret;
> 2450 unsigned long stop_at;
> 2451
> 2452 VERBOSE_TOROUT_STRING("rcu_torture_stall task started");
> 2453 if (rcu_cpu_stall_notifiers) {
> 2454 ret = rcu_stall_chain_notifier_register(&rcu_torture_stall_block);
> 2455 if (ret)
> 2456 pr_info("%s: rcu_stall_chain_notifier_register() returned %d, %sexpected.\n",
> 2457 __func__, ret, !IS_ENABLED(CONFIG_RCU_STALL_COMMON) ? "un" : "");
>
> ret is only initialized if rcu_cpu_stall_notifiers is true.
>
> 2458 }
> 2459 if (stall_cpu_holdoff > 0) {
> 2460 VERBOSE_TOROUT_STRING("rcu_torture_stall begin holdoff");
> 2461 schedule_timeout_interruptible(stall_cpu_holdoff * HZ);
> 2462 VERBOSE_TOROUT_STRING("rcu_torture_stall end holdoff");
> 2463 }
> 2464 if (!kthread_should_stop() && stall_gp_kthread > 0) {
> 2465 VERBOSE_TOROUT_STRING("rcu_torture_stall begin GP stall");
> 2466 rcu_gp_set_torture_wait(stall_gp_kthread * HZ);
> 2467 for (idx = 0; idx < stall_gp_kthread + 2; idx++) {
> 2468 if (kthread_should_stop())
> 2469 break;
> 2470 schedule_timeout_uninterruptible(HZ);
> 2471 }
> 2472 }
> 2473 if (!kthread_should_stop() && stall_cpu > 0) {
> 2474 VERBOSE_TOROUT_STRING("rcu_torture_stall begin CPU stall");
> 2475 stop_at = ktime_get_seconds() + stall_cpu;
> 2476 /* RCU CPU stall is expected behavior in following code. */
> 2477 idx = cur_ops->readlock();
> 2478 if (stall_cpu_irqsoff)
> 2479 local_irq_disable();
> 2480 else if (!stall_cpu_block)
> 2481 preempt_disable();
> 2482 pr_alert("%s start on CPU %d.\n",
> 2483 __func__, raw_smp_processor_id());
> 2484 while (ULONG_CMP_LT((unsigned long)ktime_get_seconds(),
> 2485 stop_at))
> 2486 if (stall_cpu_block) {
> 2487 #ifdef CONFIG_PREEMPTION
> 2488 preempt_schedule();
> 2489 #else
> 2490 schedule_timeout_uninterruptible(HZ);
> 2491 #endif
> 2492 } else if (stall_no_softlockup) {
> 2493 touch_softlockup_watchdog();
> 2494 }
> 2495 if (stall_cpu_irqsoff)
> 2496 local_irq_enable();
> 2497 else if (!stall_cpu_block)
> 2498 preempt_enable();
> 2499 cur_ops->readunlock(idx);
> 2500 }
> 2501 pr_alert("%s end.\n", __func__);
> --> 2502 if (!ret) {
>
> Uninitialized here
Good catch, thank you!!!
> 2503 if (rcu_cpu_stall_notifiers) {
>
> But maybe we can just reverse the tests or delete one of the conditions?
>
> 2504 ret = rcu_stall_chain_notifier_unregister(&rcu_torture_stall_block);
> 2505 if (ret)
> 2506 pr_info("%s: rcu_stall_chain_notifier_unregister() returned %d.\n", __func__, ret);
> 2507 }
> 2508 }
> 2509 torture_shutdown_absorb("rcu_torture_stall");
> 2510 while (!kthread_should_stop())
> 2511 schedule_timeout_interruptible(10 * HZ);
> 2512 return 0;
> 2513 }
How does the following patch look, to be folded into the original with
attribution?
Thanx, Paul
------------------------------------------------------------------------
diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index 9f0e6c1cad443..50ac86a348768 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -2503,12 +2503,10 @@ static int rcu_torture_stall(void *args)
cur_ops->readunlock(idx);
}
pr_alert("%s end.\n", __func__);
- if (!ret) {
- if (rcu_cpu_stall_notifiers) {
- ret = rcu_stall_chain_notifier_unregister(&rcu_torture_stall_block);
- if (ret)
- pr_info("%s: rcu_stall_chain_notifier_unregister() returned %d.\n", __func__, ret);
- }
+ if (rcu_cpu_stall_notifiers && !ret) {
+ ret = rcu_stall_chain_notifier_unregister(&rcu_torture_stall_block);
+ if (ret)
+ pr_info("%s: rcu_stall_chain_notifier_unregister() returned %d.\n", __func__, ret);
}
torture_shutdown_absorb("rcu_torture_stall");
while (!kthread_should_stop())
