Git Product home page Git Product logo

Comments (13)

charles1129 avatar charles1129 commented on May 18, 2024 1

Wentao Sun [email protected]
Thank you

from optee_os.

jenswi-linaro avatar jenswi-linaro commented on May 18, 2024

It seems that the thread holding tee_ta_mutex either is killed by the OOM killer or perhaps is blocked by something else in the kernel driver.

from optee_os.

charles1129 avatar charles1129 commented on May 18, 2024

hi @jenswi-linaro

I find that the deadlock reason is session ref_count != 1.

image

so I think should put session before close when panicked, right?

diff --git a/core/kernel/tee_ta_manager.c b/core/kernel/tee_ta_manager.c
index b9fd3201e..9e2c84cbe 100644
--- a/core/kernel/tee_ta_manager.c
+++ b/core/kernel/tee_ta_manager.c
@@ -727,6 +727,7 @@ TEE_Result tee_ta_open_session(TEE_ErrorOrigin *err,

        if (!ctx || ctx->panicked) {
                DMSG("panicked, call tee_ta_close_session()");
+               tee_ta_put_session(s);
                tee_ta_close_session(s, open_sessions, KERN_IDENTITY);
                *err = TEE_ORIGIN_TEE;
                return TEE_ERROR_TARGET_DEAD;

Thanks

from optee_os.

jenswi-linaro avatar jenswi-linaro commented on May 18, 2024

Tricky! Yes, it looks like a tee_ta_put_session() is missing. How about using the common exit path of the function as:

--- a/core/kernel/tee_ta_manager.c
+++ b/core/kernel/tee_ta_manager.c
@@ -745,9 +745,9 @@ TEE_Result tee_ta_open_session(TEE_ErrorOrigin *err,
 
 	if (!ctx || ctx->panicked) {
 		DMSG("panicked, call tee_ta_close_session()");
-		tee_ta_close_session(s, open_sessions, KERN_IDENTITY);
-		*err = TEE_ORIGIN_TEE;
-		return TEE_ERROR_TARGET_DEAD;
+		res = TEE_ERROR_TARGET_DEAD;
+		panicked = true;
+		goto out;
 	}
 
 	*sess = s;
@@ -768,6 +768,7 @@ TEE_Result tee_ta_open_session(TEE_ErrorOrigin *err,
 	panicked = ctx->panicked;
 	s->param = NULL;
 
+out:
 	/*
 	 * Origin error equal to TEE_ORIGIN_TRUSTED_APP for "regular" error,
 	 * apart from panicking.

Does that work?

from optee_os.

charles1129 avatar charles1129 commented on May 18, 2024

hi @jenswi-linaro ,
I used your patch, and there has been no deadlock issue by now. But there is other issue:
thread 2 destroyed ctx, but thread 1 still in enter_invoke_cmd, TEE will panic somewhere.

I try to fix it as:

diff --git a/core/kernel/tee_ta_manager.c b/core/kernel/tee_ta_manager.c
index b9fd3201e..0f5fd6879 100644
--- a/core/kernel/tee_ta_manager.c
+++ b/core/kernel/tee_ta_manager.c
@@ -793,14 +794,14 @@ TEE_Result tee_ta_invoke_command(TEE_ErrorOrigin *err,
        }

        ta_ctx = ts_to_ta_ctx(ts_ctx);
-       if (ta_ctx->panicked) {
-               DMSG("Panicked !");
-               destroy_ta_ctx_from_session(sess);
+       if (!ta_ctx) {
                *err = TEE_ORIGIN_TEE;
                return TEE_ERROR_TARGET_DEAD;
        }

        tee_ta_set_busy(ta_ctx);
+       if (ta_ctx->panicked)
+               goto out;

        sess->param = param;
        set_invoke_timeout(sess, cancel_req_to);
@@ -809,7 +810,9 @@ TEE_Result tee_ta_invoke_command(TEE_ErrorOrigin *err,
        sess->param = NULL;
        tee_ta_clear_busy(ta_ctx);

+out:
        if (ta_ctx->panicked) {
+               DMSG("Panicked !");
                destroy_ta_ctx_from_session(sess);
                *err = TEE_ORIGIN_TEE;
                return TEE_ERROR_TARGET_DEAD;

And fix the panic issues I found.

diff --git a/core/kernel/pseudo_ta.c b/core/kernel/pseudo_ta.c
index 0a85eda96..9defd0623 100644
--- a/core/kernel/pseudo_ta.c
+++ b/core/kernel/pseudo_ta.c
@@ -247,7 +247,7 @@ static const struct ts_ops pseudo_ta_ops = {

 bool is_pseudo_ta_ctx(struct ts_ctx *ctx)
 {
-       return ctx->ops == &pseudo_ta_ops;
+       return ctx && ctx->ops == &pseudo_ta_ops;
 }
diff --git a/core/include/kernel/user_mode_ctx.h b/core/include/kernel/user_mode_ctx.h
index 6d65d1470..2424808f4 100644
--- a/core/include/kernel/user_mode_ctx.h
+++ b/core/include/kernel/user_mode_ctx.h
@@ -25,8 +25,10 @@ static inline struct user_mode_ctx *to_user_mode_ctx(struct ts_ctx *ctx)
                return &to_user_ta_ctx(ctx)->uctx;
        else if (is_sp_ctx(ctx))
                return &to_sp_ctx(ctx)->uctx;
-       else
+       else if (is_stmm_ctx(ctx))
                return &to_stmm_ctx(ctx)->uctx;
+       else
+               panic("no such ctx");
 }

System will panic at here, I think this is as expected.

to_user_mode_ctx: panic("no such ctx");
ts_to_ta_ctx: panic("bad context");

How do you think of this?
By the way, do you think the same issue will appear in the open or close processes?

Thanks

from optee_os.

jenswi-linaro avatar jenswi-linaro commented on May 18, 2024

It's getting hard to follow exactly what you're proposing.
Would you mind creating a PR with the changes you think are needed at the moment?
We

By the way, do you think the same issue will appear in the open or close processes?

That can't be ruled out. Do you think it's feasible to create a couple of test cases for this to put in xtest? It's not clear to me exactly how you're testing this. I can help out with this if needed.

from optee_os.

jenswi-linaro avatar jenswi-linaro commented on May 18, 2024

I've been able to create a test case to reproduce the deadlock and then also the panic. I'm investigating this error further.

from optee_os.

charles1129 avatar charles1129 commented on May 18, 2024

ok, thank you.
I find one of the panic reason is:

Thread 1 trigger panic, and running in user mode or other steps,
Then thread 2 destroy the ctx.
Thread 1 still running  with invalid ctx and panicked.

image

from optee_os.

jenswi-linaro avatar jenswi-linaro commented on May 18, 2024

It seems the handling of panicked TAs has a few races. I have a good test case to reproduce it so I'm looking into this now.

from optee_os.

jenswi-linaro avatar jenswi-linaro commented on May 18, 2024

I believe I have a fix for this in #6281. Can you confirm that it fixes the issue?

from optee_os.

charles1129 avatar charles1129 commented on May 18, 2024

ok, I'll confirm, thank you.

from optee_os.

charles1129 avatar charles1129 commented on May 18, 2024

I believe I have a fix for this in #6281. Can you confirm that it fixes the issue?

Test passed for v3.18, thank you for your help.

I'll try to apply the patch to TEE v3.8 or v2.4 next.
Is there any dependencies if I apply the patch to other version?

from optee_os.

jforissier avatar jforissier commented on May 18, 2024

@charles1129 if you want to give a Tested-by: Your Name <[email protected]> tag in the discussion for #6281 please feel free to do so and we will add it to the commit message. Thanks.

from optee_os.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.