From: Borislav Petkov <bp@alien8.de>
To: Tony Luck <tony.luck@intel.com>
Cc: x86@kernel.org, Andrew Morton <akpm@linux-foundation.org>,
Peter Zijlstra <peterz@infradead.org>,
Darren Hart <dvhart@infradead.org>,
Andy Lutomirski <luto@kernel.org>,
linux-kernel@vger.kernel.org, linux-edac@vger.kernel.org,
linux-mm@kvack.org
Subject: Re: [PATCH v3] x86/mce: Avoid infinite loop for copy from user recovery
Date: Fri, 15 Jan 2021 16:27:54 +0100 [thread overview]
Message-ID: <20210115152754.GC9138@zn.tnic> (raw)
In-Reply-To: <20210115003817.23657-1-tony.luck@intel.com>
On Thu, Jan 14, 2021 at 04:38:17PM -0800, 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 "mce_busy" counter so that task_work_add() is only called once
> per faulty page in this task.
Yeah, that sentence can be removed now too.
> Do not allow too many repeated machine checks, or machine checks to
> a different page from the first.
>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>
> V3: Thanks to extensive commentary from Andy & Boris
>
> Throws out the changes to get_user() and subsequent changes to core
> code. Everything is now handled in the machine check code. Downside is
> that we can (and do) take multiple machine checks from a single poisoned
> page before generic kernel code finally gets the message that a page is
> really and truly gone (but all the failed get_user() calls still return
> the legacy -EFAULT code, so none of that code will ever mistakenly use
> a value from a bad page). But even on an old machine that does broadcast
> interrupts for each machine check things survive multiple cycles of my
> test injection into a futex operation.
Nice.
>
> I picked "10" as the magic upper limit for how many times the machine
> check code will allow a fault from the same page before deciding to
> panic. We can bike shed that value if you like.
>
> arch/x86/kernel/cpu/mce/core.c | 27 ++++++++++++++++++++-------
> include/linux/sched.h | 1 +
> 2 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 13d3f1cbda17..25daf6517dc9 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1246,6 +1246,7 @@ static void kill_me_maybe(struct callback_head *cb)
> struct task_struct *p = container_of(cb, struct task_struct, mce_kill_me);
> int flags = MF_ACTION_REQUIRED;
>
> + p->mce_count = 0;
> pr_err("Uncorrected hardware memory error in user-access at %llx", p->mce_addr);
>
> if (!p->mce_ripv)
> @@ -1266,12 +1267,24 @@ 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)
So this function gets called in the user mode MCE case too:
if ((m.cs & 3) == 3) {
queue_task_work(&m, msg, kill_current_task);
}
Do we want to panic for multiple MCEs to different addresses in user
mode?
I don't think so - that should go down the memory failure page
offlining path...
> - 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 (current->mce_count++ == 0) {
> + 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);
> + }
> +
/* Magic number should be large enough */
> + if (current->mce_count > 10)
> + mce_panic("Too many machine checks while accessing user data", m, msg);
> +
> + if (current->mce_count > 1 || (current->mce_addr >> PAGE_SHIFT) != (m->addr >> PAGE_SHIFT))
> + mce_panic("Machine checks to different user pages", m, msg);
Will this second part of the test expression, after the "||" ever hit?
You do above in the first branch:
if (current->mce_count++ == 0) {
...
current->mce_addr = m->addr;
and ->mce_count becomes 1.
In that case that
(current->mce_addr >> PAGE_SHIFT) != (m->addr >> PAGE_SHIFT)
gets tested but that won't ever be true because ->mce_addr = ->addr
above.
And then, for other values of mce_count, mce_count > 1 will hit.
In any case, what are you trying to catch with this? Two get_user() to
different pages both catching MCEs?
> +
> + /* Do not call task_work_add() more than once */
> + if (current->mce_count > 1)
> + return;
That won't happen either, AFAICT. It'll panic above.
Regardless, I like how this is all confined to the MCE code and there's
no need to touch stuff outside...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
next prev parent reply other threads:[~2021-01-15 15:28 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-08 22:22 [PATCH 0/2] Fix infinite machine check loop in futex_wait_setup() Tony Luck
2021-01-08 22:22 ` [PATCH 1/2] x86/mce: Avoid infinite loop for copy from user recovery Tony Luck
2021-01-08 22:22 ` [PATCH 2/2] futex, x86/mce: Avoid double machine checks Tony Luck
2021-01-08 22:47 ` Peter Zijlstra
2021-01-08 23:08 ` Luck, Tony
2021-01-08 23:14 ` Peter Zijlstra
2021-01-08 23:20 ` Luck, Tony
2021-01-11 21:44 ` [PATCH v2 0/3] Fix infinite machine check loop in futex_wait_setup() Tony Luck
2021-01-11 21:44 ` [PATCH v2 1/3] x86/mce: Avoid infinite loop for copy from user recovery Tony Luck
2021-01-11 22:11 ` Andy Lutomirski
2021-01-11 22:20 ` Luck, Tony
2021-01-12 17:00 ` Andy Lutomirski
2021-01-12 17:16 ` Luck, Tony
2021-01-12 17:21 ` Andy Lutomirski
2021-01-12 18:23 ` Luck, Tony
2021-01-12 18:57 ` Andy Lutomirski
2021-01-12 20:52 ` Luck, Tony
2021-01-12 22:04 ` Andy Lutomirski
2021-01-13 1:50 ` Luck, Tony
2021-01-13 4:15 ` Andy Lutomirski
2021-01-13 10:00 ` Borislav Petkov
2021-01-13 16:06 ` Luck, Tony
2021-01-13 16:19 ` Borislav Petkov
2021-01-13 16:32 ` Luck, Tony
2021-01-13 17:35 ` Borislav Petkov
2021-01-14 20:22 ` Borislav Petkov
2021-01-14 21:05 ` Luck, Tony
2021-01-11 21:44 ` [PATCH v2 2/3] x86/mce: Add new return value to get_user() for machine check Tony Luck
2021-01-11 21:44 ` [PATCH v2 3/3] futex, x86/mce: Avoid double machine checks Tony Luck
2021-01-14 17:22 ` [PATCH v2 0/3] Fix infinite machine check loop in futex_wait_setup() Andy Lutomirski
2021-01-15 0:38 ` [PATCH v3] x86/mce: Avoid infinite loop for copy from user recovery Tony Luck
2021-01-15 15:27 ` Borislav Petkov [this message]
2021-01-15 19:34 ` Luck, Tony
2021-01-15 20:51 ` [PATCH v4] " Luck, Tony
2021-01-15 23:23 ` Luck, Tony
2021-01-19 10:56 ` Borislav Petkov
2021-01-19 23:57 ` Luck, Tony
2021-01-20 12:18 ` Borislav Petkov
2021-01-20 17:17 ` Luck, Tony
2021-01-21 21:09 ` Luck, Tony
2021-01-25 22:55 ` [PATCH v5] " Luck, Tony
2021-01-26 11:03 ` Borislav Petkov
2021-01-26 22:36 ` Luck, Tony
2021-01-28 17:57 ` Borislav Petkov
2021-02-01 18:58 ` Luck, Tony
2021-02-02 11:01 ` Borislav Petkov
2021-02-02 16:04 ` Luck, Tony
2021-02-02 21:06 ` Borislav Petkov
2021-02-02 22:12 ` Luck, Tony
2021-01-18 15:39 ` [PATCH v3] " Borislav Petkov
-- strict thread matches above, loose matches on Subject: below --
2021-07-06 19:06 [PATCH 0/3] More machine check recovery fixes Tony Luck
2021-08-18 0:29 ` [PATCH v2 " Tony Luck
2021-08-18 0:29 ` [PATCH v2 1/3] x86/mce: Avoid infinite loop for copy from user recovery Tony Luck
2021-09-13 9:24 ` Borislav Petkov
2021-09-13 21:52 ` [PATCH v3] " Luck, Tony
2021-09-14 8:28 ` Borislav Petkov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210115152754.GC9138@zn.tnic \
--to=bp@alien8.de \
--cc=akpm@linux-foundation.org \
--cc=dvhart@infradead.org \
--cc=linux-edac@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=luto@kernel.org \
--cc=peterz@infradead.org \
--cc=tony.luck@intel.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.