Messages in this thread | | | From | Sumit Garg <> | Date | Thu, 11 May 2023 13:09:28 +0530 | Subject | Re: [PATCH v6 1/4] tee: optee: system call property |
| |
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)
| |