Message-ID: <CAHUa44EZ2r=QRcsWfFs-ZqnTYEOpB4Ou4qpk4Y49NEPh-TTCeg@mail.gmail.com>
Date: Tue, 26 Nov 2024 09:32:56 +0100
From: Jens Wiklander <jens.wiklander@...aro.org>
To: Amirreza Zarrabi <quic_azarrabi@...cinc.com>
Cc: Sumit Garg <sumit.garg@...aro.org>, op-tee@...ts.trustedfirmware.org, 
	linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH RFC 3/3] tee: introduce orphan tee_shm and default context

On Mon, Nov 25, 2024 at 9:55 PM Amirreza Zarrabi
<quic_azarrabi@...cinc.com> wrote:
>
>
>
> On 11/25/2024 6:51 PM, Sumit Garg wrote:
> > On Mon, 25 Nov 2024 at 12:53, Jens Wiklander <jens.wiklander@...aro.org> wrote:
> >>
> >> On Mon, Nov 25, 2024 at 7:14 AM Sumit Garg <sumit.garg@...aro.org> wrote:
> >>>
> >>> On Mon, 25 Nov 2024 at 03:00, Amirreza Zarrabi
> >>> <quic_azarrabi@...cinc.com> wrote:
> >>>>
> >>>>
> >>>> Hi Sumit,
> >>>>
> >>>> Thank you so much for the comemnts :).
> >>>>
> >>>> On 11/23/2024 9:32 PM, Sumit Garg wrote:
> >>>>> Hi Amirreza,
> >>>>>
> >>>>> Thanks for proposing this.
> >>>>>
> >>>>> On Fri, 22 Nov 2024 at 06:38, Amirreza Zarrabi
> >>>>> <quic_azarrabi@...cinc.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 11/21/2024 11:08 PM, Jens Wiklander wrote:
> >>>>>>
> >>>>>> Hi Jens,
> >>>>>>
> >>>>>>> Hi Amirreza,
> >>>>>>>
> >>>>>>> On Thu, Nov 21, 2024 at 2:37 AM Amirreza Zarrabi
> >>>>>>> <quic_azarrabi@...cinc.com> wrote:
> >>>>>>>>
> >>>>>>>> The default context has a lifespan similar to the tee_device.
> >>>>>
> >>>>> Since it's associated with tee_device context, let's call it obvious
> >>>>> via renaming it as device context instead (s/def_ctx/dev_ctx/ in this
> >>>>> patch).
> >>>>>
> >>>>
> >>>> Make sense, I'll rename it.
> >>>>
> >>>>>>>> It is used as a context for shared memory if the context to which the
> >>>>>>>> shared memory belongs is released, making the tee_shm an orphan.
> >>>>>>>> This allows the driver implementing shm_unregister to safely make
> >>>>>>>> subsequent calls, such as to a supplicant if needed.
> >>>>>>>>
> >>>>>>>> It also enables users to free the shared memory while the driver is
> >>>>>>>> blocked on unregister_tee_device safely.
> >>>>>>>>
> >>>>>>>> Preferably, this should be used for all driver internal uses, using
> >>>>>>>> teedev_get_def_context rather than calling teedev_open.
> >>>>>
> >>>>> Makes sense to me.
> >>>>>
> >>>>>>>>
> >>>>>>>> Signed-off-by: Amirreza Zarrabi <quic_azarrabi@...cinc.com>
> >>>>>>>> ---
> >>>>>>>>  drivers/tee/optee/core.c    |  2 +-
> >>>>>>>>  drivers/tee/optee/ffa_abi.c |  2 +-
> >>>>>>>>  drivers/tee/optee/smc_abi.c |  2 +-
> >>>>>>>>  drivers/tee/tee_core.c      | 83 +++++++++++++++++++++++++++++----------------
> >>>>>>>>  drivers/tee/tee_private.h   |  3 --
> >>>>>>>>  drivers/tee/tee_shm.c       | 18 ++--------
> >>>>>>>>  include/linux/tee_core.h    | 15 ++++++++
> >>>>>>>>  include/linux/tee_drv.h     |  7 ----
> >>>>>>>>  8 files changed, 73 insertions(+), 59 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> >>>>>>>> index c75fddc83576..78d43d0c8014 100644
> >>>>>>>> --- a/drivers/tee/optee/core.c
> >>>>>>>> +++ b/drivers/tee/optee/core.c
> >>>>>>>> @@ -173,7 +173,7 @@ void optee_remove_common(struct optee *optee)
> >>>>>>>>
> >>>>>>>>         optee_notif_uninit(optee);
> >>>>>>>>         optee_shm_arg_cache_uninit(optee);
> >>>>>>>> -       teedev_close_context(optee->ctx);
> >>>>>>>> +
> >>>>>>>>         /*
> >>>>>>>>          * The two devices have to be unregistered before we can free the
> >>>>>>>>          * other resources.
> >>>>>>>> diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> >>>>>>>> index f3af5666bb11..6ad94f0788ad 100644
> >>>>>>>> --- a/drivers/tee/optee/ffa_abi.c
> >>>>>>>> +++ b/drivers/tee/optee/ffa_abi.c
> >>>>>>>> @@ -949,7 +949,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> >>>>>>>>         optee_shm_arg_cache_init(optee, arg_cache_flags);
> >>>>>>>>         mutex_init(&optee->rpmb_dev_mutex);
> >>>>>>>>         ffa_dev_set_drvdata(ffa_dev, optee);
> >>>>>>>> -       ctx = teedev_open(optee->teedev);
> >>>>>>>> +       ctx = teedev_get_def_context(optee->teedev);
> >>>>>>>>         if (IS_ERR(ctx)) {
> >>>>>>>>                 rc = PTR_ERR(ctx);
> >>>>>>>>                 goto err_rhashtable_free;
> >>>>>>>> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> >>>>>>>> index e9456e3e74cc..c77a3e631d04 100644
> >>>>>>>> --- a/drivers/tee/optee/smc_abi.c
> >>>>>>>> +++ b/drivers/tee/optee/smc_abi.c
> >>>>>>>> @@ -1722,7 +1722,7 @@ static int optee_probe(struct platform_device *pdev)
> >>>>>>>>         mutex_init(&optee->rpmb_dev_mutex);
> >>>>>>>>
> >>>>>>>>         platform_set_drvdata(pdev, optee);
> >>>>>>>> -       ctx = teedev_open(optee->teedev);
> >>>>>>>> +       ctx = teedev_get_def_context(optee->teedev);
> >>>>>>>>         if (IS_ERR(ctx)) {
> >>>>>>>>                 rc = PTR_ERR(ctx);
> >>>>>>>>                 goto err_supp_uninit;
> >>>>>>>> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> >>>>>>>> index 93f3b330aec8..805e1336089d 100644
> >>>>>>>> --- a/drivers/tee/tee_core.c
> >>>>>>>> +++ b/drivers/tee/tee_core.c
> >>>>>>>> @@ -57,7 +57,6 @@ struct tee_context *teedev_open(struct tee_device *teedev)
> >>>>>>>>                 goto err;
> >>>>>>>>         }
> >>>>>>>>
> >>>>>>>> -       kref_init(&ctx->refcount);
> >>>>>>>>         ctx->teedev = teedev;
> >>>>>>>>         INIT_LIST_HEAD(&ctx->list_shm);
> >>>>>>>>         rc = teedev->desc->ops->open(ctx);
> >>>>>>>> @@ -73,36 +72,43 @@ struct tee_context *teedev_open(struct tee_device *teedev)
> >>>>>>>>  }
> >>>>>>>>  EXPORT_SYMBOL_GPL(teedev_open);
> >>>>>>>>
> >>>>>>>> -void teedev_ctx_get(struct tee_context *ctx)
> >>>>>>>> +struct tee_context *teedev_get_def_context(struct tee_device *teedev)
> >>>>>>>>  {
> >>>>>>>> -       if (ctx->releasing)
> >>>>>>>> -               return;
> >>>>>>>> +       int rc;
> >>>>>>>> +       struct tee_context *ctx = &teedev->def_ctx;
> >>>>>>>>
> >>>>>>>> -       kref_get(&ctx->refcount);
> >>>>>>>> -}
> >>>>>>>> +       ctx->teedev = teedev;
> >>>>>>>> +       INIT_LIST_HEAD(&ctx->list_shm);
> >>>>>>>> +       rc = teedev->desc->ops->open(ctx);
> >>>>>>>> +       if (rc)
> >>>>>>>> +               return ERR_PTR(rc);
> >>>>>>>
> >>>>>>> I think ctx->teedev and ctx->list_shm must always be initialized or
> >>>>>>> &teedev->def_ctx can't be used in teedev_close_context().
> >>>>>>
> >>>>>> True, but &teedev->def_ctx is never used in teedev_close_context().
> >>>>>> The closing of the &teedev->def_ctx simply ignored. So once opened,
> >>>>>> &teedev->def_ctx will always remain open until the tee_device is alive.
> >>>>>>
> >>>>>>> We could initialize teedev->def_ctx on the first call to teedev_open()
> >>>>>>> on that tee_device. We need a way to tell the
> >>>>>>> teedev->desc->ops->open() to the backed driver that it's initializing
> >>>>>>> the default context though, or optee_open() can't handle the
> >>>>>>> tee-supplicant case properly.
> >>>>>>>
> >>>>>>
> >>>>>> That's a good point. This way, it is guaranteed that there is one def_ctx
> >>>>>> per teedev. There should be a way to tell the open() callback that it is
> >>>>>> a def_ctx, so it is not registered as a supplicant context.
> >>>>>>
> >>>>>>
> >>>>>>> Should we allow this function to be called more than once for each teedev?
> >>>>>>
> >>>>>> Yes, moving to teedev_open() will fix the issue.
> >>>>>>
> >>>>>>> Do we need serialization in this function if it's called after the
> >>>>>>> driver is probed?
> >>>>>>>
> >>>>>>
> >>>>>> True. I'll make sure there is no race.
> >>>>>>
> >>>>>>>>
> >>>>>>>> -static void teedev_ctx_release(struct kref *ref)
> >>>>>>>> -{
> >>>>>>>> -       struct tee_context *ctx = container_of(ref, struct tee_context,
> >>>>>>>> -                                              refcount);
> >>>>>>>> -       ctx->releasing = true;
> >>>>>>>> -       ctx->teedev->desc->ops->release(ctx);
> >>>>>>>> -       kfree(ctx);
> >>>>>>>> +       return ctx;
> >>>>>>>>  }
> >>>>>>>> +EXPORT_SYMBOL_GPL(teedev_get_def_context);
> >>>>>>>>
> >>>>>>>> -void teedev_ctx_put(struct tee_context *ctx)
> >>>>>>>> +void teedev_close_context(struct tee_context *ctx)
> >>>>>>>>  {
> >>>>>>>> -       if (ctx->releasing)
> >>>>>>>> +       struct tee_device *teedev = ctx->teedev;
> >>>>>>>> +       struct tee_shm *shm;
> >>>>>>>> +
> >>>>>>>> +       if (ctx == &teedev->def_ctx)
> >>>>>>>>                 return;
> >>>>>>>>
> >>>>>>>> -       kref_put(&ctx->refcount, teedev_ctx_release);
> >>>>>>>> -}
> >>>>>>>> +       teedev->desc->ops->release(ctx);
> >>>>>>>>
> >>>>>>>> -void teedev_close_context(struct tee_context *ctx)
> >>>>>>>> -{
> >>>>>>>> -       struct tee_device *teedev = ctx->teedev;
> >>>>>>>> +       mutex_lock(&teedev->mutex);
> >>>>>>>> +       list_for_each_entry(shm, &ctx->list_shm, link) {
> >>>>>>>> +               /* Context released. However, shm still holding a teedev reference.
> >>>>>>>> +                * Replace shm->ctx with the default context so that tee_shm_get_from_id()
> >>>>>>>> +                * fails (i.e. it is not accessible from userspace) but shm still
> >>>>>>>> +                * holds a valid context for further clean up, e.g. shm_unregister().
> >>>>>>>> +                */
> >>>>>>>
> >>>>>>> /*
> >>>>>>>  * Please format
> >>>>>>>  * multiline comments
> >>>>>>>  * like this. Please
> >>>>>>>  * keep the lines at
> >>>>>>>  * max 80 columns
> >>>>>>>  * here and at other
> >>>>>>>  * places in the patch-
> >>>>>>>  * set.
> >>>>>>>  */
> >>>>>>>
> >>>>>>
> >>>>>> Ack.
> >>>>>>
> >>>>>>>> +               shm->ctx = &teedev->def_ctx;
> >>>>>>>
> >>>>>>> shm->ctx will always point to a valid context, even if it is the
> >>>>>>> default context. It seems that we can always get hold of the correct
> >>>>>>> teedev via shm->ctx->teedev. Do we need "tee: revert removal of
> >>>>>>> redundant teedev in struct tee_shm"?
> >>>>>>>
> >>>>>>
> >>>>>> It was there in case we wanted to use NULL, but with def_ctx, it is not
> >>>>>> necessary. I am withdrawing that commit. :).
> >>>>>>
> >>>>>>> Shouldn't the shm be removed from the ctx->list_shm and be moved to
> >>>>>>> teedev->def_ctx.list_shm?
> >>>>>
> >>>>> +1
> >>>>>
> >>>>
> >>>> Ack.
> >>>>
> >>>>>>>
> >>>>>>
> >>>>>> Not really. If we put shm in the teedev->def_ctx.list_shm, by the time
> >>>>>> we are closing the def_ctx, the list is guaranteed to be empty.
> >>>>>>
> >>>>>> However, I understand it is cleaner and more consistent to do that rather
> >>>>>> than making changes to tee_shm_put().
> >>>>>>
> >>>>>> I'll do it.
> >>>>>>
> >>>>>>>> +       }
> >>>>>>>> +       mutex_unlock(&teedev->mutex);
> >>>>>>>>
> >>>>>>>> -       teedev_ctx_put(ctx);
> >>>>>>>> +       kfree(ctx);
> >>>>>>>>         tee_device_put(teedev);
> >>>>>>>>  }
> >>>>>>>>  EXPORT_SYMBOL_GPL(teedev_close_context);
> >>>>>>>> @@ -946,6 +952,8 @@ struct tee_device *tee_device_alloc(const struct tee_desc *teedesc,
> >>>>>>>>
> >>>>>>>>         teedev->desc = teedesc;
> >>>>>>>>         teedev->pool = pool;
> >>>>>>>> +       /* Only open default context when teedev_get_def_context() called. */
> >>>>>>>> +       teedev->def_ctx.teedev = NULL;
> >>>>>
> >>>>> Why don't you open the device context here only? This will associate
> >>>>> it automatically with teedev lifespan and then
> >>>>> teedev_get_def_context() will just return a reference to that.
> >>>>>
> >>>>> -Sumit
> >>>>>
> >>>>
> >>>> So my assumption is that the tee_devic_alloc() is called as part of
> >>>> the driver initialization; there is no guarantee that at this time the
> >>>> driver is actually ready to accept any open() callback.
> >>>>
> >>>
> >>> The drivers should be able to handle open() callback since we already
> >>> check for !teedesc->ops->open in the beginning of tee_devic_alloc().
> >>> Also, we need to open a device context for !TEE_DESC_PRIVILEGED such
> >>> that we don't open a supplicant device context there.
> >>
> >> It would be nice to have the device context fully initialized when the
> >> probe function returns. How about adding a "bool is_dev_ctx" to struct
> >> tee_context so the open() callback can tell that this is a special
> >> tee_contex?
> >
> > Sure, that will be useful to distinguish the device context from
> > normal client context.
> >
> > -Sumit
> >
>
> So, as far as the open() callback, I do not believe checking if it is not null
> is reasonable for calling it here. Most drivers allocate resources and then
> initialize them. So, assume these steps for a TEE driver:
>  (1) allocate internal data structures,
>  (2) allocate the device,
>  (3) initialize the internal data structurse and then
>  (4) register the device.
>
> Having these steps for a backend driver means that if you call open() at
> step (2), the internal data structures are not ready.
>
> I was originally thinking of going with Jens' suggestion to open dev_ctx in
> the teedev_open(), and use a flag to distinguish the type of context for
> the open() callback
>
> What about this:
> Open the dev_ctx in the tee_device_register(), at the last step before
> setting the TEE_DEVICE_FLAG_REGISTERED flag. Then the open() callback can
> check for this flag to determine if it is a normal context or dev_ctx.
> If the open() is called while the device has not been registered, it should
> handle it differently

That makes sense, the driver should be prepared to handle open() calls
after tee_device_register() anyway.
However, there is no serialization of the flags field in struct
tee_device. Hmm, would it be too hacky for the open() callback to
check if &ctx->teedev.dev_ctx == ctx? We could add a helper function
to wrap that check.

Cheers,
Jens

>
> - Amir
>
> >>
> >> Cheers,
> >> Jens
> >>
> >>>
> >>> -Sumit

Powered by blists - more mailing lists