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 |  1 +
 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   |  2 ++
 5 files changed, 21 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index 7f8d1e11dbee..b5ffe104af4c 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file *file)
 	struct sgx_encl *encl;
 	int ret;
 
+	sgx_inc_usage_count();
 	encl = kzalloc(sizeof(*encl), GFP_KERNEL);
 	if (!encl)
 		return -ENOMEM;
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 8ce352fc72ac..80d565e6f2ad 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -914,6 +914,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;
+}
+
+void sgx_dec_usage_count(void)
+{
+	atomic64_dec(&sgx_usage_count);
+}
+
 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..83de0907f32c 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -255,6 +255,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
 	xa_destroy(&vepc->page_array);
 	kfree(vepc);
 
+	sgx_dec_usage_count();
 	return 0;
 }
 
@@ -262,6 +263,7 @@ static int sgx_vepc_open(struct inode *inode, struct file *file)
 {
 	struct sgx_vepc *vepc;
 
+	sgx_inc_usage_count();
 	vepc = kzalloc(sizeof(struct sgx_vepc), GFP_KERNEL);
 	if (!vepc)
 		return -ENOMEM;
-- 
2.45.2

On Mon, May 19, 2025 at 10:24:27AM +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 |  1 +
>  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   |  2 ++
>  5 files changed, 21 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> index 7f8d1e11dbee..b5ffe104af4c 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file *file)
>  	struct sgx_encl *encl;
>  	int ret;
>  
> +	sgx_inc_usage_count();
>  	encl = kzalloc(sizeof(*encl), GFP_KERNEL);
>  	if (!encl)
>  		return -ENOMEM;
> 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 8ce352fc72ac..80d565e6f2ad 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -914,6 +914,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;
> +}
> +
> +void sgx_dec_usage_count(void)
> +{
> +	atomic64_dec(&sgx_usage_count);
> +}
> +
>  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..83de0907f32c 100644
> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -255,6 +255,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
>  	xa_destroy(&vepc->page_array);
>  	kfree(vepc);
>  
> +	sgx_dec_usage_count();
>  	return 0;
>  }
>  
> @@ -262,6 +263,7 @@ static int sgx_vepc_open(struct inode *inode, struct file *file)
>  {
>  	struct sgx_vepc *vepc;
>  
> +	sgx_inc_usage_count();
>  	vepc = kzalloc(sizeof(struct sgx_vepc), GFP_KERNEL);
>  	if (!vepc)
>  		return -ENOMEM;
> -- 
> 2.45.2
> 

Maybe just use raw atomic_inc() and atomic_dec() at the sites?

IMHO, it makes only sense to wrap, when it makes sense to wrap.

BR, Jarkko

> Maybe just use raw atomic_inc() and atomic_dec() at the sites?
> 
> IMHO, it makes only sense to wrap, when it makes sense to wrap.

You mean for this patch or overall? For overall we discussed in v4
why we would like to raise it to atomic64.
Or do I misunderstand your comment?

Best Regards,
Elena.

On Tue, May 20, 2025 at 06:25:04AM +0000, Reshetova, Elena wrote:
> > Maybe just use raw atomic_inc() and atomic_dec() at the sites?
> > 
> > IMHO, it makes only sense to wrap, when it makes sense to wrap.
> 
> You mean for this patch or overall? For overall we discussed in v4
> why we would like to raise it to atomic64.
> Or do I misunderstand your comment?

So I was thinking whether we want wrappers but I guess we can keep
them.

> 
> Best Regards,
> Elena.

BR, Jarkko

On Mon, 2025-05-19 at 10:24 +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 |  1 +
>  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   |  2 ++
>  5 files changed, 21 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> index 7f8d1e11dbee..b5ffe104af4c 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file *file)
>  	struct sgx_encl *encl;
>  	int ret;
>  
> +	sgx_inc_usage_count();

This should be done at the end of sgx_open() where the open cannot fail, since
sgx_release() is not called if sgx_open() failed AFAICT.

>  	encl = kzalloc(sizeof(*encl), GFP_KERNEL);
>  	if (!encl)
>  		return -ENOMEM;
> 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 8ce352fc72ac..80d565e6f2ad 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -914,6 +914,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;
> +}

What's the purpose of the returning int?  Seems no caller uses the return value.

> +
> +void sgx_dec_usage_count(void)
> +{
> +	atomic64_dec(&sgx_usage_count);
> +}
> +
>  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..83de0907f32c 100644
> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -255,6 +255,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
>  	xa_destroy(&vepc->page_array);
>  	kfree(vepc);
>  
> +	sgx_dec_usage_count();
>  	return 0;
>  }
>  
> @@ -262,6 +263,7 @@ static int sgx_vepc_open(struct inode *inode, struct file *file)
>  {
>  	struct sgx_vepc *vepc;
>  
> +	sgx_inc_usage_count();

Ditto to sgx_open().

>  	vepc = kzalloc(sizeof(struct sgx_vepc), GFP_KERNEL);
>  	if (!vepc)
>  		return -ENOMEM;

> On Mon, 2025-05-19 at 10:24 +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 |  1 +
> >  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   |  2 ++
> >  5 files changed, 21 insertions(+)
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> b/arch/x86/kernel/cpu/sgx/driver.c
> > index 7f8d1e11dbee..b5ffe104af4c 100644
> > --- a/arch/x86/kernel/cpu/sgx/driver.c
> > +++ b/arch/x86/kernel/cpu/sgx/driver.c
> > @@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file *file)
> >  	struct sgx_encl *encl;
> >  	int ret;
> >
> > +	sgx_inc_usage_count();
> 
> This should be done at the end of sgx_open() where the open cannot fail,
> since
> sgx_release() is not called if sgx_open() failed AFAICT.


Could you please elaborate a bit more on this? 
In case sgx_inc_usage_count fails, we dont allocate resources yet, so what is 
wrong? 

Best Regards,
Elena.

On Mon, 2025-05-19 at 11:47 +0000, Reshetova, Elena wrote:
> > On Mon, 2025-05-19 at 10:24 +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 |  1 +
> > >  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   |  2 ++
> > >  5 files changed, 21 insertions(+)
> > > 
> > > diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> > b/arch/x86/kernel/cpu/sgx/driver.c
> > > index 7f8d1e11dbee..b5ffe104af4c 100644
> > > --- a/arch/x86/kernel/cpu/sgx/driver.c
> > > +++ b/arch/x86/kernel/cpu/sgx/driver.c
> > > @@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file *file)
> > >  	struct sgx_encl *encl;
> > >  	int ret;
> > > 
> > > +	sgx_inc_usage_count();
> > 
> > This should be done at the end of sgx_open() where the open cannot fail,
> > since
> > sgx_release() is not called if sgx_open() failed AFAICT.
> 
> 
> Could you please elaborate a bit more on this? 
> In case sgx_inc_usage_count fails, we dont allocate resources yet, so what is 
> wrong? 
> 

I haven't looked into (details of) the last patch yet, but for _this_ patch
only, doing sgx_inc_usage_count() at the beginning of sgx_open() will result in
the usage count being increased even when sgx_open() fails at a later time. 
Since sgx_release() is not called when sgx_open() fails, the usage count will
not be decreased correctly.

No?

> > > > diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> > > b/arch/x86/kernel/cpu/sgx/driver.c
> > > > index 7f8d1e11dbee..b5ffe104af4c 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/driver.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/driver.c
> > > > @@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file
> *file)
> > > >  	struct sgx_encl *encl;
> > > >  	int ret;
> > > >
> > > > +	sgx_inc_usage_count();
> > >
> > > This should be done at the end of sgx_open() where the open cannot fail,
> > > since
> > > sgx_release() is not called if sgx_open() failed AFAICT.
> >
> >
> > Could you please elaborate a bit more on this?
> > In case sgx_inc_usage_count fails, we dont allocate resources yet, so what is
> > wrong?
> >
> 
> I haven't looked into (details of) the last patch yet, but for _this_ patch
> only, doing sgx_inc_usage_count() at the beginning of sgx_open() will result in
> the usage count being increased even when sgx_open() fails at a later time.
> Since sgx_release() is not called when sgx_open() fails, the usage count will
> not be decreased correctly.
> 
> No?

Yes, I already yesterday understood that you meant *this* patch and
yes, you are correct in this patch it must be moved to the end that we
don't increment in case of failures. 

On Mon, May 19, 2025 at 11:47:32AM +0000, Reshetova, Elena wrote:
> > On Mon, 2025-05-19 at 10:24 +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 |  1 +
> > >  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   |  2 ++
> > >  5 files changed, 21 insertions(+)
> > >
> > > diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> > b/arch/x86/kernel/cpu/sgx/driver.c
> > > index 7f8d1e11dbee..b5ffe104af4c 100644
> > > --- a/arch/x86/kernel/cpu/sgx/driver.c
> > > +++ b/arch/x86/kernel/cpu/sgx/driver.c
> > > @@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file *file)
> > >  	struct sgx_encl *encl;
> > >  	int ret;
> > >
> > > +	sgx_inc_usage_count();
> > 
> > This should be done at the end of sgx_open() where the open cannot fail,
> > since
> > sgx_release() is not called if sgx_open() failed AFAICT.
> 
> 
> Could you please elaborate a bit more on this? 
> In case sgx_inc_usage_count fails, we dont allocate resources yet, so what is 
> wrong? 

It's fine to do (or even perhaps advicable) to do right at the get go,
before doing anything else, because it is "keep alive counter". I.e.,
I think the call is at the right place.

And now there is a proper rollback procedure, as my previous review
comments have been adressed.

> 
> Best Regards,
> Elena.


BR, Jarkko

On Mon, 2025-05-19 at 10:47 +0000, Huang, Kai wrote:
> > +/* 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;
> > +}
> 
> What's the purpose of the returning int?  Seems no caller uses the return value.

I saw your last patch uses the returning value.  Perhaps add a sentence to
mention in the changelog, otherwise it's kinda out of blue.

> On Mon, 2025-05-19 at 10:47 +0000, Huang, Kai wrote:
> > > +/* 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;
> > > +}
> >
> > What's the purpose of the returning int?  Seems no caller uses the return
> value.
> 
> I saw your last patch uses the returning value.  Perhaps add a sentence to
> mention in the changelog, otherwise it's kinda out of blue.

Yes, I can add a comment. I didn’t want to change the function definition.