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

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 -----

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 -----

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

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 -----

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?

Quoted 8 lines · Jens Wiklander

Hi,

Quoted 3 lines · Vikas Gupta

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

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.

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 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

Signature · 1 line

Florian

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 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.