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.
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
- Amir
>>
>> Cheers,
>> Jens
>>
>>>
>>> -Sumit
