| Message ID | 20210609002326.210024-6-tyhicks@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tee: Improve support for kexec and kdump | expand |
Hi Tyler, On Wed, 9 Jun 2021 at 05:55, Tyler Hicks <tyhicks@linux.microsoft.com> wrote: > > Uncouple the registration of dynamic shared memory buffers from the > TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory > regions but do not need them to be backed by a dma-buf when the memory > region is private to the driver. In this case drivers should use tee_shm_register() instead where the memory allocated is actually private to the driver. However, you need to remove TEE_SHM_DMA_BUF as a mandatory flag for tee_shm_register(). Have a look at an example here [1]. So modifying tee_shm_alloc() for this purpose doesn't look appropriate to me. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n73 -Sumit > > Allow callers of tee_shm_alloc() to specify the TEE_SHM_REGISTER flag to > request registration. If the TEE implementation does not require dynamic > shared memory to be registered, clear the flag prior to calling the > corresponding pool alloc function. Update the OP-TEE driver to respect > TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to > (un)register on alloc/free operations. The AMD-TEE driver continues to > ignore the TEE_SHM_REGISTER flag. > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > --- > drivers/tee/optee/shm_pool.c | 5 ++--- > drivers/tee/tee_shm.c | 11 ++++++++++- > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c > index da06ce9b9313..6054343a29fb 100644 > --- a/drivers/tee/optee/shm_pool.c > +++ b/drivers/tee/optee/shm_pool.c > @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, > shm->paddr = page_to_phys(page); > shm->size = PAGE_SIZE << order; > > - if (shm->flags & TEE_SHM_DMA_BUF) { > + if (shm->flags & TEE_SHM_REGISTER) { > unsigned int nr_pages = 1 << order, i; > struct page **pages; > > @@ -42,7 +42,6 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, > page++; > } > > - shm->flags |= TEE_SHM_REGISTER; > rc = optee_shm_register(shm->ctx, shm, pages, nr_pages, > (unsigned long)shm->kaddr); > kfree(pages); > @@ -60,7 +59,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, > static void pool_op_free(struct tee_shm_pool_mgr *poolm, > struct tee_shm *shm) > { > - if (shm->flags & TEE_SHM_DMA_BUF) > + if (shm->flags & TEE_SHM_REGISTER) > optee_shm_unregister(shm->ctx, shm); > > free_pages((unsigned long)shm->kaddr, get_order(shm->size)); > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c > index 00472f5ce22e..1c0176550b9c 100644 > --- a/drivers/tee/tee_shm.c > +++ b/drivers/tee/tee_shm.c > @@ -117,7 +117,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) > return ERR_PTR(-EINVAL); > } > > - if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) { > + if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER))) { > dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags); > return ERR_PTR(-EINVAL); > } > @@ -137,6 +137,15 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) > goto err_dev_put; > } > > + if (!teedev->desc->ops->shm_register || > + !teedev->desc->ops->shm_unregister) { > + /* registration is not required by the TEE implementation */ > + flags &= ~TEE_SHM_REGISTER; > + } else if (flags & TEE_SHM_DMA_BUF) { > + /* all dma-buf backed shm allocations are registered */ > + flags |= TEE_SHM_REGISTER; > + } > + > shm->flags = flags | TEE_SHM_POOL; > shm->ctx = ctx; > if (flags & TEE_SHM_DMA_BUF) > -- > 2.25.1 >
On 2021-06-09 09:59:04, Sumit Garg wrote: > Hi Tyler, Hey Sumit - Thanks for the review. > > On Wed, 9 Jun 2021 at 05:55, Tyler Hicks <tyhicks@linux.microsoft.com> wrote: > > > > Uncouple the registration of dynamic shared memory buffers from the > > TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory > > regions but do not need them to be backed by a dma-buf when the memory > > region is private to the driver. > > In this case drivers should use tee_shm_register() instead where the > memory allocated is actually private to the driver. However, you need > to remove TEE_SHM_DMA_BUF as a mandatory flag for tee_shm_register(). > Have a look at an example here [1]. So modifying tee_shm_alloc() for > this purpose doesn't look appropriate to me. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n73 I noticed what you did in commit 2a6ba3f794e8 ("tee: enable support to register kernel memory") and considered moving ftpm and tee_bnxt_fw over to tee_shm_register(). I think that's likely the right long term approach but I decided against it since this series is a minimal set of bug fixes that will hopefully go to stable (I'm affected by these bugs in 5.4). Here are my reasons for feeling like moving to tee_shm_register() isn't minimal in terms of a stable-focused fix: - tee_shm_alloc() looks like it should work fine with AMD-TEE today. tee_shm_register() definitely does not since AMD-TEE doesn't provide a .shm_register or .shm_unregister hook. This may break existing users of AMD-TEE? - tee_shm_register() has not historically been used for kernel allocations and is not fixed wrt the bug that Jens fixed in commit f1bbacedb0af ("tee: don't assign shm id for private shms"). - tee_shm_alloc() performs allocations using contiguous pages from alloc_pages() while tee_shm_register() performs non-contiguous allocations with kcalloc(). I suspect this would be fine but I don't know the secure world side of these things well enough to assess the risk involved with such a change on the kernel side. I should have mentioned this in the cover letter but my hope was that these minimal changes would be accepted and then additional work could be done to merge tee_shm_alloc() and tee_shm_register() in a way that would allow the caller to request contiguous or non-contiguous pages, fix up the additional issues mentioned above, and then adjust the call sites in ftpm and tee_bnxt_fw as appropriate. I think that's a bigger set of changes because there are several things that still confuse/concern me: - Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register() uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three exist? - Why does tee_shm_register() unconditionally use non-contiguous allocations without ever taking into account whether or not OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers. - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is specific to OP-TEE? How to better abstract that away? Let me know if you agree with the more minimal approach that I took for these bug fix series or still feel like tee_shm_register() should be fixed up so that it is usable. Thanks! Tyler > > -Sumit > > > > > Allow callers of tee_shm_alloc() to specify the TEE_SHM_REGISTER flag to > > request registration. If the TEE implementation does not require dynamic > > shared memory to be registered, clear the flag prior to calling the > > corresponding pool alloc function. Update the OP-TEE driver to respect > > TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to > > (un)register on alloc/free operations. The AMD-TEE driver continues to > > ignore the TEE_SHM_REGISTER flag. > > > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > > --- > > drivers/tee/optee/shm_pool.c | 5 ++--- > > drivers/tee/tee_shm.c | 11 ++++++++++- > > 2 files changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c > > index da06ce9b9313..6054343a29fb 100644 > > --- a/drivers/tee/optee/shm_pool.c > > +++ b/drivers/tee/optee/shm_pool.c > > @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, > > shm->paddr = page_to_phys(page); > > shm->size = PAGE_SIZE << order; > > > > - if (shm->flags & TEE_SHM_DMA_BUF) { > > + if (shm->flags & TEE_SHM_REGISTER) { > > unsigned int nr_pages = 1 << order, i; > > struct page **pages; > > > > @@ -42,7 +42,6 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, > > page++; > > } > > > > - shm->flags |= TEE_SHM_REGISTER; > > rc = optee_shm_register(shm->ctx, shm, pages, nr_pages, > > (unsigned long)shm->kaddr); > > kfree(pages); > > @@ -60,7 +59,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, > > static void pool_op_free(struct tee_shm_pool_mgr *poolm, > > struct tee_shm *shm) > > { > > - if (shm->flags & TEE_SHM_DMA_BUF) > > + if (shm->flags & TEE_SHM_REGISTER) > > optee_shm_unregister(shm->ctx, shm); > > > > free_pages((unsigned long)shm->kaddr, get_order(shm->size)); > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c > > index 00472f5ce22e..1c0176550b9c 100644 > > --- a/drivers/tee/tee_shm.c > > +++ b/drivers/tee/tee_shm.c > > @@ -117,7 +117,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) > > return ERR_PTR(-EINVAL); > > } > > > > - if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) { > > + if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER))) { > > dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags); > > return ERR_PTR(-EINVAL); > > } > > @@ -137,6 +137,15 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) > > goto err_dev_put; > > } > > > > + if (!teedev->desc->ops->shm_register || > > + !teedev->desc->ops->shm_unregister) { > > + /* registration is not required by the TEE implementation */ > > + flags &= ~TEE_SHM_REGISTER; > > + } else if (flags & TEE_SHM_DMA_BUF) { > > + /* all dma-buf backed shm allocations are registered */ > > + flags |= TEE_SHM_REGISTER; > > + } > > + > > shm->flags = flags | TEE_SHM_POOL; > > shm->ctx = ctx; > > if (flags & TEE_SHM_DMA_BUF) > > -- > > 2.25.1 > > >
+ Rijo On Wed, 9 Jun 2021 at 11:16, Tyler Hicks <tyhicks@linux.microsoft.com> wrote: > > On 2021-06-09 09:59:04, Sumit Garg wrote: > > Hi Tyler, > > Hey Sumit - Thanks for the review. > > > > > On Wed, 9 Jun 2021 at 05:55, Tyler Hicks <tyhicks@linux.microsoft.com> wrote: > > > > > > Uncouple the registration of dynamic shared memory buffers from the > > > TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory > > > regions but do not need them to be backed by a dma-buf when the memory > > > region is private to the driver. > > > > In this case drivers should use tee_shm_register() instead where the > > memory allocated is actually private to the driver. However, you need > > to remove TEE_SHM_DMA_BUF as a mandatory flag for tee_shm_register(). > > Have a look at an example here [1]. So modifying tee_shm_alloc() for > > this purpose doesn't look appropriate to me. > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n73 > > I noticed what you did in commit 2a6ba3f794e8 ("tee: enable support to > register kernel memory") and considered moving ftpm and tee_bnxt_fw over > to tee_shm_register(). I think that's likely the right long term > approach but I decided against it since this series is a minimal set of > bug fixes that will hopefully go to stable (I'm affected by these bugs > in 5.4). Here are my reasons for feeling like moving to > tee_shm_register() isn't minimal in terms of a stable-focused fix: > > - tee_shm_alloc() looks like it should work fine with AMD-TEE today. > tee_shm_register() definitely does not since AMD-TEE doesn't provide a > .shm_register or .shm_unregister hook. This may break existing users > of AMD-TEE? AFAIK, ftpm and tee_bnxt_fw drivers only support OP-TEE at this point. See ftpm_tee_match() and optee_ctx_match() APIs in corresponding drivers. > - tee_shm_register() has not historically been used for kernel > allocations and is not fixed wrt the bug that Jens fixed in commit > f1bbacedb0af ("tee: don't assign shm id for private shms"). Yes, that's what I meant earlier to make the TEE_SHM_DMA_BUF flag optional. > - tee_shm_alloc() performs allocations using contiguous pages > from alloc_pages() while tee_shm_register() performs non-contiguous > allocations with kcalloc(). I suspect this would be fine but I don't > know the secure world side of these things well enough to assess the > risk involved with such a change on the kernel side. > I don't think that would make any difference. > I should have mentioned this in the cover letter but my hope was that > these minimal changes would be accepted and then additional work could > be done to merge tee_shm_alloc() and tee_shm_register() in a way that > would allow the caller to request contiguous or non-contiguous pages, > fix up the additional issues mentioned above, and then adjust the > call sites in ftpm and tee_bnxt_fw as appropriate. > > I think that's a bigger set of changes because there are several things > that still confuse/concern me: > > - Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register() > uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three > exist? AFAIK, its due the the inherent nature of tee_shm_alloc() and tee_shm_register() where tee_shm_alloc() doesn't need to know whether its a kernel or user-space memory since it is the one that allocates whereas tee_shm_register() need to know that since it has to register pre-allocated client memory. > - Why does tee_shm_register() unconditionally use non-contiguous > allocations without ever taking into account whether or not > OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required > from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers. Yeah, but do we have platforms in OP-TEE that don't support dynamic shared memory? I guess it has become the sane default which is a mandatory requirement when it comes to OP-TEE driver in u-boot. > - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is > specific to OP-TEE? How to better abstract that away? > I would like you to go through Section "3.2.4. Shared Memory" in TEE Client API Specification. There are two standard ways for shared memory approach with TEE: 1. A Shared Memory block can either be existing Client Application memory (kernel driver in our case) which is subsequently registered with the TEE Client API (using tee_shm_register() in our case). 2. Or memory which is allocated on behalf of the Client Application using the TEE Client API (using tee_shm_alloc() in our case). > Let me know if you agree with the more minimal approach that I took for > these bug fix series or still feel like tee_shm_register() should be > fixed up so that it is usable. Thanks! From drivers perspective I think the change should be: tee_shm_alloc() to kcalloc() tee_shm_register() -Sumit > > > Allow callers of tee_shm_alloc() to specify the TEE_SHM_REGISTER flag to. > > > request registration. If the TEE implementation does not require dynamic > > > shared memory to be registered, clear the flag prior to calling the > > > corresponding pool alloc function. Update the OP-TEE driver to respect > > > TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to > > > (un)register on alloc/free operations. The AMD-TEE driver continues to > > > ignore the TEE_SHM_REGISTER flag. > > > > > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > > > --- > > > drivers/tee/optee/shm_pool.c | 5 ++--- > > > drivers/tee/tee_shm.c | 11 ++++++++++- > > > 2 files changed, 12 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c > > > index da06ce9b9313..6054343a29fb 100644 > > > --- a/drivers/tee/optee/shm_pool.c > > > +++ b/drivers/tee/optee/shm_pool.c > > > @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, > > > shm->paddr = page_to_phys(page); > > > shm->size = PAGE_SIZE << order; > > > > > > - if (shm->flags & TEE_SHM_DMA_BUF) { > > > + if (shm->flags & TEE_SHM_REGISTER) { > > > unsigned int nr_pages = 1 << order, i; > > > struct page **pages; > > > > > > @@ -42,7 +42,6 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, > > > page++; > > > } > > > > > > - shm->flags |= TEE_SHM_REGISTER; > > > rc = optee_shm_register(shm->ctx, shm, pages, nr_pages, > > > (unsigned long)shm->kaddr); > > > kfree(pages); > > > @@ -60,7 +59,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, > > > static void pool_op_free(struct tee_shm_pool_mgr *poolm, > > > struct tee_shm *shm) > > > { > > > - if (shm->flags & TEE_SHM_DMA_BUF) > > > + if (shm->flags & TEE_SHM_REGISTER) > > > optee_shm_unregister(shm->ctx, shm); > > > > > > free_pages((unsigned long)shm->kaddr, get_order(shm->size)); > > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c > > > index 00472f5ce22e..1c0176550b9c 100644 > > > --- a/drivers/tee/tee_shm.c > > > +++ b/drivers/tee/tee_shm.c > > > @@ -117,7 +117,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) > > > return ERR_PTR(-EINVAL); > > > } > > > > > > - if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) { > > > + if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER))) { > > > dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags); > > > return ERR_PTR(-EINVAL); > > > } > > > @@ -137,6 +137,15 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) > > > goto err_dev_put; > > > } > > > > > > + if (!teedev->desc->ops->shm_register || > > > + !teedev->desc->ops->shm_unregister) { > > > + /* registration is not required by the TEE implementation */ > > > + flags &= ~TEE_SHM_REGISTER; > > > + } else if (flags & TEE_SHM_DMA_BUF) { > > > + /* all dma-buf backed shm allocations are registered */ > > > + flags |= TEE_SHM_REGISTER; > > > + } > > > + > > > shm->flags = flags | TEE_SHM_POOL; > > > shm->ctx = ctx; > > > if (flags & TEE_SHM_DMA_BUF) > > > -- > > > 2.25.1 > > > > >
Hi, On Wed, Jun 09, 2021 at 04:22:49PM +0530, Sumit Garg wrote: > + Rijo > > On Wed, 9 Jun 2021 at 11:16, Tyler Hicks <tyhicks@linux.microsoft.com> wrote: > > > > On 2021-06-09 09:59:04, Sumit Garg wrote: > > > Hi Tyler, > > > > Hey Sumit - Thanks for the review. > > > > > > > > On Wed, 9 Jun 2021 at 05:55, Tyler Hicks <tyhicks@linux.microsoft.com> wrote: > > > > > > > > Uncouple the registration of dynamic shared memory buffers from the > > > > TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory > > > > regions but do not need them to be backed by a dma-buf when the memory > > > > region is private to the driver. > > > > > > In this case drivers should use tee_shm_register() instead where the > > > memory allocated is actually private to the driver. However, you need > > > to remove TEE_SHM_DMA_BUF as a mandatory flag for tee_shm_register(). > > > Have a look at an example here [1]. So modifying tee_shm_alloc() for > > > this purpose doesn't look appropriate to me. > > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n73 > > > > I noticed what you did in commit 2a6ba3f794e8 ("tee: enable support to > > register kernel memory") and considered moving ftpm and tee_bnxt_fw over > > to tee_shm_register(). I think that's likely the right long term > > approach but I decided against it since this series is a minimal set of > > bug fixes that will hopefully go to stable (I'm affected by these bugs > > in 5.4). Here are my reasons for feeling like moving to > > tee_shm_register() isn't minimal in terms of a stable-focused fix: > > > > - tee_shm_alloc() looks like it should work fine with AMD-TEE today. > > tee_shm_register() definitely does not since AMD-TEE doesn't provide a > > .shm_register or .shm_unregister hook. This may break existing users > > of AMD-TEE? > > AFAIK, ftpm and tee_bnxt_fw drivers only support OP-TEE at this point. > See ftpm_tee_match() and optee_ctx_match() APIs in corresponding > drivers. > > > - tee_shm_register() has not historically been used for kernel > > allocations and is not fixed wrt the bug that Jens fixed in commit > > f1bbacedb0af ("tee: don't assign shm id for private shms"). > > Yes, that's what I meant earlier to make the TEE_SHM_DMA_BUF flag optional. > > > - tee_shm_alloc() performs allocations using contiguous pages > > from alloc_pages() while tee_shm_register() performs non-contiguous > > allocations with kcalloc(). I suspect this would be fine but I don't > > know the secure world side of these things well enough to assess the > > risk involved with such a change on the kernel side. > > > > I don't think that would make any difference. > > > I should have mentioned this in the cover letter but my hope was that > > these minimal changes would be accepted and then additional work could > > be done to merge tee_shm_alloc() and tee_shm_register() in a way that > > would allow the caller to request contiguous or non-contiguous pages, > > fix up the additional issues mentioned above, and then adjust the > > call sites in ftpm and tee_bnxt_fw as appropriate. > > > > I think that's a bigger set of changes because there are several things > > that still confuse/concern me: > > > > - Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register() > > uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three > > exist? > > AFAIK, its due the the inherent nature of tee_shm_alloc() and > tee_shm_register() where tee_shm_alloc() doesn't need to know whether > its a kernel or user-space memory since it is the one that allocates > whereas tee_shm_register() need to know that since it has to register > pre-allocated client memory. > > > - Why does tee_shm_register() unconditionally use non-contiguous > > allocations without ever taking into account whether or not > > OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required > > from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers. > > Yeah, but do we have platforms in OP-TEE that don't support dynamic > shared memory? I guess it has become the sane default which is a > mandatory requirement when it comes to OP-TEE driver in u-boot. > > > - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is > > specific to OP-TEE? How to better abstract that away? > > > > I would like you to go through Section "3.2.4. Shared Memory" in TEE > Client API Specification. There are two standard ways for shared > memory approach with TEE: > > 1. A Shared Memory block can either be existing Client Application > memory (kernel driver in our case) which is subsequently registered > with the TEE Client API (using tee_shm_register() in our case). > > 2. Or memory which is allocated on behalf of the Client Application > using the TEE > Client API (using tee_shm_alloc() in our case). > > > Let me know if you agree with the more minimal approach that I took for > > these bug fix series or still feel like tee_shm_register() should be > > fixed up so that it is usable. Thanks! > > From drivers perspective I think the change should be: > > tee_shm_alloc() > > to > > kcalloc() > tee_shm_register() I've just posted "[PATCH 0/7] tee: shared memory updates", https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/ Where tee_shm_alloc() is replaced by among other functions tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the problem with TEE_SHM_DMA_BUF. Cheers, Jens
On 2021-06-09 14:15:33, Jens Wiklander wrote: > Hi, > > On Wed, Jun 09, 2021 at 04:22:49PM +0530, Sumit Garg wrote: > > + Rijo > > > > On Wed, 9 Jun 2021 at 11:16, Tyler Hicks <tyhicks@linux.microsoft.com> wrote: > > > > > > On 2021-06-09 09:59:04, Sumit Garg wrote: > > > > Hi Tyler, > > > > > > Hey Sumit - Thanks for the review. > > > > > > > > > > > On Wed, 9 Jun 2021 at 05:55, Tyler Hicks <tyhicks@linux.microsoft.com> wrote: > > > > > > > > > > Uncouple the registration of dynamic shared memory buffers from the > > > > > TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory > > > > > regions but do not need them to be backed by a dma-buf when the memory > > > > > region is private to the driver. > > > > > > > > In this case drivers should use tee_shm_register() instead where the > > > > memory allocated is actually private to the driver. However, you need > > > > to remove TEE_SHM_DMA_BUF as a mandatory flag for tee_shm_register(). > > > > Have a look at an example here [1]. So modifying tee_shm_alloc() for > > > > this purpose doesn't look appropriate to me. > > > > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n73 > > > > > > I noticed what you did in commit 2a6ba3f794e8 ("tee: enable support to > > > register kernel memory") and considered moving ftpm and tee_bnxt_fw over > > > to tee_shm_register(). I think that's likely the right long term > > > approach but I decided against it since this series is a minimal set of > > > bug fixes that will hopefully go to stable (I'm affected by these bugs > > > in 5.4). Here are my reasons for feeling like moving to > > > tee_shm_register() isn't minimal in terms of a stable-focused fix: > > > > > > - tee_shm_alloc() looks like it should work fine with AMD-TEE today. > > > tee_shm_register() definitely does not since AMD-TEE doesn't provide a > > > .shm_register or .shm_unregister hook. This may break existing users > > > of AMD-TEE? > > > > AFAIK, ftpm and tee_bnxt_fw drivers only support OP-TEE at this point. > > See ftpm_tee_match() and optee_ctx_match() APIs in corresponding > > drivers. > > > > > - tee_shm_register() has not historically been used for kernel > > > allocations and is not fixed wrt the bug that Jens fixed in commit > > > f1bbacedb0af ("tee: don't assign shm id for private shms"). > > > > Yes, that's what I meant earlier to make the TEE_SHM_DMA_BUF flag optional. > > > > > - tee_shm_alloc() performs allocations using contiguous pages > > > from alloc_pages() while tee_shm_register() performs non-contiguous > > > allocations with kcalloc(). I suspect this would be fine but I don't > > > know the secure world side of these things well enough to assess the > > > risk involved with such a change on the kernel side. > > > > > > > I don't think that would make any difference. > > > > > I should have mentioned this in the cover letter but my hope was that > > > these minimal changes would be accepted and then additional work could > > > be done to merge tee_shm_alloc() and tee_shm_register() in a way that > > > would allow the caller to request contiguous or non-contiguous pages, > > > fix up the additional issues mentioned above, and then adjust the > > > call sites in ftpm and tee_bnxt_fw as appropriate. > > > > > > I think that's a bigger set of changes because there are several things > > > that still confuse/concern me: > > > > > > - Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register() > > > uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three > > > exist? > > > > AFAIK, its due the the inherent nature of tee_shm_alloc() and > > tee_shm_register() where tee_shm_alloc() doesn't need to know whether > > its a kernel or user-space memory since it is the one that allocates > > whereas tee_shm_register() need to know that since it has to register > > pre-allocated client memory. > > > > > - Why does tee_shm_register() unconditionally use non-contiguous > > > allocations without ever taking into account whether or not > > > OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required > > > from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers. > > > > Yeah, but do we have platforms in OP-TEE that don't support dynamic > > shared memory? I guess it has become the sane default which is a > > mandatory requirement when it comes to OP-TEE driver in u-boot. > > > > > - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is > > > specific to OP-TEE? How to better abstract that away? > > > > > > > I would like you to go through Section "3.2.4. Shared Memory" in TEE > > Client API Specification. There are two standard ways for shared > > memory approach with TEE: > > > > 1. A Shared Memory block can either be existing Client Application > > memory (kernel driver in our case) which is subsequently registered > > with the TEE Client API (using tee_shm_register() in our case). > > > > 2. Or memory which is allocated on behalf of the Client Application > > using the TEE > > Client API (using tee_shm_alloc() in our case). > > > > > Let me know if you agree with the more minimal approach that I took for > > > these bug fix series or still feel like tee_shm_register() should be > > > fixed up so that it is usable. Thanks! > > > > From drivers perspective I think the change should be: > > > > tee_shm_alloc() > > > > to > > > > kcalloc() > > tee_shm_register() > > I've just posted "[PATCH 0/7] tee: shared memory updates", > https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/ > > Where tee_shm_alloc() is replaced by among other functions > tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the > problem with TEE_SHM_DMA_BUF. Thanks! At first glance, that series would take care of the last three patches in my kexec/kdump series. I'm a bit worried that it is a rewrite of the shm allocator. Do you plan to send all of that to stable? (I mentioned earlier in this thread that I'm affected by these bugs in linux-5.4.y.) Also, you and Sumit don't seem to have the same opinion on kernel drivers making use of tee_shm_register() for allocations that are only used internally. Can you comment on that? I'm not clear on the next steps for fixing these kexec/kdump bugs in older releases. I appreciate any guidance here. Tyler > > Cheers, > Jens >
On 2021-06-09 08:42:28, Tyler Hicks wrote: > On 2021-06-09 14:15:33, Jens Wiklander wrote: > > Hi, > > > > On Wed, Jun 09, 2021 at 04:22:49PM +0530, Sumit Garg wrote: > > > + Rijo > > > > > > On Wed, 9 Jun 2021 at 11:16, Tyler Hicks <tyhicks@linux.microsoft.com> wrote: > > > > > > > > On 2021-06-09 09:59:04, Sumit Garg wrote: > > > > > Hi Tyler, > > > > > > > > Hey Sumit - Thanks for the review. > > > > > > > > > > > > > > On Wed, 9 Jun 2021 at 05:55, Tyler Hicks <tyhicks@linux.microsoft.com> wrote: > > > > > > > > > > > > Uncouple the registration of dynamic shared memory buffers from the > > > > > > TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory > > > > > > regions but do not need them to be backed by a dma-buf when the memory > > > > > > region is private to the driver. > > > > > > > > > > In this case drivers should use tee_shm_register() instead where the > > > > > memory allocated is actually private to the driver. However, you need > > > > > to remove TEE_SHM_DMA_BUF as a mandatory flag for tee_shm_register(). > > > > > Have a look at an example here [1]. So modifying tee_shm_alloc() for > > > > > this purpose doesn't look appropriate to me. > > > > > > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n73 > > > > > > > > I noticed what you did in commit 2a6ba3f794e8 ("tee: enable support to > > > > register kernel memory") and considered moving ftpm and tee_bnxt_fw over > > > > to tee_shm_register(). I think that's likely the right long term > > > > approach but I decided against it since this series is a minimal set of > > > > bug fixes that will hopefully go to stable (I'm affected by these bugs > > > > in 5.4). Here are my reasons for feeling like moving to > > > > tee_shm_register() isn't minimal in terms of a stable-focused fix: > > > > > > > > - tee_shm_alloc() looks like it should work fine with AMD-TEE today. > > > > tee_shm_register() definitely does not since AMD-TEE doesn't provide a > > > > .shm_register or .shm_unregister hook. This may break existing users > > > > of AMD-TEE? > > > > > > AFAIK, ftpm and tee_bnxt_fw drivers only support OP-TEE at this point. > > > See ftpm_tee_match() and optee_ctx_match() APIs in corresponding > > > drivers. > > > > > > > - tee_shm_register() has not historically been used for kernel > > > > allocations and is not fixed wrt the bug that Jens fixed in commit > > > > f1bbacedb0af ("tee: don't assign shm id for private shms"). > > > > > > Yes, that's what I meant earlier to make the TEE_SHM_DMA_BUF flag optional. > > > > > > > - tee_shm_alloc() performs allocations using contiguous pages > > > > from alloc_pages() while tee_shm_register() performs non-contiguous > > > > allocations with kcalloc(). I suspect this would be fine but I don't > > > > know the secure world side of these things well enough to assess the > > > > risk involved with such a change on the kernel side. > > > > > > > > > > I don't think that would make any difference. > > > > > > > I should have mentioned this in the cover letter but my hope was that > > > > these minimal changes would be accepted and then additional work could > > > > be done to merge tee_shm_alloc() and tee_shm_register() in a way that > > > > would allow the caller to request contiguous or non-contiguous pages, > > > > fix up the additional issues mentioned above, and then adjust the > > > > call sites in ftpm and tee_bnxt_fw as appropriate. > > > > > > > > I think that's a bigger set of changes because there are several things > > > > that still confuse/concern me: > > > > > > > > - Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register() > > > > uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three > > > > exist? > > > > > > AFAIK, its due the the inherent nature of tee_shm_alloc() and > > > tee_shm_register() where tee_shm_alloc() doesn't need to know whether > > > its a kernel or user-space memory since it is the one that allocates > > > whereas tee_shm_register() need to know that since it has to register > > > pre-allocated client memory. > > > > > > > - Why does tee_shm_register() unconditionally use non-contiguous > > > > allocations without ever taking into account whether or not > > > > OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required > > > > from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers. > > > > > > Yeah, but do we have platforms in OP-TEE that don't support dynamic > > > shared memory? I guess it has become the sane default which is a > > > mandatory requirement when it comes to OP-TEE driver in u-boot. > > > > > > > - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is > > > > specific to OP-TEE? How to better abstract that away? > > > > > > > > > > I would like you to go through Section "3.2.4. Shared Memory" in TEE > > > Client API Specification. There are two standard ways for shared > > > memory approach with TEE: > > > > > > 1. A Shared Memory block can either be existing Client Application > > > memory (kernel driver in our case) which is subsequently registered > > > with the TEE Client API (using tee_shm_register() in our case). > > > > > > 2. Or memory which is allocated on behalf of the Client Application > > > using the TEE > > > Client API (using tee_shm_alloc() in our case). > > > > > > > Let me know if you agree with the more minimal approach that I took for > > > > these bug fix series or still feel like tee_shm_register() should be > > > > fixed up so that it is usable. Thanks! > > > > > > From drivers perspective I think the change should be: > > > > > > tee_shm_alloc() > > > > > > to > > > > > > kcalloc() > > > tee_shm_register() > > > > I've just posted "[PATCH 0/7] tee: shared memory updates", > > https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/ > > > > Where tee_shm_alloc() is replaced by among other functions > > tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the > > problem with TEE_SHM_DMA_BUF. > > Thanks! At first glance, that series would take care of the last three > patches in my kexec/kdump series. Correction: Your series would not completely take care of the last three patches in my kexec/kdump series because your series doesn't implement the .shutdown() hook for tee_bnxt_fw. Does it make sense to take my series first and then rebase your series on top of it? That would allow my fixes to flow back to stable, then your changes would greatly clean up the implementation in future releases. Tyler > > I'm a bit worried that it is a rewrite of the shm allocator. Do you plan > to send all of that to stable? (I mentioned earlier in this thread that > I'm affected by these bugs in linux-5.4.y.) > > Also, you and Sumit don't seem to have the same opinion on kernel > drivers making use of tee_shm_register() for allocations that are only > used internally. Can you comment on that? > > I'm not clear on the next steps for fixing these kexec/kdump bugs in > older releases. I appreciate any guidance here. > > Tyler > > > > > Cheers, > > Jens > >
On Wed, Jun 09, 2021 at 04:22:49PM +0530, Sumit Garg wrote: > + Rijo > > On Wed, 9 Jun 2021 at 11:16, Tyler Hicks <tyhicks@linux.microsoft.com> wrote: [snip] > > > - tee_shm_alloc() performs allocations using contiguous pages > > from alloc_pages() while tee_shm_register() performs non-contiguous > > allocations with kcalloc(). I suspect this would be fine but I don't > > know the secure world side of these things well enough to assess the > > risk involved with such a change on the kernel side. > > > > I don't think that would make any difference. Agree. > > > I should have mentioned this in the cover letter but my hope was that > > these minimal changes would be accepted and then additional work could > > be done to merge tee_shm_alloc() and tee_shm_register() in a way that > > would allow the caller to request contiguous or non-contiguous pages, > > fix up the additional issues mentioned above, and then adjust the > > call sites in ftpm and tee_bnxt_fw as appropriate. > > > > I think that's a bigger set of changes because there are several things > > that still confuse/concern me: > > > > - Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register() > > uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three > > exist? > > AFAIK, its due the the inherent nature of tee_shm_alloc() and > tee_shm_register() where tee_shm_alloc() doesn't need to know whether > its a kernel or user-space memory since it is the one that allocates > whereas tee_shm_register() need to know that since it has to register > pre-allocated client memory. > > > - Why does tee_shm_register() unconditionally use non-contiguous > > allocations without ever taking into account whether or not > > OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required > > from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers. > > Yeah, but do we have platforms in OP-TEE that don't support dynamic > shared memory? I guess it has become the sane default which is a > mandatory requirement when it comes to OP-TEE driver in u-boot. > > > - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is > > specific to OP-TEE? How to better abstract that away? > > > > I would like you to go through Section "3.2.4. Shared Memory" in TEE > Client API Specification. There are two standard ways for shared > memory approach with TEE: > > 1. A Shared Memory block can either be existing Client Application > memory (kernel driver in our case) which is subsequently registered > with the TEE Client API (using tee_shm_register() in our case). > > 2. Or memory which is allocated on behalf of the Client Application > using the TEE > Client API (using tee_shm_alloc() in our case). > > > Let me know if you agree with the more minimal approach that I took for > > these bug fix series or still feel like tee_shm_register() should be > > fixed up so that it is usable. Thanks! > > From drivers perspective I think the change should be: > > tee_shm_alloc() > > to > > kcalloc() > tee_shm_register() I had another approach in mind in "[PATCH 0/7] tee: shared memory updates", https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/ The flags needed by tee_shm_alloc() and tee_shm_register() aren't very intuitive and in fact only accept quite few combinations. So my idea was to hide those flags from callers outside of the TEE subsystem with tee_shm_alloc_kernel_buf(). The approach with tee_shm_register() you suggest above has the drawback that the TEE driver is forced to be able to handle any kernel memory. This is OK with OP-TEE and dynamic shared memory enabled, but there are platforms where dynamic shared memory isn't enabled. In those case must the memory be allocated from a special pool. Do you see any problem with instead replacing tee_shm_alloc() with tee_shm_alloc_kernel_buf()? Cheers, Jens
On Wed, Jun 09, 2021 at 08:51:04AM -0500, Tyler Hicks wrote: [snip] > > > I've just posted "[PATCH 0/7] tee: shared memory updates", > > > https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/ > > > > > > Where tee_shm_alloc() is replaced by among other functions > > > tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the > > > problem with TEE_SHM_DMA_BUF. > > > > Thanks! At first glance, that series would take care of the last three > > patches in my kexec/kdump series. > > Correction: Your series would not completely take care of the last three > patches in my kexec/kdump series because your series doesn't implement > the .shutdown() hook for tee_bnxt_fw. > > Does it make sense to take my series first and then rebase your series > on top of it? That would allow my fixes to flow back to stable, then > your changes would greatly clean up the implementation in future > releases. Yes, we could try that. I'd like to see tee_shm_alloc_kernel_buf() being used instead of tee_shm_alloc() in ftpm_tee_probe() and tee_bnxt_fw_probe(). So it would be great if you could include "tee: add tee_shm_alloc_kernel_buf()" in your patch set. My patch set would then shrink a bit. By the way, thanks for reviewing it. Cheers, Jens
>> >> AFAIK, its due the the inherent nature of tee_shm_alloc() and >> tee_shm_register() where tee_shm_alloc() doesn't need to know whether >> its a kernel or user-space memory since it is the one that allocates >> whereas tee_shm_register() need to know that since it has to register >> pre-allocated client memory. >> >>> - Why does tee_shm_register() unconditionally use non-contiguous >>> allocations without ever taking into account whether or not >>> OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required >>> from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers. >> >> Yeah, but do we have platforms in OP-TEE that don't support dynamic >> shared memory? I guess it has become the sane default which is a >> mandatory requirement when it comes to OP-TEE driver in u-boot. >> >>> - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is >>> specific to OP-TEE? How to better abstract that away? >>> >> >> I would like you to go through Section "3.2.4. Shared Memory" in TEE >> Client API Specification. There are two standard ways for shared >> memory approach with TEE: >> >> 1. A Shared Memory block can either be existing Client Application >> memory (kernel driver in our case) which is subsequently registered >> with the TEE Client API (using tee_shm_register() in our case). >> >> 2. Or memory which is allocated on behalf of the Client Application >> using the TEE >> Client API (using tee_shm_alloc() in our case). >> >>> Let me know if you agree with the more minimal approach that I took for >>> these bug fix series or still feel like tee_shm_register() should be >>> fixed up so that it is usable. Thanks! >> >> From drivers perspective I think the change should be: >> >> tee_shm_alloc() >> >> to >> >> kcalloc() >> tee_shm_register() > > I've just posted "[PATCH 0/7] tee: shared memory updates", > https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/ > > Where tee_shm_alloc() is replaced by among other functions > tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the > problem with TEE_SHM_DMA_BUF. > Thanks Jens. The series looks fine. Tested too. - Allen
On Wed, Jun 09, 2021 at 08:42:25AM -0500, Tyler Hicks wrote: [snip] > > I've just posted "[PATCH 0/7] tee: shared memory updates", > > https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/ > > > > Where tee_shm_alloc() is replaced by among other functions > > tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the > > problem with TEE_SHM_DMA_BUF. > > Thanks! At first glance, that series would take care of the last three > patches in my kexec/kdump series. > > I'm a bit worried that it is a rewrite of the shm allocator. Do you plan > to send all of that to stable? (I mentioned earlier in this thread that > I'm affected by these bugs in linux-5.4.y.) No, that might be a bit much. > Also, you and Sumit don't seem to have the same opinion on kernel > drivers making use of tee_shm_register() for allocations that are only > used internally. Can you comment on that? > > I'm not clear on the next steps for fixing these kexec/kdump bugs in > older releases. I appreciate any guidance here. Neither am I be honest. You're the only one that has brought up this problem so perhaps it's enough to focus on the stable branch you need to have fixed. If I've understood it correctly it's best if it's possible to cherry-pick the fixes from mainline to the stable branch in question. So we must make sure to get your needed patches in before any rewrites that would make cherry-picking impossible. The rewrite I'm proposing isn't urgent so it can be held off for a while. Cheers, Jens
Hi Jens, On Thu, 10 Jun 2021 at 12:48, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > On Wed, Jun 09, 2021 at 04:22:49PM +0530, Sumit Garg wrote: > > + Rijo > > > > On Wed, 9 Jun 2021 at 11:16, Tyler Hicks <tyhicks@linux.microsoft.com> wrote: > [snip] > > > > > - tee_shm_alloc() performs allocations using contiguous pages > > > from alloc_pages() while tee_shm_register() performs non-contiguous > > > allocations with kcalloc(). I suspect this would be fine but I don't > > > know the secure world side of these things well enough to assess the > > > risk involved with such a change on the kernel side. > > > > > > > I don't think that would make any difference. > > Agree. > > > > > > I should have mentioned this in the cover letter but my hope was that > > > these minimal changes would be accepted and then additional work could > > > be done to merge tee_shm_alloc() and tee_shm_register() in a way that > > > would allow the caller to request contiguous or non-contiguous pages, > > > fix up the additional issues mentioned above, and then adjust the > > > call sites in ftpm and tee_bnxt_fw as appropriate. > > > > > > I think that's a bigger set of changes because there are several things > > > that still confuse/concern me: > > > > > > - Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register() > > > uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three > > > exist? > > > > AFAIK, its due the the inherent nature of tee_shm_alloc() and > > tee_shm_register() where tee_shm_alloc() doesn't need to know whether > > its a kernel or user-space memory since it is the one that allocates > > whereas tee_shm_register() need to know that since it has to register > > pre-allocated client memory. > > > > > - Why does tee_shm_register() unconditionally use non-contiguous > > > allocations without ever taking into account whether or not > > > OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required > > > from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers. > > > > Yeah, but do we have platforms in OP-TEE that don't support dynamic > > shared memory? I guess it has become the sane default which is a > > mandatory requirement when it comes to OP-TEE driver in u-boot. > > > > > - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is > > > specific to OP-TEE? How to better abstract that away? > > > > > > > I would like you to go through Section "3.2.4. Shared Memory" in TEE > > Client API Specification. There are two standard ways for shared > > memory approach with TEE: > > > > 1. A Shared Memory block can either be existing Client Application > > memory (kernel driver in our case) which is subsequently registered > > with the TEE Client API (using tee_shm_register() in our case). > > > > 2. Or memory which is allocated on behalf of the Client Application > > using the TEE > > Client API (using tee_shm_alloc() in our case). > > > > > Let me know if you agree with the more minimal approach that I took for > > > these bug fix series or still feel like tee_shm_register() should be > > > fixed up so that it is usable. Thanks! > > > > From drivers perspective I think the change should be: > > > > tee_shm_alloc() > > > > to > > > > kcalloc() > > tee_shm_register() > > I had another approach in mind in "[PATCH 0/7] tee: shared memory updates", > https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/ > > The flags needed by tee_shm_alloc() and tee_shm_register() aren't > very intuitive and in fact only accept quite few combinations. So my > idea was to hide those flags from callers outside of the TEE subsystem > with tee_shm_alloc_kernel_buf(). > That looks like a good idea to hide flags from users. BTW, my only objection earlier with Tyler's and your patch-set is the usage of TEE_SHM_REGISTER flag in generic TEE methods: tee_shm_alloc*. AFAIU, the only reason for such an additional flag is in case of OP-TEE only because the OP-TEE driver could implement allocated shared memory via re-using dynamic shared memory approach as well. And that additional flag is only needed to differentiate that OP-TEE driver's private memory shouldn't be registered with OP-TEE. If this understanding is correct then we should introduce a separate flag as TEE_SHM_PRIV that should only be set inside tee_shm_alloc_anon_kernel_buf(). As otherwise passing TEE_SHM_REGISTER flag for shared memory alloc API for other TEEs like AMD-TEE etc. would be useless. > The approach with tee_shm_register() you suggest above has the drawback > that the TEE driver is forced to be able to handle any kernel memory. That's the value-add in the problem that Tyler is trying to resolve that driver should be able to free up the memory as needed as a private buffer. > This is OK with OP-TEE and dynamic shared memory enabled, but there are > platforms where dynamic shared memory isn't enabled. In those case must > the memory be allocated from a special pool. Is there any limitation for those platforms to not support dynamic shared memory in OP-TEE? If there isn't then we should able to handle this via match for TEE_GEN_CAP_REG_MEM in the ftpm_tee_match() and optee_ctx_match() APIs. > > Do you see any problem with instead replacing tee_shm_alloc() > with tee_shm_alloc_kernel_buf()? I don't see any problems apart from one mentioned above. -Sumit > > Cheers, > Jens
On 2021-06-10 09:34:24, Jens Wiklander wrote: > On Wed, Jun 09, 2021 at 08:51:04AM -0500, Tyler Hicks wrote: > [snip] > > > > I've just posted "[PATCH 0/7] tee: shared memory updates", > > > > https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/ > > > > > > > > Where tee_shm_alloc() is replaced by among other functions > > > > tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the > > > > problem with TEE_SHM_DMA_BUF. > > > > > > Thanks! At first glance, that series would take care of the last three > > > patches in my kexec/kdump series. > > > > Correction: Your series would not completely take care of the last three > > patches in my kexec/kdump series because your series doesn't implement > > the .shutdown() hook for tee_bnxt_fw. > > > > Does it make sense to take my series first and then rebase your series > > on top of it? That would allow my fixes to flow back to stable, then > > your changes would greatly clean up the implementation in future > > releases. > > Yes, we could try that. I'd like to see tee_shm_alloc_kernel_buf() being > used instead of tee_shm_alloc() in ftpm_tee_probe() and > tee_bnxt_fw_probe(). So it would be great if you could include "tee: add > tee_shm_alloc_kernel_buf()" in your patch set. That would be no problem at all. I like that idea and I've prepared a v4 with that change. I'll send it out shortly once I've finished testing. > My patch set would then shrink a bit. By the way, thanks for reviewing > it. No problem! I feel like I'm starting to understand the TEE subsystem and OP-TEE driver a bit so I'm happy to help out. Tyler > > Cheers, > Jens >
On 2021-06-10 09:49:48, Jens Wiklander wrote: > On Wed, Jun 09, 2021 at 08:42:25AM -0500, Tyler Hicks wrote: > [snip] > > > I've just posted "[PATCH 0/7] tee: shared memory updates", > > > https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/ > > > > > > Where tee_shm_alloc() is replaced by among other functions > > > tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the > > > problem with TEE_SHM_DMA_BUF. > > > > Thanks! At first glance, that series would take care of the last three > > patches in my kexec/kdump series. > > > > I'm a bit worried that it is a rewrite of the shm allocator. Do you plan > > to send all of that to stable? (I mentioned earlier in this thread that > > I'm affected by these bugs in linux-5.4.y.) > > No, that might be a bit much. > > > Also, you and Sumit don't seem to have the same opinion on kernel > > drivers making use of tee_shm_register() for allocations that are only > > used internally. Can you comment on that? > > > > I'm not clear on the next steps for fixing these kexec/kdump bugs in > > older releases. I appreciate any guidance here. > > Neither am I be honest. You're the only one that has brought up this > problem so perhaps it's enough to focus on the stable branch you need to > have fixed. I've already added Fixes tags to all of my patches. If you are comfortable with them going to stable, you'd add 'Cc: stable@vger.kernel.org' to them if/when you merge them so that the stable team will ensure that they're applied. Note that I'm not the only person that brought up this bug: https://github.com/OP-TEE/optee_os/issues/3637 Once I started digging in, I realized that there were more kexec/kdump bugs and the series grew. > If I've understood it correctly it's best if it's possible to > cherry-pick the fixes from mainline to the stable branch in question. > So we must make sure to get your needed patches in before any rewrites > that would make cherry-picking impossible. The rewrite I'm proposing > isn't urgent so it can be held off for a while. Thanks for holding off. I'll be quick on my revisions so that you don't have to moth ball your series for too much longer. Tyler > > Cheers, > Jens >
diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c index da06ce9b9313..6054343a29fb 100644 --- a/drivers/tee/optee/shm_pool.c +++ b/drivers/tee/optee/shm_pool.c @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, shm->paddr = page_to_phys(page); shm->size = PAGE_SIZE << order; - if (shm->flags & TEE_SHM_DMA_BUF) { + if (shm->flags & TEE_SHM_REGISTER) { unsigned int nr_pages = 1 << order, i; struct page **pages; @@ -42,7 +42,6 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, page++; } - shm->flags |= TEE_SHM_REGISTER; rc = optee_shm_register(shm->ctx, shm, pages, nr_pages, (unsigned long)shm->kaddr); kfree(pages); @@ -60,7 +59,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm) { - if (shm->flags & TEE_SHM_DMA_BUF) + if (shm->flags & TEE_SHM_REGISTER) optee_shm_unregister(shm->ctx, shm); free_pages((unsigned long)shm->kaddr, get_order(shm->size)); diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 00472f5ce22e..1c0176550b9c 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -117,7 +117,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) return ERR_PTR(-EINVAL); } - if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) { + if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER))) { dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags); return ERR_PTR(-EINVAL); } @@ -137,6 +137,15 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) goto err_dev_put; } + if (!teedev->desc->ops->shm_register || + !teedev->desc->ops->shm_unregister) { + /* registration is not required by the TEE implementation */ + flags &= ~TEE_SHM_REGISTER; + } else if (flags & TEE_SHM_DMA_BUF) { + /* all dma-buf backed shm allocations are registered */ + flags |= TEE_SHM_REGISTER; + } + shm->flags = flags | TEE_SHM_POOL; shm->ctx = ctx; if (flags & TEE_SHM_DMA_BUF)
Uncouple the registration of dynamic shared memory buffers from the TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory regions but do not need them to be backed by a dma-buf when the memory region is private to the driver. Allow callers of tee_shm_alloc() to specify the TEE_SHM_REGISTER flag to request registration. If the TEE implementation does not require dynamic shared memory to be registered, clear the flag prior to calling the corresponding pool alloc function. Update the OP-TEE driver to respect TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to (un)register on alloc/free operations. The AMD-TEE driver continues to ignore the TEE_SHM_REGISTER flag. Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> --- drivers/tee/optee/shm_pool.c | 5 ++--- drivers/tee/tee_shm.c | 11 ++++++++++- 2 files changed, 12 insertions(+), 4 deletions(-)