| Message ID | 1501626977-6983-2-git-send-email-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 01, 2017 at 03:36:16PM -0700, Sean Christopherson wrote: > Freeze the swap thread, ksgxswapd, during suspend/hibernate instead of > stopping and re-starting the thread on suspend/resume. Freezing, as > opposed to stopping/running, eliminates the possibility of kthread_run > failing during resume, which in turn eliminates the need to handle the > scenario where there is no swap thread running when an enclave needs > to page in EPC memory. Makes sense. Risk would be low but if we can eliminate it completely that is even better. > Previously, sgx_pm_resume didn't check the return of kthread_run, which > can fail with -ENOMEM. Upon failure, SGX applications would hang due > to the lack of a kernel thread to perform EPC swapping, and the kernel > would crash on the next system suspend due to passing an error pointer > to kthread_stop. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> I apply this and other patch. Thanks! /Jarkko > --- > drivers/platform/x86/intel_sgx/sgx.h | 2 -- > drivers/platform/x86/intel_sgx/sgx_main.c | 11 +---------- > drivers/platform/x86/intel_sgx/sgx_page_cache.c | 15 ++++++++++----- > 3 files changed, 11 insertions(+), 17 deletions(-) > > diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h > index b5b21d4..b1d0ece 100644 > --- a/drivers/platform/x86/intel_sgx/sgx.h > +++ b/drivers/platform/x86/intel_sgx/sgx.h > @@ -215,9 +215,7 @@ void sgx_tgid_ctx_release(struct kref *ref); > > extern struct mutex sgx_tgid_ctx_mutex; > extern struct list_head sgx_tgid_ctx_list; > -extern struct task_struct *ksgxswapd_tsk; > > -int ksgxswapd(void *p); > int sgx_add_epc_bank(resource_size_t start, unsigned long size); > int sgx_page_cache_init(void); > void sgx_page_cache_teardown(void); > diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c b/drivers/platform/x86/intel_sgx/sgx_main.c > index 32214cd..8469633 100644 > --- a/drivers/platform/x86/intel_sgx/sgx_main.c > +++ b/drivers/platform/x86/intel_sgx/sgx_main.c > @@ -164,9 +164,6 @@ static int sgx_pm_suspend(struct device *dev) > struct sgx_tgid_ctx *ctx; > struct sgx_encl *encl; > > - kthread_stop(ksgxswapd_tsk); > - ksgxswapd_tsk = NULL; > - > list_for_each_entry(ctx, &sgx_tgid_ctx_list, list) { > list_for_each_entry(encl, &ctx->encl_list, encl_list) { > sgx_invalidate(encl, false); > @@ -178,13 +175,7 @@ static int sgx_pm_suspend(struct device *dev) > return 0; > } > > -static int sgx_pm_resume(struct device *dev) > -{ > - ksgxswapd_tsk = kthread_run(ksgxswapd, NULL, "kswapd"); > - return 0; > -} > - > -static SIMPLE_DEV_PM_OPS(sgx_drv_pm, sgx_pm_suspend, sgx_pm_resume); > +static SIMPLE_DEV_PM_OPS(sgx_drv_pm, sgx_pm_suspend, NULL); > > static int sgx_dev_init(struct device *dev) > { > diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c > index 356f2c2..f1553324 100644 > --- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c > +++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c > @@ -78,7 +78,7 @@ static unsigned int sgx_nr_total_epc_pages; > static unsigned int sgx_nr_free_pages; > static unsigned int sgx_nr_low_pages = SGX_NR_LOW_EPC_PAGES_DEFAULT; > static unsigned int sgx_nr_high_pages; > -struct task_struct *ksgxswapd_tsk; > +static struct task_struct *ksgxswapd_tsk; > static DECLARE_WAIT_QUEUE_HEAD(ksgxswapd_waitq); > > > @@ -368,12 +368,17 @@ static void sgx_swap_pages(unsigned long nr_to_scan) > kref_put(&ctx->refcount, sgx_tgid_ctx_release); > } > > -int ksgxswapd(void *p) > +static int ksgxswapd(void *p) > { > + set_freezable(); > + > while (!kthread_should_stop()) { > - wait_event_interruptible(ksgxswapd_waitq, > - kthread_should_stop() || > - sgx_nr_free_pages < sgx_nr_high_pages); > + if (try_to_freeze()) > + continue; > + > + wait_event_freezable(ksgxswapd_waitq, > + kthread_should_stop() || > + sgx_nr_free_pages < sgx_nr_high_pages); > > if (sgx_nr_free_pages < sgx_nr_high_pages) > sgx_swap_pages(SGX_NR_SWAP_CLUSTER_MAX); > -- > 2.7.4 > > _______________________________________________ > intel-sgx-kernel-dev mailing list > intel-sgx-kernel-dev@lists.01.org > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h index b5b21d4..b1d0ece 100644 --- a/drivers/platform/x86/intel_sgx/sgx.h +++ b/drivers/platform/x86/intel_sgx/sgx.h @@ -215,9 +215,7 @@ void sgx_tgid_ctx_release(struct kref *ref); extern struct mutex sgx_tgid_ctx_mutex; extern struct list_head sgx_tgid_ctx_list; -extern struct task_struct *ksgxswapd_tsk; -int ksgxswapd(void *p); int sgx_add_epc_bank(resource_size_t start, unsigned long size); int sgx_page_cache_init(void); void sgx_page_cache_teardown(void); diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c b/drivers/platform/x86/intel_sgx/sgx_main.c index 32214cd..8469633 100644 --- a/drivers/platform/x86/intel_sgx/sgx_main.c +++ b/drivers/platform/x86/intel_sgx/sgx_main.c @@ -164,9 +164,6 @@ static int sgx_pm_suspend(struct device *dev) struct sgx_tgid_ctx *ctx; struct sgx_encl *encl; - kthread_stop(ksgxswapd_tsk); - ksgxswapd_tsk = NULL; - list_for_each_entry(ctx, &sgx_tgid_ctx_list, list) { list_for_each_entry(encl, &ctx->encl_list, encl_list) { sgx_invalidate(encl, false); @@ -178,13 +175,7 @@ static int sgx_pm_suspend(struct device *dev) return 0; } -static int sgx_pm_resume(struct device *dev) -{ - ksgxswapd_tsk = kthread_run(ksgxswapd, NULL, "kswapd"); - return 0; -} - -static SIMPLE_DEV_PM_OPS(sgx_drv_pm, sgx_pm_suspend, sgx_pm_resume); +static SIMPLE_DEV_PM_OPS(sgx_drv_pm, sgx_pm_suspend, NULL); static int sgx_dev_init(struct device *dev) { diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c index 356f2c2..f1553324 100644 --- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c +++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c @@ -78,7 +78,7 @@ static unsigned int sgx_nr_total_epc_pages; static unsigned int sgx_nr_free_pages; static unsigned int sgx_nr_low_pages = SGX_NR_LOW_EPC_PAGES_DEFAULT; static unsigned int sgx_nr_high_pages; -struct task_struct *ksgxswapd_tsk; +static struct task_struct *ksgxswapd_tsk; static DECLARE_WAIT_QUEUE_HEAD(ksgxswapd_waitq); @@ -368,12 +368,17 @@ static void sgx_swap_pages(unsigned long nr_to_scan) kref_put(&ctx->refcount, sgx_tgid_ctx_release); } -int ksgxswapd(void *p) +static int ksgxswapd(void *p) { + set_freezable(); + while (!kthread_should_stop()) { - wait_event_interruptible(ksgxswapd_waitq, - kthread_should_stop() || - sgx_nr_free_pages < sgx_nr_high_pages); + if (try_to_freeze()) + continue; + + wait_event_freezable(ksgxswapd_waitq, + kthread_should_stop() || + sgx_nr_free_pages < sgx_nr_high_pages); if (sgx_nr_free_pages < sgx_nr_high_pages) sgx_swap_pages(SGX_NR_SWAP_CLUSTER_MAX);
Freeze the swap thread, ksgxswapd, during suspend/hibernate instead of stopping and re-starting the thread on suspend/resume. Freezing, as opposed to stopping/running, eliminates the possibility of kthread_run failing during resume, which in turn eliminates the need to handle the scenario where there is no swap thread running when an enclave needs to page in EPC memory. Previously, sgx_pm_resume didn't check the return of kthread_run, which can fail with -ENOMEM. Upon failure, SGX applications would hang due to the lack of a kernel thread to perform EPC swapping, and the kernel would crash on the next system suspend due to passing an error pointer to kthread_stop. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- drivers/platform/x86/intel_sgx/sgx.h | 2 -- drivers/platform/x86/intel_sgx/sgx_main.c | 11 +---------- drivers/platform/x86/intel_sgx/sgx_page_cache.c | 15 ++++++++++----- 3 files changed, 11 insertions(+), 17 deletions(-)