From: "Luck, Tony" <tony.luck@intel.com>
To: Borislav Petkov <bp@alien8.de>
Cc: Jue Wang <juew@google.com>, Ding Hui <dinghui@sangfor.com.cn>,
naoya.horiguchi@nec.com, osalvador@suse.de,
Youquan Song <youquan.song@intel.com>,
huangcun@sangfor.com.cn, x86@kernel.org,
linux-edac@vger.kernel.org, linux-mm@kvack.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] x86/mce: Avoid infinite loop for copy from user recovery
Date: Fri, 20 Aug 2021 11:59:45 -0700 [thread overview]
Message-ID: <20210820185945.GA1623421@agluck-desk2.amr.corp.intel.com> (raw)
In-Reply-To: <YR/m/8PCmCTbogey@zn.tnic>
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
next prev parent reply other threads:[~2021-08-20 18:59 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-06 19:06 [PATCH 0/3] More machine check recovery fixes Tony Luck
2021-07-06 19:06 ` [PATCH 1/3] x86/mce: Change to not send SIGBUS error during copy from user Tony Luck
2021-07-06 19:06 ` [PATCH 2/3] x86/mce: Avoid infinite loop for copy from user recovery Tony Luck
2021-07-06 19:06 ` [PATCH 3/3] x86/mce: Drop copyin special case for #MC Tony Luck
2021-08-18 0:29 ` [PATCH v2 0/3] More machine check recovery fixes Tony Luck
2021-08-18 0:29 ` [PATCH v2 1/3] x86/mce: Avoid infinite loop for copy from user recovery Tony Luck
2021-08-20 17:31 ` Borislav Petkov
2021-08-20 18:59 ` Luck, Tony [this message]
2021-08-20 19:27 ` Borislav Petkov
2021-08-20 20:23 ` Luck, Tony
2021-08-21 4:51 ` Tony Luck
2021-08-21 21:51 ` Al Viro
2021-08-22 14:36 ` Borislav Petkov
2021-08-20 20:33 ` Luck, Tony
2021-08-22 14:46 ` Borislav Petkov
2021-08-23 15:24 ` Luck, Tony
2021-09-13 9:24 ` Borislav Petkov
2021-09-13 21:52 ` [PATCH v3] " Luck, Tony
2021-09-14 8:28 ` Borislav Petkov
2021-08-18 0:29 ` [PATCH v2 2/3] x86/mce: Change to not send SIGBUS error during copy from user Tony Luck
2021-09-21 7:52 ` [tip: ras/core] " tip-bot2 for Tony Luck
2021-08-18 0:29 ` [PATCH v2 3/3] x86/mce: Drop copyin special case for #MC Tony Luck
2021-09-20 9:13 ` Borislav Petkov
2021-09-20 16:18 ` Luck, Tony
2021-09-20 16:37 ` Borislav Petkov
2021-09-20 16:43 ` Luck, Tony
2021-09-21 7:52 ` [tip: ras/core] " tip-bot2 for Tony Luck
2021-08-18 16:14 ` [PATCH v2 0/3] More machine check recovery fixes Luck, Tony
-- strict thread matches above, loose matches on Subject: below --
2021-01-08 22:22 [PATCH 0/2] Fix infinite machine check loop in futex_wait_setup() Tony Luck
2021-01-11 21:44 ` [PATCH v2 0/3] " 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
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=20210820185945.GA1623421@agluck-desk2.amr.corp.intel.com \
--to=tony.luck@intel.com \
--cc=bp@alien8.de \
--cc=dinghui@sangfor.com.cn \
--cc=huangcun@sangfor.com.cn \
--cc=juew@google.com \
--cc=linux-edac@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=naoya.horiguchi@nec.com \
--cc=osalvador@suse.de \
--cc=x86@kernel.org \
--cc=youquan.song@intel.com \
/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.