Hi Igor,
On Thu, 4 Apr 2024 at 14:40, Igor Opaniuk <igor.opan...@gmail.com> wrote: > > Ilias, > > On Thu, Apr 4, 2024 at 1:00 PM Ilias Apalodimas <ilias.apalodi...@linaro.org> > wrote: >> >> Hi Igor, >> >> On Thu, 4 Apr 2024 at 13:18, Igor Opaniuk <igor.opan...@gmail.com> wrote: >> > >> > Hi Ilias, >> > >> > On Thu, Apr 4, 2024 at 10:54 AM Ilias Apalodimas >> > <ilias.apalodi...@linaro.org> wrote: >> >> >> >> Hi Igor, >> >> >> >> On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas >> >> <ilias.apalodi...@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.opan...@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. > > I'll provide a bit more context here. The problem initially was in the TEE > sandbox > driver implementation(drivers/tee/sandbox.c) and it's limitations, which > doesn't > permit to have multiple simultaneous sessions with different TAs. > This is what actually happened in this CI run [1], firstly "optee_rpmb" > cmd was executed (and after execution we had one session open), and > then "scp03", which also makes calls to OP-TEE, however it fails > in sandbox_tee_open_session() because of this check: > > if (state->ta) { > printf("A session is already open\n"); > return -EBUSY; > } > > So basically I had two ways in mind to address that: > 1. Close a session on each optee_rpmb cmd invocation. > I don't see any reason to keep this session open, as obviously > there is no other mechanism (tbh, I don't know if DM calls ".remove" for > active > devices) to close it automatically before handing over control to > Linux kernel. As a result we might end up with some orphaned sessions > registered in OP-TEE OS core (obvious resource leak). > 2. Extend TEE sandbox driver, add support for multiple > simultaneous sessions just to handle the case. > > I've chosen the first approach, as IMO it was "kill two birds with one stone", > I could address resource leak in OP-TEE and bypass limitations of > TEE sandbox driver. Thanks, this helps. All this needs to be in the commit message, instead of a mail thread. Can you send a new revision with the commit message amended? Thanks /Ilias > > [1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/jobs/792216 > >> >> >> 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.opan...@gmail.com >> > skype: igor.opanyuk >> > https://www.linkedin.com/in/iopaniuk > > > > -- > Best regards - Atentamente - Meilleures salutations > > Igor Opaniuk > > mailto: igor.opan...@gmail.com > skype: igor.opanyuk > https://www.linkedin.com/in/iopaniuk