Re: [PATCH] tee: optee: Add support for supplicant timeout

[Jens Wiklander]

On Mon, Jan 20, 2025 at 10:24 AM Sumit Garg <sumit.garg@xxxxxxxxxx> wrote:
>
> Hi Jens,
>
> On Fri, 13 Dec 2024 at 16:45, Sumit Garg <sumit.garg@xxxxxxxxxx> wrote:
> >
> > OP-TEE supplicant is a user-space daemon and it's possible for it
> > being crashed or killed in the middle of processing an OP-TEE RPC call.
> > It becomes more complicated when there is incorrect shutdown ordering
> > of the supplicant process vs the OP-TEE client application which can
> > eventually lead to system hang-up waiting for the closure of the client
> > application.
> >
> > In order to gracefully handle this scenario, let's add a long enough
> > timeout to wait for supplicant to process requests. In case there is a
> > timeout then we return a proper error code for the RPC request.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@xxxxxxxxxx>
> > ---
> >  drivers/tee/optee/supp.c | 58 +++++++++++++++++++++++++---------------
> >  1 file changed, 36 insertions(+), 22 deletions(-)
> >
>
> Do you have any further comments here? Or is it fine for you to pick it up?

I don't like the timeout, it's a bit too hacky.

Cheers,
Jens

>
> -Sumit
>
> > diff --git a/drivers/tee/optee/supp.c b/drivers/tee/optee/supp.c
> > index 322a543b8c27..92e86ac4cdd4 100644
> > --- a/drivers/tee/optee/supp.c
> > +++ b/drivers/tee/optee/supp.c
> > @@ -7,6 +7,15 @@
> >  #include <linux/uaccess.h>
> >  #include "optee_private.h"
> >
> > +/*
> > + * OP-TEE supplicant timeout, the user-space supplicant may get
> > + * crashed or killed while servicing an RPC call. This will just lead
> > + * to OP-TEE client hung indefinitely just waiting for supplicant to
> > + * serve requests which isn't expected. It is rather expected to fail
> > + * gracefully with a timeout which is long enough.
> > + */
> > +#define SUPP_TIMEOUT   (msecs_to_jiffies(10000))
> > +
> >  struct optee_supp_req {
> >         struct list_head link;
> >
> > @@ -52,8 +61,10 @@ void optee_supp_release(struct optee_supp *supp)
> >
> >         /* Abort all queued requests */
> >         list_for_each_entry_safe(req, req_tmp, &supp->reqs, link) {
> > -               list_del(&req->link);
> > -               req->in_queue = false;
> > +               if (req->in_queue) {
> > +                       list_del(&req->link);
> > +                       req->in_queue = false;
> > +               }
> >                 req->ret = TEEC_ERROR_COMMUNICATION;
> >                 complete(&req->c);
> >         }
> > @@ -82,6 +93,7 @@ u32 optee_supp_thrd_req(struct tee_context *ctx, u32 func, size_t num_params,
> >         struct optee_supp_req *req;
> >         bool interruptable;
> >         u32 ret;
> > +       int res = 1;
> >
> >         /*
> >          * Return in case there is no supplicant available and
> > @@ -108,28 +120,28 @@ u32 optee_supp_thrd_req(struct tee_context *ctx, u32 func, size_t num_params,
> >         /* Tell an eventual waiter there's a new request */
> >         complete(&supp->reqs_c);
> >
> > -       /*
> > -        * Wait for supplicant to process and return result, once we've
> > -        * returned from wait_for_completion(&req->c) successfully we have
> > -        * exclusive access again.
> > -        */
> > -       while (wait_for_completion_interruptible(&req->c)) {
> > +       /* Wait for supplicant to process and return result */
> > +       while (res) {
> > +               res = wait_for_completion_interruptible_timeout(&req->c,
> > +                                                               SUPP_TIMEOUT);
> > +               /* Check if supplicant served the request */
> > +               if (res > 0)
> > +                       break;
> > +
> >                 mutex_lock(&supp->mutex);
> > +               /*
> > +                * There's no supplicant available and since the supp->mutex
> > +                * currently is held none can become available until the mutex
> > +                * released again.
> > +                *
> > +                * Interrupting an RPC to supplicant is only allowed as a way
> > +                * of slightly improving the user experience in case the
> > +                * supplicant hasn't been started yet. During normal operation
> > +                * the supplicant will serve all requests in a timely manner and
> > +                * interrupting then wouldn't make sense.
> > +                */
> >                 interruptable = !supp->ctx;
> > -               if (interruptable) {
> > -                       /*
> > -                        * There's no supplicant available and since the
> > -                        * supp->mutex currently is held none can
> > -                        * become available until the mutex released
> > -                        * again.
> > -                        *
> > -                        * Interrupting an RPC to supplicant is only
> > -                        * allowed as a way of slightly improving the user
> > -                        * experience in case the supplicant hasn't been
> > -                        * started yet. During normal operation the supplicant
> > -                        * will serve all requests in a timely manner and
> > -                        * interrupting then wouldn't make sense.
> > -                        */
> > +               if (interruptable || (res == 0)) {
> >                         if (req->in_queue) {
> >                                 list_del(&req->link);
> >                                 req->in_queue = false;
> > @@ -141,6 +153,8 @@ u32 optee_supp_thrd_req(struct tee_context *ctx, u32 func, size_t num_params,
> >                         req->ret = TEEC_ERROR_COMMUNICATION;
> >                         break;
> >                 }
> > +               if (res == 0)
> > +                       req->ret = TEE_ERROR_TIMEOUT;
> >         }
> >
> >         ret = req->ret;
> > --
> > 2.43.0
> >