[PATCH v2 0/3] More machine check recovery fixes

[PATCH v2 0/3] More machine check recovery fixes

[Tony Luck]

Fix a couple of issues in machine check handling

1) A repeated machine check inside the kernel without calling the task
   work function between machine checks it will go into an infinite
   loop
2) Machine checks in kernel functions copying data from user addresses
   send SIGBUS to the user as if the application had consumed the
   poison. But this is wrong. The user should see either an -EFAULT
   error return or a reduced byte count (in the case of write(2)).

My latest tests have been on v4.14-rc6 with this patch (that's already
in -mm) applied:
https://lore.kernel.org/r/20210817053703.2267588-1-naoya.horiguchi@linux.dev

Changes since v1:
1) Fix bug in kill_me_never() that forgot to clear p->mce_count so
   repeated recovery in the same task would trigger the panic for
	"Machine checks to different user pages"
   [Note to Jue Wang ... this *might* be why your test that injects
    two errors into the same buffer passed to a write(2) syscall
    failed with this message]
2) Re-order patches so that "Avoid infinite loop" can be backported
   to stable.

Note that the other two parts of this series depend upon Al Viro's
extensive re-work to lib/iov_iter.c ... so don't try to backport those
without also picking up Al's work.

Tony Luck (3):
  x86/mce: Avoid infinite loop for copy from user recovery
  x86/mce: Change to not send SIGBUS error during copy from user
  x86/mce: Drop copyin special case for #MC

 arch/x86/kernel/cpu/mce/core.c | 62 ++++++++++++++++++++++++----------
 arch/x86/lib/copy_user_64.S    | 13 -------
 include/linux/sched.h          |  1 +
 3 files changed, 45 insertions(+), 31 deletions(-)


base-commit: 7c60610d476766e128cc4284bb6349732cbd6606
-- 
2.29.2

[PATCH v2 1/3] x86/mce: Avoid infinite loop for copy from user recovery

[Tony Luck]

Recovery action when get_user() triggers a machine check uses the fixup
path to make get_user() return -EFAULT.  Also queue_task_work() sets up
so that kill_me_maybe() will be called on return to user mode to send
a SIGBUS to the current process.

But there are places in the kernel where the code assumes that this
EFAULT return was simply because of a page fault. The code takes some
action to fix that, and then retries the access. This results in a second
machine check.

While processing this second machine check queue_task_work() is called
again. But since this uses the same callback_head structure that was used
in the first call, the net result is an entry on the current->task_works
list that points to itself. When task_work_run() is called it loops
forever in this code:

        do {
                next = work->next;
                work->func(work);
                work = next;
                cond_resched();
        } while (work);

Add a counter (current->mce_count) to keep track of repeated machine
checks before task_work() is called. First machine check saves the address
information and calls task_work_add(). Subsequent machine checks before
that task_work call back is executed check that the address is in the
same page as the first machine check (since the callback will offline
exactly one page).

Expected worst case is two machine checks before moving on (e.g. one user
access with page faults disabled, then a repeat to the same addrsss with
page faults enabled). Just in case there is some code that loops forever
enforce a limit of 10.

Cc: <stable@vger.kernel.org>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/mce/core.c | 43 +++++++++++++++++++++++++---------
 include/linux/sched.h          |  1 +
 2 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 22791aadc085..94830ee9581c 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1250,6 +1250,9 @@ static void __mc_scan_banks(struct mce *m, struct pt_regs *regs, struct mce *fin
 
 static void kill_me_now(struct callback_head *ch)
 {
+	struct task_struct *p = container_of(ch, struct task_struct, mce_kill_me);
+
+	p->mce_count = 0;
 	force_sig(SIGBUS);
 }
 
@@ -1259,6 +1262,7 @@ static void kill_me_maybe(struct callback_head *cb)
 	int flags = MF_ACTION_REQUIRED;
 	int ret;
 
+	p->mce_count = 0;
 	pr_err("Uncorrected hardware memory error in user-access at %llx", p->mce_addr);
 
 	if (!p->mce_ripv)
@@ -1287,17 +1291,34 @@ static void kill_me_maybe(struct callback_head *cb)
 	}
 }
 
-static void queue_task_work(struct mce *m, int kill_current_task)
+static void queue_task_work(struct mce *m, char *msg, int kill_current_task)
 {
-	current->mce_addr = m->addr;
-	current->mce_kflags = m->kflags;
-	current->mce_ripv = !!(m->mcgstatus & MCG_STATUS_RIPV);
-	current->mce_whole_page = whole_page(m);
+	int count = ++current->mce_count;
 
-	if (kill_current_task)
-		current->mce_kill_me.func = kill_me_now;
-	else
-		current->mce_kill_me.func = kill_me_maybe;
+	/* First call, save all the details */
+	if (count == 1) {
+		current->mce_addr = m->addr;
+		current->mce_kflags = m->kflags;
+		current->mce_ripv = !!(m->mcgstatus & MCG_STATUS_RIPV);
+		current->mce_whole_page = whole_page(m);
+
+		if (kill_current_task)
+			current->mce_kill_me.func = kill_me_now;
+		else
+			current->mce_kill_me.func = kill_me_maybe;
+	}
+
+	/* Ten is likley overkill. Don't expect more than two faults before task_work() */
+	if (count > 10)
+		mce_panic("Too many machine checks while accessing user data", m, msg);
+
+	/* Second or later call, make sure page address matches the one from first call */
+	if (count > 1 && (current->mce_addr >> PAGE_SHIFT) != (m->addr >> PAGE_SHIFT))
+		mce_panic("Machine checks to different user pages", m, msg);
+
+	/* Do not call task_work_add() more than once */
+	if (count > 1)
+		return;
 
 	task_work_add(current, &current->mce_kill_me, TWA_RESUME);
 }
@@ -1435,7 +1456,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
 		/* If this triggers there is no way to recover. Die hard. */
 		BUG_ON(!on_thread_stack() || !user_mode(regs));
 
-		queue_task_work(&m, kill_current_task);
+		queue_task_work(&m, msg, kill_current_task);
 
 	} else {
 		/*
@@ -1453,7 +1474,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
 		}
 
 		if (m.kflags & MCE_IN_KERNEL_COPYIN)
-			queue_task_work(&m, kill_current_task);
+			queue_task_work(&m, msg, kill_current_task);
 	}
 out:
 	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index ec8d07d88641..f6935787e7e8 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1394,6 +1394,7 @@ struct task_struct {
 					mce_whole_page : 1,
 					__mce_reserved : 62;
 	struct callback_head		mce_kill_me;
+	int				mce_count;
 #endif
 
 #ifdef CONFIG_KRETPROBES
-- 
2.29.2

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

[Borislav Petkov]

On Tue, Aug 17, 2021 at 05:29:40PM -0700, Tony Luck wrote:
> @@ -1287,17 +1291,34 @@ static void kill_me_maybe(struct callback_head *cb)
>  	}
>  }
>  
> -static void queue_task_work(struct mce *m, int kill_current_task)
> +static void queue_task_work(struct mce *m, char *msg, int kill_current_task)
>  {
> -	current->mce_addr = m->addr;
> -	current->mce_kflags = m->kflags;
> -	current->mce_ripv = !!(m->mcgstatus & MCG_STATUS_RIPV);
> -	current->mce_whole_page = whole_page(m);
> +	int count = ++current->mce_count;
>  
> -	if (kill_current_task)
> -		current->mce_kill_me.func = kill_me_now;
> -	else
> -		current->mce_kill_me.func = kill_me_maybe;
> +	/* First call, save all the details */
> +	if (count == 1) {
> +		current->mce_addr = m->addr;
> +		current->mce_kflags = m->kflags;
> +		current->mce_ripv = !!(m->mcgstatus & MCG_STATUS_RIPV);
> +		current->mce_whole_page = whole_page(m);
> +
> +		if (kill_current_task)
> +			current->mce_kill_me.func = kill_me_now;
> +		else
> +			current->mce_kill_me.func = kill_me_maybe;
> +	}
> +
> +	/* Ten is likley overkill. Don't expect more than two faults before task_work() */

"likely"

> +	if (count > 10)
> +		mce_panic("Too many machine checks while accessing user data", m, msg);

Ok, aren't we too nasty here? Why should we panic the whole box even
with 10 MCEs? It is still user memory...

IOW, why not:

	if (count > 10)
		current->mce_kill_me.func = kill_me_now;

and when we return, that user process dies immediately.

> +	/* Second or later call, make sure page address matches the one from first call */
> +	if (count > 1 && (current->mce_addr >> PAGE_SHIFT) != (m->addr >> PAGE_SHIFT))
> +		mce_panic("Machine checks to different user pages", m, msg);

Same question here.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Luck, Tony]

On Fri, Aug 20, 2021 at 07:31:43PM +0200, Borislav Petkov wrote:
> On Tue, Aug 17, 2021 at 05:29:40PM -0700, Tony Luck wrote:
> > +	/* Ten is likley overkill. Don't expect more than two faults before task_work() */
> 
> "likely"

Oops.

> 
> > +	if (count > 10)
> > +		mce_panic("Too many machine checks while accessing user data", m, msg);
> 
> Ok, aren't we too nasty here? Why should we panic the whole box even
> with 10 MCEs? It is still user memory...
> 
> IOW, why not:
> 
> 	if (count > 10)
> 		current->mce_kill_me.func = kill_me_now;
> 
> and when we return, that user process dies immediately.

It's the "when we return" part that is the problem here. Logical
trace looks like:

user-syscall:

	kernel does get_user() or copyin(), hits user poison address

		machine check
		sees that this was kernel get_user()/copyin() and
		uses extable to "return" to exception path

	still in kernel, see that get_user() or copyin() failed

	Kernel does another get_user() or copyin() (maybe the first
	was inside a pagefault_disable() region, and kernel is trying
	again to see if the error was a fixable page fault. But that
	wasn't the problem so ...

		machine check
		sees that this was kernel get_user()/copyin() and
		uses extable to "return" to exception path

	still in kernel ... but persistently thinks that just trying again
	might fix it.

		machine check
		sees that this was kernel get_user()/copyin() and
		uses extable to "return" to exception path

	still in kernel ... this time for sure! get_user()

		machine check
		sees that this was kernel get_user()/copyin() and
		uses extable to "return" to exception path

	still in kernel ... but you may see the pattern get_user()

		machine check
		sees that this was kernel get_user()/copyin() and
		uses extable to "return" to exception path

	I'm bored typing this, but the kernel may not ever give up

		machine check
		sees that this was kernel get_user()/copyin() and
		uses extable to "return" to exception path

I.e. the kernel doesn't ever get to call current->mce_kill_me.func()

I do have tests that show as many as 4 consecutive machine checks
before the kernel gives up trying and returns to the user to complete
recovery.

Maybe the message could be clearer?

	mce_panic("Too many consecutive machine checks in kernel while accessing user data", m, msg);

> 
> > +	/* Second or later call, make sure page address matches the one from first call */
> > +	if (count > 1 && (current->mce_addr >> PAGE_SHIFT) != (m->addr >> PAGE_SHIFT))
> > +		mce_panic("Machine checks to different user pages", m, msg);
> 
> Same question here.

Not quite the same answer ... but similar.  We could in theory handle
multiple different machine check addresses by turning the "mce_addr"
field in the task structure into an array and saving each address so
that when the kernel eventually gives up poking at poison and tries
to return to user kill_me_maybe() could loop through them and deal
with each poison page.

I don't think this can happen. Jue Wang suggested that multiple poisoned
pages passed to a single write(2) syscall might trigger this panic (and
because of a bug in my earlier version, he managed to trigger this
"different user pages" panic). But this fixed up version survives the
"Jue test".

-Tony

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

[Borislav Petkov]

On Fri, Aug 20, 2021 at 11:59:45AM -0700, Luck, Tony wrote:
> It's the "when we return" part that is the problem here. Logical
> trace looks like:
> 
> user-syscall:
> 
> 	kernel does get_user() or copyin(), hits user poison address
> 
> 		machine check
> 		sees that this was kernel get_user()/copyin() and
> 		uses extable to "return" to exception path
> 
> 	still in kernel, see that get_user() or copyin() failed
> 
> 	Kernel does another get_user() or copyin() (maybe the first

I forgot all the details we were talking at the time but there's no way
to tell the kernel to back off here, is it?

As in: there was an MCE while trying to access this user memory, you
should not do get_user anymore. You did add that

         * Return zero to pretend that this copy succeeded. This
         * is counter-intuitive, but needed to prevent the code
         * in lib/iov_iter.c from retrying and running back into

which you're removing with the last patch so I'm confused.

IOW, the problem is that with repeated MCEs while the kernel is
accessing that memory, it should be the kernel which should back off.
And then we should kill that process too but apparently we don't even
come to that.

> Maybe the message could be clearer?
> 
> 	mce_panic("Too many consecutive machine checks in kernel while accessing user data", m, msg);

That's not my point - it is rather: this is a recoverable error because
it is in user memory even if it is the kernel which tries to access it.
And maybe we should not panic the whole box but try to cordon off the
faulty memory only and poison it after having killed the process using
it...

> Not quite the same answer ... but similar.  We could in theory handle
> multiple different machine check addresses by turning the "mce_addr"
> field in the task structure into an array and saving each address so
> that when the kernel eventually gives up poking at poison and tries
> to return to user kill_me_maybe() could loop through them and deal
> with each poison page.

Yes, I like the aspect of making the kernel give up poking at poison and
when we return we should kill the process and poison all pages collected
so that the error source is hopefully contained.

But again, I think the important thing is how to make the kernel to back
off quicker so that we can poison the pages at all...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Luck, Tony]

On Fri, Aug 20, 2021 at 09:27:44PM +0200, Borislav Petkov wrote:
> On Fri, Aug 20, 2021 at 11:59:45AM -0700, Luck, Tony wrote:
> > It's the "when we return" part that is the problem here. Logical
> > trace looks like:
> > 
> > user-syscall:
> > 
> > 	kernel does get_user() or copyin(), hits user poison address
> > 
> > 		machine check
> > 		sees that this was kernel get_user()/copyin() and
> > 		uses extable to "return" to exception path
> > 
> > 	still in kernel, see that get_user() or copyin() failed
> > 
> > 	Kernel does another get_user() or copyin() (maybe the first
> 
> I forgot all the details we were talking at the time but there's no way
> to tell the kernel to back off here, is it?
> 
> As in: there was an MCE while trying to access this user memory, you
> should not do get_user anymore. You did add that
> 
>          * Return zero to pretend that this copy succeeded. This
>          * is counter-intuitive, but needed to prevent the code
>          * in lib/iov_iter.c from retrying and running back into
> 
> which you're removing with the last patch so I'm confused.
> 
> IOW, the problem is that with repeated MCEs while the kernel is
> accessing that memory, it should be the kernel which should back off.
> And then we should kill that process too but apparently we don't even
> come to that.

My first foray into this just tried to fix the futex() case ... which is one
of the first copy is with pagefault_disable() set, then try again with it
clear.  My attempt there was to make get_user() return -EHWPOISON, and
change the futex code to give up immediatley when it saw that code.

AndyL (and maybe others) barfed all over that (and rightly so ... there
are thousands of get_user() and copyin() calls ... making sure all of them
did the right thing with a new error code would be a huge effort. Very
error prone (because testing all these paths is hard). New direction was
just deal with the fact that we might take more than one machine check
before the kernel is finished poking at the poison.

> 
> > Maybe the message could be clearer?
> > 
> > 	mce_panic("Too many consecutive machine checks in kernel while accessing user data", m, msg);
> 
> That's not my point - it is rather: this is a recoverable error because
> it is in user memory even if it is the kernel which tries to access it.
> And maybe we should not panic the whole box but try to cordon off the
> faulty memory only and poison it after having killed the process using
> it...

To recover we need to have some other place to jump to (besides the
normal extable error return ... which isn't working if we find ourselves
in this situation) when we hit a fault covered by an extable entry. And
also know how many machine checks is "normal" before taking the other path.

For futex(2) things resolve in two machine checks (one with
pagefault_disable() and then one without).  For write(2) I see up to
four machine cehcks (I didn't do a detailed track ... but I think it is
because copyin() does a retry to see if a failure in a many-bytes-at-atime
copy might be able to get a few more bytes by doing byte-at-a-time).

The logical spot for the alternate return would be to unwind the stack
back to the syscall entry point, and then force an error return from
there.  But that seems a perilous path ... what if some code between
syscall entry and the copyin() grabbed a mutex? We would also need to
know about that and release it as part of the recovery.

Another failed approach was to mark the page not present in the page
tables of the process accessing the poison. That would get us out of the
machine check loop.  But walking page tables and changing them while still
in machine check context can't be done in a safe way (the process may be
multi-threaded and other threads could still be running on other cores).

Bottom line is that I don't think this panic can actually happen unless
there is some buggy kernel code that retries get_user() or copyin()
indefinitely.

Probably the same for the two different addresses case ... though I'm
not 100% confident about that. There could be some ioctl() that peeks
at two parts of a passed in structure, and the user might pass in a
structure that spans across a page boundary with both pages poisoned.
But that would only hit if the driver code ignored the failure of the
first get_user() and blindly tried the second. So I'd count that as a
critically bad driver bug.

-Tony

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

[Tony Luck]

On Fri, Aug 20, 2021 at 1:25 PM Luck, Tony <tony.luck@intel.com> wrote:
> Probably the same for the two different addresses case ... though I'm
> not 100% confident about that. There could be some ioctl() that peeks
> at two parts of a passed in structure, and the user might pass in a
> structure that spans across a page boundary with both pages poisoned.
> But that would only hit if the driver code ignored the failure of the
> first get_user() and blindly tried the second. So I'd count that as a
> critically bad driver bug.

Or maybe driver writers are just evil :-(

for (i = 0; i < len; i++) {
       tx_wait(10);
       get_user(dsp56k_host_interface.data.b[1], bin++);
       get_user(dsp56k_host_interface.data.b[2], bin++);
       get_user(dsp56k_host_interface.data.b[3], bin++);
}

-Tony

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

[Al Viro]

On Fri, Aug 20, 2021 at 09:51:41PM -0700, Tony Luck wrote:
> On Fri, Aug 20, 2021 at 1:25 PM Luck, Tony <tony.luck@intel.com> wrote:
> > Probably the same for the two different addresses case ... though I'm
> > not 100% confident about that. There could be some ioctl() that peeks
> > at two parts of a passed in structure, and the user might pass in a
> > structure that spans across a page boundary with both pages poisoned.
> > But that would only hit if the driver code ignored the failure of the
> > first get_user() and blindly tried the second. So I'd count that as a
> > critically bad driver bug.
> 
> Or maybe driver writers are just evil :-(
> 
> for (i = 0; i < len; i++) {
>        tx_wait(10);
>        get_user(dsp56k_host_interface.data.b[1], bin++);
>        get_user(dsp56k_host_interface.data.b[2], bin++);
>        get_user(dsp56k_host_interface.data.b[3], bin++);
> }

Almost any unchecked get_user()/put_user() is a bug.  Fortunately, there's
not a lot of them
<greps>
93 for put_user() and 73 for get_user().  _Some_ of the former variety might
be legitimate, but most should be taken out and shot.

And dsp56k should be taken out and shot, period ;-/  This is far from the
worst in there...

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

[Borislav Petkov]

On Fri, Aug 20, 2021 at 01:23:46PM -0700, Luck, Tony wrote:
> To recover we need to have some other place to jump to (besides the
> normal extable error return ... which isn't working if we find ourselves
> in this situation) when we hit a fault covered by an extable entry. And
> also know how many machine checks is "normal" before taking the other path.

Hohumm, we're on the same page here.

...

> Bottom line is that I don't think this panic can actually happen unless
> there is some buggy kernel code that retries get_user() or copyin()
> indefinitely.

You know how such statements of "well, this should not really happen in
practice" get disproved by, well, practice. :-)

I guess we'll see anyway what actually happens in practice.

> Probably the same for the two different addresses case ... though I'm
> not 100% confident about that. There could be some ioctl() that peeks
> at two parts of a passed in structure, and the user might pass in a
> structure that spans across a page boundary with both pages poisoned.
> But that would only hit if the driver code ignored the failure of the
> first get_user() and blindly tried the second. So I'd count that as a
> critically bad driver bug.

Right.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Luck, Tony]

On Fri, Aug 20, 2021 at 09:27:44PM +0200, Borislav Petkov wrote:
> On Fri, Aug 20, 2021 at 11:59:45AM -0700, Luck, Tony wrote:
> As in: there was an MCE while trying to access this user memory, you
> should not do get_user anymore. You did add that
> 
>          * Return zero to pretend that this copy succeeded. This
>          * is counter-intuitive, but needed to prevent the code
>          * in lib/iov_iter.c from retrying and running back into
> 
> which you're removing with the last patch so I'm confused.

Forget to address this part in the earlier reply.

My original code that forced a zero return has a hack. It
allowed recovery to complete, but only because there was
going to be a SIGBUS.  There were some unplesant side effects.
E.g. on a write syscall the file size was updated as if the
write had succeeded. That would be very confusing for anyone
trying to clean up afterwards as the file would have good
data that was copied from the user up to the point where
the machine check interrupted things. Then NUL bytes after
(because the kernel clears pages that are allocated into
the page cache).

The new version (thanks to All fixing iov_iter.c) now does
exactly what POSIX says should happen.  If I have a buffer
with poison at offset 213, and I do this:

	ret = write(fd, buf, 512);

Then the return from write is 213, and the first 213 bytes
from the buffer appear in the file, and the file size is
incremented by 213 (assuming the write started with the lseek
offset at the original size of the file).

-Tony

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

[Borislav Petkov]

On Fri, Aug 20, 2021 at 01:33:56PM -0700, Luck, Tony wrote:
> The new version (thanks to All fixing iov_iter.c) now does
> exactly what POSIX says should happen.  If I have a buffer
> with poison at offset 213, and I do this:
> 
> 	ret = write(fd, buf, 512);
> 
> Then the return from write is 213, and the first 213 bytes
> from the buffer appear in the file, and the file size is
> incremented by 213 (assuming the write started with the lseek
> offset at the original size of the file).

... and the user still gets a SIGBUS so that it gets a chance to handle
the encountered poison? I.e., not retry the write for the remaining 512
- 213 bytes?

If so, do we document that somewhere so that application writers can
know what they should do in such cases?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Luck, Tony]

On Sun, Aug 22, 2021 at 04:46:14PM +0200, Borislav Petkov wrote:
> On Fri, Aug 20, 2021 at 01:33:56PM -0700, Luck, Tony wrote:
> > The new version (thanks to All fixing iov_iter.c) now does
> > exactly what POSIX says should happen.  If I have a buffer
> > with poison at offset 213, and I do this:
> > 
> > 	ret = write(fd, buf, 512);
> > 
> > Then the return from write is 213, and the first 213 bytes
> > from the buffer appear in the file, and the file size is
> > incremented by 213 (assuming the write started with the lseek
> > offset at the original size of the file).
> 
> ... and the user still gets a SIGBUS so that it gets a chance to handle
> the encountered poison? I.e., not retry the write for the remaining 512
> - 213 bytes?

Whether the user gets a SIGBUS depends on what they do next.  In a typical
user loop trying to do a write:

	while (nbytes) {
		ret = write(fd, buf, nbytes);
		if (ret == -1)
			return ret;
		buf += ret;
		nbytes -= ret;
	}

The next iteration after the short write caused by the machine check
will return ret == -1, errno = EFAULT.

Andy Lutomirski convinced me that the kernel should not send a SIGBUS
to an application when the kernel accesses the poison in user memory.

If the user tries to access the page with the poison directly they'll
get a SIGBUS (page was unmapped so user gets a #PF, but the x86 fault
handler sees that the page was unmapped because of poison, so sends a
SIGBUS).

> If so, do we document that somewhere so that application writers can
> know what they should do in such cases?

Applications see a failed write ... they should do whatever they would
normally do for a failed write.

-Tony

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

[Borislav Petkov]

On Tue, Aug 17, 2021 at 05:29:40PM -0700, Tony Luck wrote:
> Recovery action when get_user() triggers a machine check uses the fixup
> path to make get_user() return -EFAULT.  Also queue_task_work() sets up
> so that kill_me_maybe() will be called on return to user mode to send
> a SIGBUS to the current process.
> 
> But there are places in the kernel where the code assumes that this
> EFAULT return was simply because of a page fault. The code takes some
> action to fix that, and then retries the access. This results in a second
> machine check.
> 
> While processing this second machine check queue_task_work() is called
> again. But since this uses the same callback_head structure that was used
> in the first call, the net result is an entry on the current->task_works
> list that points to itself. When task_work_run() is called it loops
> forever in this code:
> 
>         do {
>                 next = work->next;
>                 work->func(work);
>                 work = next;
>                 cond_resched();
>         } while (work);
> 
> Add a counter (current->mce_count) to keep track of repeated machine
> checks before task_work() is called. First machine check saves the address
> information and calls task_work_add(). Subsequent machine checks before
> that task_work call back is executed check that the address is in the
> same page as the first machine check (since the callback will offline
> exactly one page).
> 
> Expected worst case is two machine checks before moving on (e.g. one user
> access with page faults disabled, then a repeat to the same addrsss with
> page faults enabled). Just in case there is some code that loops forever
> enforce a limit of 10.
> 
> Cc: <stable@vger.kernel.org>

What about a Fixes: tag?

I guess backporting this to the respective kernels is predicated upon
the existence of those other "places" in the kernel where code assumes
the EFAULT was because of a #PF.

Hmmm?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Luck, Tony]

There are two cases for machine check recovery:
1) The machine check was triggered by ring3 (application) code.
   This is the simpler case. The machine check handler simply queues
   work to be executed on return to user. That code unmaps the page
   from all users and arranges to send a SIGBUS to the task that
   triggered the poison.
2) The machine check was triggered in kernel code that is covered by
   an extable entry.
   In this case the machine check handler still queues a work entry to
   unmap the page, etc. but this will not be called right away because
   the #MC handler returns to the fix up code address in the extable
   entry.

Problems occur if the kernel triggers another machine check before the
return to user processes the first queued work item.

Specifically the work is queued using the "mce_kill_me" callback
structure in the task struct for the current thread. Attempting to queue
a second work item using this same callback results in a loop in the
linked list of work functions to call. So when the kernel does return to
user it enters an infinite loop processing the same entry for ever.

There are some legitimate scenarios where the kernel may take a second
machine check before returning to the user.

1) Some code (e.g. futex) first tries a get_user() with page faults
   disabled. If this fails, the code retries with page faults enabled
   expecting that this will resolve the page fault.
2) Copy from user code retries a copy in byte-at-time mode to check
   whether any additional bytes can be copied.

On the other side of the fence are some bad drivers that do not check
the return value from individual get_user() calls and may access
multiple user addresses without noticing that some/all calls have
failed.

Fix by adding a counter (current->mce_count) to keep track of repeated
machine checks before task_work() is called. First machine check saves
the address information and calls task_work_add(). Subsequent machine
checks before that task_work call back is executed check that the address
is in the same page as the first machine check (since the callback will
offline exactly one page).

Expected worst case is four machine checks before moving on (e.g. one
user access with page faults disabled, then a repeat to the same address
with page faults enabled ... repeat in copy tail bytes). Just in case
there is some code that loops forever enforce a limit of 10.

Also mark queue_task_work() as "noinstr" (as reported kernel test robot
<lkp@intel.com>)

Cc: <stable@vger.kernel.org>
Fixes: 5567d11c21a1 ("x86/mce: Send #MC singal from task work")
Signed-off-by: Tony Luck <tony.luck@intel.com>
---

> What about a Fixes: tag?

Added a Fixes tag.

Also added "noinstr" to queue_task_work() per a kernel robot report.

Also re-wrote the commit comment (based on questions raised against v2)

> I guess backporting this to the respective kernels is predicated upon
> the existence of those other "places" in the kernel where code assumes
> the EFAULT was because of a #PF.

Not really. I don't expect to change any kernel code that just bounces
off the same machine check a few times. This patch does work best in
conjunction with patches 2 & 3 (unchanged, not reposted here). But it
will fix some old issues even without those two.

 arch/x86/kernel/cpu/mce/core.c | 43 +++++++++++++++++++++++++---------
 include/linux/sched.h          |  1 +
 2 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 8cb7816d03b4..9891b4070a61 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1253,6 +1253,9 @@ static void __mc_scan_banks(struct mce *m, struct pt_regs *regs, struct mce *fin
 
 static void kill_me_now(struct callback_head *ch)
 {
+	struct task_struct *p = container_of(ch, struct task_struct, mce_kill_me);
+
+	p->mce_count = 0;
 	force_sig(SIGBUS);
 }
 
@@ -1262,6 +1265,7 @@ static void kill_me_maybe(struct callback_head *cb)
 	int flags = MF_ACTION_REQUIRED;
 	int ret;
 
+	p->mce_count = 0;
 	pr_err("Uncorrected hardware memory error in user-access at %llx", p->mce_addr);
 
 	if (!p->mce_ripv)
@@ -1290,17 +1294,34 @@ static void kill_me_maybe(struct callback_head *cb)
 	}
 }
 
-static void queue_task_work(struct mce *m, int kill_current_task)
+static noinstr void queue_task_work(struct mce *m, char *msg, int kill_current_task)
 {
-	current->mce_addr = m->addr;
-	current->mce_kflags = m->kflags;
-	current->mce_ripv = !!(m->mcgstatus & MCG_STATUS_RIPV);
-	current->mce_whole_page = whole_page(m);
+	int count = ++current->mce_count;
 
-	if (kill_current_task)
-		current->mce_kill_me.func = kill_me_now;
-	else
-		current->mce_kill_me.func = kill_me_maybe;
+	/* First call, save all the details */
+	if (count == 1) {
+		current->mce_addr = m->addr;
+		current->mce_kflags = m->kflags;
+		current->mce_ripv = !!(m->mcgstatus & MCG_STATUS_RIPV);
+		current->mce_whole_page = whole_page(m);
+
+		if (kill_current_task)
+			current->mce_kill_me.func = kill_me_now;
+		else
+			current->mce_kill_me.func = kill_me_maybe;
+	}
+
+	/* Ten is likley overkill. Don't expect more than two faults before task_work() */
+	if (count > 10)
+		mce_panic("Too many machine checks while accessing user data", m, msg);
+
+	/* Second or later call, make sure page address matches the one from first call */
+	if (count > 1 && (current->mce_addr >> PAGE_SHIFT) != (m->addr >> PAGE_SHIFT))
+		mce_panic("Machine checks to different user pages", m, msg);
+
+	/* Do not call task_work_add() more than once */
+	if (count > 1)
+		return;
 
 	task_work_add(current, &current->mce_kill_me, TWA_RESUME);
 }
@@ -1438,7 +1459,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
 		/* If this triggers there is no way to recover. Die hard. */
 		BUG_ON(!on_thread_stack() || !user_mode(regs));
 
-		queue_task_work(&m, kill_current_task);
+		queue_task_work(&m, msg, kill_current_task);
 
 	} else {
 		/*
@@ -1456,7 +1477,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
 		}
 
 		if (m.kflags & MCE_IN_KERNEL_COPYIN)
-			queue_task_work(&m, kill_current_task);
+			queue_task_work(&m, msg, kill_current_task);
 	}
 out:
 	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index e12b524426b0..39039ce8ac4c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1471,6 +1471,7 @@ struct task_struct {
 					mce_whole_page : 1,
 					__mce_reserved : 62;
 	struct callback_head		mce_kill_me;
+	int				mce_count;
 #endif
 
 #ifdef CONFIG_KRETPROBES
-- 
2.31.1

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

[Borislav Petkov]

On Mon, Sep 13, 2021 at 02:52:39PM -0700, Luck, Tony wrote:
> Also mark queue_task_work() as "noinstr" (as reported kernel test robot
> <lkp@intel.com>)

Yeah, that's not enough - I have a patchset in the works for all this so
I'm going to drop your annotation.

> Cc: <stable@vger.kernel.org>
> Fixes: 5567d11c21a1 ("x86/mce: Send #MC singal from task work")

Ah ok, that one makes sense.

> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> 
> > What about a Fixes: tag?
> 
> Added a Fixes tag.
> 
> Also added "noinstr" to queue_task_work() per a kernel robot report.
> 
> Also re-wrote the commit comment (based on questions raised against v2)

Thanks - very much appreciated and it reads really good!

> > I guess backporting this to the respective kernels is predicated upon
> > the existence of those other "places" in the kernel where code assumes
> > the EFAULT was because of a #PF.
> 
> Not really. I don't expect to change any kernel code that just bounces
> off the same machine check a few times. This patch does work best in
> conjunction with patches 2 & 3 (unchanged, not reposted here). But it
> will fix some old issues even without those two.

Ok, got it.

/me queues.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v2 2/3] x86/mce: Change to not send SIGBUS error during copy from user

[Tony Luck]

Sending a SIGBUS for a copy from user is not the correct semantic.
System calls should return -EFAULT (or a short count for write(2)).

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/mce/core.c | 35 +++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 94830ee9581c..957ec60cd2a8 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1269,7 +1269,7 @@ static void kill_me_maybe(struct callback_head *cb)
 		flags |= MF_MUST_KILL;
 
 	ret = memory_failure(p->mce_addr >> PAGE_SHIFT, flags);
-	if (!ret && !(p->mce_kflags & MCE_IN_KERNEL_COPYIN)) {
+	if (!ret) {
 		set_mce_nospec(p->mce_addr >> PAGE_SHIFT, p->mce_whole_page);
 		sync_core();
 		return;
@@ -1283,15 +1283,21 @@ static void kill_me_maybe(struct callback_head *cb)
 	if (ret == -EHWPOISON)
 		return;
 
-	if (p->mce_vaddr != (void __user *)-1l) {
-		force_sig_mceerr(BUS_MCEERR_AR, p->mce_vaddr, PAGE_SHIFT);
-	} else {
-		pr_err("Memory error not recovered");
-		kill_me_now(cb);
-	}
+	pr_err("Memory error not recovered");
+	kill_me_now(cb);
+}
+
+static void kill_me_never(struct callback_head *cb)
+{
+	struct task_struct *p = container_of(cb, struct task_struct, mce_kill_me);
+
+	p->mce_count = 0;
+	pr_err("Kernel accessed poison in user space at %llx\n", p->mce_addr);
+	if (!memory_failure(p->mce_addr >> PAGE_SHIFT, 0))
+		set_mce_nospec(p->mce_addr >> PAGE_SHIFT, p->mce_whole_page);
 }
 
-static void queue_task_work(struct mce *m, char *msg, int kill_current_task)
+static void queue_task_work(struct mce *m, char *msg, void (*func)(struct callback_head *))
 {
 	int count = ++current->mce_count;
 
@@ -1301,11 +1307,7 @@ static void queue_task_work(struct mce *m, char *msg, int kill_current_task)
 		current->mce_kflags = m->kflags;
 		current->mce_ripv = !!(m->mcgstatus & MCG_STATUS_RIPV);
 		current->mce_whole_page = whole_page(m);
-
-		if (kill_current_task)
-			current->mce_kill_me.func = kill_me_now;
-		else
-			current->mce_kill_me.func = kill_me_maybe;
+		current->mce_kill_me.func = func;
 	}
 
 	/* Ten is likley overkill. Don't expect more than two faults before task_work() */
@@ -1456,7 +1458,10 @@ noinstr void do_machine_check(struct pt_regs *regs)
 		/* If this triggers there is no way to recover. Die hard. */
 		BUG_ON(!on_thread_stack() || !user_mode(regs));
 
-		queue_task_work(&m, msg, kill_current_task);
+		if (kill_current_task)
+			queue_task_work(&m, msg, kill_me_now);
+		else
+			queue_task_work(&m, msg, kill_me_maybe);
 
 	} else {
 		/*
@@ -1474,7 +1479,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
 		}
 
 		if (m.kflags & MCE_IN_KERNEL_COPYIN)
-			queue_task_work(&m, msg, kill_current_task);
+			queue_task_work(&m, msg, kill_me_never);
 	}
 out:
 	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);
-- 
2.29.2

[PATCH v2 3/3] x86/mce: Drop copyin special case for #MC

[Tony Luck]

Fixes to the iterator code to handle faults that are not on
page boundaries mean that the special case for machine check
during copy from user is no longer needed.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/lib/copy_user_64.S | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/arch/x86/lib/copy_user_64.S b/arch/x86/lib/copy_user_64.S
index 57b79c577496..2797e630b9b1 100644
--- a/arch/x86/lib/copy_user_64.S
+++ b/arch/x86/lib/copy_user_64.S
@@ -234,24 +234,11 @@ EXPORT_SYMBOL(copy_user_enhanced_fast_string)
  */
 SYM_CODE_START_LOCAL(.Lcopy_user_handle_tail)
 	movl %edx,%ecx
-	cmp $X86_TRAP_MC,%eax		/* check if X86_TRAP_MC */
-	je 3f
 1:	rep movsb
 2:	mov %ecx,%eax
 	ASM_CLAC
 	ret
 
-	/*
-	 * Return zero to pretend that this copy succeeded. This
-	 * is counter-intuitive, but needed to prevent the code
-	 * in lib/iov_iter.c from retrying and running back into
-	 * the poison cache line again. The machine check handler
-	 * will ensure that a SIGBUS is sent to the task.
-	 */
-3:	xorl %eax,%eax
-	ASM_CLAC
-	ret
-
 	_ASM_EXTABLE_CPY(1b, 2b)
 SYM_CODE_END(.Lcopy_user_handle_tail)
 
-- 
2.29.2

Re: [PATCH v2 3/3] x86/mce: Drop copyin special case for #MC

[Borislav Petkov]

On Tue, Aug 17, 2021 at 05:29:42PM -0700, Tony Luck wrote:
> Fixes to the iterator code to handle faults

Can we name some of those fixes here pls?

We will forget which those were in the future so it would be good to
leave some breadcrumbs at least...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: [PATCH v2 3/3] x86/mce: Drop copyin special case for #MC

[Luck, Tony]

> Can we name some of those fixes here pls?

Some/all of this bunch from Al Viro:

a180bd1d7e16 iov_iter: remove uaccess_kernel() warning from iov_iter_init()
6852df126699 csum_and_copy_to_pipe_iter(): leave handling of csum_state to caller
2a510a744beb clean up copy_mc_pipe_to_iter()
893839fd5733 pipe_zero(): we don't need no stinkin' kmap_atomic()...
2495bdcc86dc iov_iter: clean csum_and_copy_...() primitives up a bit
55ca375c5dcc copy_page_from_iter(): don't need kmap_atomic() for kvec/bvec cases
c1d4d6a9ae88 copy_page_to_iter(): don't bother with kmap_atomic() for bvec/kvec cases
4b179e9a9c7c iterate_xarray(): only of the first iteration we might get offset != 0
a6e4ec7bfd32 pull handling of ->iov_offset into iterate_{iovec,bvec,xarray}
7baa5099002f iov_iter: make iterator callbacks use base and len instead of iovec
622838f3fde2 iov_iter: make the amount already copied available to iterator callbacks
21b56c847753 iov_iter: get rid of separate bvec and xarray callbacks
1b4fb5ffd79b iov_iter: teach iterate_{bvec,xarray}() about possible short copies
7491a2bf64e3 iterate_bvec(): expand bvec.h macro forest, massage a bit
5c67aa90cd5c iov_iter: unify iterate_iovec and iterate_kvec
7a1bcb5d255d iov_iter: massage iterate_iovec and iterate_kvec to logics similar to iterate_bvec
f5da83545f4e iterate_and_advance(): get rid of magic in case when n is 0
594e450b3f44 csum_and_copy_to_iter(): massage into form closer to csum_and_copy_from_iter()
f0b65f39ac50 iov_iter: replace iov_iter_copy_from_user_atomic() with iterator-advancing variant
e4f8df86798a [xarray] iov_iter_npages(): just use DIV_ROUND_UP()
66531c65aa25 iov_iter_npages(): don't bother with iterate_all_kinds()
3d671ca62a08 get rid of iterate_all_kinds() in iov_iter_get_pages()/iov_iter_get_pages_alloc()
610c7a71543d iov_iter_gap_alignment(): get rid of iterate_all_kinds()
9221d2e37b72 iov_iter_alignment(): don't bother with iterate_all_kinds()
8409a0d261e2 sanitize iov_iter_fault_in_readable()
185ac4d43669 iov_iter: optimize iov_iter_advance() for iovec and kvec
8cd54c1c8480 iov_iter: separate direction from flavour
556351c1c09a iov_iter_advance(): don't modify ->iov_offset for ITER_DISCARD
28f38db7edbf iov_iter: reorder handling of flavours in primitives
4b6c132b7da6 iov_iter: switch ..._full() variants of primitives to use of iov_iter_revert()
3b3fc051cd2c iov_iter_advance(): use consistent semantics for move past the end
0e8f0d674015 [xarray] iov_iter_fault_in_readable() should do nothing in xarray case
a506abc7b644 copy_page_to_iter(): fix ITER_DISCARD case
08aa64796016 teach copy_page_to_iter() to handle compound pages
66cd071a1f83 iov_iter: Remove iov_iter_for_each_range()

-Tony

Re: [PATCH v2 3/3] x86/mce: Drop copyin special case for #MC

[Borislav Petkov]

On Mon, Sep 20, 2021 at 04:18:58PM +0000, Luck, Tony wrote:
> > Can we name some of those fixes here pls?
> 
> Some/all of this bunch from Al Viro:

Is this how you generated that list, per chance?

$ git log --oneline v5.14 -- lib/iov_iter.c

?

Output looks at least similar to what you've pasted...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: [PATCH v2 3/3] x86/mce: Drop copyin special case for #MC

[Luck, Tony]

> Is this how you generated that list, per chance?
>
> $ git log --oneline v5.14 -- lib/iov_iter.c

Almost. I had "v5.14 ^v5.13" to stop git from going back
further than when I think Al started applying those fixes.

-Tony

RE: [PATCH v2 0/3] More machine check recovery fixes

[Luck, Tony]

> Changes since v1:
> 1) Fix bug in kill_me_never() that forgot to clear p->mce_count so
>   repeated recovery in the same task would trigger the panic for
>	"Machine checks to different user pages"
>   [Note to Jue Wang ... this *might* be why your test that injects
>    two errors into the same buffer passed to a write(2) syscall
>    failed with this message]

I recreated Jue's specific test today with uncorrected errors in two
pages passed to a write(2) syscall.

	buf = alloc(2 pages);
	inject(buf + 0x440);
	inject*buf + 0x11c0);
	n = write(fd, buf, 8K);

Result was that the write returned 0x440 (i.e. bytes written up to the
first poison location).

-Tony