[PATCH 0/2] optee: fix OOM seen due to tee_shm_free()

[PATCH 0/2] optee: fix OOM seen due to tee_shm_free()

[Allen Pais]

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() in optee core as well as bnxt firmware driver
to handle the release of the buffers correctly.
    
More info:
https://github.com/OP-TEE/optee_os/issues/3637

Allen Pais (2):
  optee: fix tee out of memory failure seen during kexec reboot
  firmware: tee_bnxt: implement shutdown method to handle kexec reboots

 drivers/firmware/broadcom/tee_bnxt_fw.c |  9 ++++
 drivers/tee/optee/core.c                | 69 ++++++++++++++++++-------
 2 files changed, 58 insertions(+), 20 deletions(-)

-- 
2.25.1

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

[Allen Pais]

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

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

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

[Dhananjay Phadke]

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

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

[Allen Pais]

> 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

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

[Jens Wiklander]

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

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

[Allen Pais]

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

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

[Jens Wiklander]

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

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

[Allen Pais]

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

[PATCH 2/2] firmware: tee_bnxt: implement shutdown method to handle kexec reboots

[Allen Pais]

From: Allen Pais <apais@linux.microsoft.com>

 On kexec reboot the firmware driver fails to deallocate
shm memory leading to a memory leak. Implement .shutdown()
method to handle kexec reboots and to release shm buffers
correctly.

Signed-off-by: Allen Pais <apais@linux.microsoft.com>
---
 drivers/firmware/broadcom/tee_bnxt_fw.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/firmware/broadcom/tee_bnxt_fw.c b/drivers/firmware/broadcom/tee_bnxt_fw.c
index ed10da5313e8..4c62e044a99f 100644
--- a/drivers/firmware/broadcom/tee_bnxt_fw.c
+++ b/drivers/firmware/broadcom/tee_bnxt_fw.c
@@ -242,6 +242,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 +265,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,
 	},
 };
 
-- 
2.25.1

[PATCH 0/2] optee: fix OOM seen due to tee_shm_free()

[Dhananjay Phadke]

From: Allen Pais <apais@linux.microsoft.com>

On Wed, 17 Feb 2021 14:57:12 +0530, Allen Pais wrote:
> 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() in optee core as well as bnxt firmware driver
> to handle the release of the buffers correctly.
>     
> More info:
> https://github.com/OP-TEE/optee_os/issues/3637

CC: linux-kernel@vger.kernel.org instead of linux-mips?
TEE / TrustZone is ARM.

Also, for Broadcom specific -
CC: bcm-kernel-feedback-list@broadcom.com

Re: [PATCH 0/2] optee: fix OOM seen due to tee_shm_free()

[Allen Pais]

> 
> 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() in optee core as well as bnxt firmware driver
> to handle the release of the buffers correctly.
>      
> More info:
> https://github.com/OP-TEE/optee_os/issues/3637

Jens,

   Could you please take sometime out and review the series.

Thanks.

> 
> Allen Pais (2):
>    optee: fix tee out of memory failure seen during kexec reboot
>    firmware: tee_bnxt: implement shutdown method to handle kexec reboots
> 
>   drivers/firmware/broadcom/tee_bnxt_fw.c |  9 ++++
>   drivers/tee/optee/core.c                | 69 ++++++++++++++++++-------
>   2 files changed, 58 insertions(+), 20 deletions(-)
>