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?
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