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@xxxxxxxxxx> > 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@xxxxxxxxxx/T/#u > > Reported-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx> > > Fixes: 51ab30eb2ad4 ("x86/sgx: Replace section->init_laundry_list with > > sgx_dirty_page_list") > > Signed-off-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx> > > > > Cc: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx> > > Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx> > > Cc: Reinette Chatre <reinette.chatre@xxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxx> > Thanks > Haitao Thank you. BR, Jarkko
