Re: [PATCH 7/7] tee: replace tee_shm_alloc()

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 jens.wiklander@linaro.org

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(&param->a1, &param->a2, pa); reg_pair_from_64(&param->a4, &param->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