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;
+}
+
+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..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;
}
static long sgx_vepc_ioctl(struct file *file,
--
2.45.2On 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
> -----Original Message-----
> From: Jarkko Sakkinen <jarkko@kernel.org>
> Sent: Friday, May 23, 2025 6:54 PM
> To: Reshetova, Elena <elena.reshetova@intel.com>
> Cc: Hansen, Dave <dave.hansen@intel.com>; seanjc@google.com; Huang, Kai
> <kai.huang@intel.com>; mingo@kernel.org; linux-sgx@vger.kernel.org; linux-
> kernel@vger.kernel.org; x86@kernel.org; Mallick, Asit K
> <asit.k.mallick@intel.com>; Scarlata, Vincent R
> <vincent.r.scarlata@intel.com>; Cai, Chong <chongc@google.com>; Aktas,
> Erdem <erdemaktas@google.com>; Annapurve, Vishal
> <vannapurve@google.com>; dionnaglaze@google.com;
> bondarn@google.com; Raynor, Scott <scott.raynor@intel.com>
> Subject: Re: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the
> sgx_(vepc_)open()
> > /*
> > 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?
The last patch is cleaner if the prototype is already
returning int here. Also error unwinding takes this into account
right in this patch. Do you have objections to leave it as it is?
Best Regards,
Elena.
>
>
> > > /*
> > > 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?
>
> The last patch is cleaner if the prototype is already
> returning int here. Also error unwinding takes this into account
> right in this patch. Do you have objections to leave it as it is?
>
>
You can clarify this in the changelog of this patch (which I also suggested in
v5).
> -----Original Message-----
> From: Huang, Kai <kai.huang@intel.com>
> Sent: Tuesday, May 27, 2025 2:20 AM
> To: Reshetova, Elena <elena.reshetova@intel.com>; jarkko@kernel.org
> Cc: Raynor, Scott <scott.raynor@intel.com>; Hansen, Dave
> <dave.hansen@intel.com>; mingo@kernel.org; Scarlata, Vincent R
> <vincent.r.scarlata@intel.com>; x86@kernel.org; linux-sgx@vger.kernel.org;
> Annapurve, Vishal <vannapurve@google.com>; linux-kernel@vger.kernel.org;
> Mallick, Asit K <asit.k.mallick@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; Cai, Chong <chongc@google.com>;
> bondarn@google.com; seanjc@google.com; dionnaglaze@google.com
> Subject: Re: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the
> sgx_(vepc_)open()
>
> >
> >
> > > > /*
> > > > 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?
> >
> > The last patch is cleaner if the prototype is already
> > returning int here. Also error unwinding takes this into account
> > right in this patch. Do you have objections to leave it as it is?
> >
> >
>
> You can clarify this in the changelog of this patch (which I also suggested in
> v5).
Sure, will do. The reason I didn’t do it in v6 now is because I actually added
the error handling in case error is returned in this patch, so thought it was
self-explanatory, but as this comment from Jarkko shows I was wrong.
Best Regards,
Elena.
On 5/23/25 08:54, Jarkko Sakkinen wrote:
>> +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.
If they're just used in a single file, make them 'static' and let the
compiler decide whether to inline them or not.
On Fri, May 23, 2025 at 08:58:54AM -0700, Dave Hansen wrote:
> On 5/23/25 08:54, Jarkko Sakkinen wrote:
> >> +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.
>
> If they're just used in a single file, make them 'static' and let the
> compiler decide whether to inline them or not.
This would be totally fine.
BR, Jarkko
> -----Original Message-----
> From: Jarkko Sakkinen <jarkko@kernel.org>
> Sent: Saturday, May 24, 2025 2:46 AM
> To: Hansen, Dave <dave.hansen@intel.com>
> Cc: Reshetova, Elena <elena.reshetova@intel.com>; seanjc@google.com;
> Huang, Kai <kai.huang@intel.com>; mingo@kernel.org; linux-
> sgx@vger.kernel.org; linux-kernel@vger.kernel.org; x86@kernel.org; Mallick,
> Asit K <asit.k.mallick@intel.com>; Scarlata, Vincent R
> <vincent.r.scarlata@intel.com>; Cai, Chong <chongc@google.com>; Aktas,
> Erdem <erdemaktas@google.com>; Annapurve, Vishal
> <vannapurve@google.com>; dionnaglaze@google.com;
> bondarn@google.com; Raynor, Scott <scott.raynor@intel.com>
> Subject: Re: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the
> sgx_(vepc_)open()
>
> On Fri, May 23, 2025 at 08:58:54AM -0700, Dave Hansen wrote:
> > On 5/23/25 08:54, Jarkko Sakkinen wrote:
> > >> +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.
> >
> > If they're just used in a single file, make them 'static' and let the
> > compiler decide whether to inline them or not.
But they are not used in single file. They are used in driver.c, encl.c and virt.c
So just to covert to static enough or?
Best Regards,
Elena.
© 2016 - 2025 Red Hat, Inc.