Re: [PATCH v3] x86/sgx: Do not consider unsanitized pages an error

[Jarkko Sakkinen <jarkko@kernel.org>]

On Thu, Aug 25, 2022 at 09:57:11AM -0500, Haitao Huang wrote:
> Hi Jarkko,
> 
> On Thu, 25 Aug 2022 03:08:02 -0500, Jarkko Sakkinen <jarkko@kernel.org>
> wrote:
> 
> > If sgx_dirty_page_list ends up being non-empty, currently this triggers
> > WARN_ON(), which produces a lot of noise, and can potentially crash the
> > kernel, depending on the kernel command line.
> > 
> > However, if the SGX subsystem initialization is retracted, the
> > sanitization
> > process could end up in the middle, and sgx_dirty_page_list be left
> > non-empty for legit reasons.
> > 
> > Replace this faulty behavior with more verbose version
> > __sgx_sanitize_pages(), which can optionally print EREMOVE error code and
> > the number of unsanitized pages.
> > 
> > Link: https://lore.kernel.org/linux-sgx/20220825051827.246698-1-jarkko@kernel.org/T/#u
> > Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> > Fixes: 51ab30eb2ad4 ("x86/sgx: Replace section->init_laundry_list with
> > sgx_dirty_page_list")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > Cc: Haitao Huang <haitao.huang@linux.intel.com>
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: Reinette Chatre <reinette.chatre@intel.com>
> > ---
> > v3:
> > - Remove WARN_ON().
> > - Tuned comments and the commit message a bit.
> > 
> > v2:
> > - Replaced WARN_ON() with optional pr_info() inside
> >   __sgx_sanitize_pages().
> > - Rewrote the commit message.
> > - Added the fixes tag.
> > ---
> >  arch/x86/kernel/cpu/sgx/main.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> > b/arch/x86/kernel/cpu/sgx/main.c
> > index 515e2a5f25bb..d204520a5e26 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -50,16 +50,17 @@ static LIST_HEAD(sgx_dirty_page_list);
> >   * from the input list, and made available for the page allocator. SECS
> > pages
> >   * prepending their children in the input list are left intact.
> >   */
> > -static void __sgx_sanitize_pages(struct list_head *dirty_page_list)
> > +static void __sgx_sanitize_pages(struct list_head *dirty_page_list,
> > bool verbose)
> >  {
> >  	struct sgx_epc_page *page;
> > +	int dirty_count = 0;
> >  	LIST_HEAD(dirty);
> >  	int ret;
> > 	/* dirty_page_list is thread-local, no need for a lock: */
> 
> Just a nitpick,
> Although it is not added in this patch, the above comment is not accurate.
> The list is accessed one thread only: filled first in main thread, then
> only ever accessed here.
> 
> IIUC, could you remove or update that comment?

Well, if we cut hairs here, it's actually expectation for the
caller, right? :-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index d204520a5e26..c6f416307812 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -49,6 +49,8 @@ static LIST_HEAD(sgx_dirty_page_list);
  * Reset post-kexec EPC pages to the uninitialized state. The pages are removed
  * from the input list, and made available for the page allocator. SECS pages
  * prepending their children in the input list are left intact.
+ *
+ * @dirty_page_list must be thread-local, i.e. no need for a lock.
  */
 static void __sgx_sanitize_pages(struct list_head *dirty_page_list, bool verbose)
 {
@@ -57,7 +59,6 @@ static void __sgx_sanitize_pages(struct list_head *dirty_page_list, bool verbose
        LIST_HEAD(dirty);
        int ret;

-       /* dirty_page_list is thread-local, no need for a lock: */
        while (!list_empty(dirty_page_list)) {
                if (kthread_should_stop())
                        break;

> 
> Other than that, FWIW:
> Reviewed-by: Haitao Huang <haitao.huang@linux.intel.com>
> Thanks
> Haitao

Thank you.

BR, Jarkko