* [bug report] rcutorture: Add races with task-exit processing
@ 2020-04-29 13:24 Dan Carpenter
2020-04-29 15:44 ` Paul E. McKenney
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2020-04-29 13:24 UTC (permalink / raw)
To: paulmck; +Cc: rcu
Hello Paul E. McKenney,
The patch e02882cd57e3: "rcutorture: Add races with task-exit
processing" from Apr 24, 2020, leads to the following static checker
warning:
kernel/rcu/rcutorture.c:2429 rcu_torture_read_exit()
warn: 'rep' was already freed.
kernel/rcu/rcutorture.c
2369 static int rcu_torture_read_exit(void *unused)
2370 {
2371 int count = 0;
2372 bool errexit = false;
2373 int i;
2374 struct task_struct **rep;
2375 struct torture_random_state *trsp;
2376
2377 // Allocate and initialize.
2378 set_user_nice(current, MAX_NICE);
2379 rep = kcalloc(read_exit, sizeof(*rep), GFP_KERNEL);
2380 trsp = kcalloc(read_exit, sizeof(*trsp), GFP_KERNEL);
2381 if (rep && trsp) {
2382 for (i = 0; i < read_exit; i++)
2383 torture_random_init(&trsp[i]);
2384 VERBOSE_TOROUT_STRING("rcu_torture_read_exit: Start of test");
2385 } else {
2386 kfree(rep);
^^^
2387 kfree(trsp);
^^^^
Freed.
2388 errexit = true;
2389 VERBOSE_TOROUT_ERRSTRING("out of memory");
2390 }
2391
2392 // Each pass through this loop does one read-exit episode.
2393 while (!errexit && ! READ_ONCE(read_exit_child_stop)) {
2394 if (++count > read_exit_burst) {
2395 VERBOSE_TOROUT_STRING("rcu_torture_read_exit: End of episode");
2396 schedule_timeout_uninterruptible(HZ * read_exit_delay);
2397 VERBOSE_TOROUT_STRING("rcu_torture_read_exit: Start of episode");
2398 count = 0;
2399 }
2400 // Spawn children.
2401 for (i = 0; i < read_exit && i <= num_online_cpus(); i++) {
2402 // We don't want per-child console messages.
2403 rep[i] = kthread_run(rcu_torture_read_exit_child,
2404 &trsp[i], "%s",
2405 "rcu_torture_read_exit_child");
2406 if (IS_ERR(rep[i])) {
2407 VERBOSE_TOROUT_ERRSTRING("out of memory");
2408 errexit = true;
2409 rep[i] = NULL;
2410 break;
2411 }
2412 cond_resched();
2413 }
2414 n_read_exits += i;
2415 // Reap children.
2416 for (i--; i >= 0; i--) {
2417 kthread_stop(rep[i]);
2418 rep[i] = NULL;
2419 cond_resched();
2420 }
2421 rcu_barrier(); // Wait for task_struct freeing, avoid OOM.
2422 stutter_wait("rcu_torture_read_exit");
2423 }
2424
2425 // Clean up and exit.
2426 smp_store_release(&read_exit_child_stopped, true); // After reaping.
2427 smp_mb(); // Store before wakeup.
2428 wake_up(&read_exit_wq);
2429 kfree(rep);
^^^
2430 kfree(trsp);
^^^^
Double freed.
2431 while (!torture_must_stop())
2432 schedule_timeout_uninterruptible(1);
2433 torture_kthread_stopping("rcu_torture_read_exit");
2434 return 0;
2435 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] rcutorture: Add races with task-exit processing
2020-04-29 13:24 [bug report] rcutorture: Add races with task-exit processing Dan Carpenter
@ 2020-04-29 15:44 ` Paul E. McKenney
2020-04-29 17:43 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Paul E. McKenney @ 2020-04-29 15:44 UTC (permalink / raw)
To: Dan Carpenter; +Cc: rcu
On Wed, Apr 29, 2020 at 04:24:02PM +0300, Dan Carpenter wrote:
> Hello Paul E. McKenney,
>
> The patch e02882cd57e3: "rcutorture: Add races with task-exit
> processing" from Apr 24, 2020, leads to the following static checker
> warning:
>
> kernel/rcu/rcutorture.c:2429 rcu_torture_read_exit()
> warn: 'rep' was already freed.
Ah, good catch, will fix!
> kernel/rcu/rcutorture.c
> 2369 static int rcu_torture_read_exit(void *unused)
> 2370 {
> 2371 int count = 0;
> 2372 bool errexit = false;
> 2373 int i;
> 2374 struct task_struct **rep;
> 2375 struct torture_random_state *trsp;
> 2376
> 2377 // Allocate and initialize.
> 2378 set_user_nice(current, MAX_NICE);
> 2379 rep = kcalloc(read_exit, sizeof(*rep), GFP_KERNEL);
> 2380 trsp = kcalloc(read_exit, sizeof(*trsp), GFP_KERNEL);
> 2381 if (rep && trsp) {
> 2382 for (i = 0; i < read_exit; i++)
> 2383 torture_random_init(&trsp[i]);
> 2384 VERBOSE_TOROUT_STRING("rcu_torture_read_exit: Start of test");
> 2385 } else {
> 2386 kfree(rep);
> ^^^
> 2387 kfree(trsp);
> ^^^^
> Freed.
And setting both rep and trsp to NULL here takes care of this, correct?
As in kfree(NULL) is a no-op. Or is your tool more strict?
Thanx, Paul
> 2388 errexit = true;
> 2389 VERBOSE_TOROUT_ERRSTRING("out of memory");
> 2390 }
> 2391
> 2392 // Each pass through this loop does one read-exit episode.
> 2393 while (!errexit && ! READ_ONCE(read_exit_child_stop)) {
> 2394 if (++count > read_exit_burst) {
> 2395 VERBOSE_TOROUT_STRING("rcu_torture_read_exit: End of episode");
> 2396 schedule_timeout_uninterruptible(HZ * read_exit_delay);
> 2397 VERBOSE_TOROUT_STRING("rcu_torture_read_exit: Start of episode");
> 2398 count = 0;
> 2399 }
> 2400 // Spawn children.
> 2401 for (i = 0; i < read_exit && i <= num_online_cpus(); i++) {
> 2402 // We don't want per-child console messages.
> 2403 rep[i] = kthread_run(rcu_torture_read_exit_child,
> 2404 &trsp[i], "%s",
> 2405 "rcu_torture_read_exit_child");
> 2406 if (IS_ERR(rep[i])) {
> 2407 VERBOSE_TOROUT_ERRSTRING("out of memory");
> 2408 errexit = true;
> 2409 rep[i] = NULL;
> 2410 break;
> 2411 }
> 2412 cond_resched();
> 2413 }
> 2414 n_read_exits += i;
> 2415 // Reap children.
> 2416 for (i--; i >= 0; i--) {
> 2417 kthread_stop(rep[i]);
> 2418 rep[i] = NULL;
> 2419 cond_resched();
> 2420 }
> 2421 rcu_barrier(); // Wait for task_struct freeing, avoid OOM.
> 2422 stutter_wait("rcu_torture_read_exit");
> 2423 }
> 2424
> 2425 // Clean up and exit.
> 2426 smp_store_release(&read_exit_child_stopped, true); // After reaping.
> 2427 smp_mb(); // Store before wakeup.
> 2428 wake_up(&read_exit_wq);
> 2429 kfree(rep);
> ^^^
> 2430 kfree(trsp);
> ^^^^
> Double freed.
>
> 2431 while (!torture_must_stop())
> 2432 schedule_timeout_uninterruptible(1);
> 2433 torture_kthread_stopping("rcu_torture_read_exit");
> 2434 return 0;
> 2435 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] rcutorture: Add races with task-exit processing
2020-04-29 15:44 ` Paul E. McKenney
@ 2020-04-29 17:43 ` Dan Carpenter
2020-04-29 17:47 ` Paul E. McKenney
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2020-04-29 17:43 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: rcu
On Wed, Apr 29, 2020 at 08:44:21AM -0700, Paul E. McKenney wrote:
> On Wed, Apr 29, 2020 at 04:24:02PM +0300, Dan Carpenter wrote:
> > Hello Paul E. McKenney,
> >
> > The patch e02882cd57e3: "rcutorture: Add races with task-exit
> > processing" from Apr 24, 2020, leads to the following static checker
> > warning:
> >
> > kernel/rcu/rcutorture.c:2429 rcu_torture_read_exit()
> > warn: 'rep' was already freed.
>
> Ah, good catch, will fix!
>
> > kernel/rcu/rcutorture.c
> > 2369 static int rcu_torture_read_exit(void *unused)
> > 2370 {
> > 2371 int count = 0;
> > 2372 bool errexit = false;
> > 2373 int i;
> > 2374 struct task_struct **rep;
> > 2375 struct torture_random_state *trsp;
> > 2376
> > 2377 // Allocate and initialize.
> > 2378 set_user_nice(current, MAX_NICE);
> > 2379 rep = kcalloc(read_exit, sizeof(*rep), GFP_KERNEL);
> > 2380 trsp = kcalloc(read_exit, sizeof(*trsp), GFP_KERNEL);
> > 2381 if (rep && trsp) {
> > 2382 for (i = 0; i < read_exit; i++)
> > 2383 torture_random_init(&trsp[i]);
> > 2384 VERBOSE_TOROUT_STRING("rcu_torture_read_exit: Start of test");
> > 2385 } else {
> > 2386 kfree(rep);
> > ^^^
> > 2387 kfree(trsp);
> > ^^^^
> > Freed.
>
> And setting both rep and trsp to NULL here takes care of this, correct?
> As in kfree(NULL) is a no-op. Or is your tool more strict?
Yes, yes. Setting it to NULL works. I thought about that, or returning
and I wasn't sure which was prefered.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] rcutorture: Add races with task-exit processing
2020-04-29 17:43 ` Dan Carpenter
@ 2020-04-29 17:47 ` Paul E. McKenney
0 siblings, 0 replies; 4+ messages in thread
From: Paul E. McKenney @ 2020-04-29 17:47 UTC (permalink / raw)
To: Dan Carpenter; +Cc: rcu
On Wed, Apr 29, 2020 at 08:43:07PM +0300, Dan Carpenter wrote:
> On Wed, Apr 29, 2020 at 08:44:21AM -0700, Paul E. McKenney wrote:
> > On Wed, Apr 29, 2020 at 04:24:02PM +0300, Dan Carpenter wrote:
> > > Hello Paul E. McKenney,
> > >
> > > The patch e02882cd57e3: "rcutorture: Add races with task-exit
> > > processing" from Apr 24, 2020, leads to the following static checker
> > > warning:
> > >
> > > kernel/rcu/rcutorture.c:2429 rcu_torture_read_exit()
> > > warn: 'rep' was already freed.
> >
> > Ah, good catch, will fix!
> >
> > > kernel/rcu/rcutorture.c
> > > 2369 static int rcu_torture_read_exit(void *unused)
> > > 2370 {
> > > 2371 int count = 0;
> > > 2372 bool errexit = false;
> > > 2373 int i;
> > > 2374 struct task_struct **rep;
> > > 2375 struct torture_random_state *trsp;
> > > 2376
> > > 2377 // Allocate and initialize.
> > > 2378 set_user_nice(current, MAX_NICE);
> > > 2379 rep = kcalloc(read_exit, sizeof(*rep), GFP_KERNEL);
> > > 2380 trsp = kcalloc(read_exit, sizeof(*trsp), GFP_KERNEL);
> > > 2381 if (rep && trsp) {
> > > 2382 for (i = 0; i < read_exit; i++)
> > > 2383 torture_random_init(&trsp[i]);
> > > 2384 VERBOSE_TOROUT_STRING("rcu_torture_read_exit: Start of test");
> > > 2385 } else {
> > > 2386 kfree(rep);
> > > ^^^
> > > 2387 kfree(trsp);
> > > ^^^^
> > > Freed.
> >
> > And setting both rep and trsp to NULL here takes care of this, correct?
> > As in kfree(NULL) is a no-op. Or is your tool more strict?
>
> Yes, yes. Setting it to NULL works. I thought about that, or returning
> and I wasn't sure which was prefered.
Ironically, what is likely to happen is restructuring the code to
eliminate the need for memory allocation. The idea being to minimize
the time between RCU reader and kthread exit.
Thanx, Paul
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-29 17:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-29 13:24 [bug report] rcutorture: Add races with task-exit processing Dan Carpenter
2020-04-29 15:44 ` Paul E. McKenney
2020-04-29 17:43 ` Dan Carpenter
2020-04-29 17:47 ` Paul E. McKenney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).