Message-ID: <CAOq732KJ658+mRTZV+SdMRVTdKBUARzWz2WCoTyHqtF98fkdWA@mail.gmail.com>
Date: Wed, 12 Feb 2025 01:22:13 +0100
From: Andrew Zaborowski <andrew.zaborowski@...el.com>
To: Dave Hansen <dave.hansen@...el.com>
Cc: x86@...nel.org, linux-sgx@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Dave Hansen <dave.hansen@...ux.intel.com>, Tony Luck <tony.luck@...el.com>, 
	Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>, 
	"H . Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH] x86: sgx: Don't track poisoned pages for reclaiming

On Tue, 11 Feb 2025 at 17:25, Dave Hansen <dave.hansen@...el.com> wrote:
>         git log arch/x86/kernel/cpu/sgx/
>
> That usually works for every little nook and cranny of the kernel and
> will show you what the subject rules are.
>
> Could you do that for this patch for v2, please?

My bad, I'll use x86/sgx: ...

>
> Also, this isn't about _tracking_ pages per se. It's avoiding SGX page
> reclaim, don't you think?

Ok, the goal is to avoid the crash so I'll update the subject line.
That said clearing SGX_EPC_PAGE_RECLAIMER_TRACKED for a page that
cannot be reclaimed seems reasonable on its own.

>
> On 2/11/25 07:01, Andrew Zaborowski wrote:
> > Pages used by an enclave only get page->poison set in
>
> Could we please call these "epc_page" instead of "page"?

Ok, makes sense.

>
> > arch_memory_failure() but stay on sgx_active_page_list.
> > page->poison is not checked in the reclaimer logic meaning that a page could be
> > reclaimed and go through ETRACK, EBLOCK and EWB.  This can lead to the
> > firmware receiving and MCE in one of those operations and going into
> > "unbreakable shutdown" and triggering a kernel panic on remaining cores.
>
> This requires low-level SGX implementation knowledge to fully
> understand. Both what "ETRACK, EBLOCK and EWB" are in the first place,
> how they are involved in reclaim and also why EREMOVE doesn't lead to
> the same fate.
>
> Can it be written in a more approachable way?
>
> During SGX reclaim, the CPU actually touches the SGX data page,
> encrypting and writing its contents out to normal memory. These "EWB"
> writeback operations are implemented in what are effectively big,
> complicated chunks of microcode. Any machine checks encountered during
> this writeback operation are usually fatal to the entire system.
>
> If an epc_page has poison, reclaiming it is highly likely to bring the
> whole system down. The SGX reclaim code does not currently check for poison.

Ok, I agree part of this explanation is fit for the commit message.
In a busy area of code I don't think every commit touching it should
explain what the code does but here it makes sense.

As a side note with a set of enclaves fighting badly enough for the
EPC memory the probability of an MCE happening inside one of the EWB
operations can become considerable because of the sheer amount of time
spent in them and I don't think anything can be done about it.

>
> --
>
> > Remove the affected page from sgx_active_page_list but don't add it
> > immediately to &node->sgx_poison_page_list to keep most of the current
> > semantics.
>
> What semantics are being kept? Are they important?

That's a good question.  An epc_page can be on one of 3 or 4 lists, or
none, so the logic is already complicated.  My guess is that moving
the page to &node->sgx_poison_page_list only after EREMOVE is done
adds some order, because functionally it doesn't change anything.
(But __sgx_sanitize_pages skips the EREMOVE for poisoned pages)

So epc_page->poison is used temporarily while the page is mapped into
an enclave.  Once the enclave is released a poisoned page is moved to
sgx_poison_page_list.  The current semantics ensure that by that time
the epc_page is not referenced by an encl_page and is not on any other
list so for all purposes it's forgotten.

>
> > Tested with CONFIG_PROVE_LOCKING as suggested by Tony Luck.
>
> "I tested it with lockdep and it didn't blow up" is definitely better
> than "I booted this and it didn't blow up" or not testing it at all.
>
> But even better would be demonstrating in the changelog that the locking
> rules were understood and respected in this patch.
>
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index 671c26513..7076464d4 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -719,6 +719,8 @@ int arch_memory_failure(unsigned long pfn, int flags)
> >               goto out;
> >       }
> >
> > +     sgx_unmark_page_reclaimable(page);
> > +
> >       /*
> >        * TBD: Add additional plumbing to enable pre-emptive
> >        * action for asynchronous poison notification. Until
>
> I'll 100% buy that this is the most expeditious fix.
>
> But is it the _best_ one?
>
> In the end, this patch has the semantics of avoiding SGX reclaim on
> poisoned pages. Wouldn't it be most straightforward to implement that in
> the SGX *reclaim* code?

I don't know.  If we know we cannot reclaim a page should we force the
reclaimer to still look at its flags?

I'm sure we don't want to have to check epc_page->poison multiple
times if we can remove it from a relevant list once. Perhaps it's not
great that my patch adds a period when the epc_page is not on *any*
list, everywhere else this is done only for very short times.  I'm not
sure what it would take to eremove the page earlier so it can be added
to sgx_poison_page_list, I think the "TBD" comment in
arch_memory_failure() refers to that and I hoped to leave it as a TBD.

Best regards

Powered by blists - more mailing lists