Re: [PATCH v34 11/24] x86/sgx: Add SGX enclave driver

[Sean Christopherson]

Man, I really need to type faster.

On Tue, Jul 07, 2020 at 07:11:51AM +0300, Jarkko Sakkinen wrote:
> On Tue, Jul 07, 2020 at 04:36:17AM +0100, Matthew Wilcox wrote:
> > What's a leaf function?  Is it like a CPU instruction?
> 
> Yeah, the opcode is ENCLS for ring-0 (enclave management and
> construction) and ENCLU for ring-3 (entrance to the enclave etc).
> The leaf function number goes to EAX.

To add to Jarkko's comments, for all intents and purposes they are individual
instructions, e.g. all of their own entries in the SDM, but are buried behind
a single opcode that switches on EAX, e.g. ECREATE is EAX=0,  EADD is EAX=1,
EINIT is EAX=2.  It's purely a way to save opcode space when the extra
overhead is a non-issue, e.g. SMX/TXT's GETSEC does the same shenanigans.

> > > +	atomic_set(&encl->flags, 0);
> > > +	kref_init(&encl->refcount);
> > > +	INIT_RADIX_TREE(&encl->page_tree, GFP_KERNEL);
> > 
> > Why are you using a radix tree instead of an xarray?
> 
> Because xarray did not exist in 2017 and nobody has pointed out to use
> it. Now I know it exists (yet do not know what it is).

I've followed xarrays a little, but obviously not closely enough to
understand their advantages over radix trees.  At a glance, range-based
iteration alone is probably justification enough to switch.

> > > +int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> > > +		     unsigned long end, unsigned long vm_prot_bits)
> > > +{
> > > +	unsigned long idx, idx_start, idx_end;
> > > +	struct sgx_encl_page *page;
> > > +
> > > +	/*
> > > +	 * Disallow RIE tasks as their VMA permissions might conflict with the
> > > +	 * enclave page permissions.
> > > +	 */
> > > +	if (!!(current->personality & READ_IMPLIES_EXEC))
> > > +		return -EACCES;
> > > +
> > > +	idx_start = PFN_DOWN(start);
> > > +	idx_end = PFN_DOWN(end - 1);
> > > +
> > > +	for (idx = idx_start; idx <= idx_end; ++idx) {
> > > +		mutex_lock(&encl->lock);
> > > +		page = radix_tree_lookup(&encl->page_tree, idx);
> > > +		mutex_unlock(&encl->lock);
> > > +
> > > +		if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
> > > +			return -EACCES;
> > 
> > You should really use an iterator here instead of repeated lookups.
> > xas_for_each() will probably be what you want.
> 
> Thank you for your remarks. I'll look into using xarray for this.

Question for Matthew:

To enforce the "page must be populated" rule, is there a clean way to retrieve
the index of the current entry?  Our entries/pages don't have information
about their index.  Or should we just count the number of entries and check
'em at the end? E.g.

        xas_for_each(...) {
                if (~page->vm_max_prot_bits & vm_prot_bits)
                        return -EACCES;
                nr_entries++;
        }

        if (nr_entries != (end_index - start_index))
                return -EACCES;