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

From: Borislav Petkov <bp@alien8.de>
To: "Luck, Tony" <tony.luck@intel.com>
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 21:27:44 +0200	[thread overview]
Message-ID: <YSACMCEoU6FxjDNh@zn.tnic> (raw)
In-Reply-To: <20210820185945.GA1623421@agluck-desk2.amr.corp.intel.com>

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