Re: [intel-sgx-kernel-dev] [PATCH RFC v3 07/12] intel_sgx: driver for Intel Software Guard Extensions

From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: intel-sgx-kernel-dev@lists.01.org, platform-driver-x86@vger.kernel.org
Subject: Re: [intel-sgx-kernel-dev] [PATCH RFC v3 07/12] intel_sgx: driver for Intel Software Guard Extensions
Date: Tue, 10 Oct 2017 11:26:06 -0700	[thread overview]
Message-ID: <20171010182606.GA20986@linux.intel.com> (raw)
In-Reply-To: <20171010143258.21623-8-jarkko.sakkinen@linux.intel.com>

On Tue, Oct 10, 2017 at 05:32:53PM +0300, Jarkko Sakkinen wrote:
> diff --git a/drivers/platform/x86/intel_sgx/sgx_encl.c b/drivers/platform/x86/intel_sgx/sgx_encl.c
> new file mode 100644
> index 000000000000..aa0deed08cee
> --- /dev/null
> +++ b/drivers/platform/x86/intel_sgx/sgx_encl.c
> @@ -0,0 +1,989 @@
>
> +/**
> + * sgx_encl_find - find an enclave
> + * @mm:		mm struct of the current process
> + * @addr:	address in the ELRANGE
> + * @created	is the enclave already created?
> + * @vma:	the resulting VMA
> + *
> + * Finds an enclave identified by the given address. Gives back the VMA, that is
> + * part of the enclave, located in that address.
> + *
> + * Return:
> + * 0 on success,
> + * -EINVAL if not found,
> + */
> +int sgx_encl_find(struct mm_struct *mm, unsigned long addr, bool created,
> +		  struct vm_area_struct **vma)
> +{
> +	struct vm_area_struct *result;
> +	struct sgx_encl *encl;
> +
> +	result = find_vma(mm, addr);
> +	if (!result || result->vm_ops != &sgx_vm_ops || addr < result->vm_start)
> +		return -EINVAL;
> +
> +	encl = result->vm_private_data;
> +	if (created) {
> +		if (!encl)
> +			return -EINVAL;
> +	} else {
> +		if (encl)
> +			return -EINVAL;
> +	}

What about removing @created and returning -ENOENT (or -ENXIO?) if 
result->vm_private_data is NULL?  Removing @created will eliminate
any potential confusion for the common case of @created=true.  For
@created=false, which should be limited to sgx_encl_create, I think
that explicitly checking for "ret != -ENOENT" is more intuitive
than checking whether or not sgx_encl_find succeeded, e.g. I knew
the intent of the check in sgx_encl_create ahead of time and I still
had to walk through sgx_encl_find to verify the behavior.

> +
> +	*vma = result;
> +	return 0;
> +}
> +