Re: [RFC PATCH v6 03/25] x86/sgx: Wipe out EREMOVE from sgx_free_epc_page()

From: Kai Huang <kai.huang@intel.com>
To: Dave Hansen <dave@sr71.net>, Sean Christopherson <seanjc@google.com>
Cc: linux-sgx@vger.kernel.org, kvm@vger.kernel.org, x86@kernel.org,
	jarkko@kernel.org, luto@kernel.org, rick.p.edgecombe@intel.com,
	haitao.huang@intel.com, pbonzini@redhat.com, bp@alien8.de,
	tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com
Subject: Re: [RFC PATCH v6 03/25] x86/sgx: Wipe out EREMOVE from sgx_free_epc_page()
Date: Mon, 01 Mar 2021 19:13:25 +1300	[thread overview]
Message-ID: <c528da37e6ea6172d68270d8bdc1280afc1e98c8.camel@intel.com> (raw)
In-Reply-To: <55e0f003-ca2b-24d2-5a23-31a77c5b943d@sr71.net>

On Fri, 2021-02-26 at 12:12 -0800, Dave Hansen wrote:
> On 2/26/21 11:52 AM, Sean Christopherson wrote:
> > > We must give a more informative message saying that the page is leaked.
> > >  Ideally, we'd also make this debuggable by dumping out how many of
> > > these pages there have been somewhere.  That can wait, though, until we
> > > have some kind of stats coming out of the code (there's nothing now).  A
> > > comment to remind us to do this would be nice.
> > Eh, having debugged these several times, the WARN_ONCE in sgx_reset_epc_page()
> > is probably sufficient.  IIRC, when I hit this, things were either laughably
> > broken and every page was failing, or there was another ENCLS failure somewhere
> > else that provided additional info.  Not saying don't add more debug info,
> > rather that it's probably not a priority.
> 
> Minimally, I just want a warning that says, "Whoops, I leaked a page".
> Or EREMOVE could even say, "whoops, this *MIGHT* leak a page".
> 
> My beef is mostly that "EREMOVE failed" doesn't tell and end user squat
> about what this means for their system.  At least if we say "leaked",
> they have some inclination that they've got to reboot to get the page back.

Agreed that a msg to say EPC page is leaked is useful. However I found with current
sgx_reset_epc_page() I cannot find a suitable place to add:

Theoretically, it's not that right to add "EPC page is leaked", or even *might* (btw,
I don't think we should use *might* since it is vague), in to sgx_reset_epc_page(),
since whether leak or not is controlled by whether to call sgx_free_epc_page() upon
error, which is not in sgx_reset_epc_page(). And

	if (!sgx_reset_epc_page())
		sgx_free_epc_page();

is called 3 times so I don't want to add a msg for each of them.

I ended up with this solution: 

1) Rename existing sgx_free_epc_page() to sgx_encl_free_epc_page() to make it more
specific that it is used to free EPC page that is assigned to an enclave. 2) Wrap
non-EREMOVE part (putting back to free EPC pool) to sgx_free_epc_page() so it can be
used by virtual EPC.

In this way we can just put the error msg in sgx_encl_free_epc_page().

And as you said it's time to get RFC tag off, so I'll send out formal patch with
above solution, but w/o your Acked-by on this particular patch. Thanks :)