Re: [PATCH v41 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE

From: Dave Hansen
Date: Mon Nov 16 2020 - 12:54:58 EST


Hillf, I noticed that you removed a bunch of folks from cc, including
me. Was there a reason for that? I haven't been seeing your feedback
on these patches at all.

On 11/14/20 8:40 PM, Hillf Danton wrote:
> On Fri, 13 Nov 2020 00:01:23 +0200 Jarkko Sakkinen wrote:
>> +long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>> +{
>> + struct sgx_encl *encl = filep->private_data;
>> + int ret;
>> +
>> + if (test_and_set_bit(SGX_ENCL_IOCTL, &encl->flags))
>> + return -EBUSY;
> Looks like encl->ioctl_mutex is needed to exlusively serialize
> concurrent ioctl threads and make encl->flags free of the duty.
> Plus it can cut the confusing EBUSY in userspace as it is not
> a critical path in any form.

I actually think the -EBUSY might be a bit too nice. This is a *bad*
condition that userspace shouldn't be hitting.

Sean had a great explanation of this in a private mail:

> Reclaiming (i.e. swapping) pages out of an enclave can be done while an enclave
> is being built. Reclaiming involves ENCLS, which needs to be serialized for a
> given enclave, i.e. reclaiming pages needs to take encl->lock. To help adjust
> to EPC pressure, reclaim is automatically performed when allocating an EPC page,
> i.e. is triggered by ioctls. Holding encl->lock for the entire ioctl() will
> thus deadlock if an enclave reclaims from itself.
>
> There are other ways to solve the deadlock problem, e.g. only reclaim from
> workers and never from process context, but there are other motivations for
> the atomic shenanigans (see below).

> ENCLS instructions must be serialized for a given enclave, but holding
> encl->lock for an entire ioctl() will result in deadlock due to an enclave
> triggering reclaim on itself.
>
> Building an enclave must also be serialized, i.e. userspace can't queue up
> EADD on multiple threads, because the order in which pages are added to an
> enclave affects the measurement. In other words, rejecting the ioctl() as
> opposed to waiting on a lock is also desirable.

Sounds like we need should follow up with an add-on patch to get some of
that into a comment.