[PATCH v4 2/5] cmd: optee_rpmb: close tee session
Ilias Apalodimas
ilias.apalodimas at linaro.org
Thu Apr 4 12:59:46 CEST 2024
Hi Igor,
On Thu, 4 Apr 2024 at 13:18, Igor Opaniuk <igor.opaniuk at gmail.com> wrote:
>
> Hi Ilias,
>
> On Thu, Apr 4, 2024 at 10:54 AM Ilias Apalodimas <ilias.apalodimas at linaro.org> wrote:
>>
>> Hi Igor,
>>
>> On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas
>> <ilias.apalodimas at linaro.org> wrote:
>> >
>> > Hi Igor,
>> >
>> > I was about to apply the series, but noticed neither me or Jens were cc'ed
>> > on this. Adding Jens to the party
>> >
>> > On Thu, Apr 04, 2024 at 10:13:49AM +0200, Igor Opaniuk wrote:
>> > > Add calls for closing tee session after every read/write operation.
>> >
>> > What the patch is pretty obvious, but I am missing an explanation of why
>> > this is needed
>> >
>> > >
>> > > Signed-off-by: Igor Opaniuk <igor.opaniuk at gmail.com>
>> > > ---
>> > >
>> > > (no changes since v1)
>> > >
>> > > cmd/optee_rpmb.c | 23 +++++++++++++++++------
>> > > 1 file changed, 17 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/cmd/optee_rpmb.c b/cmd/optee_rpmb.c
>> > > index e0e44bbed04..b3cafd92410 100644
>> > > --- a/cmd/optee_rpmb.c
>> > > +++ b/cmd/optee_rpmb.c
>> > > @@ -87,8 +87,10 @@ static int read_persistent_value(const char *name,
>> > >
>> > > rc = tee_shm_alloc(tee, name_size,
>> > > TEE_SHM_ALLOC, &shm_name);
>> > > - if (rc)
>> > > - return -ENOMEM;
>> > > + if (rc) {
>> > > + rc = -ENOMEM;
>> > > + goto close_session;
>> > > + }
>> >
>> > I don't think you need the session to be opened to allocate memory.
>> > So instead of of doing this, why don't we just move the alloc call before
>> > opening the session?
>>
>> Looking at it again, we do need tee != NULL, but I think it's cleaner
>> to add something like
>> if (!dev)
>> return -ENODEV
>> to __tee_shm_add() instead.
>
> Do you mind if I address that in a separate patch series, as tbh I
> don't want to add additional patches to the existing series?
We still completely change the functionality. So, the patchset is not
a 'tiny cleanup', it instead closes the session every time instead of
keeping it open.
There are a few things going on, that aren't explained clearly in the
commit message
- Why is this needed? Looking at the code it is an actual problem
since sessions tied to the AVB uuid will remain open
- Is there any side effect by always closing the session?
I don't mind merging it as is with a proper commit message, but I
think the alternative is just easier.
Thanks
/Ilias
>
>>
>>
>> Cheers
>> /Ilias
>> >
>> > >
>> > > rc = tee_shm_alloc(tee, buffer_size,
>> > > TEE_SHM_ALLOC, &shm_buf);
>> > > @@ -125,6 +127,9 @@ out:
>> > > tee_shm_free(shm_buf);
>> > > free_name:
>> > > tee_shm_free(shm_name);
>> > > +close_session:
>> > > + tee_close_session(tee, session);
>> > > + tee = NULL;
>> > >
>> > > return rc;
>> > > }
>> > > @@ -139,17 +144,20 @@ static int write_persistent_value(const char *name,
>> > > struct tee_param param[2];
>> > > size_t name_size = strlen(name) + 1;
>> > >
>> > > + if (!value_size)
>> > > + return -EINVAL;
>> > > +
>> > > if (!tee) {
>> > > if (avb_ta_open_session())
>> > > return -ENODEV;
>> > > }
>> > > - if (!value_size)
>> > > - return -EINVAL;
>> > >
>> > > rc = tee_shm_alloc(tee, name_size,
>> > > TEE_SHM_ALLOC, &shm_name);
>> > > - if (rc)
>> > > - return -ENOMEM;
>> > > + if (rc) {
>> > > + rc = -ENOMEM;
>> > > + goto close_session;
>> > > + }
>> >
>> > ditto
>> >
>> > >
>> > > rc = tee_shm_alloc(tee, value_size,
>> > > TEE_SHM_ALLOC, &shm_buf);
>> > > @@ -178,6 +186,9 @@ out:
>> > > tee_shm_free(shm_buf);
>> > > free_name:
>> > > tee_shm_free(shm_name);
>> > > +close_session:
>> > > + tee_close_session(tee, session);
>> > > + tee = NULL;
>> > >
>> > > return rc;
>> > > }
>> > > --
>> > > 2.34.1
>> > >
>> >
>> > Thanks
>> > /Ilias
>
>
>
>
> --
> Best regards - Atentamente - Meilleures salutations
>
> Igor Opaniuk
>
> mailto: igor.opaniuk at gmail.com
> skype: igor.opanyuk
> https://www.linkedin.com/in/iopaniuk
More information about the U-Boot
mailing list