On Fri, Apr 25, 2025, Dave Hansen wrote:
> On 4/25/25 12:29, Sean Christopherson wrote:
> > --- 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;
> > }
>
> ->release() is not close(). Userspace doesn't have control over when
> release() gets called, so it's a poor thing to say: "wait until all SGX
> struct files have been released, then do EUPDATESVN". At least that's
> what folks have always told me when I went poking around the VFS.
>
> That alone would make this a non-starter.
And what frees all the EPC pages? Hint: it's starts with an 'r'.
Userspace is going to be waiting on ->release() no matter what. There's no
getting
around that, all enclaves and virtual EPC instances must be fully destroyed to
ensure the EPC is empty.
At least with this approach, userspace can know that the EPC is "busy", whereas
with automatic updates, userspace has zero visibility. The only breadcrumb it
would get is the SVN, which presumably can only be retrieved from within an
encave.
But even if userspace can easily read the SVN, unless userspace has a priori
knowledge of the SVN it expects, userspace has no way of knowing if the SVN
isn't
changing because no update was required, or if the SVN isn't changing because
the kernel can't find a window where there's no active EPC page.
Exposing -EBUSY to userspace gives it a variety of options. E.g. retry N times,
with M delay, and then force a reboot if all else fails.
If we wanted to be more explicit, it would be trivial to add a getter, but IMO
that's completely unnecessary, as it's fully redundant with the errno from the
setter.
E.g.
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index e8fcf032e842..4dc95d59c11f 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -958,14 +958,26 @@ static int sgx_update_svn(const char *buffer, const
struct kernel_param *kp)
}
}
+static int sgx_can_update_svn(char *buffer, const struct kernel_param *kp)
+{
+ if (!sgx_has_eupdatesvn)
+ return sysfs_emit(buffer, "unsupported\n");
+
+ if (atomic_read(&sgx_usage_count))
+ return sysfs_emit(buffer, "busy\n");
+
+ return sysfs_emit(buffer, "ready\n");
+}
+
static const struct kernel_param_ops sgx_update_svn_ops = {
.set = sgx_update_svn,
+ .get = sgx_can_update_svn,
};
#undef MODULE_PARAM_PREFIX
#define MODULE_PARAM_PREFIX "sgx."
-device_param_cb(update_svn, &sgx_update_svn_ops, NULL, 0200);
-__MODULE_PARM_TYPE(update_svn, "bool");
+device_param_cb(update_svn, &sgx_update_svn_ops, NULL, 0644);
+__MODULE_PARM_TYPE(update_svn, "int");
static int __init sgx_init(void)
{
