[PATCH] optee: Do not send requests to supplicant during shutdown

[PATCH] optee: Do not send requests to supplicant during shutdown

[Lars Persson]

[-- Attachment #1: Type: text/plain, Size: 3608 bytes --]

The addition of a shutdown hook by commit f25889f93184 ("optee:
fix tee out of memory failure seen during kexec reboot") introduced a
kernel shutdown regression that can be triggered after running the
xtest suites.

Once the shutdown hook is called it is not possible to communicate any
more with the supplicant process because the system is not scheduling
task any longer. Thus if the optee driver shutdown path receives a
supplicant RPC request from the OP-TEE we will deadlock the kernel's
shutdown.

This unexpected event will in fact occur after the xtest suite has
been run. It seems some cached SHM kept alive a context object which
in turn kept alive a session towards a PTA or TA. Closing the session
results in a socket RPC command being sent back from OP-TEE.

This sequence of events is captured by a 5.15 kernel annotated with
extra prints:

 Calling OPTEE_SMC_DISABLE_SHM_CACHE
 OPTEE_SMC_DISABLE_SHM_CACHE returned 0
  freeing SHM ptr 0xFFFFFF8001079380
 Calling OPTEE_SMC_DISABLE_SHM_CACHE
 OPTEE_SMC_DISABLE_SHM_CACHE returned 0
  freeing SHM ptr 0xFFFFFF8001CC5580
 Calling OPTEE_SMC_DISABLE_SHM_CACHE
 OPTEE_SMC_DISABLE_SHM_CACHE returned 0
  freeing SHM ptr 0xFFFFFF8006308A80
 Calling OPTEE_SMC_DISABLE_SHM_CACHE
 OPTEE_SMC_DISABLE_SHM_CACHE returned 0
  freeing SHM ptr 0xFFFFFF8006308B00
 optee: optee_handle_rpc: a0=0XFFFF0000 a1=0XA0 a2=0X0
 optee: optee_handle_rpc: a0=0XFFFF0005 a1=0XFFFFFF80 a2=0X61E6500
 optee: handle_rpc_func_cmd: cmd = 0XA
 optee_supp_thrd_req: func=0XA

Introduce a shutdown state in the optee device object to return an
immediate error to all RPC requests in the shutdown path.

Fixes: f25889f93184 ("optee: fix tee out of memory failure seen during kexec reboot
Signed-off-by: Lars Persson <larper@axis.com>
---
 drivers/tee/optee/optee_private.h | 1 +
 drivers/tee/optee/smc_abi.c       | 5 ++++-
 drivers/tee/optee/supp.c          | 8 ++++++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 46f74ab07c7e..83380974ff44 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -164,6 +164,7 @@ struct optee {
 	bool   scan_bus_done;
 	struct workqueue_struct *scan_bus_wq;
 	struct work_struct scan_bus_work;
+	bool shutting_down;
 };
 
 struct optee_session {
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
index 449d6a72d289..10af747da816 100644
--- a/drivers/tee/optee/smc_abi.c
+++ b/drivers/tee/optee/smc_abi.c
@@ -1356,7 +1356,10 @@ static int optee_smc_remove(struct platform_device *pdev)
  */
 static void optee_shutdown(struct platform_device *pdev)
 {
-	optee_disable_shm_cache(platform_get_drvdata(pdev));
+	struct optee *optee = platform_get_drvdata(pdev);
+
+	optee->shutting_down = true;
+	optee_disable_shm_cache(optee);
 }
 
 static int optee_probe(struct platform_device *pdev)
diff --git a/drivers/tee/optee/supp.c b/drivers/tee/optee/supp.c
index 322a543b8c27..801b4ec659e8 100644
--- a/drivers/tee/optee/supp.c
+++ b/drivers/tee/optee/supp.c
@@ -83,6 +83,14 @@ u32 optee_supp_thrd_req(struct tee_context *ctx, u32 func, size_t num_params,
 	bool interruptable;
 	u32 ret;
 
+	/*
+	 * When the system is shutting down we cannot talk
+	 * to the supplicant anymore even if we seem to have
+	 * an open session to it.
+	 */
+	if (optee->shutting_down)
+		return TEEC_ERROR_COMMUNICATION;
+
 	/*
 	 * Return in case there is no supplicant available and
 	 * non-blocking request.
-- 
2.30.2

Re: [PATCH] optee: Do not send requests to supplicant during shutdown

[Tyler Hicks]

[-- Attachment #1: Type: text/plain, Size: 4559 bytes --]

On 2022-01-19 18:49:33, Lars Persson wrote:
> The addition of a shutdown hook by commit f25889f93184 ("optee:
> fix tee out of memory failure seen during kexec reboot") introduced a
> kernel shutdown regression that can be triggered after running the
> xtest suites.
> 
> Once the shutdown hook is called it is not possible to communicate any
> more with the supplicant process because the system is not scheduling
> task any longer. Thus if the optee driver shutdown path receives a
> supplicant RPC request from the OP-TEE we will deadlock the kernel's
> shutdown.

The system that I'm working on, and initially developed the kexec fixes
for, doesn't use a supplicant process so that would explain why I
haven't seen this issue.

What I'm a little unclear about is why the new(ish) .shutdown hook would
be the cause for this deadlock because it doesn't disable scheduling of
tasks (unless I'm forgetting something). Does disabling the shm cache
also cause tasks to no longer be scheduled? I'm having trouble finding
the documentation that describes this operation and my memory is fuzzy
since it has been a while since I've worked on OP-TEE issues.

> This unexpected event will in fact occur after the xtest suite has
> been run. It seems some cached SHM kept alive a context object which
> in turn kept alive a session towards a PTA or TA. Closing the session
> results in a socket RPC command being sent back from OP-TEE.
> 
> This sequence of events is captured by a 5.15 kernel annotated with
> extra prints:
> 
>  Calling OPTEE_SMC_DISABLE_SHM_CACHE
>  OPTEE_SMC_DISABLE_SHM_CACHE returned 0
>   freeing SHM ptr 0xFFFFFF8001079380
>  Calling OPTEE_SMC_DISABLE_SHM_CACHE
>  OPTEE_SMC_DISABLE_SHM_CACHE returned 0
>   freeing SHM ptr 0xFFFFFF8001CC5580
>  Calling OPTEE_SMC_DISABLE_SHM_CACHE
>  OPTEE_SMC_DISABLE_SHM_CACHE returned 0
>   freeing SHM ptr 0xFFFFFF8006308A80
>  Calling OPTEE_SMC_DISABLE_SHM_CACHE
>  OPTEE_SMC_DISABLE_SHM_CACHE returned 0
>   freeing SHM ptr 0xFFFFFF8006308B00
>  optee: optee_handle_rpc: a0=0XFFFF0000 a1=0XA0 a2=0X0
>  optee: optee_handle_rpc: a0=0XFFFF0005 a1=0XFFFFFF80 a2=0X61E6500
>  optee: handle_rpc_func_cmd: cmd = 0XA
>  optee_supp_thrd_req: func=0XA
> 
> Introduce a shutdown state in the optee device object to return an
> immediate error to all RPC requests in the shutdown path.
> 
> Fixes: f25889f93184 ("optee: fix tee out of memory failure seen during kexec reboot

Minor syntax error at the end of this line as it is missing the closing
double quotes and parenthesis. 

Tyler

> Signed-off-by: Lars Persson <larper@axis.com>
> ---
>  drivers/tee/optee/optee_private.h | 1 +
>  drivers/tee/optee/smc_abi.c       | 5 ++++-
>  drivers/tee/optee/supp.c          | 8 ++++++++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 46f74ab07c7e..83380974ff44 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -164,6 +164,7 @@ struct optee {
>  	bool   scan_bus_done;
>  	struct workqueue_struct *scan_bus_wq;
>  	struct work_struct scan_bus_work;
> +	bool shutting_down;
>  };
>  
>  struct optee_session {
> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> index 449d6a72d289..10af747da816 100644
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -1356,7 +1356,10 @@ static int optee_smc_remove(struct platform_device *pdev)
>   */
>  static void optee_shutdown(struct platform_device *pdev)
>  {
> -	optee_disable_shm_cache(platform_get_drvdata(pdev));
> +	struct optee *optee = platform_get_drvdata(pdev);
> +
> +	optee->shutting_down = true;
> +	optee_disable_shm_cache(optee);
>  }
>  
>  static int optee_probe(struct platform_device *pdev)
> diff --git a/drivers/tee/optee/supp.c b/drivers/tee/optee/supp.c
> index 322a543b8c27..801b4ec659e8 100644
> --- a/drivers/tee/optee/supp.c
> +++ b/drivers/tee/optee/supp.c
> @@ -83,6 +83,14 @@ u32 optee_supp_thrd_req(struct tee_context *ctx, u32 func, size_t num_params,
>  	bool interruptable;
>  	u32 ret;
>  
> +	/*
> +	 * When the system is shutting down we cannot talk
> +	 * to the supplicant anymore even if we seem to have
> +	 * an open session to it.
> +	 */
> +	if (optee->shutting_down)
> +		return TEEC_ERROR_COMMUNICATION;
> +
>  	/*
>  	 * Return in case there is no supplicant available and
>  	 * non-blocking request.
> -- 
> 2.30.2
> 

Re: [PATCH] optee: Do not send requests to supplicant during shutdown

[Lars Persson]

[-- Attachment #1: Type: text/plain, Size: 2452 bytes --]



On 2022-01-20 02:02, Tyler Hicks wrote:
> On 2022-01-19 18:49:33, Lars Persson wrote:
>> The addition of a shutdown hook by commit f25889f93184 ("optee:
>> fix tee out of memory failure seen during kexec reboot") introduced a
>> kernel shutdown regression that can be triggered after running the
>> xtest suites.
>> 
>> Once the shutdown hook is called it is not possible to communicate any
>> more with the supplicant process because the system is not scheduling
>> task any longer. Thus if the optee driver shutdown path receives a
>> supplicant RPC request from the OP-TEE we will deadlock the kernel's
>> shutdown.
> 
> The system that I'm working on, and initially developed the kexec fixes
> for, doesn't use a supplicant process so that would explain why I
> haven't seen this issue.
> 
> What I'm a little unclear about is why the new(ish) .shutdown hook would
> be the cause for this deadlock because it doesn't disable scheduling of
> tasks (unless I'm forgetting something). Does disabling the shm cache
> also cause tasks to no longer be scheduled? I'm having trouble finding
> the documentation that describes this operation and my memory is fuzzy
> since it has been a while since I've worked on OP-TEE issues.
> 

I tried to look for an explicit disable of the scheduler starting from 
do_sys_reboot but that was impossible to find. It might be hidden 
somewhere in the use of the system_state variable.

Anyways since the do_sys_reboot system call is the last step of shutting 
down a system, any interaction with user-space will be a BadThing. The 
supplicant would typically be killed by the init process already in a 
clean shutdown, and if it wasn't, then device_shutdown() will remove the 
block devices that the file-system depends on. Any page fault in the 
supplicant would thus kill it. I am sure there is somewhere a piece of 
code that disables task scheduling before calling device_shutdown 
because so many crazy things could happen otherwise.

>> Introduce a shutdown state in the optee device object to return an
>> immediate error to all RPC requests in the shutdown path.
>> 
>> Fixes: f25889f93184 ("optee: fix tee out of memory failure seen during kexec reboot
> 
> Minor syntax error at the end of this line as it is missing the closing
> double quotes and parenthesis.
> 

Thanks. Let's see if Jens makes a fixup for this. Otherwise I can submit 
a v2.

/Lars

Re: [PATCH] optee: Do not send requests to supplicant during shutdown

[Jens Wiklander]

[-- Attachment #1: Type: text/plain, Size: 2656 bytes --]

On Wed, Jan 19, 2022 at 06:49:33PM +0100, Lars Persson wrote:
> The addition of a shutdown hook by commit f25889f93184 ("optee:
> fix tee out of memory failure seen during kexec reboot") introduced a
> kernel shutdown regression that can be triggered after running the
> xtest suites.
> 
> Once the shutdown hook is called it is not possible to communicate any
> more with the supplicant process because the system is not scheduling
> task any longer. Thus if the optee driver shutdown path receives a
> supplicant RPC request from the OP-TEE we will deadlock the kernel's
> shutdown.
> 
> This unexpected event will in fact occur after the xtest suite has
> been run. It seems some cached SHM kept alive a context object which
> in turn kept alive a session towards a PTA or TA. Closing the session
> results in a socket RPC command being sent back from OP-TEE.
> 
> This sequence of events is captured by a 5.15 kernel annotated with
> extra prints:
> 
>  Calling OPTEE_SMC_DISABLE_SHM_CACHE
>  OPTEE_SMC_DISABLE_SHM_CACHE returned 0
>   freeing SHM ptr 0xFFFFFF8001079380
>  Calling OPTEE_SMC_DISABLE_SHM_CACHE
>  OPTEE_SMC_DISABLE_SHM_CACHE returned 0
>   freeing SHM ptr 0xFFFFFF8001CC5580
>  Calling OPTEE_SMC_DISABLE_SHM_CACHE
>  OPTEE_SMC_DISABLE_SHM_CACHE returned 0
>   freeing SHM ptr 0xFFFFFF8006308A80
>  Calling OPTEE_SMC_DISABLE_SHM_CACHE
>  OPTEE_SMC_DISABLE_SHM_CACHE returned 0
>   freeing SHM ptr 0xFFFFFF8006308B00
>  optee: optee_handle_rpc: a0=0XFFFF0000 a1=0XA0 a2=0X0
>  optee: optee_handle_rpc: a0=0XFFFF0005 a1=0XFFFFFF80 a2=0X61E6500
>  optee: handle_rpc_func_cmd: cmd = 0XA
>  optee_supp_thrd_req: func=0XA
> 
> Introduce a shutdown state in the optee device object to return an
> immediate error to all RPC requests in the shutdown path.
> 
> Fixes: f25889f93184 ("optee: fix tee out of memory failure seen during kexec reboot
> Signed-off-by: Lars Persson <larper@axis.com>
> ---
>  drivers/tee/optee/optee_private.h | 1 +
>  drivers/tee/optee/smc_abi.c       | 5 ++++-
>  drivers/tee/optee/supp.c          | 8 ++++++++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 46f74ab07c7e..83380974ff44 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -164,6 +164,7 @@ struct optee {
>  	bool   scan_bus_done;
>  	struct workqueue_struct *scan_bus_wq;
>  	struct work_struct scan_bus_work;
> +	bool shutting_down;

Please move this to right after "bool scan_bus_done" above to avoid
unnecessary extra padding.

Thanks,
Jens