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

[Sean Christopherson <sean.j.christopherson@intel.com>]

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
>