out of memory failures in xtest due to tee_shm_free() from Linux not unregistering SHM DMA buffer in kexec path

[RajeshRavi-brcm]

Problem Description

Case A
If tee_shm_free() is called in shutdown() of Linux tee_client_driver and kexec kernel is booted, shutdown() & tee_shm_free() are invoked but tee_shm_release() is not invoked on DMA SHM buffer.

Case B
If tee_shm_free() is called on SHM DMA buffer previously allocated from rmmod path[module_exit()]
It unregisters the SHM memory and sends OPTEE_MSG_CMD_UNREGISTER_SHM to optee_os through optee_shm_unregister().

Call Sequence

Case A: kexec path
.shutdown()-->tee_shm_free()-->dma_buf_put()

Case B: rmmod path
.shutdown()-->tee_shm_free()-->dma_buf_put()()-->tee_shm_release()-->optee_shm_unregister()
-->optee_do_call_with_arg() [cmd = OPTEE_MSG_CMD_UNREGISTER_SHM]

Repercussions of the issue: xtest failure due to out of memory

If we register a big buffer of say 8MB in Linux tee client driver, if the same memory is not unregistered, it can cause overhead of 210248 = 16KB memory overhead for shm page book keeping data structures calloc'd in optee_os. After kexec it causes 16 +16=32KB which significant memory on a minimal heap of size, say 64KB. This causes failures with asymmetric crypto operations of xtest due to out of memory error.

Context
In Linux kernel tee_client_driver probe() we 're calling tee_shm_alloc() with flags=TEE_SHM_MAPPED | TEE_SHM_DMA_BUF

In remove() & shutdown() functions of the driver: we 're calling tee_shm_free() on shm reference allocated in probe.

[jenswi-linaro]

In tee_shm_free() when shm->flags & TEE_SHM_DMA_BUF we rely on dma_buf_put() to do the release. The actual release is done once the dmabuf has lost all references. So perhaps something else is referencing this dmabuf?

[vikasbrcm]

We can see only one reference is there that too should go away with dma_buf_put(). The same code path works when rmmod is called. This issue is seen in kexec execution case as shutdown in drivers is called.
Shutdown eventually land to dma_buf_put(). All TEE drivers will leak memory on OPTEE side if kexec is executed.
It looks like dma_buf_put() is not doing favor here, which could be related to dma_buf_put() only as it could not call release before shutdown(triggered by kexec) was hit.

[jenswi-linaro]

Perhaps we need a way to tell OP-TEE that normal world is starting over?

[github-actions]

This issue has been marked as a stale issue because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

[samliddicott]

It's a strange optimism that supposes bugs can fix themselves by being left alone for 35 days.

I believe that I have a related scenario where after kexec, use of tee-os either causes a kernel panic (either null de-reference or Unable to handle kernel paging request at virtual address) or a more sane:

[ 415.656408] optee: handle_rpc_func_cmd: tee_shm_get_va ffff80013af83e80 failed
[ 415.663709] optee: handle_rpc_func_cmd: tee_shm_get_va ffff80013af83e80 failed
[ 415.670990] optee: handle_rpc_func_cmd: tee_shm_get_va ffff80013af83e80 failed
E/LD: init_elf:238 sys_open_ta_bin(f4e750bb-1437-4fbf-8785-8d3580c34994)
E/TC:? 0 init_with_ldelf:279 ldelf failed with res: 0xffff000c
E/TC:? 0 tee_ta_open_session:725 Failed. Return error 0xffff000c

I'm going to try building optee as a kernel module and unloading before kexec to see if that fixes that problem

[jforissier]

It's a strange optimism that supposes bugs can fix themselves by being left alone for 35 days.

Haha who knows? 😆

Seriously though, the reason for marking issues "stale" and auto-closing is that we want the list of open issues to be somewhat representative of what people are either working on, or concerned about (so that others can possibly pick up meaningful tasks and contribute). By that logic, a closed issue is not necessarily a fixed one. [Edit] Also note that issues are not always bugs: they may be questions or configuration mistakes etc.

I'm going to try building optee as a kernel module and unloading before kexec to see if that fixes that problem

Sure, as always, contributions are welcome. Please do not hesitate to re-open this issue if you are actively investigating it.

[jbech-linaro]

I second what @jforissier said, but I have thought about updating the bot to not close things that have a certain label ("bug" for example). The core team of OP-TEE are just a few persons, who are involved in other things too at the companies where they are working. So even if we want too, it's impossible to jump on every single issue being mentioned. Therefore we and the OP-TEE project as such are very grateful when other developers pick up certain issues and try to chime in.

This is also one of the reasons starting with the bot, we had too many questions and open issues that never got closed, so it was almost impossible to keep track of what made sense to look into. A common theme is that people ask questions, but don't close the issue when they've got the answer or that we ask for more information and we never hear anything again. A pure bug report happens more seldom. Pure bugs should have the "bug" label I think.

[jforissier]

I have thought about updating the bot to not close things that have a certain label ("bug" for example)

+1 that's a good idea IMO

[samliddicott]

I confirm that unloaded the optee module before kexec means that optee works for me again after kexec.

I don't know if there is a kernel -pre-kexec subscription that optee module could register with, which would give it time to say goodbye to tee-os nicely. I would hope tee-os would accept a synchronous "I'm outta here" message from linux kernel.

I think that leaving tee-supplicant running during kexec may be what led to the kernel segfault and merely leaving optee module loaded may be what led to non-fatal failure to function, but that needs more testing.

But certainly, making optee a loadable module, and stopping tee-supplicant and unloading the modules before kexec appears to have solved the immediate problem for me.

[jforissier]

I confirm that unloaded the optee module before kexec means that optee works for me again after kexec.

OK, good.

I don't know if there is a kernel -pre-kexec subscription that optee module could register with, which would give it time to say goodbye to tee-os nicely. I would hope tee-os would accept a synchronous "I'm outta here" message from linux kernel.

What about this?

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 99698b8a3a74..5426b07a80cb 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -710,6 +710,7 @@ MODULE_DEVICE_TABLE(of, optee_dt_match);
 static struct platform_driver optee_driver = {
	.probe  = optee_probe,
	.remove = optee_remove,
+	.shutdown = optee_remove,
	.driver = {
		.name = "optee",
		.of_match_table = optee_dt_match,

From https://patchwork.kernel.org/patch/9568549/ and http://lkml.iu.edu/hypermail/linux/kernel/1607.2/04442.html I believe it is what we're after here.

Perhaps a more optimized version would be to release only secure world resources, something like that:

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 99698b8a3a74..00b893a51222 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -584,6 +584,15 @@ static int optee_remove(struct platform_device *pdev)
	return 0;
 }
 
+static int optee_shutdown(struct platform_device *pdev)
+{
+	struct optee *optee = platform_get_drvdata(pdev);
+
+	optee_disable_shm_cache(optee);
+
+	return 0;
+}
+
 static int optee_probe(struct platform_device *pdev)
 {
	optee_invoke_fn *invoke_fn;
@@ -710,6 +719,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,

[samliddicott]

Great comments @jforissier and some good lkml conversation there.

Things seem to have moved on considerably since then, and optee module is no longer a platform module with a platform_driver struct registered with module_platform_driver

The init and exit functions are registered with module_init() and module_exit() macros with no module_shutdown or use other useful macro variants available.

I guess the module_exit() ought to have appropriate effect with kexec but doesn't (lklm mailing list discussion bore no fruit).

the rmmod shutdown path naturally requires that all device instances are already closed and so the exit function is sufficient.

So after a few hours, I can't find any way or anywhere even to link in optee_disable_shm_cache(optee) on a kexec path.

No doubt the cdev is a clue, but I don't see how to attach a shutdown handler there yet either; and I'm not sure that it would easily help as we'd only want to invoke optee_disable_shm_cache once but there could possibly be multiple cdev registered.

[jforissier]

Things seem to have moved on considerably since then, and optee module is no longer a platform module with a platform_driver struct registered with module_platform_driver

What is your reference? The patches I have proposed are based on upstream master (v5.8-rc2-64-g8be3a53e18e0).

[samliddicott]

Thanks for the correction.

I'm using a 4.14 marvell branch.

elixir.bootlin.com doesn't go beyond 5.7 right now which explains why I couldn't find anything that looked like yours.

[jforissier]

elixir.bootlin.com doesn't go beyond 5.7 right now which explains why I couldn't find anything that looked like yours.

No, you did not look closely enough instead ;-)
https://elixir.bootlin.com/linux/v5.7.2/source/drivers/tee/optee/core.c#L710

[samliddicott]

How right you are! Nothing I said appears to have been reliable.

I'll slink away in shame.

I can't trust myself to safely work out what could have been going through my mind.

thanks for being nice about it

[jforissier]

@samliddicott haha don't be too hard on yourself! ;-)

Anyway, If something needs to be fixed in the OP-TEE driver for kexec support, it should preferably go to the LKML directly. Thanks.

[bgooty]

@jforissier Would like to know if you have plans to push the optimized optee_shutdown() to LKML?
Thanks,
-Bharat

[sbranden]

@jforissier Would like to know if you have plans to push the optimized optee_shutdown() to LKML?
Thanks,
-Bharat

This patch does appear to solve our issues.

[bgooty]

I confirm that unloaded the optee module before kexec means that optee works for me again after kexec.

OK, good.

I don't know if there is a kernel -pre-kexec subscription that optee module could register with, which would give it time to say goodbye to tee-os nicely. I would hope tee-os would accept a synchronous "I'm outta here" message from linux kernel.

What about this?

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 99698b8a3a74..5426b07a80cb 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -710,6 +710,7 @@ MODULE_DEVICE_TABLE(of, optee_dt_match);
 static struct platform_driver optee_driver = {
	.probe  = optee_probe,
	.remove = optee_remove,
+	.shutdown = optee_remove,
	.driver = {
		.name = "optee",
		.of_match_table = optee_dt_match,

From https://patchwork.kernel.org/patch/9568549/ and http://lkml.iu.edu/hypermail/linux/kernel/1607.2/04442.html I believe it is what we're after here.

Perhaps a more optimized version would be to release only secure world resources, something like that:

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 99698b8a3a74..00b893a51222 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -584,6 +584,15 @@ static int optee_remove(struct platform_device *pdev)
	return 0;
 }
 
+static int optee_shutdown(struct platform_device *pdev)
+{
+	struct optee *optee = platform_get_drvdata(pdev);
+
+	optee_disable_shm_cache(optee);
+
+	return 0;
+}
+
 static int optee_probe(struct platform_device *pdev)
 {
	optee_invoke_fn *invoke_fn;
@@ -710,6 +719,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,

@jforissier I am not sure whether you have seen my previous message.
Do you have any plans to push this changes to LKML?
Thanks,
-Bharat

[jforissier]

@bgooty sorry for the late reply :-/
Please feel free to send the patch to LKML yourself -- you will probably provide a better description than me. If you want you may add a Suggested-by: Jerome Forissier <jerome@forissier.org>.

[bgooty]

@bgooty sorry for the late reply :-/
Please feel free to send the patch to LKML yourself -- you will probably provide a better description than me. If you want you may add a Suggested-by: Jerome Forissier <jerome@forissier.org>.
Thanks @jforissier . Let me attempt to push to LKML and keep you in CC.
As you suggested, I will add "Suggested-by: Jerome Forissier jerome@forissier.org" in the commit

[tyhicks]

@bgooty hey - I don't see the patch submission on LKML or the optee list archives so I wanted to give you a poke. Please cc me (Tyler Hicks tyhicks@linux.microsoft.com) on your submission and I'll give it a review. Thanks!

[bgooty]

@bgooty hey - I don't see the patch submission on LKML or the optee list archives so I wanted to give you a poke. Please cc me (Tyler Hicks tyhicks@linux.microsoft.com) on your submission and I'll give it a review. Thanks!

Sorry, adding optee_shutdown() does not fix the issue.

[allenpais]

@bgooty Is an alternate fix in the works considering optee_shutdown() did not solve the problem. Thanks.

[tyhicks]

A fix for this and other kexec/kdump related issues with OP-TEE is available here:

https://lore.kernel.org/lkml/20210614223317.999867-1-tyhicks@linux.microsoft.com/

[jforissier]

@tyhicks thanks for the update. I intend to apply the series onto linaro-swg/linux branch optee as soon as it reaches mainline.

[jenswi-linaro]

I created linaro-swg/linux#94 to get some more automated testing of these patches.