diff mbox series

[1/2] optee: fix tee out of memory failure seen during kexec reboot

Message ID 20210217092714.121297-2-allen.lkml@gmail.com (mailing list archive)
State Not Applicable
Headers show
Series optee: fix OOM seen due to tee_shm_free() | expand

Commit Message

Allen Feb. 17, 2021, 9:27 a.m. UTC 
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>
---
 drivers/tee/optee/core.c | 69 ++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 20 deletions(-)

Comments

Dhananjay Phadke Feb. 18, 2021, 8:03 p.m. UTC | #1 
From: Allen Pais <allen.lkml@gmail.com>

On Wed, 17 Feb 2021 14:57:12 +0530, Allen Pais wrote:
> -	/*
> -	 * Ask OP-TEE to free all cached shared memory objects to decrease
> -	 * reference counters and also avoid wild pointers in secure world
> -	 * into the old shared memory range.
> -	 */
> -	optee_disable_shm_cache(optee);
> +	if (shutdown) {
> +		optee_disable_shm_cache(optee);
> +	} else {
> +		/*
> +		 * Ask OP-TEE to free all cached shared memory
> +		 * objects to decrease reference counters and
> +		 * also avoid wild pointers in secure world
> +		 * into the old shared memory range.
> +		 */
> +		optee_disable_shm_cache(optee);
 
Calling optee_disable_shm_cache() in both if and else. It could be
put in front of if().
Allen Pais Feb. 22, 2021, 12:45 p.m. UTC | #2 
> On Wed, 17 Feb 2021 14:57:12 +0530, Allen Pais wrote:
>> -	/*
>> -	 * Ask OP-TEE to free all cached shared memory objects to decrease
>> -	 * reference counters and also avoid wild pointers in secure world
>> -	 * into the old shared memory range.
>> -	 */
>> -	optee_disable_shm_cache(optee);
>> +	if (shutdown) {
>> +		optee_disable_shm_cache(optee);
>> +	} else {
>> +		/*
>> +		 * Ask OP-TEE to free all cached shared memory
>> +		 * objects to decrease reference counters and
>> +		 * also avoid wild pointers in secure world
>> +		 * into the old shared memory range.
>> +		 */
>> +		optee_disable_shm_cache(optee);
>   
> Calling optee_disable_shm_cache() in both if and else. It could be
> put in front of if().
> 

   Ideally, I could just use optee_remove for shutdown() too.
But it would not look good. Hence this approach.

- Allen
Jens Wiklander Feb. 23, 2021, 8:19 a.m. UTC | #3 
On Mon, Feb 22, 2021 at 06:15:08PM +0530, Allen Pais wrote:
> 
> > On Wed, 17 Feb 2021 14:57:12 +0530, Allen Pais wrote:
> > > -	/*
> > > -	 * Ask OP-TEE to free all cached shared memory objects to decrease
> > > -	 * reference counters and also avoid wild pointers in secure world
> > > -	 * into the old shared memory range.
> > > -	 */
> > > -	optee_disable_shm_cache(optee);
> > > +	if (shutdown) {
> > > +		optee_disable_shm_cache(optee);
> > > +	} else {
> > > +		/*
> > > +		 * Ask OP-TEE to free all cached shared memory
> > > +		 * objects to decrease reference counters and
> > > +		 * also avoid wild pointers in secure world
> > > +		 * into the old shared memory range.
> > > +		 */
> > > +		optee_disable_shm_cache(optee);
> > Calling optee_disable_shm_cache() in both if and else. It could be
> > put in front of if().
> > 
> 
>   Ideally, I could just use optee_remove for shutdown() too.
> But it would not look good. Hence this approach.

What is the problem with using optee_remove() for shutdown()?

Cheers,
Jens
Allen Pais Feb. 23, 2021, 4:26 p.m. UTC | #4 
>>>> -	/*
>>>> -	 * Ask OP-TEE to free all cached shared memory objects to decrease
>>>> -	 * reference counters and also avoid wild pointers in secure world
>>>> -	 * into the old shared memory range.
>>>> -	 */
>>>> -	optee_disable_shm_cache(optee);
>>>> +	if (shutdown) {
>>>> +		optee_disable_shm_cache(optee);
>>>> +	} else {
>>>> +		/*
>>>> +		 * Ask OP-TEE to free all cached shared memory
>>>> +		 * objects to decrease reference counters and
>>>> +		 * also avoid wild pointers in secure world
>>>> +		 * into the old shared memory range.
>>>> +		 */
>>>> +		optee_disable_shm_cache(optee);
>>> Calling optee_disable_shm_cache() in both if and else. It could be
>>> put in front of if().
>>>
>>
>>    Ideally, I could just use optee_remove for shutdown() too.
>> But it would not look good. Hence this approach.
> 
> What is the problem with using optee_remove() for shutdown()?
> 

  There is no problem, I just thought it would be more cleaner/readable
with this approach. If you'd like to keep it simple by just calling
optee_remove() for shutdown() too, I could quickly send out V2.

Thanks for the review.

- Allen
Jens Wiklander Feb. 24, 2021, 8:15 a.m. UTC | #5 
On Tue, Feb 23, 2021 at 09:56:13PM +0530, Allen Pais wrote:
> 
> 
> > > > > -	/*
> > > > > -	 * Ask OP-TEE to free all cached shared memory objects to decrease
> > > > > -	 * reference counters and also avoid wild pointers in secure world
> > > > > -	 * into the old shared memory range.
> > > > > -	 */
> > > > > -	optee_disable_shm_cache(optee);
> > > > > +	if (shutdown) {
> > > > > +		optee_disable_shm_cache(optee);
> > > > > +	} else {
> > > > > +		/*
> > > > > +		 * Ask OP-TEE to free all cached shared memory
> > > > > +		 * objects to decrease reference counters and
> > > > > +		 * also avoid wild pointers in secure world
> > > > > +		 * into the old shared memory range.
> > > > > +		 */
> > > > > +		optee_disable_shm_cache(optee);
> > > > Calling optee_disable_shm_cache() in both if and else. It could be
> > > > put in front of if().
> > > > 
> > > 
> > >    Ideally, I could just use optee_remove for shutdown() too.
> > > But it would not look good. Hence this approach.
> > 
> > What is the problem with using optee_remove() for shutdown()?
> > 
> 
>  There is no problem, I just thought it would be more cleaner/readable
> with this approach. If you'd like to keep it simple by just calling
> optee_remove() for shutdown() too, I could quickly send out V2.

In the patch you posted it looks like you'd like to call
only optee_disable_shm_cache() in the case of shutdown. Like:

static void optee_shutdown(struct platform_device *pdev)
{
        optee_disable_shm_cache(platform_get_drvdata(pdev));
}

and optee_remove() kept as it was before this patch.

Cheers,
Jens
Allen Pais Feb. 24, 2021, 4:53 p.m. UTC | #6 
>>>>>> -	/*
>>>>>> -	 * Ask OP-TEE to free all cached shared memory objects to decrease
>>>>>> -	 * reference counters and also avoid wild pointers in secure world
>>>>>> -	 * into the old shared memory range.
>>>>>> -	 */
>>>>>> -	optee_disable_shm_cache(optee);
>>>>>> +	if (shutdown) {
>>>>>> +		optee_disable_shm_cache(optee);
>>>>>> +	} else {
>>>>>> +		/*
>>>>>> +		 * Ask OP-TEE to free all cached shared memory
>>>>>> +		 * objects to decrease reference counters and
>>>>>> +		 * also avoid wild pointers in secure world
>>>>>> +		 * into the old shared memory range.
>>>>>> +		 */
>>>>>> +		optee_disable_shm_cache(optee);
>>>>> Calling optee_disable_shm_cache() in both if and else. It could be
>>>>> put in front of if().
>>>>>
>>>>
>>>>     Ideally, I could just use optee_remove for shutdown() too.
>>>> But it would not look good. Hence this approach.
>>>
>>> What is the problem with using optee_remove() for shutdown()?
>>>
>>
>>   There is no problem, I just thought it would be more cleaner/readable
>> with this approach. If you'd like to keep it simple by just calling
>> optee_remove() for shutdown() too, I could quickly send out V2.
> 
> In the patch you posted it looks like you'd like to call
> only optee_disable_shm_cache() in the case of shutdown. Like:
> 
> static void optee_shutdown(struct platform_device *pdev)
> {
>          optee_disable_shm_cache(platform_get_drvdata(pdev));
> }
> 
> and optee_remove() kept as it was before this patch.
> 

  Sure, Will have it fixed and send out V2.

Thanks.
diff mbox series

Patch

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index cf4718c6d35d..b402e5eace7b 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -582,36 +582,64 @@  static optee_invoke_fn *get_invoke_func(struct device *dev)
 	return ERR_PTR(-EINVAL);
 }
 
-static int optee_remove(struct platform_device *pdev)
+static int __optee_shutoff(struct platform_device *pdev, bool shutdown)
 {
 	struct optee *optee = platform_get_drvdata(pdev);
 
-	/*
-	 * Ask OP-TEE to free all cached shared memory objects to decrease
-	 * reference counters and also avoid wild pointers in secure world
-	 * into the old shared memory range.
-	 */
-	optee_disable_shm_cache(optee);
+	if (shutdown) {
+		optee_disable_shm_cache(optee);
+	} else {
+		/*
+		 * Ask OP-TEE to free all cached shared memory
+		 * objects to decrease reference counters and
+		 * also avoid wild pointers in secure world
+		 * into the old shared memory range.
+		 */
+		optee_disable_shm_cache(optee);
 
-	/*
-	 * The two devices have to be unregistered before we can free the
-	 * other resources.
-	 */
-	tee_device_unregister(optee->supp_teedev);
-	tee_device_unregister(optee->teedev);
+		/*
+		 * The two devices have to be unregistered before
+		 * we can free the other resources.
+		 */
+		tee_device_unregister(optee->supp_teedev);
+		tee_device_unregister(optee->teedev);
 
-	tee_shm_pool_free(optee->pool);
-	if (optee->memremaped_shm)
-		memunmap(optee->memremaped_shm);
-	optee_wait_queue_exit(&optee->wait_queue);
-	optee_supp_uninit(&optee->supp);
-	mutex_destroy(&optee->call_queue.mutex);
+		tee_shm_pool_free(optee->pool);
+		if (optee->memremaped_shm)
+			memunmap(optee->memremaped_shm);
+		optee_wait_queue_exit(&optee->wait_queue);
+		optee_supp_uninit(&optee->supp);
+		mutex_destroy(&optee->call_queue.mutex);
 
-	kfree(optee);
+		kfree(optee);
+	}
 
 	return 0;
 }
 
+/* optee_remove - Device Removal Routine
+ * @pdev: platform device information struct
+ *
+ * optee_remove is called by platform subsystem to alter the driver
+ * that it should release the device
+ */
+static int optee_remove(struct platform_device *pdev)
+{
+	return __optee_shutoff(pdev, false);
+}
+
+/* optee_shutdown - Device Removal Routine
+ * @pdev: platform device information struct
+ *
+ * platform_shutdown is called by the platform subsystem to alter
+ * the driver that a shutdown/reboot(or kexec) is happening and
+ * device must be disabled.
+ */
+static void optee_shutdown(struct platform_device *pdev)
+{
+	__optee_shutoff(pdev, true);
+}
+
 static int optee_probe(struct platform_device *pdev)
 {
 	optee_invoke_fn *invoke_fn;
@@ -738,6 +766,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,