On Thu, 11 May 2023 at 12:50, Etienne Carriere etienne.carriere@linaro.org wrote:
On Thu, 11 May 2023 at 08:03, Sumit Garg sumit.garg@linaro.org wrote:
(snip)
int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, @@ -408,12 +412,15 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, struct optee_msg_arg *msg_arg; struct optee_session *sess; struct tee_shm *shm;
bool system_thread; u_int offs; int rc; /* Check that the session is valid */ mutex_lock(&ctxdata->mutex); sess = find_session(ctxdata, arg->session);if (sess)This check is redundant if we move the assignment below...
Here we change the sesssion attribute while the mutex is locked, in case some equivalent call with that session is issued. Below we return to caller once mutex is unlocked. I think it is the safer behavior. What do you think?
Aren't we only reading session attribute in order to capture value in a local variable: system_thread? I don't think that it would require a mutex.
optee_system_session() sets session::use_sys_thread with mutex locked hence I think we should get the attribute with the mutex locked. See "[PATCH v6 3/4] tee: optee: support tracking system threads".
Okay I see your point. Although I don't see a practical race between optee_invoke_func() vs optee_system_session(), you never know what complex kernel TEE client use-case comes up. So I can live with it being protected by a mutex.
-Sumit
Etienne
-Sumit
Best regards, Etienne
system_thread = sess->use_sys_thread; mutex_unlock(&ctxdata->mutex); if (!sess) return -EINVAL;...here as: system_thread = sess->use_sys_thread; (snip)