Re: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Jarkko Sakkinen <jarkko@kernel.org>]

On Thu, May 22, 2025 at 12:21:34PM +0300, Elena Reshetova wrote:
> Currently SGX does not have a global counter to count the
> active users from userspace or hypervisor. Implement such a counter,
> sgx_usage_count. It will be used by the driver when attempting
> to call EUPDATESVN SGX instruction.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver.c | 22 ++++++++++++++++------
>  arch/x86/kernel/cpu/sgx/encl.c   |  1 +
>  arch/x86/kernel/cpu/sgx/main.c   | 14 ++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h    |  3 +++
>  arch/x86/kernel/cpu/sgx/virt.c   | 16 ++++++++++++++--
>  5 files changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> index 7f8d1e11dbee..a2994a74bdff 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -19,9 +19,15 @@ static int sgx_open(struct inode *inode, struct file *file)
>  	struct sgx_encl *encl;
>  	int ret;
>  
> +	ret = sgx_inc_usage_count();
> +	if (ret)
> +		return ret;
> +
>  	encl = kzalloc(sizeof(*encl), GFP_KERNEL);
> -	if (!encl)
> -		return -ENOMEM;
> +	if (!encl) {
> +		ret = -ENOMEM;
> +		goto err_usage_count;
> +	}
>  
>  	kref_init(&encl->refcount);
>  	xa_init(&encl->page_array);
> @@ -31,14 +37,18 @@ static int sgx_open(struct inode *inode, struct file *file)
>  	spin_lock_init(&encl->mm_lock);
>  
>  	ret = init_srcu_struct(&encl->srcu);
> -	if (ret) {
> -		kfree(encl);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_encl;
>  
>  	file->private_data = encl;
>  
>  	return 0;
> +
> +err_encl:
> +	kfree(encl);
> +err_usage_count:
> +	sgx_dec_usage_count();
> +	return ret;
>  }
>  
>  static int sgx_release(struct inode *inode, struct file *file)
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 279148e72459..3b54889ae4a4 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -765,6 +765,7 @@ void sgx_encl_release(struct kref *ref)
>  	WARN_ON_ONCE(encl->secs.epc_page);
>  
>  	kfree(encl);
> +	sgx_dec_usage_count();
>  }
>  
>  /*
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 2de01b379aa3..a018b01b8736 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -917,6 +917,20 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
>  }
>  EXPORT_SYMBOL_GPL(sgx_set_attribute);
>  
> +/* Counter to count the active SGX users */
> +static atomic64_t sgx_usage_count;
> +
> +int sgx_inc_usage_count(void)
> +{
> +	atomic64_inc(&sgx_usage_count);
> +	return 0;
> +}

Maybe this was discussed but why this is not just a void-function?

> +
> +void sgx_dec_usage_count(void)
> +{
> +	atomic64_dec(&sgx_usage_count);
> +}

I think these both should be static inlines in arch/x86/kernel/cpu/sgx.h.
Global symbols is over the top. Even if I think disassembly (when doing
debugging, bug hunting or similar tasks), it'd nicer that way.

> +
>  static int __init sgx_init(void)
>  {
>  	int ret;
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index d2dad21259a8..f5940393d9bd 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -102,6 +102,9 @@ static inline int __init sgx_vepc_init(void)
>  }
>  #endif
>  
> +int sgx_inc_usage_count(void);
> +void sgx_dec_usage_count(void);
> +
>  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
>  
>  #endif /* _X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
> index 7aaa3652e31d..6ce908ed51c9 100644
> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -255,22 +255,34 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
>  	xa_destroy(&vepc->page_array);
>  	kfree(vepc);
>  
> +	sgx_dec_usage_count();
>  	return 0;
>  }
>  
>  static int sgx_vepc_open(struct inode *inode, struct file *file)
>  {
>  	struct sgx_vepc *vepc;
> +	int ret;
> +
> +	ret = sgx_inc_usage_count();
> +	if (ret)
> +		return ret;
>  
>  	vepc = kzalloc(sizeof(struct sgx_vepc), GFP_KERNEL);
> -	if (!vepc)
> -		return -ENOMEM;
> +	if (!vepc) {
> +		ret = -ENOMEM;
> +		goto err_usage_count;
> +	}
>  	mutex_init(&vepc->lock);
>  	xa_init(&vepc->page_array);
>  
>  	file->private_data = vepc;
>  
>  	return 0;
> +
> +err_usage_count:
> +	sgx_dec_usage_count();
> +	return ret;

Right, mirrors here my earlier suggestion for rollback, great (two
iterations ago)!

>  }
>  
>  static long sgx_vepc_ioctl(struct file *file,
> -- 
> 2.45.2
> 
> 

BR, Jrakko