Possible Oprofile crash/race when stopping

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

Hi folks !

We've hit a strange crash internally, that we -think- we have tracked
down to an oprofile bug. It's hard to hit, so I can't guarantee yet that
we have fully smashed it but I'd like to share our findings in case you
guys have a better idea.

So the initial observation is a spinlock bad magic followed by a crash
in the spinlock debug code:

[ 1541.586531] BUG: spinlock bad magic on CPU#5, events/5/136
[ 1541.597564] Unable to handle kernel paging request for data at address 0x6b6b6b6b6b6b6d03

Backtrace looks like:

	spin_bug+0x74/0xd4
	._raw_spin_lock+0x48/0x184
	._spin_lock+0x10/0x24
	.get_task_mm+0x28/0x8c
	.sync_buffer+0x1b4/0x598
	.wq_sync_buffer+0xa0/0xdc
	.worker_thread+0x1d8/0x2a8
	.kthread+0xa8/0xb4
	.kernel_thread+0x54/0x70

So we are accessing a freed task struct in the work queue when
processing the samples.

Now that shouldn't happen since we should be handed off the dying tasks
via our task handoff notifier, and only free them after all the events
have been processed.

However I also observed using xmon that at the point of the crash, the
task_free_notifier structure had nothing attached to it.

Thus my tentative explanation:

When we stop oprofile, we call sync_stop() which I copied below:

void sync_stop(void)
{
	unregister_module_notifier(&module_load_nb);
	profile_event_unregister(PROFILE_MUNMAP, &munmap_nb);
	profile_event_unregister(PROFILE_TASK_EXIT, &task_exit_nb);
	task_handoff_unregister(&task_free_nb);
	end_sync();
	free_cpumask_var(marked_cpus);
}

You see the call to task_handoff_unregister(). This is when we take
ourselves off the list. So any task freed after that point will not be
retained by oprofile.

However, we have done nothing to clear the pending sample buffers yes.
So if we have a buffer with samples for a task, and that task gets
destroyed just after we have done the handoff, and before we have
cancelled the work queue, then we can potentially access a stale task
struct.

Now, end_sync() does that cancelling, as you see, -after- we removed the
handoff:

static void end_sync(void)
{
	end_cpu_work(); <--- This is where we stop the workqueues
	/* make sure we don't leak task structs */
	process_task_mortuary();
	process_task_mortuary();
}

I think the right sequence however requires breaking up end_sync. Ie, we
need to do in that order:

  - cancel the workqueues
  - unregister the notifier
  - process the mortuary

What do you think ?

Cheers,
Ben.