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

[Sean Christopherson]

On Mon, Oct 19, 2020 at 01:21:09PM -0700, Dave Hansen wrote:
> On 10/17/20 9:26 PM, Jarkko Sakkinen wrote:
> >>> +long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> >>> +{
> >>> +	struct sgx_encl *encl = filep->private_data;
> >>> +	int ret, encl_flags;
> >>> +
> >>> +	encl_flags = atomic_fetch_or(SGX_ENCL_IOCTL, &encl->flags);
> >>> +	if (encl_flags & SGX_ENCL_IOCTL)
> >>> +		return -EBUSY;
> >>
> >> Is the SGX_ENCL_IOCTL bit essentially just a lock to single-thread
> >> ioctl()s?  Should we name it as such?
> > 
> > Yes. It makes the concurrency overally easier if we can assume that
> > only a single ioctl is in progress. There is no good reason to do
> > them in parallel.
> > 
> > E.g. when you add pages you want to do that serially because the
> > order changes the MRENCLAVE.
> 
> There are also hardware concurrency requirements, right?  A bunch of the
> SGX functions seem not not even support being called in parallel.

Yes, and the driver, even when "holding" SGX_ENCL_IOCTL, takes encl->lock
when executing an ENCLS leaf.  The separate IOCTL flag avoids complications
with reclaim, specifically it allows the ioctls to initiate reclaim without
hitting a deadlock.

Reclaim needs to take encl->lock, e.g. to do ENCLS[EBLOCK], and reclaim is by
default initiated during allocation if there are no pages available.  I.e. if
an ioctl() simply held encl->lock, it would deadlock in the scenario where it
triggered reclaim on the current enclave.

In other words, the flag is necessary even if it weren't being used a lock
primitive, e.g. it'd still need to exist even if encl->lock were taken to set
and check the flag.  The atomic shenanigans were added as an optimization to
allow reclaim in parallel with the bulk of the ioctl flows, and partly because
using atomic_fetch_or() avoided having to drop encl->lock in an error flow,
i.e. yielded less code.

> > So should I rename it as SGX_ENCL_IOCTL_LOCKED?
> 
> I'd rather not see hand-rolled locking primitives frankly.

IOCTL_IN_PROGRESS would be my vote if we want a more descriptive name.