firmware: tee_bnxt: Release TEE shm, session, and context during kexec

PATCH Applied Vikas Gupta 4 participants 7 messages Jul 16, 2021 – Jul 20, 2021 1 message not in the archive

Subsystems (2)

MIPS Keys-trusted

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 20210716022332.GC3232@sequoia

Vikas Gupta

author

2021-07-16 02:48 UTC #1 lore raw

Hi Allen/Tyler,
The patch looks good to me.

Thanks,
Vikas

Quoted 81 lines

Quoted 16 lines
From: Allen Pais <[email protected]>
Implement a .shutdown hook that will be called during a kexec operation so that the TEE shared memory, session, and context that were set up during .probe can be properly freed/closed.
Additionally, don't use dma-buf backed shared memory for the fw_shm_pool. dma-buf backed shared memory cannot be reliably freed and unregistered during a kexec operation even when tee_shm_free() is called on the shm from a .shutdown hook. The problem occurs because dma_buf_put() calls fput() which then uses task_work_add(), with the TWA_RESUME parameter, to queue tee_shm_release() to be called before the current task returns to user mode. However, the current task never returns to user mode before the kexec completes so the memory is never freed nor unregistered.
Quoted 6 lines
Use tee_shm_alloc_kernel_buf() to avoid dma-buf backed shared memory allocation so that tee_shm_free() can directly call tee_shm_release(). This will ensure that the shm can be freed and unregistered during a kexec operation.
Fixes 246880958ac9 ("firmware: broadcom: add OP-TEE based BNXT f/w
manager")
Review
Quoted 9 lines
Cc stable(a)vger.kernel.org
Signed-off-by Allen Pais <[email protected]>
Co-developed-by Tyler Hicks <[email protected]>
Signed-off-by Tyler Hicks <[email protected]>
---
 drivers/firmware/broadcom/tee_bnxt_fw.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
(quoted diff) +0 -0
b/drivers/firmware/broadcom/tee_bnxt_fw.c
drivers/firmware/broadcom/tee_bnxt_fw.c static int tee_bnxt_fw_probe(struct device *dev) · line 215
Quoted 10 lines
index ed10da5313e8..a5bf4c3f6dc7 100644
drivers/firmware/broadcom/tee_bnxt_fw.c +1 -2
@@ -212,10 +212,9 @@ static int tee_bnxt_fw_probe(struct device *dev)
 
      pvt_data.dev = dev;
 
-     fw_shm_pool = tee_shm_alloc(pvt_data.ctx, MAX_SHM_MEM_SZ,
-                                 TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+     fw_shm_pool = tee_shm_alloc_kernel_buf(pvt_data.ctx,
MAX_SHM_MEM_SZ);
drivers/firmware/broadcom/tee_bnxt_fw.c static int tee_bnxt_fw_probe(struct device *dev) · line 217
Quoted 3 lines
drivers/firmware/broadcom/tee_bnxt_fw.c +1 -1
      if (IS_ERR(fw_shm_pool)) {
-             dev_err(pvt_data.dev, "tee_shm_alloc failed\n");
+             dev_err(pvt_data.dev, "tee_shm_alloc_kernel_buf
failed\n");
drivers/firmware/broadcom/tee_bnxt_fw.c static struct tee_client_driver tee_bnxt_fw_driver =
Quoted 19 lines
drivers/firmware/broadcom/tee_bnxt_fw.c +8 -0
              err = PTR_ERR(fw_shm_pool);
              goto out_sess;
      }
@@ -242,6 +241,14 @@ static int tee_bnxt_fw_remove(struct device *dev)
      return 0;
 }
 
+static void tee_bnxt_fw_shutdown(struct device *dev)
+{
+     tee_shm_free(pvt_data.fw_shm_pool);
+     tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+     tee_client_close_context(pvt_data.ctx);
+     pvt_data.ctx = NULL;
+}
+
 static const struct tee_client_device_id tee_bnxt_fw_id_table[] = {
      {UUID_INIT(0x6272636D, 0x2019, 0x0716,
                  0x42, 0x43, 0x4D, 0x5F, 0x53, 0x43, 0x48, 0x49)},
@@ -257,6 +264,7 @@ static struct tee_client_driver tee_bnxt_fw_driver =
{
Quoted 10 lines
drivers/firmware/broadcom/tee_bnxt_fw.c +1 -1
              .bus            = &tee_bus_type,
              .probe          = tee_bnxt_fw_probe,
              .remove         = tee_bnxt_fw_remove,
+             .shutdown       = tee_bnxt_fw_shutdown,
      },
 };
 
--
2.25.1
----- End forwarded message -----

Jens Wiklander

2021-07-19 10:49 UTC #2 lore raw

Hi,

Quoted 3 lines · Vikas Gupta

Hi Allen/Tyler,
The patch looks good to me.

Thanks.

Rafal, is it OK if I include this patch together with the rest of the patches in this patch set in a pull request to arm-soc?

Thanks,
Jens

Quoted 80 lines

Thanks,
Vikas
Quoted 76 lines
Quoted 72 lines
From: Allen Pais <[email protected]>
Implement a .shutdown hook that will be called during a kexec operation so that the TEE shared memory, session, and context that were set up during .probe can be properly freed/closed.
Additionally, don't use dma-buf backed shared memory for the fw_shm_pool. dma-buf backed shared memory cannot be reliably freed and unregistered during a kexec operation even when tee_shm_free() is called on the shm from a .shutdown hook. The problem occurs because dma_buf_put() calls fput() which then uses task_work_add(), with the TWA_RESUME parameter, to queue tee_shm_release() to be called before the current task returns to user mode. However, the current task never returns to user mode before the kexec completes so the memory is never freed nor unregistered.
Use tee_shm_alloc_kernel_buf() to avoid dma-buf backed shared memory allocation so that tee_shm_free() can directly call tee_shm_release(). This will ensure that the shm can be freed and unregistered during a kexec operation.
Fixes 246880958ac9 ("firmware: broadcom: add OP-TEE based BNXT f/w manager")
Cc stable(a)vger.kernel.org
Signed-off-by Allen Pais <[email protected]>
Co-developed-by Tyler Hicks <[email protected]>
Signed-off-by Tyler Hicks <[email protected]>
---
 drivers/firmware/broadcom/tee_bnxt_fw.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/firmware/broadcom/tee_bnxt_fw.c +11 -4
index ed10da5313e8..a5bf4c3f6dc7 100644
@@ -212,10 +212,9 @@ static int tee_bnxt_fw_probe(struct device *dev)
 
      pvt_data.dev = dev;
 
-     fw_shm_pool = tee_shm_alloc(pvt_data.ctx, MAX_SHM_MEM_SZ,
-                                 TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+     fw_shm_pool = tee_shm_alloc_kernel_buf(pvt_data.ctx, MAX_SHM_MEM_SZ);
      if (IS_ERR(fw_shm_pool)) {
-             dev_err(pvt_data.dev, "tee_shm_alloc failed\n");
+             dev_err(pvt_data.dev, "tee_shm_alloc_kernel_buf failed\n");
              err = PTR_ERR(fw_shm_pool);
              goto out_sess;
      }
@@ -242,6 +241,14 @@ static int tee_bnxt_fw_remove(struct device *dev)
      return 0;
 }
 
+static void tee_bnxt_fw_shutdown(struct device *dev)
+{
+     tee_shm_free(pvt_data.fw_shm_pool);
+     tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+     tee_client_close_context(pvt_data.ctx);
+     pvt_data.ctx = NULL;
+}
+
 static const struct tee_client_device_id tee_bnxt_fw_id_table[] = {
      {UUID_INIT(0x6272636D, 0x2019, 0x0716,
                  0x42, 0x43, 0x4D, 0x5F, 0x53, 0x43, 0x48, 0x49)},
@@ -257,6 +264,7 @@ static struct tee_client_driver tee_bnxt_fw_driver = {
              .bus            = &tee_bus_type,
              .probe          = tee_bnxt_fw_probe,
              .remove         = tee_bnxt_fw_remove,
+             .shutdown       = tee_bnxt_fw_shutdown,
      },
 };
 
--
Quoted 2 lines
2.25.1
----- End forwarded message -----

Florian Fainelli

2021-07-20 02:32 UTC #3 lore raw

Quoted 11 lines · Jens Wiklander

Hi,
Quoted 3 lines · Vikas Gupta
Hi Allen/Tyler,
The patch looks good to me.
Thanks.
Rafal, is it OK if I include this patch together with the rest of the patches in this patch set in a pull request to arm-soc?

I can take those patches through the Broadcom ARM SoC pull request, Rafal would that work for you? We seem to have a bit of a maintainer coverage blind spot for that directory.

Signature · 1 line

Florian

Florian Fainelli

Applied

2021-07-20 17:57 UTC #4 lore raw

Quoted 11 lines · Florian Fainelli

Quoted 8 lines · Jens Wiklander
Hi,
Quoted 3 lines · Vikas Gupta
Hi Allen/Tyler,
The patch looks good to me.

Quoted 8 lines

Quoted 4 lines
Thanks.
Rafal, is it OK if I include this patch together with the rest of the patches in this patch set in a pull request to arm-soc?
I can take those patches through the Broadcom ARM SoC pull request, Rafal would that work for you? We seem to have a bit of a maintainer coverage blind spot for that directory.

Applied to drivers/fixes:
https://github.com/Broadcom/stblinux/commit/4ecd797b7e16eb7f1b86fbfd7e4a7887b192535b

Signature · 1 line

Florian

Tyler Hicks

2021-07-20 18:15 UTC #5 lore raw

Quoted 16 lines · Florian Fainelli

Quoted 13 lines · Florian Fainelli
Quoted 10 lines · Jens Wiklander
Hi,
Quoted 3 lines · Vikas Gupta
Hi Allen/Tyler,
The patch looks good to me.
Thanks.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Rafal, is it OK if I include this patch together with the rest of the patches in this patch set in a pull request to arm-soc?
I can take those patches through the Broadcom ARM SoC pull request, Rafal would that work for you? We seem to have a bit of a maintainer coverage blind spot for that directory.
Applied to drivers/fixes: https://github.com/Broadcom/stblinux/commit/4ecd797b7e16eb7f1b86fbfd7e4a7887b192535b

Thanks, Florian, but note that you won't be able to build that branch since the commit uses a new function (tee_shm_alloc_kernel_buf()) that's added earlier in the full series. It seems like it is going to be easier for this to all go through Jens.

Tyler

Quoted 3 lines

Signature · 1 line
Florian

Florian Fainelli

Acked-by

2021-07-20 18:59 UTC #6 lore raw

Quoted 24 lines · Tyler Hicks

Quoted 23 lines · Florian Fainelli
Quoted 19 lines · Florian Fainelli
Quoted 5 lines · Jens Wiklander
Hi,
Quoted 1 line · Vikas Gupta
Quoted 7 lines
Quoted 2 lines
Hi Allen/Tyler,
The patch looks good to me.
Thanks.
Rafal, is it OK if I include this patch together with the rest of the patches in this patch set in a pull request to arm-soc?
I can take those patches through the Broadcom ARM SoC pull request, Rafal would that work for you? We seem to have a bit of a maintainer coverage blind spot for that directory.

Quoted 6 lines

Quoted 1 line
Applied to drivers/fixes: https://github.com/Broadcom/stblinux/commit/4ecd797b7e16eb7f1b86fbfd7e4a7887b192535b
Thanks, Florian, but note that you won't be able to build that branch since the commit uses a new function (tee_shm_alloc_kernel_buf()) that's added earlier in the full series. It seems like it is going to be easier for this to all go through Jens.

I was grepping for the new functions added and could find all references, though it looks like I missed tee_shm_alloc_kernel_buf() somehow, so yes, having Jens merge that series all together would make more sense here. If you need it:

Acked-by Florian Fainelli <[email protected]>

Signature · 1 line

Florian

Vikas Gupta

author

2021-07-16 02:56 UTC #7 lore raw

In reply to a message not in the archive

Hi Tyler/Allen,
The patch looks good to me.

Thanks,
Vikas

Quoted 75 lines

Quoted 71 lines
From: Allen Pais <[email protected]>
Implement a .shutdown hook that will be called during a kexec operation so that the TEE shared memory, session, and context that were set up during .probe can be properly freed/closed.
Additionally, don't use dma-buf backed shared memory for the fw_shm_pool. dma-buf backed shared memory cannot be reliably freed and unregistered during a kexec operation even when tee_shm_free() is called on the shm from a .shutdown hook. The problem occurs because dma_buf_put() calls fput() which then uses task_work_add(), with the TWA_RESUME parameter, to queue tee_shm_release() to be called before the current task returns to user mode. However, the current task never returns to user mode before the kexec completes so the memory is never freed nor unregistered.
Use tee_shm_alloc_kernel_buf() to avoid dma-buf backed shared memory allocation so that tee_shm_free() can directly call tee_shm_release(). This will ensure that the shm can be freed and unregistered during a kexec operation.
Fixes 246880958ac9 ("firmware: broadcom: add OP-TEE based BNXT f/w manager")
Cc stable(a)vger.kernel.org
Signed-off-by Allen Pais <[email protected]>
Co-developed-by Tyler Hicks <[email protected]>
Signed-off-by Tyler Hicks <[email protected]>
---
 drivers/firmware/broadcom/tee_bnxt_fw.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/firmware/broadcom/tee_bnxt_fw.c +11 -4
index ed10da5313e8..a5bf4c3f6dc7 100644
@@ -212,10 +212,9 @@ static int tee_bnxt_fw_probe(struct device *dev)
 
      pvt_data.dev = dev;
 
-     fw_shm_pool = tee_shm_alloc(pvt_data.ctx, MAX_SHM_MEM_SZ,
-                                 TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+     fw_shm_pool = tee_shm_alloc_kernel_buf(pvt_data.ctx, MAX_SHM_MEM_SZ);
      if (IS_ERR(fw_shm_pool)) {
-             dev_err(pvt_data.dev, "tee_shm_alloc failed\n");
+             dev_err(pvt_data.dev, "tee_shm_alloc_kernel_buf failed\n");
              err = PTR_ERR(fw_shm_pool);
              goto out_sess;
      }
@@ -242,6 +241,14 @@ static int tee_bnxt_fw_remove(struct device *dev)
      return 0;
 }
 
+static void tee_bnxt_fw_shutdown(struct device *dev)
+{
+     tee_shm_free(pvt_data.fw_shm_pool);
+     tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+     tee_client_close_context(pvt_data.ctx);
+     pvt_data.ctx = NULL;
+}
+
 static const struct tee_client_device_id tee_bnxt_fw_id_table[] = {
      {UUID_INIT(0x6272636D, 0x2019, 0x0716,
                  0x42, 0x43, 0x4D, 0x5F, 0x53, 0x43, 0x48, 0x49)},
@@ -257,6 +264,7 @@ static struct tee_client_driver tee_bnxt_fw_driver = {
              .bus            = &tee_bus_type,
              .probe          = tee_bnxt_fw_probe,
              .remove         = tee_bnxt_fw_remove,
+             .shutdown       = tee_bnxt_fw_shutdown,
      },
 };
 
--
Quoted 2 lines
2.25.1
----- End forwarded message -----