[PATCH] x86/sgx: rip off the refcount from sgx_encl_add_page flow

[PATCH] x86/sgx: rip off the refcount from sgx_encl_add_page flow

[Jarkko Sakkinen]

With the file bound approach we can flush the work queue when the file
is closed.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 3 +--
 arch/x86/kernel/cpu/sgx/driver/main.c  | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index d17c60dca114..f56d3c712dc4 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -172,7 +172,7 @@ static void sgx_add_page_worker(struct work_struct *work)
 
 next:
 		kfree(req);
-	} while (!kref_put(&encl->refcount, sgx_encl_release) && !is_empty);
+	} while (!is_empty);
 }
 
 static u32 sgx_calc_ssaframesize(u32 miscselect, u64 xfrm)
@@ -520,7 +520,6 @@ static int __sgx_encl_add_page(struct sgx_encl *encl,
 	req->encl_page = encl_page;
 	req->mrmask = mrmask;
 	empty = list_empty(&encl->add_page_reqs);
-	kref_get(&encl->refcount);
 	list_add_tail(&req->list, &encl->add_page_reqs);
 	if (empty)
 		queue_work(sgx_encl_wq, &encl->work);
diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index 0c831ee5e2de..9ea082372682 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -47,6 +47,7 @@ static int sgx_release(struct inode *inode, struct file *file)
 {
 	struct sgx_encl *encl = file->private_data;
 
+	flush_work(&encl->work);
 	kref_put(&encl->refcount, sgx_encl_release);
 
 	return 0;
-- 
2.20.1

Re: [PATCH] x86/sgx: rip off the refcount from sgx_encl_add_page flow

[Sean Christopherson]

On Wed, Jun 19, 2019 at 03:21:43PM +0300, Jarkko Sakkinen wrote:
> With the file bound approach we can flush the work queue when the file
> is closed.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 3 +--
>  arch/x86/kernel/cpu/sgx/driver/main.c  | 1 +
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index d17c60dca114..f56d3c712dc4 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -172,7 +172,7 @@ static void sgx_add_page_worker(struct work_struct *work)
>  
>  next:
>  		kfree(req);
> -	} while (!kref_put(&encl->refcount, sgx_encl_release) && !is_empty);
> +	} while (!is_empty);
>  }
>  
>  static u32 sgx_calc_ssaframesize(u32 miscselect, u64 xfrm)
> @@ -520,7 +520,6 @@ static int __sgx_encl_add_page(struct sgx_encl *encl,
>  	req->encl_page = encl_page;
>  	req->mrmask = mrmask;
>  	empty = list_empty(&encl->add_page_reqs);
> -	kref_get(&encl->refcount);
>  	list_add_tail(&req->list, &encl->add_page_reqs);
>  	if (empty)
>  		queue_work(sgx_encl_wq, &encl->work);
> diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
> index 0c831ee5e2de..9ea082372682 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/main.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/main.c
> @@ -47,6 +47,7 @@ static int sgx_release(struct inode *inode, struct file *file)
>  {
>  	struct sgx_encl *encl = file->private_data;
>  
> +	flush_work(&encl->work);

We should set SGX_ENCL_DEAD before flush_work().  Other than that I can't
think of any issues with this approach.  Though hopefully we can purge the
worker altogether and make it a moot point :-).

>  	kref_put(&encl->refcount, sgx_encl_release);
>  
>  	return 0;
> -- 
> 2.20.1
> 

Re: [PATCH] x86/sgx: rip off the refcount from sgx_encl_add_page flow

[Jarkko Sakkinen]

On Wed, Jun 19, 2019 at 08:43:41AM -0700, Sean Christopherson wrote:
> We should set SGX_ENCL_DEAD before flush_work().  Other than that I can't
> think of any issues with this approach.  Though hopefully we can purge the
> worker altogether and make it a moot point :-).

Thanks, I squashed the change now. And definitely we can considere to
completely purge that. This change probably won't make it more
difficult :-)

I also removed PM notifier and SGX_ENCL_SUSPEND. Did not bother to cycle
that change here because on that we've already made conclusions. Just
have forgot to do that change.

/Jarkko