[PATCH 0/5] tee: shared memory cleanup

[PATCH 0/5] tee: shared memory cleanup

[Jens Wiklander]

Hi,

Here's a group of patches to the tee subsystem with cleanups and not so
urgent fixes related to tee shared memory.

- Unused parts of the shared memory object (struct tee_shm) are removed.
- Shared memory ids usable from user space are not assigned to driver
  private shared memory objects
- The TEE_SHM_USER_MAPPED is used instead of TEE_SHM_REGISTER to accurately
  tell if a shared memory object originates from another user space mapping.

Only unused "features" should be removed with this patch set, there should
be no change in behaviour or breakage with other code.

Thanks,
Jens

Jens Wiklander (5):
  tee: remove linked list of struct tee_shm
  tee: remove unused tee_shm_priv_alloc()
  tee: don't assign shm id for private shms
  tee: remove redundant teedev in struct tee_shm
  tee: tee_shm_op_mmap(): use TEE_SHM_USER_MAPPED

 drivers/tee/tee_core.c    |  1 -
 drivers/tee/tee_private.h |  3 +-
 drivers/tee/tee_shm.c     | 85 +++++++++++----------------------------
 include/linux/tee_drv.h   | 19 +--------
 4 files changed, 27 insertions(+), 81 deletions(-)

-- 
2.17.1

[PATCH 1/5] tee: remove linked list of struct tee_shm

[Jens Wiklander]

Removes list_shm from struct tee_context since the linked list isn't used
any longer.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_core.c  |  1 -
 drivers/tee/tee_shm.c   | 12 +-----------
 include/linux/tee_drv.h |  3 ---
 3 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 37d22e39fd8d..6aec502c495c 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -44,7 +44,6 @@ static struct tee_context *teedev_open(struct tee_device *teedev)
 
 	kref_init(&ctx->refcount);
 	ctx->teedev = teedev;
-	INIT_LIST_HEAD(&ctx->list_shm);
 	rc = teedev->desc->ops->open(ctx);
 	if (rc)
 		goto err;
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 09ddcd06c715..6cabb834db7d 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -17,8 +17,6 @@ static void tee_shm_release(struct tee_shm *shm)
 
 	mutex_lock(&teedev->mutex);
 	idr_remove(&teedev->idr, shm->id);
-	if (shm->ctx)
-		list_del(&shm->link);
 	mutex_unlock(&teedev->mutex);
 
 	if (shm->flags & TEE_SHM_POOL) {
@@ -174,12 +172,8 @@ static struct tee_shm *__tee_shm_alloc(struct tee_context *ctx,
 		}
 	}
 
-	if (ctx) {
+	if (ctx)
 		teedev_ctx_get(ctx);
-		mutex_lock(&teedev->mutex);
-		list_add_tail(&shm->link, &ctx->list_shm);
-		mutex_unlock(&teedev->mutex);
-	}
 
 	return shm;
 err_rem:
@@ -307,10 +301,6 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
 		}
 	}
 
-	mutex_lock(&teedev->mutex);
-	list_add_tail(&shm->link, &ctx->list_shm);
-	mutex_unlock(&teedev->mutex);
-
 	return shm;
 err:
 	if (shm) {
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 7a03f68fb982..cbddb883a7f8 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -49,7 +49,6 @@ struct tee_shm_pool;
  */
 struct tee_context {
 	struct tee_device *teedev;
-	struct list_head list_shm;
 	void *data;
 	struct kref refcount;
 	bool releasing;
@@ -170,7 +169,6 @@ void tee_device_unregister(struct tee_device *teedev);
  * struct tee_shm - shared memory object
  * @teedev:	device used to allocate the object
  * @ctx:	context using the object, if NULL the context is gone
- * @link	link element
  * @paddr:	physical address of the shared memory
  * @kaddr:	virtual address of the shared memory
  * @size:	size of shared memory
@@ -187,7 +185,6 @@ void tee_device_unregister(struct tee_device *teedev);
 struct tee_shm {
 	struct tee_device *teedev;
 	struct tee_context *ctx;
-	struct list_head link;
 	phys_addr_t paddr;
 	void *kaddr;
 	size_t size;
-- 
2.17.1

[PATCH 2/5] tee: remove unused tee_shm_priv_alloc()

[Jens Wiklander]

tee_shm_priv_alloc() isn't useful in the current state and it's also not
not used so remove it.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_shm.c   | 33 ++-------------------------------
 include/linux/tee_drv.h | 12 ------------
 2 files changed, 2 insertions(+), 43 deletions(-)

diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 6cabb834db7d..8afe08b23242 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -95,20 +95,14 @@ static const struct dma_buf_ops tee_shm_dma_buf_ops = {
 	.mmap = tee_shm_op_mmap,
 };
 
-static struct tee_shm *__tee_shm_alloc(struct tee_context *ctx,
-				       struct tee_device *teedev,
-				       size_t size, u32 flags)
+struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
 {
+	struct tee_device *teedev = ctx->teedev;
 	struct tee_shm_pool_mgr *poolm = NULL;
 	struct tee_shm *shm;
 	void *ret;
 	int rc;
 
-	if (ctx && ctx->teedev != teedev) {
-		dev_err(teedev->dev.parent, "ctx and teedev mismatch\n");
-		return ERR_PTR(-EINVAL);
-	}
-
 	if (!(flags & TEE_SHM_MAPPED)) {
 		dev_err(teedev->dev.parent,
 			"only mapped allocations supported\n");
@@ -188,31 +182,8 @@ static struct tee_shm *__tee_shm_alloc(struct tee_context *ctx,
 	tee_device_put(teedev);
 	return ret;
 }
-
-/**
- * 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_* in <linux/tee_drv.h>. 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.
- */
-struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
-{
-	return __tee_shm_alloc(ctx, ctx->teedev, size, flags);
-}
 EXPORT_SYMBOL_GPL(tee_shm_alloc);
 
-struct tee_shm *tee_shm_priv_alloc(struct tee_device *teedev, size_t size)
-{
-	return __tee_shm_alloc(NULL, teedev, size, TEE_SHM_MAPPED);
-}
-EXPORT_SYMBOL_GPL(tee_shm_priv_alloc);
-
 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 cbddb883a7f8..42687f6c546d 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -315,18 +315,6 @@ void *tee_get_drvdata(struct tee_device *teedev);
  */
 struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags);
 
-/**
- * tee_shm_priv_alloc() - Allocate shared memory privately
- * @dev:	Device that allocates the shared memory
- * @size:	Requested size of shared memory
- *
- * Allocates shared memory buffer that is not associated with any client
- * context. Such buffers are owned by TEE driver and used for internal calls.
- *
- * @returns a pointer to 'struct tee_shm'
- */
-struct tee_shm *tee_shm_priv_alloc(struct tee_device *teedev, size_t size);
-
 /**
  * tee_shm_register() - Register shared memory buffer
  * @ctx:	Context that registers the shared memory
-- 
2.17.1

[PATCH 3/5] tee: don't assign shm id for private shms

[Jens Wiklander]

Private shared memory object must not be referenced from user space. To
guarantee that, don't assign an id to shared memory objects which are
driver private.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_private.h |  3 ++-
 drivers/tee/tee_shm.c     | 31 ++++++++++++++++++-------------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h
index f797171f0434..e55204df31ce 100644
--- a/drivers/tee/tee_private.h
+++ b/drivers/tee/tee_private.h
@@ -37,7 +37,8 @@ struct tee_shm_pool {
  * @num_users:	number of active users of this device
  * @c_no_user:	completion used when unregistering the device
  * @mutex:	mutex protecting @num_users and @idr
- * @idr:	register of shared memory object allocated on this device
+ * @idr:	register of user space shared memory objects allocated or
+ *		registered on this device
  * @pool:	shared memory pool
  */
 struct tee_device {
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 8afe08b23242..e636cf82acdb 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -15,9 +15,11 @@ static void tee_shm_release(struct tee_shm *shm)
 {
 	struct tee_device *teedev = shm->teedev;
 
-	mutex_lock(&teedev->mutex);
-	idr_remove(&teedev->idr, shm->id);
-	mutex_unlock(&teedev->mutex);
+	if (shm->flags & TEE_SHM_DMA_BUF) {
+		mutex_lock(&teedev->mutex);
+		idr_remove(&teedev->idr, shm->id);
+		mutex_unlock(&teedev->mutex);
+	}
 
 	if (shm->flags & TEE_SHM_POOL) {
 		struct tee_shm_pool_mgr *poolm;
@@ -143,17 +145,18 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
 		goto err_kfree;
 	}
 
-	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;
-	}
 
 	if (flags & TEE_SHM_DMA_BUF) {
 		DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
 
+		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;
+		}
+
 		exp_info.ops = &tee_shm_dma_buf_ops;
 		exp_info.size = shm->size;
 		exp_info.flags = O_RDWR;
@@ -171,9 +174,11 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
 
 	return shm;
 err_rem:
-	mutex_lock(&teedev->mutex);
-	idr_remove(&teedev->idr, shm->id);
-	mutex_unlock(&teedev->mutex);
+	if (flags & TEE_SHM_DMA_BUF) {
+		mutex_lock(&teedev->mutex);
+		idr_remove(&teedev->idr, shm->id);
+		mutex_unlock(&teedev->mutex);
+	}
 err_pool_free:
 	poolm->ops->free(poolm, shm);
 err_kfree:
-- 
2.17.1

[PATCH 4/5] tee: remove redundant teedev in struct tee_shm

[Jens Wiklander]

The ctx element in struct tee_shm is always valid. So remove the now
redundant teedev element.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_shm.c   | 7 ++-----
 include/linux/tee_drv.h | 4 +---
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index e636cf82acdb..c64ec5c5e464 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -13,7 +13,7 @@
 
 static void tee_shm_release(struct tee_shm *shm)
 {
-	struct tee_device *teedev = shm->teedev;
+	struct tee_device *teedev = shm->ctx->teedev;
 
 	if (shm->flags & TEE_SHM_DMA_BUF) {
 		mutex_lock(&teedev->mutex);
@@ -44,8 +44,7 @@ static void tee_shm_release(struct tee_shm *shm)
 		kfree(shm->pages);
 	}
 
-	if (shm->ctx)
-		teedev_ctx_put(shm->ctx);
+	teedev_ctx_put(shm->ctx);
 
 	kfree(shm);
 
@@ -132,7 +131,6 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
 	}
 
 	shm->flags = flags | TEE_SHM_POOL;
-	shm->teedev = teedev;
 	shm->ctx = ctx;
 	if (flags & TEE_SHM_DMA_BUF)
 		poolm = teedev->pool->dma_buf_mgr;
@@ -221,7 +219,6 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
 	}
 
 	shm->flags = flags | TEE_SHM_REGISTER;
-	shm->teedev = teedev;
 	shm->ctx = ctx;
 	shm->id = -1;
 	addr = untagged_addr(addr);
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 42687f6c546d..1412e9cc79ce 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -167,8 +167,7 @@ void tee_device_unregister(struct tee_device *teedev);
 
 /**
  * struct tee_shm - shared memory object
- * @teedev:	device used to allocate the object
- * @ctx:	context using the object, if NULL the context is gone
+ * @ctx:	context using the object
  * @paddr:	physical address of the shared memory
  * @kaddr:	virtual address of the shared memory
  * @size:	size of shared memory
@@ -183,7 +182,6 @@ void tee_device_unregister(struct tee_device *teedev);
  * subsystem and from drivers that implements their own shm pool manager.
  */
 struct tee_shm {
-	struct tee_device *teedev;
 	struct tee_context *ctx;
 	phys_addr_t paddr;
 	void *kaddr;
-- 
2.17.1

[PATCH 5/5] tee: tee_shm_op_mmap(): use TEE_SHM_USER_MAPPED

[Jens Wiklander]

tee_shm_op_mmap() uses the TEE_SHM_USER_MAPPED flag instead of the
TEE_SHM_REGISTER flag to tell if a shared memory object is originating
from registered user space memory.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_shm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index c64ec5c5e464..deb22f877881 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -81,7 +81,7 @@ static int tee_shm_op_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
 	size_t size = vma->vm_end - vma->vm_start;
 
 	/* Refuse sharing shared memory provided by application */
-	if (shm->flags & TEE_SHM_REGISTER)
+	if (shm->flags & TEE_SHM_USER_MAPPED)
 		return -EINVAL;
 
 	return remap_pfn_range(vma, vma->vm_start, shm->paddr >> PAGE_SHIFT,
-- 
2.17.1

Re: [PATCH 0/5] tee: shared memory cleanup

[Jens Wiklander]

Hi,

On Thu, Jan 9, 2020 at 1:37 PM Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi,
>
> Here's a group of patches to the tee subsystem with cleanups and not so
> urgent fixes related to tee shared memory.
>
> - Unused parts of the shared memory object (struct tee_shm) are removed.
> - Shared memory ids usable from user space are not assigned to driver
>   private shared memory objects
> - The TEE_SHM_USER_MAPPED is used instead of TEE_SHM_REGISTER to accurately
>   tell if a shared memory object originates from another user space mapping.
>
> Only unused "features" should be removed with this patch set, there should
> be no change in behaviour or breakage with other code.

I'll pick up these as they are in a few days if there's no further comments.

Thanks,
Jens

>
> Thanks,
> Jens
>
> Jens Wiklander (5):
>   tee: remove linked list of struct tee_shm
>   tee: remove unused tee_shm_priv_alloc()
>   tee: don't assign shm id for private shms
>   tee: remove redundant teedev in struct tee_shm
>   tee: tee_shm_op_mmap(): use TEE_SHM_USER_MAPPED
>
>  drivers/tee/tee_core.c    |  1 -
>  drivers/tee/tee_private.h |  3 +-
>  drivers/tee/tee_shm.c     | 85 +++++++++++----------------------------
>  include/linux/tee_drv.h   | 19 +--------
>  4 files changed, 27 insertions(+), 81 deletions(-)
>
> --
> 2.17.1
>