On Wed, Nov 27, 2024 at 7:02 AM Sumit Garg sumit.garg@linaro.org wrote:
On Tue, 26 Nov 2024 at 20:52, Jens Wiklander jens.wiklander@linaro.org wrote:
On Tue, Nov 26, 2024 at 1:27 PM Sumit Garg sumit.garg@linaro.org wrote:
On Tue, 26 Nov 2024 at 14:03, Jens Wiklander jens.wiklander@linaro.org wrote:
On Mon, Nov 25, 2024 at 9:55 PM Amirreza Zarrabi quic_azarrabi@quicinc.com wrote:
On 11/25/2024 6:51 PM, Sumit Garg wrote:
On Mon, 25 Nov 2024 at 12:53, Jens Wiklander jens.wiklander@linaro.org wrote: > > On Mon, Nov 25, 2024 at 7:14 AM Sumit Garg sumit.garg@linaro.org wrote: >> >> On Mon, 25 Nov 2024 at 03:00, Amirreza Zarrabi >> quic_azarrabi@quicinc.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@quicinc.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@quicinc.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@quicinc.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.
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?
One reason for initializing dev_ctx for all tee_devices is in teedev_close_context(), where the tee_shms still active are transferred to dev_ctx. The teedev member was re-introduced in this patch set, but it can be removed again if we can depend on the dev_ctx to always be available in teedev_close_context(). Even the tee-supplicant may close its tee_context with active tee_shms at some point. It might be possible to use half-baked dev_ctx, but then we'd be burdened with keeping track of which dev_ctx can be used for what. We want as few special cases as possible.
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.
OK.
Cheers, Jens
-Sumit
Cheers, Jens
-Sumit
Cheers, Jens
- Amir
> > Cheers, > Jens > >> >> -Sumit