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.
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 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. I'd rather delay the open() callback until tee_device_register() since the dev_ctx is guaranteed not to be needed before that.
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.
Cheers, Jens
-Sumit
Cheers, Jens
- Amir
Cheers, Jens
-Sumit