Messages in this thread | | | Subject | Re: [PATCH v41 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE | From | Dave Hansen <> | Date | Mon, 16 Nov 2020 09:54:47 -0800 |
| |
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.
| |