On Tue, 26 Nov 2024 at 20:52, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
>
> On Tue, Nov 26, 2024 at 1:27 PM Sumit Garg <sumit.garg@xxxxxxxxxx> wrote:
> >
> > On Tue, 26 Nov 2024 at 14:03, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Nov 25, 2024 at 9:55 PM Amirreza Zarrabi
> > > <quic_azarrabi@xxxxxxxxxxx> wrote:
> > > >
> > > >
> > > >
> > > > On 11/25/2024 6:51 PM, Sumit Garg wrote:
> > > > > On Mon, 25 Nov 2024 at 12:53, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> > > > >>
> > > > >> On Mon, Nov 25, 2024 at 7:14 AM Sumit Garg <sumit.garg@xxxxxxxxxx> wrote:
> > > > >>>
> > > > >>> On Mon, 25 Nov 2024 at 03:00, Amirreza Zarrabi
> > > > >>> <quic_azarrabi@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > > >>>>>>>> ---
> > > > >>>>>>>> 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.
> >
> > As part of tee_device_alloc(), every driver has to pass "const struct
> > tee_desc *teedesc" fully initialized. Which internal data structures
> > are you referring too? Is there any upstream example?
>
> It's reasonable to wait with the open() callback until step 4 above,
> which should correspond with the tee_device_register() call. Data
> written only once doesn't need serialized access if the fields are
> only accessed after they have been fully initialized.
Fair enough, I can live with the device context opened after registering it.
>
> >
> > > >
> > > > 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.
> > >
> >
> > Your suggested change requires every driver to update open() callback
> > and later other callbacks may have to support it too. IMHO, only
> > teedev_get_dev_ctx() should be able to return a reference to device
> > context for usage within the TEE and the implementation driver.
>
> Yes, but it's only the OP-TEE driver that needs anything special. It
> looks like the others can be left unchanged.
I suppose it's most likely the upcoming QTEE driver requiring it.
>
> >
> > I am still not able to understand why the following won't work with a
> > clear lifetime for the device context?
> >
> > tee_device_alloc()
> > -> if (!(teedesc->flags & TEE_DESC_PRIVILEGED))
> > desc->ops->open(&teedev->dev_ctx);
>
> We must also have a fully initialized dev_ctx for the supplicant
> device.
Currently I only see following for OP-TEE driver:
ctx = teedev_open(optee->teedev);
And I can't see anything like below:
ctx = teedev_open(optee->supp_teedev);
Where do you think that the dev_ctx is required for a supplicant
device? AFAICS, currently opening a context with the supplicant device
means that the supplicant daemon is available to handle RPCs which
won't be possible during OP-TEE driver probe. Am I missing something?
> I'd rather delay the open() callback until
> tee_device_register() since the dev_ctx is guaranteed not to be needed
> before that.
Okay, the updated call chain can look like:
tee_device_register()
-> if (!(teedev->desc->flags & TEE_DESC_PRIVILEGED))
desc->ops->open(&teedev->dev_ctx);
>
> >
> > tee_device_put()
> > -> if (teedev->dev_ctx) desc->ops->release(&teedev->dev_ctx);
>
> teedev->dev_ctx is supposed to be embedded in struct tee_device, so
> the if isn't needed.
I added "if" to cover the case when dev_ctx is not initialized for the
supplicant device.
-Sumit
>
> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > > Cheers,
> > > Jens
> > >
> > > >
> > > > - Amir
> > > >
> > > > >>
> > > > >> Cheers,
> > > > >> Jens
> > > > >>
> > > > >>>
> > > > >>> -Sumit
