On 2021-06-09 10:32:18, Tyler Hicks wrote:
> On 2021-06-09 12:23:24, Jens Wiklander wrote:
> > tee_shm_alloc() is replaced by three new functions,
> >
> > tee_shm_alloc_user_buf() - for user mode allocations, replacing passing
> > the flags TEE_SHM_MAPPED | TEE_SHM_DMA_BUF
> >
> > tee_shm_alloc_kernel_buf() - for kernel mode allocations, slightly
> > optimized compared to using the flags TEE_SHM_MAPPED | TEE_SHM_DMA_BUF
> > since we now can avoid using the dma-buf registration.
> >
> > tee_shm_alloc_anon_kernel_buf() - for TEE driver internal use only
> > allowing decoupling a shared memory object from its original
> > tee_context.
> >
> > This also makes the interface easier to use as we can get rid of the
> > somewhat hard to use flags parameter.
> >
> > The TEE subsystem and the TEE drivers are updated to use the new
> > functions instead.
> >
> > Signed-off-by: Jens Wiklander <[email protected]>
> > ---
> > drivers/tee/optee/call.c | 16 ++--
> > drivers/tee/optee/core.c | 4 +-
> > drivers/tee/optee/device.c | 5 +-
> > drivers/tee/optee/rpc.c | 8 +-
> > drivers/tee/tee_core.c | 2 +-
> > drivers/tee/tee_shm.c | 186 +++++++++++++++++++++++++++----------
> > include/linux/tee_drv.h | 19 +---
> > 7 files changed, 156 insertions(+), 84 deletions(-)
> >
> > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > index 6132cc8d014c..f31257649c0e 100644
> > --- a/drivers/tee/optee/call.c
> > +++ b/drivers/tee/optee/call.c
> > @@ -183,8 +183,8 @@ static struct tee_shm *get_msg_arg(struct tee_context *ctx, size_t num_params,
> > struct tee_shm *shm;
> > struct optee_msg_arg *ma;
> >
> > - shm = tee_shm_alloc(ctx, OPTEE_MSG_GET_ARG_SIZE(num_params),
> > - TEE_SHM_MAPPED);
> > + shm = tee_shm_alloc_anon_kernel_buf(ctx,
> > + OPTEE_MSG_GET_ARG_SIZE(num_params));
>
> The error handling in get_msg_arg() should be updated to call
> tee_shm_free_anon_kernel_buf() instead of tee_shm_free().
>
> > if (IS_ERR(shm))
> > return shm;
> >
> > @@ -281,7 +281,7 @@ int optee_open_session(struct tee_context *ctx,
> > arg->ret_origin = msg_arg->ret_origin;
> > }
> > out:
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> >
> > return rc;
> > }
> > @@ -312,7 +312,7 @@ int optee_close_session(struct tee_context *ctx, u32 session)
> > msg_arg->session = session;
> > optee_do_call_with_arg(ctx, msg_parg);
> >
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > return 0;
> > }
> >
> > @@ -358,7 +358,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
> > arg->ret = msg_arg->ret;
> > arg->ret_origin = msg_arg->ret_origin;
> > out:
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > return rc;
> > }
> >
> > @@ -386,7 +386,7 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
> > msg_arg->cancel_id = cancel_id;
> > optee_do_call_with_arg(ctx, msg_parg);
> >
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > return 0;
> > }
> >
> > @@ -625,7 +625,7 @@ int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm,
> > msg_arg->ret != TEEC_SUCCESS)
> > rc = -EINVAL;
> >
> > - tee_shm_free(shm_arg);
> > + tee_shm_free_anon_kernel_buf(ctx, shm_arg);
> > out:
> > optee_free_pages_list(pages_list, num_pages);
> > return rc;
> > @@ -650,7 +650,7 @@ int optee_shm_unregister(struct tee_context *ctx, struct tee_shm *shm)
> > if (optee_do_call_with_arg(ctx, msg_parg) ||
> > msg_arg->ret != TEEC_SUCCESS)
> > rc = -EINVAL;
> > - tee_shm_free(shm_arg);
> > + tee_shm_free_anon_kernel_buf(ctx, shm_arg);
> > return rc;
> > }
> >
> > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > index 0c287345f9fe..a15dc3881636 100644
> > --- a/drivers/tee/optee/core.c
> > +++ b/drivers/tee/optee/core.c
> > @@ -277,7 +277,7 @@ static void optee_release(struct tee_context *ctx)
> > if (!ctxdata)
> > return;
> >
> > - shm = tee_shm_alloc(ctx, sizeof(struct optee_msg_arg), TEE_SHM_MAPPED);
> > + shm = tee_shm_alloc_anon_kernel_buf(ctx, sizeof(struct optee_msg_arg));
> > if (!IS_ERR(shm)) {
> > arg = tee_shm_get_va(shm, 0);
> > /*
> > @@ -305,7 +305,7 @@ static void optee_release(struct tee_context *ctx)
> > kfree(ctxdata);
> >
> > if (!IS_ERR(shm))
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> >
> > ctx->data = NULL;
> >
> > diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> > index ec1d24693eba..5a5bf86b1b95 100644
> > --- a/drivers/tee/optee/device.c
> > +++ b/drivers/tee/optee/device.c
> > @@ -113,10 +113,9 @@ static int __optee_enumerate_devices(u32 func)
> > if (rc < 0 || !shm_size)
> > goto out_sess;
> >
> > - device_shm = tee_shm_alloc(ctx, shm_size,
> > - TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> > + device_shm = tee_shm_alloc_kernel_buf(ctx, shm_size);
>
> The error handling in the 'err' label needs to use
> tee_shm_free_anon_kernel_buf() instead of tee_shm_free().
Sorry, I got confused here. The error handling in this function should
continue to use tee_shm_free().
To avoid this confusion in the future, perhaps the following macro would
be a nice touch in include/linux/tee_drv.h?
#define tee_shm_free_kernel_buf tee_shm_free
If so, then all of the new users of tee_shm_alloc_kernel_buf() should be
updated to use tee_shm_free_kernel_buf().
Tyler
>
> > if (IS_ERR(device_shm)) {
> > - pr_err("tee_shm_alloc failed\n");
> > + pr_err("tee_shm_alloc_kernel_buf failed\n");
> > rc = PTR_ERR(device_shm);
> > goto out_sess;
> > }
> > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > index 1849180b0278..9108aedb3eee 100644
> > --- a/drivers/tee/optee/rpc.c
> > +++ b/drivers/tee/optee/rpc.c
> > @@ -314,7 +314,7 @@ static void handle_rpc_func_cmd_shm_alloc(struct tee_context *ctx,
> > shm = cmd_alloc_suppl(ctx, sz);
> > break;
> > case OPTEE_RPC_SHM_TYPE_KERNEL:
> > - shm = tee_shm_alloc(ctx, sz, TEE_SHM_MAPPED);
> > + shm = tee_shm_alloc_anon_kernel_buf(ctx, sz);
>
> The error handling in the 'bad' label still uses tee_shm_free(). I guess
> it needs to do something like handle_rpc_func_cmd_shm_free()?
>
> > break;
> > default:
> > arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > @@ -424,7 +424,7 @@ static void handle_rpc_func_cmd_shm_free(struct tee_context *ctx,
> > cmd_free_suppl(ctx, shm);
> > break;
> > case OPTEE_RPC_SHM_TYPE_KERNEL:
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > break;
> > default:
> > arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > @@ -502,7 +502,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
> >
> > switch (OPTEE_SMC_RETURN_GET_RPC_FUNC(param->a0)) {
> > case OPTEE_SMC_RPC_FUNC_ALLOC:
> > - shm = tee_shm_alloc(ctx, param->a1, TEE_SHM_MAPPED);
> > + shm = tee_shm_alloc_anon_kernel_buf(ctx, param->a1);
> > if (!IS_ERR(shm) && !tee_shm_get_pa(shm, 0, &pa)) {
> > reg_pair_from_64(¶m->a1, ¶m->a2, pa);
> > reg_pair_from_64(¶m->a4, ¶m->a5,
> > @@ -516,7 +516,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
> > break;
> > case OPTEE_SMC_RPC_FUNC_FREE:
> > shm = reg_pair_to_ptr(param->a1, param->a2);
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > break;
> > case OPTEE_SMC_RPC_FUNC_FOREIGN_INTR:
> > /*
> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > index 480d294a23ab..4f5c7c17a434 100644
> > --- a/drivers/tee/tee_core.c
> > +++ b/drivers/tee/tee_core.c
> > @@ -293,7 +293,7 @@ static int tee_ioctl_shm_alloc(struct tee_context *ctx,
> > if (data.flags)
> > return -EINVAL;
> >
> > - shm = tee_shm_alloc(ctx, data.size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> > + shm = tee_shm_alloc_user_buf(ctx, data.size);
> > if (IS_ERR(shm))
> > return PTR_ERR(shm);
> >
> > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > index 63fce8d39d8b..d134e2778a3a 100644
> > --- a/drivers/tee/tee_shm.c
> > +++ b/drivers/tee/tee_shm.c
> > @@ -96,25 +96,14 @@ static const struct dma_buf_ops tee_shm_dma_buf_ops = {
> > .mmap = tee_shm_op_mmap,
> > };
> >
> > -struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> > +static struct tee_shm *shm_alloc_helper(struct tee_context *ctx, size_t size,
> > + size_t align, u32 flags)
> > {
> > struct tee_device *teedev = ctx->teedev;
> > struct tee_shm *shm;
> > - size_t align;
> > void *ret;
> > int rc;
> >
> > - if (!(flags & TEE_SHM_MAPPED)) {
> > - dev_err(teedev->dev.parent,
> > - "only mapped allocations supported\n");
> > - return ERR_PTR(-EINVAL);
> > - }
> > -
> > - if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) {
> > - dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags);
> > - return ERR_PTR(-EINVAL);
> > - }
> > -
> > if (!tee_device_get(teedev))
> > return ERR_PTR(-EINVAL);
> >
> > @@ -131,17 +120,14 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> > }
> >
> > shm->flags = flags | TEE_SHM_POOL;
> > +
> > + /*
> > + * We're assigning this as it is needed if the shm is to be
> > + * registered. If this function returns OK then the caller expected
> > + * to call teedev_ctx_get() or clear shm->ctx in case it's not
> > + * needed any longer.
> > + */
> > shm->ctx = ctx;
> > - if (flags & TEE_SHM_DMA_BUF) {
> > - align = PAGE_SIZE;
> > - /*
> > - * Request to register the shm in the pool allocator below
> > - * if supported.
> > - */
> > - shm->flags |= TEE_SHM_REGISTER;
> > - } else {
> > - align = 2 * sizeof(long);
> > - }
> >
> > rc = teedev->pool->ops->alloc(teedev->pool, shm, size, align);
> > if (rc) {
> > @@ -149,48 +135,71 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> > goto err_kfree;
> > }
> >
> > + return shm;
> > +err_kfree:
> > + kfree(shm);
> > +err_dev_put:
> > + tee_device_put(teedev);
> > + return ret;
> > +}
> >
> > - if (flags & TEE_SHM_DMA_BUF) {
> > - DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > +/**
> > + * tee_shm_alloc_user_buf() - Allocate shared memory for user space
> > + * @ctx: Context that allocates the shared memory
> > + * @size: Requested size of shared memory
> > + *
> > + * Memory allocated as user space shared memory is automatically freed when
> > + * the TEE file pointer is closed. The primary usage of this function is
> > + * when the TEE driver doesn't support registering ordinary user space
> > + * memory.
> > + *
> > + * @returns a pointer to 'struct tee_shm'
> > + */
> > +struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size)
> > +{
> > + u32 flags = TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER;
>
> Why not TEE_SHM_USER_MAPPED instead of TEE_SHM_MAPPED here?
>
> > + struct tee_device *teedev = ctx->teedev;
> > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > + struct tee_shm *shm;
> > + void *ret;
> >
> > - mutex_lock(&teedev->mutex);
> > - shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
> > - mutex_unlock(&teedev->mutex);
> > - if (shm->id < 0) {
> > - ret = ERR_PTR(shm->id);
> > - goto err_pool_free;
> > - }
> > + shm = shm_alloc_helper(ctx, size, PAGE_SIZE, flags);
> > + if (IS_ERR(shm))
> > + return shm;
> >
> > - exp_info.ops = &tee_shm_dma_buf_ops;
> > - exp_info.size = shm->size;
> > - exp_info.flags = O_RDWR;
> > - exp_info.priv = shm;
> > + mutex_lock(&teedev->mutex);
> > + shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
> > + mutex_unlock(&teedev->mutex);
> > + if (shm->id < 0) {
> > + ret = ERR_PTR(shm->id);
> > + goto err_pool_free;
> > + }
> >
> > - shm->dmabuf = dma_buf_export(&exp_info);
> > - if (IS_ERR(shm->dmabuf)) {
> > - ret = ERR_CAST(shm->dmabuf);
> > - goto err_rem;
> > - }
> > + exp_info.ops = &tee_shm_dma_buf_ops;
> > + exp_info.size = shm->size;
> > + exp_info.flags = O_RDWR;
> > + exp_info.priv = shm;
> > +
> > + shm->dmabuf = dma_buf_export(&exp_info);
> > + if (IS_ERR(shm->dmabuf)) {
> > + ret = ERR_CAST(shm->dmabuf);
> > + goto err_rem;
> > }
> >
> > teedev_ctx_get(ctx);
> > -
> > return shm;
> > err_rem:
> > - if (flags & TEE_SHM_DMA_BUF) {
> > - mutex_lock(&teedev->mutex);
> > - idr_remove(&teedev->idr, shm->id);
> > - mutex_unlock(&teedev->mutex);
> > - }
> > + mutex_lock(&teedev->mutex);
> > + idr_remove(&teedev->idr, shm->id);
> > + mutex_unlock(&teedev->mutex);
> > err_pool_free:
> > teedev->pool->ops->free(teedev->pool, shm);
> > -err_kfree:
> > kfree(shm);
> > -err_dev_put:
> > tee_device_put(teedev);
> > return ret;
> > +
> > }
> > -EXPORT_SYMBOL_GPL(tee_shm_alloc);
> > +EXPORT_SYMBOL_GPL(tee_shm_alloc_user_buf);
> >
> > /**
> > * tee_shm_alloc_kernel_buf() - Allocate shared memory for kernel buffer
> > @@ -206,10 +215,85 @@ EXPORT_SYMBOL_GPL(tee_shm_alloc);
> > */
> > struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
> > {
> > - return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> > + u32 flags = TEE_SHM_MAPPED | TEE_SHM_REGISTER;
>
> Similar question as above... why not TEE_SHM_KERNEL_MAPPED instead of
> TEE_SHM_MAPPED?
>
> > + struct tee_shm *shm;
> > +
> > + shm = shm_alloc_helper(ctx, size, PAGE_SIZE, flags);
> > + if (IS_ERR(shm))
> > + return shm;
> > +
> > + teedev_ctx_get(ctx);
> > + return shm;
> > }
> > EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
> >
> > +/**
> > + * tee_shm_alloc_anon_kernel_buf() - Allocate shared memory for anonymous
> > + * kernel buffer
> > + * @ctx: Context that allocates the shared memory
> > + * @size: Requested size of shared memory
> > + *
> > + * This function returns similar shared memory as tee_shm_alloc_kernel_buf(),
> > + * but with two differences:
> > + * 1. The memory might not be registered in secure world
> > + * in case the driver supports passing memory not registered in advance.
> > + * 2. The memory is not directly associated with the passed tee_context,
> > + * rather the tee_device used by the context.
> > + *
> > + * This function should normally only be used internally in the TEE
> > + * drivers. The memory must later only be freed using
> > + * tee_shm_free_anon_kernel_buf() with a tee_contex with the same internal
> > + * tee_device as when the memory was allocated.
> > + *
> > + * This allows allocating the shared memory using one context which is
> > + * destroyed while the memory continues to live and finally freed using
> > + * another context.
> > + *
> > + * @returns a pointer to 'struct tee_shm'
> > + */
> > +struct tee_shm *tee_shm_alloc_anon_kernel_buf(struct tee_context *ctx,
> > + size_t size)
> > +{
> > + struct tee_shm *shm;
> > +
> > + shm = shm_alloc_helper(ctx, size, sizeof(long) * 2, TEE_SHM_MAPPED);
>
> The generic TEE_SHM_MAPPED is used here, as well.
>
> Tyler
>
> > + if (IS_ERR(shm))
> > + return shm;
> > +
> > + shm->ctx = NULL;
> > + return shm;
> > +}
> > +EXPORT_SYMBOL_GPL(tee_shm_alloc_anon_kernel_buf);
> > +
> > +/**
> > + * tee_shm_free_anon_kernel_buf() - Free anonymous shared kernel memory
> > + * @ctx: Borrowed context when freeing the shared memory
> > + * @shm: Handle to shared memory to free
> > + *
> > + * This function must only be used to free a tee_shm allocated with
> > + * tee_shm_alloc_anon_kernel_buf(). The passed @ctx has to have the same
> > + * internal tee_device as was used by the tee_context passed when the
> > + * memory was allocated.
> > + */
> > +void tee_shm_free_anon_kernel_buf(struct tee_context *ctx, struct tee_shm *shm)
> > +{
> > + struct tee_device *teedev = ctx->teedev;
> > +
> > + /*
> > + * The anonymous kernel buffer isn't attached to any tee_context
> > + * we're instead assigning the current tee_context temporarily.
> > + * This is needed because an eventual call to unregister the shared
> > + * memory might need a context. As long as this context uses the
> > + * same tee_device as in the ctx in the call in
> > + * tee_shm_alloc_anon_kernel_buf() above we are OK.
> > + */
> > + shm->ctx = ctx;
> > + teedev->pool->ops->free(teedev->pool, shm);
> > + kfree(shm);
> > + tee_device_put(teedev);
> > +}
> > +EXPORT_SYMBOL_GPL(tee_shm_free_anon_kernel_buf);
> > +
> > struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
> > size_t length, u32 flags)
> > {
> > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> > index 58b319766f8e..11a4e556bdf9 100644
> > --- a/include/linux/tee_drv.h
> > +++ b/include/linux/tee_drv.h
> > @@ -267,22 +267,11 @@ static inline void tee_shm_pool_free(struct tee_shm_pool *pool)
> > */
> > void *tee_get_drvdata(struct tee_device *teedev);
> >
> > -/**
> > - * tee_shm_alloc() - Allocate shared memory
> > - * @ctx: Context that allocates the shared memory
> > - * @size: Requested size of shared memory
> > - * @flags: Flags setting properties for the requested shared memory.
> > - *
> > - * Memory allocated as global shared memory is automatically freed when the
> > - * TEE file pointer is closed. The @flags field uses the bits defined by
> > - * TEE_SHM_* above. TEE_SHM_MAPPED must currently always be set. If
> > - * TEE_SHM_DMA_BUF global shared memory will be allocated and associated
> > - * with a dma-buf handle, else driver private memory.
> > - *
> > - * @returns a pointer to 'struct tee_shm'
> > - */
> > -struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags);
> > +struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size);
> > struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
> > +struct tee_shm *tee_shm_alloc_anon_kernel_buf(struct tee_context *ctx,
> > + size_t size);
> > +void tee_shm_free_anon_kernel_buf(struct tee_context *ctx, struct tee_shm *shm);
> >
> > /**
> > * tee_shm_register() - Register shared memory buffer
> > --
> > 2.31.1
> >