Re: [PATCH v4] x86/mce: Avoid infinite loop for copy from user recovery

[Luck, Tony]

On Wed, Jan 20, 2021 at 01:18:12PM +0100, Borislav Petkov wrote:
> On Tue, Jan 19, 2021 at 03:57:59PM -0800, Luck, Tony wrote:
> > But the real validation folks took my patch and found that it has
> > destabilized cases 1 & 2 (and case 3 also chokes if you repeat a few
> > more times). System either hangs or panics. Generally before 100
> > injection/conumption cycles.
> 
> Hmm, that mce_count increment and check stuff might turn out to be
> fragile in the face of a race condition. But I don't see it...

I can't see what the race is either.  As far as I can see the flow
looks like this (for a machine check in user mode)

	user-mode

		do_machine_check() [in atomic context on IST etc.]

			queue_task_work()
				increments current->mce_count
				first (only) call saves address etc. and calls task_work_add()
		return from machine check

Plausibly we might context switch to another process here. We could even
resume on a different CPU.

		do_task_work() [in process context]

			kill_me_maybe()
				sets mce_count back to zero, ready for another #MC
				memory_failure()
				send SIGBUS

The kernel get_user() case is similar, but may take extra machine checks before
before calling code decides get_user() really is going to keep saying -EFAULT.


But, on a whim, I changed the type of mce_count from "int" to "atomic_t" and
fixeed up the increment & clear to use atomic_inc_return() and atomic_set().
See updated patch below.

I can't see why this should have made any difference. But the "int" version
crashes in a few dozen machine checks. The "atomic_t" version has run many
thousands of machine checks (10213 in the current boot according to /proc/interrupts)
with no issues.

-Tony