v4: - Incorporated 'tee: add tee_shm_alloc_kernel_buf()' from Jens to remove the need to expose TEE_SHM_REGISTER to callers of tee_shm_alloc() - Updated 'tee: Support kernel shm registration without dma-buf backing' to drop the TEE_SHM_DMA_BUF flag when tee_shm_alloc_kernel_buf() calls tee_shm_alloc() - Updated the final two patches, against ftpm and tee_bnxt_fw, to use tee_shm_alloc_kernel_buf() instead of tee_shm_alloc() - Minor cleanups to the commit messages of the updates patches v3: https://lore.kernel.org/lkml/20210609002326.210024-1-tyhicks@linux.microsoft... v2: https://lore.kernel.org/lkml/20210225090610.242623-1-allen.lkml@gmail.com/ v1: https://lore.kernel.org/lkml/20210217092714.121297-1-allen.lkml@gmail.com/
This series fixes several bugs uncovered while exercising the OP-TEE (Open Portable Trusted Execution Environment), ftpm (firmware TPM), and tee_bnxt_fw (Broadcom BNXT firmware manager) drivers with kexec and kdump (emergency kexec) based workflows.
The majority of the problems are caused by missing .shutdown hooks in the drivers. The .shutdown hooks are used by the normal kexec code path to let the drivers clean up prior to executing the target kernel. The .remove hooks, which are already implemented in these drivers, are not called as part of the kexec code path. This resulted in shared memory regions, that were cached and/or registered with OP-TEE, not being cleared/unregistered prior to kexec. The new kernel would then run into problems when handling the previously cached virtual addresses or trying to register newly allocated shared memory objects that overlapped with the previously registered virtual addresses. The TEE didn't receive notification that the old virtual addresses were no longer meaningful and that a new kernel, with a new address space, would soon be running.
However, implementing .shutdown hooks was not enough for supporting kexec. There was an additional problem caused by the TEE driver's reliance on the dma-buf subsystem for multi-page shared memory objects that were registered with the TEE. Shared memory objects backed by a dma-buf use a different mechanism for reference counting. When the final reference is released, work is scheduled to be executed to unregister the shared memory with the TEE but that work is only completed prior to the current task returning the userspace. In the case of a kexec operation, the current task that's calling the driver .shutdown hooks never returns to userspace prior to the kexec operation so the shared memory was never unregistered. This eventually caused problems from overlapping shared memory regions that were registered with the TEE after several kexec operations. The large 4M contiguous region allocated by the tee_bnxt_fw driver reliably ran into this issue on the fourth kexec on a system with 8G of RAM.
The use of dma-buf makes sense for shared memory that's in use by userspace but dma-buf's aren't needed for shared memory that will only used by the driver. This series separates dma-buf backed shared memory allocated by the kernel from multi-page shared memory that the kernel simply needs registered with the TEE for private use.
One other noteworthy change in this series is to completely refuse to load the OP-TEE driver in the kdump kernel. This is needed because the secure world may have had all of its threads in suspended state when the regular kernel crashed. The kdump kernel would then hang during boot because the OP-TEE driver's .probe function would attempt to use a secure world thread when they're all in suspended state. Another problem is that shared memory allocations could fail under the kdump kernel because the previously registered were not unregistered (the .shutdown hook is not called when kexec'ing into the kdump kernel).
The first patch in the series fixes potential memory leaks that are not directly related to kexec or kdump but were noticed during the development of this series.
Tyler
Allen Pais (2): optee: fix tee out of memory failure seen during kexec reboot firmware: tee_bnxt: Release TEE shm, session, and context during kexec
Jens Wiklander (1): tee: add tee_shm_alloc_kernel_buf()
Tyler Hicks (5): optee: Fix memory leak when failing to register shm pages optee: Refuse to load the driver under the kdump kernel optee: Clear stale cache entries during initialization tee: Support kernel shm registration without dma-buf backing tpm_ftpm_tee: Free and unregister TEE shared memory during kexec
drivers/char/tpm/tpm_ftpm_tee.c | 8 ++--- drivers/firmware/broadcom/tee_bnxt_fw.c | 14 +++++++-- drivers/tee/optee/call.c | 11 ++++++- drivers/tee/optee/core.c | 42 ++++++++++++++++++++++++- drivers/tee/optee/optee_private.h | 2 +- drivers/tee/optee/shm_pool.c | 17 +++++++--- drivers/tee/tee_shm.c | 29 ++++++++++++++++- include/linux/tee_drv.h | 1 + 8 files changed, 108 insertions(+), 16 deletions(-)
Free the previously allocated pages when we encounter an error condition while attempting to register the pages with the secure world.
Fixes: a249dd200d03 ("tee: optee: Fix dynamic shm pool allocations") Fixes: 5a769f6ff439 ("optee: Fix multi page dynamic shm pool alloc") Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com --- drivers/tee/optee/shm_pool.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c index d767eebf30bd..da06ce9b9313 100644 --- a/drivers/tee/optee/shm_pool.c +++ b/drivers/tee/optee/shm_pool.c @@ -32,8 +32,10 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, struct page **pages;
pages = kcalloc(nr_pages, sizeof(pages), GFP_KERNEL); - if (!pages) - return -ENOMEM; + if (!pages) { + rc = -ENOMEM; + goto err; + }
for (i = 0; i < nr_pages; i++) { pages[i] = page; @@ -44,8 +46,14 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, rc = optee_shm_register(shm->ctx, shm, pages, nr_pages, (unsigned long)shm->kaddr); kfree(pages); + if (rc) + goto err; }
+ return 0; + +err: + __free_pages(page, order); return rc; }
On Thu, Jun 10, 2021 at 11:09 PM Tyler Hicks tyhicks@linux.microsoft.com wrote:
Free the previously allocated pages when we encounter an error condition while attempting to register the pages with the secure world.
Fixes: a249dd200d03 ("tee: optee: Fix dynamic shm pool allocations") Fixes: 5a769f6ff439 ("optee: Fix multi page dynamic shm pool alloc") Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com
drivers/tee/optee/shm_pool.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
Looks good.
Reviewed-by: Jens Wiklander jens.wiklander@linaro.org
Fix a hung task issue, seen when booting the kdump kernel, that is caused by all of the secure world threads being in a permanent suspended state:
INFO: task swapper/0:1 blocked for more than 120 seconds. Not tainted 5.4.83 #1 "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. swapper/0 D 0 1 0 0x00000028 Call trace: __switch_to+0xc8/0x118 __schedule+0x2e0/0x700 schedule+0x38/0xb8 schedule_timeout+0x258/0x388 wait_for_completion+0x16c/0x4b8 optee_cq_wait_for_completion+0x28/0xa8 optee_disable_shm_cache+0xb8/0xf8 optee_probe+0x560/0x61c platform_drv_probe+0x58/0xa8 really_probe+0xe0/0x338 driver_probe_device+0x5c/0xf0 device_driver_attach+0x74/0x80 __driver_attach+0x64/0xe0 bus_for_each_dev+0x84/0xd8 driver_attach+0x30/0x40 bus_add_driver+0x188/0x1e8 driver_register+0x64/0x110 __platform_driver_register+0x54/0x60 optee_driver_init+0x20/0x28 do_one_initcall+0x54/0x24c kernel_init_freeable+0x1e8/0x2c0 kernel_init+0x18/0x118 ret_from_fork+0x10/0x18
The invoke_fn hook returned OPTEE_SMC_RETURN_ETHREAD_LIMIT, indicating that the secure world threads were all in a suspended state at the time of the kernel crash. This intermittently prevented the kdump kernel from booting, resulting in a failure to collect the kernel dump.
Make kernel dump collection more reliable on systems utilizing OP-TEE by refusing to load the driver under the kdump kernel.
Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com --- drivers/tee/optee/core.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index ddb8f9ecf307..5288cd767d82 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -6,6 +6,7 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/arm-smccc.h> +#include <linux/crash_dump.h> #include <linux/errno.h> #include <linux/io.h> #include <linux/module.h> @@ -612,6 +613,16 @@ static int optee_probe(struct platform_device *pdev) u32 sec_caps; int rc;
+ /* + * The kernel may have crashed at the same time that all available + * secure world threads were suspended and we cannot reschedule the + * suspended threads without access to the crashed kernel's wait_queue. + * Therefore, we cannot reliably initialize the OP-TEE driver in the + * kdump kernel. + */ + if (is_kdump_kernel()) + return -ENODEV; + invoke_fn = get_invoke_func(&pdev->dev); if (IS_ERR(invoke_fn)) return PTR_ERR(invoke_fn);
On Thu, Jun 10, 2021 at 11:09 PM Tyler Hicks tyhicks@linux.microsoft.com wrote:
Fix a hung task issue, seen when booting the kdump kernel, that is caused by all of the secure world threads being in a permanent suspended state:
INFO: task swapper/0:1 blocked for more than 120 seconds. Not tainted 5.4.83 #1 "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. swapper/0 D 0 1 0 0x00000028 Call trace: __switch_to+0xc8/0x118 __schedule+0x2e0/0x700 schedule+0x38/0xb8 schedule_timeout+0x258/0x388 wait_for_completion+0x16c/0x4b8 optee_cq_wait_for_completion+0x28/0xa8 optee_disable_shm_cache+0xb8/0xf8 optee_probe+0x560/0x61c platform_drv_probe+0x58/0xa8 really_probe+0xe0/0x338 driver_probe_device+0x5c/0xf0 device_driver_attach+0x74/0x80 __driver_attach+0x64/0xe0 bus_for_each_dev+0x84/0xd8 driver_attach+0x30/0x40 bus_add_driver+0x188/0x1e8 driver_register+0x64/0x110 __platform_driver_register+0x54/0x60 optee_driver_init+0x20/0x28 do_one_initcall+0x54/0x24c kernel_init_freeable+0x1e8/0x2c0 kernel_init+0x18/0x118 ret_from_fork+0x10/0x18
The invoke_fn hook returned OPTEE_SMC_RETURN_ETHREAD_LIMIT, indicating that the secure world threads were all in a suspended state at the time of the kernel crash. This intermittently prevented the kdump kernel from booting, resulting in a failure to collect the kernel dump.
Make kernel dump collection more reliable on systems utilizing OP-TEE by refusing to load the driver under the kdump kernel.
Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com
drivers/tee/optee/core.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
Looks good Reviewed-by: Jens Wiklander jens.wiklander@linaro.org
From: Allen Pais apais@linux.microsoft.com
The following out of memory errors are seen on kexec reboot from the optee core.
[ 0.368428] tee_bnxt_fw optee-clnt0: tee_shm_alloc failed [ 0.368461] tee_bnxt_fw: probe of optee-clnt0 failed with error -22
tee_shm_release() is not invoked on dma shm buffer.
Implement .shutdown() method to handle the release of the buffers correctly.
More info: https://github.com/OP-TEE/optee_os/issues/3637
Signed-off-by: Allen Pais apais@linux.microsoft.com Reviewed-by: Tyler Hicks tyhicks@linux.microsoft.com --- drivers/tee/optee/core.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 5288cd767d82..0987074d7ed0 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -573,6 +573,13 @@ static optee_invoke_fn *get_invoke_func(struct device *dev) return ERR_PTR(-EINVAL); }
+/* optee_remove - Device Removal Routine + * @pdev: platform device information struct + * + * optee_remove is called by platform subsystem to alert the driver + * that it should release the device + */ + static int optee_remove(struct platform_device *pdev) { struct optee *optee = platform_get_drvdata(pdev); @@ -603,6 +610,18 @@ static int optee_remove(struct platform_device *pdev) return 0; }
+/* optee_shutdown - Device Removal Routine + * @pdev: platform device information struct + * + * platform_shutdown is called by the platform subsystem to alert + * the driver that a shutdown, reboot, or kexec is happening and + * device must be disabled. + */ +static void optee_shutdown(struct platform_device *pdev) +{ + optee_disable_shm_cache(platform_get_drvdata(pdev)); +} + static int optee_probe(struct platform_device *pdev) { optee_invoke_fn *invoke_fn; @@ -739,6 +758,7 @@ MODULE_DEVICE_TABLE(of, optee_dt_match); static struct platform_driver optee_driver = { .probe = optee_probe, .remove = optee_remove, + .shutdown = optee_shutdown, .driver = { .name = "optee", .of_match_table = optee_dt_match,
On Thu, Jun 10, 2021 at 11:09 PM Tyler Hicks tyhicks@linux.microsoft.com wrote:
From: Allen Pais apais@linux.microsoft.com
The following out of memory errors are seen on kexec reboot from the optee core.
[ 0.368428] tee_bnxt_fw optee-clnt0: tee_shm_alloc failed [ 0.368461] tee_bnxt_fw: probe of optee-clnt0 failed with error -22
tee_shm_release() is not invoked on dma shm buffer.
Implement .shutdown() method to handle the release of the buffers correctly.
More info: https://github.com/OP-TEE/optee_os/issues/3637
Signed-off-by: Allen Pais apais@linux.microsoft.com Reviewed-by: Tyler Hicks tyhicks@linux.microsoft.com
Do we really need this considering the patch "optee: Refuse to load the driver under the kdump kernel"?
Jens
On 2021-06-11 11:11:33, Jens Wiklander wrote:
On Thu, Jun 10, 2021 at 11:09 PM Tyler Hicks tyhicks@linux.microsoft.com wrote:
From: Allen Pais apais@linux.microsoft.com
The following out of memory errors are seen on kexec reboot from the optee core.
[ 0.368428] tee_bnxt_fw optee-clnt0: tee_shm_alloc failed [ 0.368461] tee_bnxt_fw: probe of optee-clnt0 failed with error -22
tee_shm_release() is not invoked on dma shm buffer.
Implement .shutdown() method to handle the release of the buffers correctly.
More info: https://github.com/OP-TEE/optee_os/issues/3637
Signed-off-by: Allen Pais apais@linux.microsoft.com Reviewed-by: Tyler Hicks tyhicks@linux.microsoft.com
Do we really need this considering the patch "optee: Refuse to load the driver under the kdump kernel"?
Yes. That patch fixes boot hangs when all of the OP-TEE threads were in the suspended state at the time of a kernel panic. The kexec into the kdump kernel after a panic is an "emergency" kexec that doesn't even call .shutdown hooks. There's no way for the OP-TEE driver to clean up after itself.
This patch disables the shm cache (and unregisters the shm buffers) during a normal kexec from one perfectly working kernel into a new kernel. This is required because the new kernel will not be able to handle the virtual addresses that were cached under the old kernel. The new kernel has an entirely different memory layout and the old addresses point to unmapped memory or memory that's mapped but probably not a TEE shm.
Tyler
Jens
On Fri, Jun 11, 2021 at 07:53:26AM -0500, Tyler Hicks wrote:
On 2021-06-11 11:11:33, Jens Wiklander wrote:
On Thu, Jun 10, 2021 at 11:09 PM Tyler Hicks tyhicks@linux.microsoft.com wrote:
From: Allen Pais apais@linux.microsoft.com
The following out of memory errors are seen on kexec reboot from the optee core.
[ 0.368428] tee_bnxt_fw optee-clnt0: tee_shm_alloc failed [ 0.368461] tee_bnxt_fw: probe of optee-clnt0 failed with error -22
tee_shm_release() is not invoked on dma shm buffer.
Implement .shutdown() method to handle the release of the buffers correctly.
More info: https://github.com/OP-TEE/optee_os/issues/3637
Signed-off-by: Allen Pais apais@linux.microsoft.com Reviewed-by: Tyler Hicks tyhicks@linux.microsoft.com
Do we really need this considering the patch "optee: Refuse to load the driver under the kdump kernel"?
Yes. That patch fixes boot hangs when all of the OP-TEE threads were in the suspended state at the time of a kernel panic. The kexec into the kdump kernel after a panic is an "emergency" kexec that doesn't even call .shutdown hooks. There's no way for the OP-TEE driver to clean up after itself.
This patch disables the shm cache (and unregisters the shm buffers) during a normal kexec from one perfectly working kernel into a new kernel. This is required because the new kernel will not be able to handle the virtual addresses that were cached under the old kernel. The new kernel has an entirely different memory layout and the old addresses point to unmapped memory or memory that's mapped but probably not a TEE shm.
Got it, thanks.
Jens
On Thu, Jun 10, 2021 at 04:09:08PM -0500, Tyler Hicks wrote:
From: Allen Pais apais@linux.microsoft.com
The following out of memory errors are seen on kexec reboot from the optee core.
[ 0.368428] tee_bnxt_fw optee-clnt0: tee_shm_alloc failed [ 0.368461] tee_bnxt_fw: probe of optee-clnt0 failed with error -22
tee_shm_release() is not invoked on dma shm buffer.
Implement .shutdown() method to handle the release of the buffers correctly.
More info: https://github.com/OP-TEE/optee_os/issues/3637
Signed-off-by: Allen Pais apais@linux.microsoft.com Reviewed-by: Tyler Hicks tyhicks@linux.microsoft.com
drivers/tee/optee/core.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
Reviewed-by: Jens Wiklander jens.wiklander@linaro.org
The shm cache could contain invalid addresses if optee_disable_shm_cache() was not called from the .shutdown hook of the previous kernel before a kexec. These addresses could be unmapped or they could point to mapped but unintended locations in memory.
Clear the shared memory cache, while being careful to not translate the addresses returned from OPTEE_SMC_DISABLE_SHM_CACHE, during driver initialization. Once all pre-cache shm objects are removed, proceed with enabling the cache so that we know that we can handle cached shm objects with confidence later in the .shutdown hook.
Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com --- drivers/tee/optee/call.c | 11 ++++++++++- drivers/tee/optee/core.c | 13 +++++++++++-- drivers/tee/optee/optee_private.h | 2 +- 3 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c index 6e6eb836e9b6..5dcba6105ed7 100644 --- a/drivers/tee/optee/call.c +++ b/drivers/tee/optee/call.c @@ -419,8 +419,10 @@ void optee_enable_shm_cache(struct optee *optee) * optee_disable_shm_cache() - Disables caching of some shared memory allocation * in OP-TEE * @optee: main service struct + * @is_mapped: true if the cached shared memory addresses were mapped by this + * kernel, are safe to dereference, and should be freed */ -void optee_disable_shm_cache(struct optee *optee) +void optee_disable_shm_cache(struct optee *optee, bool is_mapped) { struct optee_call_waiter w;
@@ -439,6 +441,13 @@ void optee_disable_shm_cache(struct optee *optee) if (res.result.status == OPTEE_SMC_RETURN_OK) { struct tee_shm *shm;
+ /* + * Shared memory references that were not mapped by + * this kernel must be ignored to prevent a crash. + */ + if (!is_mapped) + continue; + shm = reg_pair_to_ptr(res.result.shm_upper32, res.result.shm_lower32); tee_shm_free(shm); diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 0987074d7ed0..6974e1104bd4 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -589,7 +589,7 @@ static int optee_remove(struct platform_device *pdev) * reference counters and also avoid wild pointers in secure world * into the old shared memory range. */ - optee_disable_shm_cache(optee); + optee_disable_shm_cache(optee, true);
/* * The two devices have to be unregistered before we can free the @@ -619,7 +619,7 @@ static int optee_remove(struct platform_device *pdev) */ static void optee_shutdown(struct platform_device *pdev) { - optee_disable_shm_cache(platform_get_drvdata(pdev)); + optee_disable_shm_cache(platform_get_drvdata(pdev), true); }
static int optee_probe(struct platform_device *pdev) @@ -716,6 +716,15 @@ static int optee_probe(struct platform_device *pdev) optee->memremaped_shm = memremaped_shm; optee->pool = pool;
+ /* + * Ensure that there are no pre-existing shm objects before enabling + * the shm cache so that there's no chance of receiving an invalid + * address during shutdown. This could occur, for example, if we're + * kexec booting from an older kernel that did not properly cleanup the + * shm cache. + */ + optee_disable_shm_cache(optee, false); + optee_enable_shm_cache(optee);
if (optee->sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index e25b216a14ef..16d8c82213e7 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -158,7 +158,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session);
void optee_enable_shm_cache(struct optee *optee); -void optee_disable_shm_cache(struct optee *optee); +void optee_disable_shm_cache(struct optee *optee, bool is_mapped);
int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm, struct page **pages, size_t num_pages,
On Thu, Jun 10, 2021 at 04:09:09PM -0500, Tyler Hicks wrote:
The shm cache could contain invalid addresses if optee_disable_shm_cache() was not called from the .shutdown hook of the previous kernel before a kexec. These addresses could be unmapped or they could point to mapped but unintended locations in memory.
Clear the shared memory cache, while being careful to not translate the addresses returned from OPTEE_SMC_DISABLE_SHM_CACHE, during driver initialization. Once all pre-cache shm objects are removed, proceed with enabling the cache so that we know that we can handle cached shm objects with confidence later in the .shutdown hook.
Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com
drivers/tee/optee/call.c | 11 ++++++++++- drivers/tee/optee/core.c | 13 +++++++++++-- drivers/tee/optee/optee_private.h | 2 +- 3 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c index 6e6eb836e9b6..5dcba6105ed7 100644 --- a/drivers/tee/optee/call.c +++ b/drivers/tee/optee/call.c @@ -419,8 +419,10 @@ void optee_enable_shm_cache(struct optee *optee)
- optee_disable_shm_cache() - Disables caching of some shared memory allocation
in OP-TEE- @optee: main service struct
- @is_mapped: true if the cached shared memory addresses were mapped by this
*/
kernel, are safe to dereference, and should be freed-void optee_disable_shm_cache(struct optee *optee) +void optee_disable_shm_cache(struct optee *optee, bool is_mapped) { struct optee_call_waiter w; @@ -439,6 +441,13 @@ void optee_disable_shm_cache(struct optee *optee) if (res.result.status == OPTEE_SMC_RETURN_OK) { struct tee_shm *shm;
/** Shared memory references that were not mapped by* this kernel must be ignored to prevent a crash.*/if (!is_mapped)continue;shm = reg_pair_to_ptr(res.result.shm_upper32, res.result.shm_lower32); tee_shm_free(shm);diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 0987074d7ed0..6974e1104bd4 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -589,7 +589,7 @@ static int optee_remove(struct platform_device *pdev) * reference counters and also avoid wild pointers in secure world * into the old shared memory range. */
- optee_disable_shm_cache(optee);
- optee_disable_shm_cache(optee, true);
Naked "true" or "false" parameters are normally not very descriptive. Would it make sense to write this as: optee_disable_shm_cache(optee, true /*is_mapped*/); instead (same for the other call sites in this patch)? That way it would be easier to see what it is that is true or false.
/Jens
/* * The two devices have to be unregistered before we can free the @@ -619,7 +619,7 @@ static int optee_remove(struct platform_device *pdev) */ static void optee_shutdown(struct platform_device *pdev) {
- optee_disable_shm_cache(platform_get_drvdata(pdev));
- optee_disable_shm_cache(platform_get_drvdata(pdev), true);
} static int optee_probe(struct platform_device *pdev) @@ -716,6 +716,15 @@ static int optee_probe(struct platform_device *pdev) optee->memremaped_shm = memremaped_shm; optee->pool = pool;
- /*
* Ensure that there are no pre-existing shm objects before enabling* the shm cache so that there's no chance of receiving an invalid* address during shutdown. This could occur, for example, if we're* kexec booting from an older kernel that did not properly cleanup the* shm cache.*/- optee_disable_shm_cache(optee, false);
- optee_enable_shm_cache(optee);
if (optee->sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index e25b216a14ef..16d8c82213e7 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -158,7 +158,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session); void optee_enable_shm_cache(struct optee *optee); -void optee_disable_shm_cache(struct optee *optee); +void optee_disable_shm_cache(struct optee *optee, bool is_mapped); int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm, struct page **pages, size_t num_pages, -- 2.25.1
On 2021-06-14 10:27:15, Jens Wiklander wrote:
On Thu, Jun 10, 2021 at 04:09:09PM -0500, Tyler Hicks wrote:
The shm cache could contain invalid addresses if optee_disable_shm_cache() was not called from the .shutdown hook of the previous kernel before a kexec. These addresses could be unmapped or they could point to mapped but unintended locations in memory.
Clear the shared memory cache, while being careful to not translate the addresses returned from OPTEE_SMC_DISABLE_SHM_CACHE, during driver initialization. Once all pre-cache shm objects are removed, proceed with enabling the cache so that we know that we can handle cached shm objects with confidence later in the .shutdown hook.
Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com
drivers/tee/optee/call.c | 11 ++++++++++- drivers/tee/optee/core.c | 13 +++++++++++-- drivers/tee/optee/optee_private.h | 2 +- 3 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c index 6e6eb836e9b6..5dcba6105ed7 100644 --- a/drivers/tee/optee/call.c +++ b/drivers/tee/optee/call.c @@ -419,8 +419,10 @@ void optee_enable_shm_cache(struct optee *optee)
- optee_disable_shm_cache() - Disables caching of some shared memory allocation
in OP-TEE- @optee: main service struct
- @is_mapped: true if the cached shared memory addresses were mapped by this
*/
kernel, are safe to dereference, and should be freed-void optee_disable_shm_cache(struct optee *optee) +void optee_disable_shm_cache(struct optee *optee, bool is_mapped) { struct optee_call_waiter w; @@ -439,6 +441,13 @@ void optee_disable_shm_cache(struct optee *optee) if (res.result.status == OPTEE_SMC_RETURN_OK) { struct tee_shm *shm;
/** Shared memory references that were not mapped by* this kernel must be ignored to prevent a crash.*/if (!is_mapped)continue;shm = reg_pair_to_ptr(res.result.shm_upper32, res.result.shm_lower32); tee_shm_free(shm);diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 0987074d7ed0..6974e1104bd4 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -589,7 +589,7 @@ static int optee_remove(struct platform_device *pdev) * reference counters and also avoid wild pointers in secure world * into the old shared memory range. */
- optee_disable_shm_cache(optee);
- optee_disable_shm_cache(optee, true);
Naked "true" or "false" parameters are normally not very descriptive. Would it make sense to write this as: optee_disable_shm_cache(optee, true /*is_mapped*/); instead (same for the other call sites in this patch)? That way it would be easier to see what it is that is true or false.
Yeah, I understand the issue with the naked bools. What about turning 'optee_disable_shm_cache(struct optee *optee, bool is_mapped)' into '__optee_disable_shm_cache(struct optee *optee, bool is_mapped)' and introducing these two wrappers:
/** * optee_disable_shm_cache() - Disables caching of mapped shared memory * allocations in OP-TEE * @optee: main service struct */ void optee_disable_shm_cache(struct optee *optee) { return __optee_disable_shm_cache(optee, true); }
/** * optee_disable_unmapped_shm_cache() - Disables caching of shared memory * allocations in OP-TEE which are not * currently mapped * @optee: main service struct */ void optee_disable_unmapped_shm_cache(struct optee *optee) { return __optee_disable_shm_cache(optee, false); }
Existing callers of optee_disable_shm_cache() remain unchanged and we just add one caller of optee_disable_unmapped_shm_cache() with this patch.
Tyler
/Jens
/* * The two devices have to be unregistered before we can free the @@ -619,7 +619,7 @@ static int optee_remove(struct platform_device *pdev) */ static void optee_shutdown(struct platform_device *pdev) {
- optee_disable_shm_cache(platform_get_drvdata(pdev));
- optee_disable_shm_cache(platform_get_drvdata(pdev), true);
} static int optee_probe(struct platform_device *pdev) @@ -716,6 +716,15 @@ static int optee_probe(struct platform_device *pdev) optee->memremaped_shm = memremaped_shm; optee->pool = pool;
- /*
* Ensure that there are no pre-existing shm objects before enabling* the shm cache so that there's no chance of receiving an invalid* address during shutdown. This could occur, for example, if we're* kexec booting from an older kernel that did not properly cleanup the* shm cache.*/- optee_disable_shm_cache(optee, false);
- optee_enable_shm_cache(optee);
if (optee->sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index e25b216a14ef..16d8c82213e7 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -158,7 +158,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session); void optee_enable_shm_cache(struct optee *optee); -void optee_disable_shm_cache(struct optee *optee); +void optee_disable_shm_cache(struct optee *optee, bool is_mapped); int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm, struct page **pages, size_t num_pages, -- 2.25.1
On Mon, Jun 14, 2021 at 02:06:46PM -0500, Tyler Hicks wrote:
On 2021-06-14 10:27:15, Jens Wiklander wrote:
On Thu, Jun 10, 2021 at 04:09:09PM -0500, Tyler Hicks wrote:
The shm cache could contain invalid addresses if optee_disable_shm_cache() was not called from the .shutdown hook of the previous kernel before a kexec. These addresses could be unmapped or they could point to mapped but unintended locations in memory.
Clear the shared memory cache, while being careful to not translate the addresses returned from OPTEE_SMC_DISABLE_SHM_CACHE, during driver initialization. Once all pre-cache shm objects are removed, proceed with enabling the cache so that we know that we can handle cached shm objects with confidence later in the .shutdown hook.
Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com
drivers/tee/optee/call.c | 11 ++++++++++- drivers/tee/optee/core.c | 13 +++++++++++-- drivers/tee/optee/optee_private.h | 2 +- 3 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c index 6e6eb836e9b6..5dcba6105ed7 100644 --- a/drivers/tee/optee/call.c +++ b/drivers/tee/optee/call.c @@ -419,8 +419,10 @@ void optee_enable_shm_cache(struct optee *optee)
- optee_disable_shm_cache() - Disables caching of some shared memory allocation
in OP-TEE- @optee: main service struct
- @is_mapped: true if the cached shared memory addresses were mapped by this
*/
kernel, are safe to dereference, and should be freed-void optee_disable_shm_cache(struct optee *optee) +void optee_disable_shm_cache(struct optee *optee, bool is_mapped) { struct optee_call_waiter w; @@ -439,6 +441,13 @@ void optee_disable_shm_cache(struct optee *optee) if (res.result.status == OPTEE_SMC_RETURN_OK) { struct tee_shm *shm;
/** Shared memory references that were not mapped by* this kernel must be ignored to prevent a crash.*/if (!is_mapped)continue;shm = reg_pair_to_ptr(res.result.shm_upper32, res.result.shm_lower32); tee_shm_free(shm);diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 0987074d7ed0..6974e1104bd4 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -589,7 +589,7 @@ static int optee_remove(struct platform_device *pdev) * reference counters and also avoid wild pointers in secure world * into the old shared memory range. */
- optee_disable_shm_cache(optee);
- optee_disable_shm_cache(optee, true);
Naked "true" or "false" parameters are normally not very descriptive. Would it make sense to write this as: optee_disable_shm_cache(optee, true /*is_mapped*/); instead (same for the other call sites in this patch)? That way it would be easier to see what it is that is true or false.
Yeah, I understand the issue with the naked bools. What about turning 'optee_disable_shm_cache(struct optee *optee, bool is_mapped)' into '__optee_disable_shm_cache(struct optee *optee, bool is_mapped)' and introducing these two wrappers:
/**
- optee_disable_shm_cache() - Disables caching of mapped shared memory
allocations in OP-TEE- @optee: main service struct
*/ void optee_disable_shm_cache(struct optee *optee) { return __optee_disable_shm_cache(optee, true); }
/**
- optee_disable_unmapped_shm_cache() - Disables caching of shared memory
allocations in OP-TEE which are notcurrently mapped- @optee: main service struct
*/ void optee_disable_unmapped_shm_cache(struct optee *optee) { return __optee_disable_shm_cache(optee, false); }
Existing callers of optee_disable_shm_cache() remain unchanged and we just add one caller of optee_disable_unmapped_shm_cache() with this patch.
Sounds good.
Jens
From: Jens Wiklander jens.wiklander@linaro.org
Adds a new function tee_shm_alloc_kernel_buf() to allocate shared memory from a kernel driver. This function can later be made more lightweight by unnecessary dma-buf export.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org Reviewed-by: Tyler Hicks tyhicks@linux.microsoft.com --- drivers/tee/tee_shm.c | 18 ++++++++++++++++++ include/linux/tee_drv.h | 1 + 2 files changed, 19 insertions(+)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 00472f5ce22e..c65e44707cd6 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -193,6 +193,24 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) } EXPORT_SYMBOL_GPL(tee_shm_alloc);
+/** + * tee_shm_alloc_kernel_buf() - Allocate shared memory for kernel buffer + * @ctx: Context that allocates the shared memory + * @size: Requested size of shared memory + * + * The returned memory registered in secure world and is suitable to be + * passed as a memory buffer in parameter argument to + * tee_client_invoke_func(). The memory allocated is later freed with a + * call to tee_shm_free(). + * + * @returns a pointer to 'struct tee_shm' + */ +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); +} +EXPORT_SYMBOL_GPL(tee_shm_alloc_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 54269e47ac9a..8990f7628387 100644 --- a/include/linux/tee_drv.h +++ b/include/linux/tee_drv.h @@ -332,6 +332,7 @@ void *tee_get_drvdata(struct tee_device *teedev); * @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_kernel_buf(struct tee_context *ctx, size_t size);
/** * tee_shm_register() - Register shared memory buffer
Uncouple the registration of kernel shared memory buffers from the TEE_SHM_DMA_BUF flag. Drivers may wish to allocate multi-page contiguous shared memory regions but do not need them to be backed by a dma-buf when the memory region is only used by the driver.
If the TEE implementation does not require shared memory to be registered, clear the flag prior to calling the corresponding pool alloc function. Update the OP-TEE driver to respect TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to (un)register on alloc/free operations. The AMD-TEE driver continues to ignore the TEE_SHM_REGISTER flag.
Allow callers of tee_shm_alloc_kernel_buf() to allocate and register a shared memory region without the backing of dma-buf.
Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com --- drivers/tee/optee/shm_pool.c | 5 ++--- drivers/tee/tee_shm.c | 13 +++++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c index da06ce9b9313..6054343a29fb 100644 --- a/drivers/tee/optee/shm_pool.c +++ b/drivers/tee/optee/shm_pool.c @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, shm->paddr = page_to_phys(page); shm->size = PAGE_SIZE << order;
- if (shm->flags & TEE_SHM_DMA_BUF) { + if (shm->flags & TEE_SHM_REGISTER) { unsigned int nr_pages = 1 << order, i; struct page **pages;
@@ -42,7 +42,6 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, page++; }
- shm->flags |= TEE_SHM_REGISTER; rc = optee_shm_register(shm->ctx, shm, pages, nr_pages, (unsigned long)shm->kaddr); kfree(pages); @@ -60,7 +59,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm) { - if (shm->flags & TEE_SHM_DMA_BUF) + if (shm->flags & TEE_SHM_REGISTER) optee_shm_unregister(shm->ctx, shm);
free_pages((unsigned long)shm->kaddr, get_order(shm->size)); diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index c65e44707cd6..26a76f817c57 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -117,7 +117,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) return ERR_PTR(-EINVAL); }
- if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) { + if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER))) { dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags); return ERR_PTR(-EINVAL); } @@ -137,6 +137,15 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) goto err_dev_put; }
+ if (!teedev->desc->ops->shm_register || + !teedev->desc->ops->shm_unregister) { + /* registration is not required by the TEE implementation */ + flags &= ~TEE_SHM_REGISTER; + } else if (flags & TEE_SHM_DMA_BUF) { + /* all dma-buf backed shm allocations are registered */ + flags |= TEE_SHM_REGISTER; + } + shm->flags = flags | TEE_SHM_POOL; shm->ctx = ctx; if (flags & TEE_SHM_DMA_BUF) @@ -207,7 +216,7 @@ 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); + return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED | TEE_SHM_REGISTER); } EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
On Fri, 11 Jun 2021 at 02:39, Tyler Hicks tyhicks@linux.microsoft.com wrote:
Uncouple the registration of kernel shared memory buffers from the TEE_SHM_DMA_BUF flag. Drivers may wish to allocate multi-page contiguous shared memory regions but do not need them to be backed by a dma-buf when the memory region is only used by the driver.
If the TEE implementation does not require shared memory to be registered, clear the flag prior to calling the corresponding pool alloc function. Update the OP-TEE driver to respect TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to (un)register on alloc/free operations.
The AMD-TEE driver continues to ignore the TEE_SHM_REGISTER flag.
That's the main point that no other TEE implementation would honour TEE_SHM_REGISTER and I think it's just the incorrect usage of TEE_SHM_REGISTER flag to suffice OP-TEE underlying implementation.
Allow callers of tee_shm_alloc_kernel_buf() to allocate and register a shared memory region without the backing of dma-buf.
Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com
drivers/tee/optee/shm_pool.c | 5 ++--- drivers/tee/tee_shm.c | 13 +++++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-)
This patch is just mixing two separate approaches to TEE shared memory. Have a look at alternative suggestions below.
diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c index da06ce9b9313..6054343a29fb 100644 --- a/drivers/tee/optee/shm_pool.c +++ b/drivers/tee/optee/shm_pool.c @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, shm->paddr = page_to_phys(page); shm->size = PAGE_SIZE << order;
if (shm->flags & TEE_SHM_DMA_BUF) {
if (shm->flags & TEE_SHM_REGISTER) {
Here you can just do following check instead:
if (!(shm->flags & TEE_SHM_PRIV)) {
And this flag needs to be passed from the call sites here [1] [2].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv... [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
unsigned int nr_pages = 1 << order, i; struct page **pages;@@ -42,7 +42,6 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, page++; }
shm->flags |= TEE_SHM_REGISTER;
This should remain as it is.
rc = optee_shm_register(shm->ctx, shm, pages, nr_pages, (unsigned long)shm->kaddr); kfree(pages);@@ -60,7 +59,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm) {
if (shm->flags & TEE_SHM_DMA_BUF)
if (shm->flags & TEE_SHM_REGISTER)
Same as above.
optee_shm_unregister(shm->ctx, shm); free_pages((unsigned long)shm->kaddr, get_order(shm->size));diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index c65e44707cd6..26a76f817c57 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -117,7 +117,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) return ERR_PTR(-EINVAL); }
if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) {
if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER))) {
No need for this change.
dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags); return ERR_PTR(-EINVAL); }@@ -137,6 +137,15 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) goto err_dev_put; }
if (!teedev->desc->ops->shm_register ||!teedev->desc->ops->shm_unregister) {/* registration is not required by the TEE implementation */flags &= ~TEE_SHM_REGISTER;} else if (flags & TEE_SHM_DMA_BUF) {/* all dma-buf backed shm allocations are registered */flags |= TEE_SHM_REGISTER;}
This change isn't required as well as underlying TEE implementation: OP-TEE in this case knows how to implement shared memory allocation whether to use reserved shared memory pool or dynamic shared memory pool. For more details see shared memory pool creation in optee_probe().
shm->flags = flags | TEE_SHM_POOL; shm->ctx = ctx; if (flags & TEE_SHM_DMA_BUF)@@ -207,7 +216,7 @@ 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);
return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED | TEE_SHM_REGISTER);
Here it could just be:
return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED);
-Sumit
} EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
-- 2.25.1
On 2021-06-11 10:46:20, Sumit Garg wrote:
On Fri, 11 Jun 2021 at 02:39, Tyler Hicks tyhicks@linux.microsoft.com wrote:
Uncouple the registration of kernel shared memory buffers from the TEE_SHM_DMA_BUF flag. Drivers may wish to allocate multi-page contiguous shared memory regions but do not need them to be backed by a dma-buf when the memory region is only used by the driver.
If the TEE implementation does not require shared memory to be registered, clear the flag prior to calling the corresponding pool alloc function. Update the OP-TEE driver to respect TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to (un)register on alloc/free operations.
The AMD-TEE driver continues to ignore the TEE_SHM_REGISTER flag.
That's the main point that no other TEE implementation would honour TEE_SHM_REGISTER and I think it's just the incorrect usage of TEE_SHM_REGISTER flag to suffice OP-TEE underlying implementation.
Allow callers of tee_shm_alloc_kernel_buf() to allocate and register a shared memory region without the backing of dma-buf.
Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com
drivers/tee/optee/shm_pool.c | 5 ++--- drivers/tee/tee_shm.c | 13 +++++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-)
This patch is just mixing two separate approaches to TEE shared memory. Have a look at alternative suggestions below.
diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c index da06ce9b9313..6054343a29fb 100644 --- a/drivers/tee/optee/shm_pool.c +++ b/drivers/tee/optee/shm_pool.c @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, shm->paddr = page_to_phys(page); shm->size = PAGE_SIZE << order;
if (shm->flags & TEE_SHM_DMA_BUF) {
if (shm->flags & TEE_SHM_REGISTER) {Here you can just do following check instead:
if (!(shm->flags & TEE_SHM_PRIV)) {
This is a bug fix series that's intended to fix the current and older kernels. tee_shm_alloc_anon_kernel_buf()/TEE_SHM_PRIV is not present in older kernels and isn't required to fix these kexec/kdump bugs. Your suggestion feels like something that should be done in the allocator rewrite that Jens is working on to clean all of this up going forward.
Tyler
And this flag needs to be passed from the call sites here [1] [2].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv... [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
unsigned int nr_pages = 1 << order, i; struct page **pages;@@ -42,7 +42,6 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, page++; }
shm->flags |= TEE_SHM_REGISTER;This should remain as it is.
rc = optee_shm_register(shm->ctx, shm, pages, nr_pages, (unsigned long)shm->kaddr); kfree(pages);@@ -60,7 +59,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm) {
if (shm->flags & TEE_SHM_DMA_BUF)
if (shm->flags & TEE_SHM_REGISTER)Same as above.
optee_shm_unregister(shm->ctx, shm); free_pages((unsigned long)shm->kaddr, get_order(shm->size));diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index c65e44707cd6..26a76f817c57 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -117,7 +117,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) return ERR_PTR(-EINVAL); }
if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) {
if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER))) {No need for this change.
dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags); return ERR_PTR(-EINVAL); }@@ -137,6 +137,15 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) goto err_dev_put; }
if (!teedev->desc->ops->shm_register ||!teedev->desc->ops->shm_unregister) {/* registration is not required by the TEE implementation */flags &= ~TEE_SHM_REGISTER;} else if (flags & TEE_SHM_DMA_BUF) {/* all dma-buf backed shm allocations are registered */flags |= TEE_SHM_REGISTER;}This change isn't required as well as underlying TEE implementation: OP-TEE in this case knows how to implement shared memory allocation whether to use reserved shared memory pool or dynamic shared memory pool. For more details see shared memory pool creation in optee_probe().
shm->flags = flags | TEE_SHM_POOL; shm->ctx = ctx; if (flags & TEE_SHM_DMA_BUF)@@ -207,7 +216,7 @@ 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);
return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED | TEE_SHM_REGISTER);Here it could just be:
return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED);-Sumit
} EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
-- 2.25.1
On 2021-06-11 08:10:01, Tyler Hicks wrote:
On 2021-06-11 10:46:20, Sumit Garg wrote:
On Fri, 11 Jun 2021 at 02:39, Tyler Hicks tyhicks@linux.microsoft.com wrote:
Uncouple the registration of kernel shared memory buffers from the TEE_SHM_DMA_BUF flag. Drivers may wish to allocate multi-page contiguous shared memory regions but do not need them to be backed by a dma-buf when the memory region is only used by the driver.
If the TEE implementation does not require shared memory to be registered, clear the flag prior to calling the corresponding pool alloc function. Update the OP-TEE driver to respect TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to (un)register on alloc/free operations.
The AMD-TEE driver continues to ignore the TEE_SHM_REGISTER flag.
That's the main point that no other TEE implementation would honour TEE_SHM_REGISTER and I think it's just the incorrect usage of TEE_SHM_REGISTER flag to suffice OP-TEE underlying implementation.
Allow callers of tee_shm_alloc_kernel_buf() to allocate and register a shared memory region without the backing of dma-buf.
Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com
drivers/tee/optee/shm_pool.c | 5 ++--- drivers/tee/tee_shm.c | 13 +++++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-)
This patch is just mixing two separate approaches to TEE shared memory. Have a look at alternative suggestions below.
diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c index da06ce9b9313..6054343a29fb 100644 --- a/drivers/tee/optee/shm_pool.c +++ b/drivers/tee/optee/shm_pool.c @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, shm->paddr = page_to_phys(page); shm->size = PAGE_SIZE << order;
if (shm->flags & TEE_SHM_DMA_BUF) {
if (shm->flags & TEE_SHM_REGISTER) {Here you can just do following check instead:
if (!(shm->flags & TEE_SHM_PRIV)) {This is a bug fix series that's intended to fix the current and older kernels. tee_shm_alloc_anon_kernel_buf()/TEE_SHM_PRIV is not present in older kernels and isn't required to fix these kexec/kdump bugs. Your suggestion feels like something that should be done in the allocator rewrite that Jens is working on to clean all of this up going forward.
I want to add that I do fully agree with you that TEE_SHM_REGISTER is an OP-TEE thing and not a TEE thing. Ideally, it wouldn't be defined in tee_drv.h and would be completely private to the OP-TEE driver. Likewise, I don't think that tee_shm_register() should exist (certainly not at the TEE level) because it only works with OP-TEE.
That said, I think the first step is to fix the kexec/kdump bugs and the second step is to clean up the code to remove the layering violation of exposing shm registration from the TEE interfaces.
Tyler
Tyler
And this flag needs to be passed from the call sites here [1] [2].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv... [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
unsigned int nr_pages = 1 << order, i; struct page **pages;@@ -42,7 +42,6 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, page++; }
shm->flags |= TEE_SHM_REGISTER;This should remain as it is.
rc = optee_shm_register(shm->ctx, shm, pages, nr_pages, (unsigned long)shm->kaddr); kfree(pages);@@ -60,7 +59,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm) {
if (shm->flags & TEE_SHM_DMA_BUF)
if (shm->flags & TEE_SHM_REGISTER)Same as above.
optee_shm_unregister(shm->ctx, shm); free_pages((unsigned long)shm->kaddr, get_order(shm->size));diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index c65e44707cd6..26a76f817c57 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -117,7 +117,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) return ERR_PTR(-EINVAL); }
if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) {
if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER))) {No need for this change.
dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags); return ERR_PTR(-EINVAL); }@@ -137,6 +137,15 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) goto err_dev_put; }
if (!teedev->desc->ops->shm_register ||!teedev->desc->ops->shm_unregister) {/* registration is not required by the TEE implementation */flags &= ~TEE_SHM_REGISTER;} else if (flags & TEE_SHM_DMA_BUF) {/* all dma-buf backed shm allocations are registered */flags |= TEE_SHM_REGISTER;}This change isn't required as well as underlying TEE implementation: OP-TEE in this case knows how to implement shared memory allocation whether to use reserved shared memory pool or dynamic shared memory pool. For more details see shared memory pool creation in optee_probe().
shm->flags = flags | TEE_SHM_POOL; shm->ctx = ctx; if (flags & TEE_SHM_DMA_BUF)@@ -207,7 +216,7 @@ 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);
return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED | TEE_SHM_REGISTER);Here it could just be:
return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED);-Sumit
} EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
-- 2.25.1
On Fri, 11 Jun 2021 at 18:46, Tyler Hicks tyhicks@linux.microsoft.com wrote:
On 2021-06-11 08:10:01, Tyler Hicks wrote:
On 2021-06-11 10:46:20, Sumit Garg wrote:
On Fri, 11 Jun 2021 at 02:39, Tyler Hicks tyhicks@linux.microsoft.com wrote:
Uncouple the registration of kernel shared memory buffers from the TEE_SHM_DMA_BUF flag. Drivers may wish to allocate multi-page contiguous shared memory regions but do not need them to be backed by a dma-buf when the memory region is only used by the driver.
If the TEE implementation does not require shared memory to be registered, clear the flag prior to calling the corresponding pool alloc function. Update the OP-TEE driver to respect TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to (un)register on alloc/free operations.
The AMD-TEE driver continues to ignore the TEE_SHM_REGISTER flag.
That's the main point that no other TEE implementation would honour TEE_SHM_REGISTER and I think it's just the incorrect usage of TEE_SHM_REGISTER flag to suffice OP-TEE underlying implementation.
Allow callers of tee_shm_alloc_kernel_buf() to allocate and register a shared memory region without the backing of dma-buf.
Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com
drivers/tee/optee/shm_pool.c | 5 ++--- drivers/tee/tee_shm.c | 13 +++++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-)
This patch is just mixing two separate approaches to TEE shared memory. Have a look at alternative suggestions below.
diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c index da06ce9b9313..6054343a29fb 100644 --- a/drivers/tee/optee/shm_pool.c +++ b/drivers/tee/optee/shm_pool.c @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, shm->paddr = page_to_phys(page); shm->size = PAGE_SIZE << order;
if (shm->flags & TEE_SHM_DMA_BUF) {
if (shm->flags & TEE_SHM_REGISTER) {Here you can just do following check instead:
if (!(shm->flags & TEE_SHM_PRIV)) {This is a bug fix series that's intended to fix the current and older kernels. tee_shm_alloc_anon_kernel_buf()/TEE_SHM_PRIV is not present in older kernels and isn't required to fix these kexec/kdump bugs. Your suggestion feels like something that should be done in the allocator rewrite that Jens is working on to clean all of this up going forward.
I want to add that I do fully agree with you that TEE_SHM_REGISTER is an OP-TEE thing and not a TEE thing. Ideally, it wouldn't be defined in tee_drv.h and would be completely private to the OP-TEE driver. Likewise, I don't think that tee_shm_register() should exist (certainly not at the TEE level) because it only works with OP-TEE.
I think there is some confusion going on. tee_shm_register() is a standard interface that is listed in TEE client API specification as an alternative approach to tee_shm_alloc(). As I earlier mentioned, please read through "3.2.4. Shared Memory" in TEE Client API Specification.
In the initial times, OP-TEE only supported tee_shm_alloc() approach but with the addition of dynamic shared memory feature it became possible to support tee_shm_register() as well but we had to add new capability as TEE_GEN_CAP_REG_MEM in order to maintain OP-TEE backwards compatibility. It can very well be the same case for AMD-TEE which currently only supports tee_shm_alloc() approach.
The reason for confusion here seems to be that OP-TEE driver is providing a way to leverage dynamic shared memory approach in order to implement tee_shm_alloc() but that doesn't mean at TEE level we should intermix both approaches via using TEE_SHM_REGISTER to implement tee_shm_alloc().
That said, I think the first step is to fix the kexec/kdump bugs and the second step is to clean up the code to remove the layering violation of exposing shm registration from the TEE interfaces.
Doesn't the following patch sound suitable to be backported to a stable kernel? It has even less changes compared to your patch as well :).
-Sumit
------------------------------------------- Subject: [PATCH] tee: Correct inappropriate usage of TEE_SHM_DMA_BUF flag
Currently TEE_SHM_DMA_BUF flag has been inappropriately used to not register shared memory allocated for private usage by underlying TEE driver: OP-TEE in this case. So rather add a new flag as TEE_SHM_PRIV that can be utilized by underlying TEE drivers for private allocation and usage of shared memory.
With this corrected, allow tee_shm_alloc_kernel_buf() to allocate a shared memory region without the backing of dma-buf.
Signed-off-by: Sumit Garg sumit.garg@linaro.org --- drivers/tee/optee/call.c | 2 +- drivers/tee/optee/core.c | 3 ++- drivers/tee/optee/shm_pool.c | 8 ++++++-- drivers/tee/tee_shm.c | 2 +- include/linux/tee_drv.h | 1 + 5 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c index 6132cc8d014c..faaa13c9172b 100644 --- a/drivers/tee/optee/call.c +++ b/drivers/tee/optee/call.c @@ -184,7 +184,7 @@ static struct tee_shm *get_msg_arg(struct tee_context *ctx, size_t num_params, struct optee_msg_arg *ma;
shm = tee_shm_alloc(ctx, OPTEE_MSG_GET_ARG_SIZE(num_params), - TEE_SHM_MAPPED); + TEE_SHM_MAPPED | TEE_SHM_PRIV); if (IS_ERR(shm)) return shm;
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index ddb8f9ecf307..eac0e91ec559 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -277,7 +277,8 @@ 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(ctx, sizeof(struct optee_msg_arg), + TEE_SHM_MAPPED | TEE_SHM_PRIV); if (!IS_ERR(shm)) { arg = tee_shm_get_va(shm, 0); /* diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c index d767eebf30bd..3b4a3853a10f 100644 --- a/drivers/tee/optee/shm_pool.c +++ b/drivers/tee/optee/shm_pool.c @@ -27,7 +27,11 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, shm->paddr = page_to_phys(page); shm->size = PAGE_SIZE << order;
- if (shm->flags & TEE_SHM_DMA_BUF) { + /* + * Shared memory private to the OP-TEE driver doesn't need + * to be registered with OP-TEE. + */ + if (!(shm->flags & TEE_SHM_PRIV)) { unsigned int nr_pages = 1 << order, i; struct page **pages;
@@ -52,7 +56,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm) { - if (shm->flags & TEE_SHM_DMA_BUF) + if (!(shm->flags & TEE_SHM_PRIV)) optee_shm_unregister(shm->ctx, shm);
free_pages((unsigned long)shm->kaddr, get_order(shm->size)); diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index c425ad80d6a6..f8b73e734094 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -207,7 +207,7 @@ 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); + return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED); } EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf); diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h index 8990f7628387..3ebfea0781f1 100644 --- a/include/linux/tee_drv.h +++ b/include/linux/tee_drv.h @@ -27,6 +27,7 @@ #define TEE_SHM_USER_MAPPED BIT(4) /* Memory mapped in user space */ #define TEE_SHM_POOL BIT(5) /* Memory allocated from pool */ #define TEE_SHM_KERNEL_MAPPED BIT(6) /* Memory mapped in kernel space */ +#define TEE_SHM_PRIV BIT(7) /* Memory private to TEE driver */
struct device; struct tee_device;
On 2021-06-12 13:49:38, Sumit Garg wrote:
On Fri, 11 Jun 2021 at 18:46, Tyler Hicks tyhicks@linux.microsoft.com wrote:
On 2021-06-11 08:10:01, Tyler Hicks wrote:
On 2021-06-11 10:46:20, Sumit Garg wrote:
On Fri, 11 Jun 2021 at 02:39, Tyler Hicks tyhicks@linux.microsoft.com wrote:
Uncouple the registration of kernel shared memory buffers from the TEE_SHM_DMA_BUF flag. Drivers may wish to allocate multi-page contiguous shared memory regions but do not need them to be backed by a dma-buf when the memory region is only used by the driver.
If the TEE implementation does not require shared memory to be registered, clear the flag prior to calling the corresponding pool alloc function. Update the OP-TEE driver to respect TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to (un)register on alloc/free operations.
The AMD-TEE driver continues to ignore the TEE_SHM_REGISTER flag.
That's the main point that no other TEE implementation would honour TEE_SHM_REGISTER and I think it's just the incorrect usage of TEE_SHM_REGISTER flag to suffice OP-TEE underlying implementation.
Allow callers of tee_shm_alloc_kernel_buf() to allocate and register a shared memory region without the backing of dma-buf.
Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com
drivers/tee/optee/shm_pool.c | 5 ++--- drivers/tee/tee_shm.c | 13 +++++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-)
This patch is just mixing two separate approaches to TEE shared memory. Have a look at alternative suggestions below.
diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c index da06ce9b9313..6054343a29fb 100644 --- a/drivers/tee/optee/shm_pool.c +++ b/drivers/tee/optee/shm_pool.c @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, shm->paddr = page_to_phys(page); shm->size = PAGE_SIZE << order;
if (shm->flags & TEE_SHM_DMA_BUF) {
if (shm->flags & TEE_SHM_REGISTER) {Here you can just do following check instead:
if (!(shm->flags & TEE_SHM_PRIV)) {This is a bug fix series that's intended to fix the current and older kernels. tee_shm_alloc_anon_kernel_buf()/TEE_SHM_PRIV is not present in older kernels and isn't required to fix these kexec/kdump bugs. Your suggestion feels like something that should be done in the allocator rewrite that Jens is working on to clean all of this up going forward.
I want to add that I do fully agree with you that TEE_SHM_REGISTER is an OP-TEE thing and not a TEE thing. Ideally, it wouldn't be defined in tee_drv.h and would be completely private to the OP-TEE driver. Likewise, I don't think that tee_shm_register() should exist (certainly not at the TEE level) because it only works with OP-TEE.
I think there is some confusion going on. tee_shm_register() is a standard interface that is listed in TEE client API specification as an alternative approach to tee_shm_alloc(). As I earlier mentioned, please read through "3.2.4. Shared Memory" in TEE Client API Specification.
Thanks for the reminder to go read this spec. I had forgotten to read it after you previously mentioned it.
Yes, there was confusion on my part due to reading the code but not discovering/reading the spec prior to now.
In the initial times, OP-TEE only supported tee_shm_alloc() approach but with the addition of dynamic shared memory feature it became possible to support tee_shm_register() as well but we had to add new capability as TEE_GEN_CAP_REG_MEM in order to maintain OP-TEE backwards compatibility. It can very well be the same case for AMD-TEE which currently only supports tee_shm_alloc() approach.
The reason for confusion here seems to be that OP-TEE driver is providing a way to leverage dynamic shared memory approach in order to implement tee_shm_alloc() but that doesn't mean at TEE level we should intermix both approaches via using TEE_SHM_REGISTER to implement tee_shm_alloc().
I think that was the reason for my confusion. I didn't understand why AMD-TEE didn't have the same need to register memory.
That said, I think the first step is to fix the kexec/kdump bugs and the second step is to clean up the code to remove the layering violation of exposing shm registration from the TEE interfaces.
Doesn't the following patch sound suitable to be backported to a stable kernel? It has even less changes compared to your patch as well :).
-Sumit
Subject: [PATCH] tee: Correct inappropriate usage of TEE_SHM_DMA_BUF flag
Currently TEE_SHM_DMA_BUF flag has been inappropriately used to not
I think the "not" at the end of this line should be removed, right?
register shared memory allocated for private usage by underlying TEE driver: OP-TEE in this case. So rather add a new flag as TEE_SHM_PRIV that can be utilized by underlying TEE drivers for private allocation and usage of shared memory.
With this corrected, allow tee_shm_alloc_kernel_buf() to allocate a shared memory region without the backing of dma-buf.
I noticed a couple things wrong with this patch during testing.
The first is obviously an oversight. tee_shm_alloc() needs to be updated to accept the TEE_SHM_PRIV or it'll throw an "invalid shm flags ..." error.
The second issue is a little more unclear. I am testing these patches along with a debugging patch that prints a bit of info when optee_shm_register() or optee_shm_unregister() is called so that I can track the (un)registrations. I built a kernel with my v4 series but with this patch in place of my 'tee: Support kernel shm registration without dma-buf backing' patch. I performed 10x kexec operations and then let the system set idle for a while. After a while of sitting idle, I noticed a couple calls to optee_shm_register() that I hadn't seen before (and haven't seen again). It made me worried that your patch could result in us registering shared memory that we previously weren't registering so I decided to try to perform another kexec operation to ensure that the two shms were properly unregistered. At this point, the system became unresponsive and I wasn't able to get a stack trace or any more useful information about what happened.
Unfortunately, my debugging patch was only printing the shm's virtual address and the shm's size. The shm's size was 4096 bytes.
My current thought is that the two new/unexpected calls to optee_shm_register() were triggered by one of the tee_shm_alloc()'s in drivers/tee/optee/rpc.c. Neither of those shm allocations would have been registered before your change but they both would be after your change. I think the easy fix is to use the TEE_SHM_PRIV flag for both of the allocations in rpc.c. Do you agree? If so, I'll make these changes and fold this patch into my series and send out a v5.
I still can't explain why the system became unresponsive after the two registrations...
Signed-off-by: Sumit Garg sumit.garg@linaro.org
drivers/tee/optee/call.c | 2 +- drivers/tee/optee/core.c | 3 ++- drivers/tee/optee/shm_pool.c | 8 ++++++-- drivers/tee/tee_shm.c | 2 +- include/linux/tee_drv.h | 1 + 5 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c index 6132cc8d014c..faaa13c9172b 100644 --- a/drivers/tee/optee/call.c +++ b/drivers/tee/optee/call.c @@ -184,7 +184,7 @@ static struct tee_shm *get_msg_arg(struct tee_context *ctx, size_t num_params, struct optee_msg_arg *ma;
shm = tee_shm_alloc(ctx, OPTEE_MSG_GET_ARG_SIZE(num_params),
TEE_SHM_MAPPED);
TEE_SHM_MAPPED | TEE_SHM_PRIV); if (IS_ERR(shm)) return shm;diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index ddb8f9ecf307..eac0e91ec559 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -277,7 +277,8 @@ 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(ctx, sizeof(struct optee_msg_arg),TEE_SHM_MAPPED | TEE_SHM_PRIV); if (!IS_ERR(shm)) { arg = tee_shm_get_va(shm, 0); /*diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c index d767eebf30bd..3b4a3853a10f 100644 --- a/drivers/tee/optee/shm_pool.c +++ b/drivers/tee/optee/shm_pool.c @@ -27,7 +27,11 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, shm->paddr = page_to_phys(page); shm->size = PAGE_SIZE << order;
if (shm->flags & TEE_SHM_DMA_BUF) {
/** Shared memory private to the OP-TEE driver doesn't need* to be registered with OP-TEE.*/if (!(shm->flags & TEE_SHM_PRIV)) { unsigned int nr_pages = 1 << order, i; struct page **pages;@@ -52,7 +56,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm) {
if (shm->flags & TEE_SHM_DMA_BUF)
if (!(shm->flags & TEE_SHM_PRIV)) optee_shm_unregister(shm->ctx, shm); free_pages((unsigned long)shm->kaddr, get_order(shm->size));diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index c425ad80d6a6..f8b73e734094 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -207,7 +207,7 @@ 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);
return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED);} EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
This was a malformed patch due to this hunk. I think an empty line was left out of the context.
Tyler
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h index 8990f7628387..3ebfea0781f1 100644 --- a/include/linux/tee_drv.h +++ b/include/linux/tee_drv.h @@ -27,6 +27,7 @@ #define TEE_SHM_USER_MAPPED BIT(4) /* Memory mapped in user space */ #define TEE_SHM_POOL BIT(5) /* Memory allocated from pool */ #define TEE_SHM_KERNEL_MAPPED BIT(6) /* Memory mapped in kernel space */ +#define TEE_SHM_PRIV BIT(7) /* Memory private to TEE driver */
struct device; struct tee_device;
On Sun, 13 Jun 2021 at 13:46, Tyler Hicks tyhicks@linux.microsoft.com wrote:
On 2021-06-12 13:49:38, Sumit Garg wrote:
On Fri, 11 Jun 2021 at 18:46, Tyler Hicks tyhicks@linux.microsoft.com wrote:
On 2021-06-11 08:10:01, Tyler Hicks wrote:
On 2021-06-11 10:46:20, Sumit Garg wrote:
On Fri, 11 Jun 2021 at 02:39, Tyler Hicks tyhicks@linux.microsoft.com wrote:
Uncouple the registration of kernel shared memory buffers from the TEE_SHM_DMA_BUF flag. Drivers may wish to allocate multi-page contiguous shared memory regions but do not need them to be backed by a dma-buf when the memory region is only used by the driver.
If the TEE implementation does not require shared memory to be registered, clear the flag prior to calling the corresponding pool alloc function. Update the OP-TEE driver to respect TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to (un)register on alloc/free operations.
The AMD-TEE driver continues to ignore the TEE_SHM_REGISTER flag.
That's the main point that no other TEE implementation would honour TEE_SHM_REGISTER and I think it's just the incorrect usage of TEE_SHM_REGISTER flag to suffice OP-TEE underlying implementation.
Allow callers of tee_shm_alloc_kernel_buf() to allocate and register a shared memory region without the backing of dma-buf.
Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com
drivers/tee/optee/shm_pool.c | 5 ++--- drivers/tee/tee_shm.c | 13 +++++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-)
This patch is just mixing two separate approaches to TEE shared memory. Have a look at alternative suggestions below.
diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c index da06ce9b9313..6054343a29fb 100644 --- a/drivers/tee/optee/shm_pool.c +++ b/drivers/tee/optee/shm_pool.c @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, shm->paddr = page_to_phys(page); shm->size = PAGE_SIZE << order;
if (shm->flags & TEE_SHM_DMA_BUF) {
if (shm->flags & TEE_SHM_REGISTER) {Here you can just do following check instead:
if (!(shm->flags & TEE_SHM_PRIV)) {This is a bug fix series that's intended to fix the current and older kernels. tee_shm_alloc_anon_kernel_buf()/TEE_SHM_PRIV is not present in older kernels and isn't required to fix these kexec/kdump bugs. Your suggestion feels like something that should be done in the allocator rewrite that Jens is working on to clean all of this up going forward.
I want to add that I do fully agree with you that TEE_SHM_REGISTER is an OP-TEE thing and not a TEE thing. Ideally, it wouldn't be defined in tee_drv.h and would be completely private to the OP-TEE driver. Likewise, I don't think that tee_shm_register() should exist (certainly not at the TEE level) because it only works with OP-TEE.
I think there is some confusion going on. tee_shm_register() is a standard interface that is listed in TEE client API specification as an alternative approach to tee_shm_alloc(). As I earlier mentioned, please read through "3.2.4. Shared Memory" in TEE Client API Specification.
Thanks for the reminder to go read this spec. I had forgotten to read it after you previously mentioned it.
Yes, there was confusion on my part due to reading the code but not discovering/reading the spec prior to now.
In the initial times, OP-TEE only supported tee_shm_alloc() approach but with the addition of dynamic shared memory feature it became possible to support tee_shm_register() as well but we had to add new capability as TEE_GEN_CAP_REG_MEM in order to maintain OP-TEE backwards compatibility. It can very well be the same case for AMD-TEE which currently only supports tee_shm_alloc() approach.
The reason for confusion here seems to be that OP-TEE driver is providing a way to leverage dynamic shared memory approach in order to implement tee_shm_alloc() but that doesn't mean at TEE level we should intermix both approaches via using TEE_SHM_REGISTER to implement tee_shm_alloc().
I think that was the reason for my confusion. I didn't understand why AMD-TEE didn't have the same need to register memory.
That said, I think the first step is to fix the kexec/kdump bugs and the second step is to clean up the code to remove the layering violation of exposing shm registration from the TEE interfaces.
Doesn't the following patch sound suitable to be backported to a stable kernel? It has even less changes compared to your patch as well :).
-Sumit
Subject: [PATCH] tee: Correct inappropriate usage of TEE_SHM_DMA_BUF flag
Currently TEE_SHM_DMA_BUF flag has been inappropriately used to not
I think the "not" at the end of this line should be removed, right?
No, actually shared memory allocated for OP-TEE driver internal usage didn't enable this flag while in all other invocations TEE_SHM_DMA_BUF is enabled as the driver's private shared memory isn't required to be registered with OP-TEE.
register shared memory allocated for private usage by underlying TEE driver: OP-TEE in this case. So rather add a new flag as TEE_SHM_PRIV that can be utilized by underlying TEE drivers for private allocation and usage of shared memory.
With this corrected, allow tee_shm_alloc_kernel_buf() to allocate a shared memory region without the backing of dma-buf.
I noticed a couple things wrong with this patch during testing.
Yeah you are right. Apologies for my oversight as this patch was compile tested only.
The first is obviously an oversight. tee_shm_alloc() needs to be updated to accept the TEE_SHM_PRIV or it'll throw an "invalid shm flags ..." error.
Agree.
The second issue is a little more unclear. I am testing these patches along with a debugging patch that prints a bit of info when optee_shm_register() or optee_shm_unregister() is called so that I can track the (un)registrations. I built a kernel with my v4 series but with this patch in place of my 'tee: Support kernel shm registration without dma-buf backing' patch. I performed 10x kexec operations and then let the system set idle for a while. After a while of sitting idle, I noticed a couple calls to optee_shm_register() that I hadn't seen before (and haven't seen again). It made me worried that your patch could result in us registering shared memory that we previously weren't registering so I decided to try to perform another kexec operation to ensure that the two shms were properly unregistered. At this point, the system became unresponsive and I wasn't able to get a stack trace or any more useful information about what happened.
Unfortunately, my debugging patch was only printing the shm's virtual address and the shm's size. The shm's size was 4096 bytes.
My current thought is that the two new/unexpected calls to optee_shm_register() were triggered by one of the tee_shm_alloc()'s in drivers/tee/optee/rpc.c. Neither of those shm allocations would have been registered before your change but they both would be after your change. I think the easy fix is to use the TEE_SHM_PRIV flag for both of the allocations in rpc.c. Do you agree? If so, I'll make these changes and fold this patch into my series and send out a v5.
Yeah I agree, please use TEE_SHM_PRIV for private shm allocations in drivers/tee/optee/rpc.c as well.
I still can't explain why the system became unresponsive after the two registrations...
Probably the two registrations created a cyclic loop and cores got hung in there.
-Sumit
Signed-off-by: Sumit Garg sumit.garg@linaro.org
drivers/tee/optee/call.c | 2 +- drivers/tee/optee/core.c | 3 ++- drivers/tee/optee/shm_pool.c | 8 ++++++-- drivers/tee/tee_shm.c | 2 +- include/linux/tee_drv.h | 1 + 5 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c index 6132cc8d014c..faaa13c9172b 100644 --- a/drivers/tee/optee/call.c +++ b/drivers/tee/optee/call.c @@ -184,7 +184,7 @@ static struct tee_shm *get_msg_arg(struct tee_context *ctx, size_t num_params, struct optee_msg_arg *ma;
shm = tee_shm_alloc(ctx, OPTEE_MSG_GET_ARG_SIZE(num_params),
TEE_SHM_MAPPED);
TEE_SHM_MAPPED | TEE_SHM_PRIV); if (IS_ERR(shm)) return shm;diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index ddb8f9ecf307..eac0e91ec559 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -277,7 +277,8 @@ 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(ctx, sizeof(struct optee_msg_arg),TEE_SHM_MAPPED | TEE_SHM_PRIV); if (!IS_ERR(shm)) { arg = tee_shm_get_va(shm, 0); /*diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c index d767eebf30bd..3b4a3853a10f 100644 --- a/drivers/tee/optee/shm_pool.c +++ b/drivers/tee/optee/shm_pool.c @@ -27,7 +27,11 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, shm->paddr = page_to_phys(page); shm->size = PAGE_SIZE << order;
if (shm->flags & TEE_SHM_DMA_BUF) {
/** Shared memory private to the OP-TEE driver doesn't need* to be registered with OP-TEE.*/if (!(shm->flags & TEE_SHM_PRIV)) { unsigned int nr_pages = 1 << order, i; struct page **pages;@@ -52,7 +56,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm) {
if (shm->flags & TEE_SHM_DMA_BUF)
if (!(shm->flags & TEE_SHM_PRIV)) optee_shm_unregister(shm->ctx, shm); free_pages((unsigned long)shm->kaddr, get_order(shm->size));diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index c425ad80d6a6..f8b73e734094 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -207,7 +207,7 @@ 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);
return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED);} EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
This was a malformed patch due to this hunk. I think an empty line was left out of the context.
Tyler
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h index 8990f7628387..3ebfea0781f1 100644 --- a/include/linux/tee_drv.h +++ b/include/linux/tee_drv.h @@ -27,6 +27,7 @@ #define TEE_SHM_USER_MAPPED BIT(4) /* Memory mapped in user space */ #define TEE_SHM_POOL BIT(5) /* Memory allocated from pool */ #define TEE_SHM_KERNEL_MAPPED BIT(6) /* Memory mapped in kernel space */ +#define TEE_SHM_PRIV BIT(7) /* Memory private to TEE driver */
struct device; struct tee_device;
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: 09e574831b27 ("tpm/tpm_ftpm_tee: A driver for firmware TPM running inside TEE") Fixes: 1760eb689ed6 ("tpm/tpm_ftpm_tee: add shutdown call back") Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com --- drivers/char/tpm/tpm_ftpm_tee.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c index 2ccdf8ac6994..6e3235565a4d 100644 --- a/drivers/char/tpm/tpm_ftpm_tee.c +++ b/drivers/char/tpm/tpm_ftpm_tee.c @@ -254,11 +254,11 @@ static int ftpm_tee_probe(struct device *dev) pvt_data->session = sess_arg.session;
/* Allocate dynamic shared memory with fTPM TA */ - pvt_data->shm = tee_shm_alloc(pvt_data->ctx, - MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE, - TEE_SHM_MAPPED | TEE_SHM_DMA_BUF); + pvt_data->shm = tee_shm_alloc_kernel_buf(pvt_data->ctx, + MAX_COMMAND_SIZE + + MAX_RESPONSE_SIZE); if (IS_ERR(pvt_data->shm)) { - dev_err(dev, "%s: tee_shm_alloc failed\n", __func__); + dev_err(dev, "%s: tee_shm_alloc_kernel_buf failed\n", __func__); rc = -ENOMEM; goto out_shm_alloc; }
On Thu, Jun 10, 2021 at 04:09:12PM -0500, Tyler Hicks wrote:
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: 09e574831b27 ("tpm/tpm_ftpm_tee: A driver for firmware TPM running inside TEE") Fixes: 1760eb689ed6 ("tpm/tpm_ftpm_tee: add shutdown call back") Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com
Acked-by: Jarkko Sakkinen jarkko@kernel.org
drivers/char/tpm/tpm_ftpm_tee.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c index 2ccdf8ac6994..6e3235565a4d 100644 --- a/drivers/char/tpm/tpm_ftpm_tee.c +++ b/drivers/char/tpm/tpm_ftpm_tee.c @@ -254,11 +254,11 @@ static int ftpm_tee_probe(struct device *dev) pvt_data->session = sess_arg.session; /* Allocate dynamic shared memory with fTPM TA */
- pvt_data->shm = tee_shm_alloc(pvt_data->ctx,
MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE,TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
- pvt_data->shm = tee_shm_alloc_kernel_buf(pvt_data->ctx,
MAX_COMMAND_SIZE +MAX_RESPONSE_SIZE);
dev_err(dev, "%s: tee_shm_alloc failed\n", __func__);
dev_err(dev, "%s: tee_shm_alloc_kernel_buf failed\n", __func__);-- 2.25.1
/Jarkko
From: Allen Pais apais@linux.microsoft.com
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") Signed-off-by: Allen Pais apais@linux.microsoft.com Co-developed-by: Tyler Hicks tyhicks@linux.microsoft.com Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com --- drivers/firmware/broadcom/tee_bnxt_fw.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/firmware/broadcom/tee_bnxt_fw.c b/drivers/firmware/broadcom/tee_bnxt_fw.c index ed10da5313e8..a5bf4c3f6dc7 100644 --- a/drivers/firmware/broadcom/tee_bnxt_fw.c +++ b/drivers/firmware/broadcom/tee_bnxt_fw.c @@ -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, }, };
op-tee@lists.trustedfirmware.org
-
Jarkko Sakkinen -
Jens Wiklander -
Sumit Garg -
Tyler Hicks