Messages in this thread Patch in this message |  | | Subject | Re: [PATCH v3] x86/sgx: Do not consider unsanitized pages an error | | Date | Thu, 25 Aug 2022 09:57:11 -0500 | | From | "Haitao Huang" <> |
| |
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?
Other than that, FWIW: Reviewed-by: Haitao Huang <haitao.huang@linux.intel.com> Thanks Haitao
|  |