On Fri, May 15, 2020 at 03:44:00AM +0300, Jarkko Sakkinen wrote:
> +static int sgx_open(struct inode *inode, struct file *file)
> +{
> + struct sgx_encl *encl;
> + int ret;
> +
> + encl = kzalloc(sizeof(*encl), GFP_KERNEL);
> + if (!encl)
> + return -ENOMEM;
> +
> + atomic_set(&encl->flags, 0);
> + kref_init(&encl->refcount);
> + INIT_RADIX_TREE(&encl->page_tree, GFP_KERNEL);
> + mutex_init(&encl->lock);
> + INIT_LIST_HEAD(&encl->mm_list);
> + spin_lock_init(&encl->mm_lock);
> +
> + ret = init_srcu_struct(&encl->srcu);
We're leaking a wee bit of memory here; enough to burn through 14gb in a few
minutes with my newly resurrected EPC cgroup test. The possibility for
failure should have been a dead giveaway that this allocates memory, but the
"init" name threw me off. :-/
> + if (ret) {
> + kfree(encl);
> + return ret;
> + }
> +
> + file->private_data = encl;
> +
> + return 0;
> +}
...
> +/**
> + * sgx_encl_release - Destroy an enclave instance
> + * @kref: address of a kref inside &sgx_encl
> + *
> + * Used together with kref_put(). Frees all the resources associated with the
> + * enclave and the instance itself.
> + */
> +void sgx_encl_release(struct kref *ref)
> +{
> + struct sgx_encl *encl = container_of(ref, struct sgx_encl, refcount);
> +
> + sgx_encl_destroy(encl);
> +
> + if (encl->backing)
> + fput(encl->backing);
The above mem leak can be fixed by adding
cleanup_srcu_struct(&encl->srcu);
> +
> + WARN_ON_ONCE(!list_empty(&encl->mm_list));
> +
> + /* Detect EPC page leak's. */
> + WARN_ON_ONCE(encl->secs_child_cnt);
> + WARN_ON_ONCE(encl->secs.epc_page);
> +
> + kfree(encl);
> +}
...
> +static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
> + unsigned long offset, unsigned long length,
> + struct sgx_secinfo *secinfo, unsigned long flags)
> +{
...
> +err_out:
> + radix_tree_delete(&encl_page->encl->page_tree,
> + PFN_DOWN(encl_page->desc));
> +
> +err_out_unlock:
> + mutex_unlock(&encl->lock);
> + up_read(¤t->mm->mmap_sem);
> +
> +err_out_free:
> + sgx_free_page(epc_page);
> + kfree(encl_page);
> +
> + /*
> + * Destroy enclave on ENCLS failure as this means that EPC has been
> + * invalidated.
> + */
> + if (ret == -EIO)
> + sgx_encl_destroy(encl);
This needs to be called with encl->lock held to prevent racing with the
reclaimer, e.g. sgx_encl_destroy() and sgx_reclaimer_write() can combine to
corrupt secs_child_cnt, among other badness.
It's probably worth adding a lockdep assert in sgx_encl_destroy() as well.
We can either keep the lock across the above frees or retake the lock. I
like retaking the lock to avoid inverting the ordering between encl->lock
and mmap_sem (even though it's benign). This is an extremely rare path,
no need to shave cycles.
> +
> + return ret;
> +}
