Skip manipulating the refcount in case of slab pages according commit b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
Fixes: b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page") Signed-off-by: Marco Felsch m.felsch@pengutronix.de --- v2: - Make use of page variable v1: - https://lore.kernel.org/all/20250325195021.3589797-1-m.felsch@pengutronix.de...
drivers/tee/tee_shm.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index daf6e5cfd59a..35f0ac359b12 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -19,16 +19,24 @@ static void shm_put_kernel_pages(struct page **pages, size_t page_count) { size_t n;
- for (n = 0; n < page_count; n++) - put_page(pages[n]); + for (n = 0; n < page_count; n++) { + struct page *page = pages[n]; + + if (!PageSlab(page)) + put_page(page); + } }
static void shm_get_kernel_pages(struct page **pages, size_t page_count) { size_t n;
- for (n = 0; n < page_count; n++) - get_page(pages[n]); + for (n = 0; n < page_count; n++) { + struct page *page = pages[n]; + + if (!PageSlab(page)) + get_page(page); + } }
static void release_registered_pages(struct tee_shm *shm)
Hi Marco,
On Tue, Mar 25, 2025 at 9:07 PM Marco Felsch m.felsch@pengutronix.de wrote:
Skip manipulating the refcount in case of slab pages according commit b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
Fixes: b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page") Signed-off-by: Marco Felsch m.felsch@pengutronix.de
v2:
- Make use of page variable
v1:
drivers/tee/tee_shm.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index daf6e5cfd59a..35f0ac359b12 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -19,16 +19,24 @@ static void shm_put_kernel_pages(struct page **pages, size_t page_count) { size_t n;
for (n = 0; n < page_count; n++)put_page(pages[n]);
for (n = 0; n < page_count; n++) {struct page *page = pages[n];if (!PageSlab(page))put_page(page);}}
static void shm_get_kernel_pages(struct page **pages, size_t page_count) { size_t n;
for (n = 0; n < page_count; n++)get_page(pages[n]);
for (n = 0; n < page_count; n++) {struct page *page = pages[n];if (!PageSlab(page))get_page(page);
b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page") mentions that more page types will have a zero refcount in the longer term. So we'll need to add exception after exception here. Is there a helper function somewhere to get all the pages we need? Or can we do this differently?
Cheers, Jens
}}
static void release_registered_pages(struct tee_shm *shm)
2.39.5
Hi Jens,
On 25-03-26, Jens Wiklander wrote:
Hi Marco,
On Tue, Mar 25, 2025 at 9:07 PM Marco Felsch m.felsch@pengutronix.de wrote:
Skip manipulating the refcount in case of slab pages according commit b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
Fixes: b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page") Signed-off-by: Marco Felsch m.felsch@pengutronix.de
v2:
- Make use of page variable
v1:
drivers/tee/tee_shm.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index daf6e5cfd59a..35f0ac359b12 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -19,16 +19,24 @@ static void shm_put_kernel_pages(struct page **pages, size_t page_count) { size_t n;
for (n = 0; n < page_count; n++)put_page(pages[n]);
for (n = 0; n < page_count; n++) {struct page *page = pages[n];if (!PageSlab(page))put_page(page);}}
static void shm_get_kernel_pages(struct page **pages, size_t page_count) { size_t n;
for (n = 0; n < page_count; n++)get_page(pages[n]);
for (n = 0; n < page_count; n++) {struct page *page = pages[n];if (!PageSlab(page))get_page(page);b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page") mentions that more page types will have a zero refcount in the longer term. So we'll need to add exception after exception here. Is there a helper function somewhere to get all the pages we need? Or can we do this differently?
You're right, but in the long-term perspective the patch also mentions "... stop taking a refcount on the pages that it uses and rely on the caller to hold whatever references are necessary to make the memory stable."
As you mentioned, in the medium term more pages are going to have a zero refcount. I think that once mm is starting to add more zero refcounted page types, they will also add a helper like "PageRefcounted()" or so.
At the moment all users are changed to cover only the slab use-case. Therefore I would keep it as it is right now and change it to the new helper later on.
Regards, Marco
Cheers, Jens
}}
static void release_registered_pages(struct tee_shm *shm)
2.39.5
On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote:
Skip manipulating the refcount in case of slab pages according commit b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
This almost certainly isn't right. I know nothing about TEE, but that you are doing this indicates a problem. The hack that we put into networking should not be blindly replicated.
Why are you taking a reference on the pages to begin with? Is it copy and pasted from somewhere else, or was there actual thought put into it?
If it's "prevent the caller from freeing the allocation", well, it never accomplished that with slab allocations. So for callers that do kmalloc (eg setup_mm_hdr() in drivers/firmware/efi/stmm/tee_stmm_efi.c), you have to rely on them not freeing the allocation while the TEE driver has it.
And if that's your API contract, then there's no point in taking refcounts on other kinds of pages either; it's just unnecessary atomic instructions. So the right patch might be something like this:
+++ b/drivers/tee/tee_shm.c @@ -15,29 +15,11 @@ #include <linux/highmem.h> #include "tee_private.h"
-static void shm_put_kernel_pages(struct page **pages, size_t page_count) -{ - size_t n; - - for (n = 0; n < page_count; n++) - put_page(pages[n]); -} - -static void shm_get_kernel_pages(struct page **pages, size_t page_count) -{ - size_t n; - - for (n = 0; n < page_count; n++) - get_page(pages[n]); -} - static void release_registered_pages(struct tee_shm *shm) { if (shm->pages) { if (shm->flags & TEE_SHM_USER_MAPPED) unpin_user_pages(shm->pages, shm->num_pages); - else - shm_put_kernel_pages(shm->pages, shm->num_pages);
kfree(shm->pages); } @@ -321,13 +303,6 @@ register_shm_helper(struct tee_context *ctx, struct iov_iter *iter, u32 flags, goto err_free_shm_pages; }
- /* - * iov_iter_extract_kvec_pages does not get reference on the pages, - * get a reference on them. - */ - if (iov_iter_is_kvec(iter)) - shm_get_kernel_pages(shm->pages, num_pages); - shm->offset = off; shm->size = len; shm->num_pages = num_pages; @@ -341,10 +316,8 @@ register_shm_helper(struct tee_context *ctx, struct iov_iter *iter, u32 flags,
return shm; err_put_shm_pages: - if (!iov_iter_is_kvec(iter)) + if (iter_is_uvec(iter)) unpin_user_pages(shm->pages, shm->num_pages); - else - shm_put_kernel_pages(shm->pages, shm->num_pages); err_free_shm_pages: kfree(shm->pages); err_free_shm:
On 25-03-26, Matthew Wilcox wrote:
On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote:
Skip manipulating the refcount in case of slab pages according commit b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
This almost certainly isn't right. I know nothing about TEE, but that you are doing this indicates a problem. The hack that we put into networking should not be blindly replicated.
Why are you taking a reference on the pages to begin with? Is it copy and pasted from somewhere else, or was there actual thought put into it?
Not sure, this belongs to the TEE maintainers.
If it's "prevent the caller from freeing the allocation", well, it never accomplished that with slab allocations. So for callers that do kmalloc (eg setup_mm_hdr() in drivers/firmware/efi/stmm/tee_stmm_efi.c), you have to rely on them not freeing the allocation while the TEE driver has it.
And if that's your API contract, then there's no point in taking refcounts on other kinds of pages either; it's just unnecessary atomic instructions. So the right patch might be something like this:
+++ b/drivers/tee/tee_shm.c @@ -15,29 +15,11 @@ #include <linux/highmem.h> #include "tee_private.h"
I had the same diff but didn't went this way since we can't be sure that iov's are always slab backed. As far as I understood IOVs. In 'worst-case' scenario an iov can be backed by different page types too.
Regards, Marco
-static void shm_put_kernel_pages(struct page **pages, size_t page_count) -{
size_t n;for (n = 0; n < page_count; n++)put_page(pages[n]);-}
-static void shm_get_kernel_pages(struct page **pages, size_t page_count) -{
size_t n;for (n = 0; n < page_count; n++)get_page(pages[n]);-}
static void release_registered_pages(struct tee_shm *shm) { if (shm->pages) { if (shm->flags & TEE_SHM_USER_MAPPED) unpin_user_pages(shm->pages, shm->num_pages);
elseshm_put_kernel_pages(shm->pages, shm->num_pages); kfree(shm->pages); }@@ -321,13 +303,6 @@ register_shm_helper(struct tee_context *ctx, struct iov_iter *iter, u32 flags, goto err_free_shm_pages; }
/** iov_iter_extract_kvec_pages does not get reference on the pages,* get a reference on them.*/if (iov_iter_is_kvec(iter))shm_get_kernel_pages(shm->pages, num_pages);shm->offset = off; shm->size = len; shm->num_pages = num_pages;@@ -341,10 +316,8 @@ register_shm_helper(struct tee_context *ctx, struct iov_iter *iter, u32 flags,
return shm;err_put_shm_pages:
if (!iov_iter_is_kvec(iter))
if (iter_is_uvec(iter)) unpin_user_pages(shm->pages, shm->num_pages);
elseshm_put_kernel_pages(shm->pages, shm->num_pages);err_free_shm_pages: kfree(shm->pages); err_free_shm:
On Wed, Mar 26, 2025 at 12:07 PM Marco Felsch m.felsch@pengutronix.de wrote:
On 25-03-26, Matthew Wilcox wrote:
On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote:
Skip manipulating the refcount in case of slab pages according commit b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
This almost certainly isn't right. I know nothing about TEE, but that you are doing this indicates a problem. The hack that we put into networking should not be blindly replicated.
Why are you taking a reference on the pages to begin with? Is it copy and pasted from somewhere else, or was there actual thought put into it?
Not sure, this belongs to the TEE maintainers.
I don't know. We were getting the user pages first, so I assume we just did the same thing when we added support for kernel pages.
If it's "prevent the caller from freeing the allocation", well, it never accomplished that with slab allocations. So for callers that do kmalloc (eg setup_mm_hdr() in drivers/firmware/efi/stmm/tee_stmm_efi.c), you have to rely on them not freeing the allocation while the TEE driver has it.
And if that's your API contract, then there's no point in taking refcounts on other kinds of pages either; it's just unnecessary atomic instructions. So the right patch might be something like this:
+++ b/drivers/tee/tee_shm.c @@ -15,29 +15,11 @@ #include <linux/highmem.h> #include "tee_private.h"
I had the same diff but didn't went this way since we can't be sure that iov's are always slab backed. As far as I understood IOVs. In 'worst-case' scenario an iov can be backed by different page types too.
We're only using kvec's. Briefly, before commit 7bdee4157591 ("tee: Use iov_iter to better support shared buffer registration") we checked with is_vmalloc_addr() || is_kmap_addr(). I like Matthew's suggestion, it's nice to fix problems by deleting code. :-)
Sumit, you know the callers better. What do you think?
Cheers, Jens
Regards, Marco
-static void shm_put_kernel_pages(struct page **pages, size_t page_count) -{
size_t n;for (n = 0; n < page_count; n++)put_page(pages[n]);-}
-static void shm_get_kernel_pages(struct page **pages, size_t page_count) -{
size_t n;for (n = 0; n < page_count; n++)get_page(pages[n]);-}
static void release_registered_pages(struct tee_shm *shm) { if (shm->pages) { if (shm->flags & TEE_SHM_USER_MAPPED) unpin_user_pages(shm->pages, shm->num_pages);
elseshm_put_kernel_pages(shm->pages, shm->num_pages); kfree(shm->pages); }@@ -321,13 +303,6 @@ register_shm_helper(struct tee_context *ctx, struct iov_iter *iter, u32 flags, goto err_free_shm_pages; }
/** iov_iter_extract_kvec_pages does not get reference on the pages,* get a reference on them.*/if (iov_iter_is_kvec(iter))shm_get_kernel_pages(shm->pages, num_pages);shm->offset = off; shm->size = len; shm->num_pages = num_pages;@@ -341,10 +316,8 @@ register_shm_helper(struct tee_context *ctx, struct iov_iter *iter, u32 flags,
return shm;err_put_shm_pages:
if (!iov_iter_is_kvec(iter))
if (iter_is_uvec(iter)) unpin_user_pages(shm->pages, shm->num_pages);
elseshm_put_kernel_pages(shm->pages, shm->num_pages);err_free_shm_pages: kfree(shm->pages); err_free_shm:
On Wed, Mar 26, 2025 at 02:47:46PM +0100, Jens Wiklander wrote:
On Wed, Mar 26, 2025 at 12:07 PM Marco Felsch m.felsch@pengutronix.de wrote:
On 25-03-26, Matthew Wilcox wrote:
On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote:
Skip manipulating the refcount in case of slab pages according commit b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
This almost certainly isn't right. I know nothing about TEE, but that you are doing this indicates a problem. The hack that we put into networking should not be blindly replicated.
Why are you taking a reference on the pages to begin with? Is it copy and pasted from somewhere else, or was there actual thought put into it?
Not sure, this belongs to the TEE maintainers.
I don't know. We were getting the user pages first, so I assume we just did the same thing when we added support for kernel pages.
If it's "prevent the caller from freeing the allocation", well, it never accomplished that with slab allocations. So for callers that do kmalloc (eg setup_mm_hdr() in drivers/firmware/efi/stmm/tee_stmm_efi.c), you have to rely on them not freeing the allocation while the TEE driver has it.
It's not just about the TEE driver but rather if the TEE implementation (a trusted OS) to whom the page is registered with. We don't want the trusted OS to work on registered kernel pages if they gets free somehow in the TEE client driver. Having a reference in the TEE subsystem assured us that won't happen. But if you say slab allocations are still prone the kernel pages getting freed even after refcount then can you suggest how should we handle this better?
As otherwise it can cause very hard to debug problems if trusted OS can manipulate kernel pages that are no longer available.
And if that's your API contract, then there's no point in taking refcounts on other kinds of pages either; it's just unnecessary atomic instructions. So the right patch might be something like this:
+++ b/drivers/tee/tee_shm.c @@ -15,29 +15,11 @@ #include <linux/highmem.h> #include "tee_private.h"
I had the same diff but didn't went this way since we can't be sure that iov's are always slab backed. As far as I understood IOVs. In 'worst-case' scenario an iov can be backed by different page types too.
We're only using kvec's. Briefly, before commit 7bdee4157591 ("tee: Use iov_iter to better support shared buffer registration") we checked with is_vmalloc_addr() || is_kmap_addr(). I like Matthew's suggestion, it's nice to fix problems by deleting code. :-)
Sumit, you know the callers better. What do you think?
If we don't have a sane way to refcont registered kernel pages in TEE subsystem then yeah we have to solely rely on the client drivers to behave properly. Nevertheless, it's still within the kernel boundaries which we can rely upon.
-Sumit
On Thu, Mar 27, 2025 at 5:42 AM Sumit Garg sumit.garg@kernel.org wrote:
On Wed, Mar 26, 2025 at 02:47:46PM +0100, Jens Wiklander wrote:
On Wed, Mar 26, 2025 at 12:07 PM Marco Felsch m.felsch@pengutronix.de wrote:
On 25-03-26, Matthew Wilcox wrote:
On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote:
Skip manipulating the refcount in case of slab pages according commit b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
This almost certainly isn't right. I know nothing about TEE, but that you are doing this indicates a problem. The hack that we put into networking should not be blindly replicated.
Why are you taking a reference on the pages to begin with? Is it copy and pasted from somewhere else, or was there actual thought put into it?
Not sure, this belongs to the TEE maintainers.
I don't know. We were getting the user pages first, so I assume we just did the same thing when we added support for kernel pages.
If it's "prevent the caller from freeing the allocation", well, it never accomplished that with slab allocations. So for callers that do kmalloc (eg setup_mm_hdr() in drivers/firmware/efi/stmm/tee_stmm_efi.c), you have to rely on them not freeing the allocation while the TEE driver has it.
It's not just about the TEE driver but rather if the TEE implementation (a trusted OS) to whom the page is registered with. We don't want the trusted OS to work on registered kernel pages if they gets free somehow in the TEE client driver. Having a reference in the TEE subsystem assured us that won't happen. But if you say slab allocations are still prone the kernel pages getting freed even after refcount then can you suggest how should we handle this better?
As otherwise it can cause very hard to debug problems if trusted OS can manipulate kernel pages that are no longer available.
We must be able to rely on the kernel callers to have the needed references before calling tee_shm_register_kernel_buf() and to keep those until after calling tee_shm_free().
And if that's your API contract, then there's no point in taking refcounts on other kinds of pages either; it's just unnecessary atomic instructions. So the right patch might be something like this:
+++ b/drivers/tee/tee_shm.c @@ -15,29 +15,11 @@ #include <linux/highmem.h> #include "tee_private.h"
I had the same diff but didn't went this way since we can't be sure that iov's are always slab backed. As far as I understood IOVs. In 'worst-case' scenario an iov can be backed by different page types too.
We're only using kvec's. Briefly, before commit 7bdee4157591 ("tee: Use iov_iter to better support shared buffer registration") we checked with is_vmalloc_addr() || is_kmap_addr(). I like Matthew's suggestion, it's nice to fix problems by deleting code. :-)
Sumit, you know the callers better. What do you think?
If we don't have a sane way to refcont registered kernel pages in TEE subsystem then yeah we have to solely rely on the client drivers to behave properly. Nevertheless, it's still within the kernel boundaries which we can rely upon.
Yes.
Cheers, Jens
Hi all,
is this issue fixed with 6.17? I ran:
git log v6.14...v6.17-rc1 drivers/tee/tee_shm.c
and saw no changes.
Regards, Marco
On 25-04-28, Jens Wiklander wrote:
On Thu, Mar 27, 2025 at 5:42 AM Sumit Garg sumit.garg@kernel.org wrote:
On Wed, Mar 26, 2025 at 02:47:46PM +0100, Jens Wiklander wrote:
On Wed, Mar 26, 2025 at 12:07 PM Marco Felsch m.felsch@pengutronix.de wrote:
On 25-03-26, Matthew Wilcox wrote:
On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote:
Skip manipulating the refcount in case of slab pages according commit b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
This almost certainly isn't right. I know nothing about TEE, but that you are doing this indicates a problem. The hack that we put into networking should not be blindly replicated.
Why are you taking a reference on the pages to begin with? Is it copy and pasted from somewhere else, or was there actual thought put into it?
Not sure, this belongs to the TEE maintainers.
I don't know. We were getting the user pages first, so I assume we just did the same thing when we added support for kernel pages.
If it's "prevent the caller from freeing the allocation", well, it never accomplished that with slab allocations. So for callers that do kmalloc (eg setup_mm_hdr() in drivers/firmware/efi/stmm/tee_stmm_efi.c), you have to rely on them not freeing the allocation while the TEE driver has it.
It's not just about the TEE driver but rather if the TEE implementation (a trusted OS) to whom the page is registered with. We don't want the trusted OS to work on registered kernel pages if they gets free somehow in the TEE client driver. Having a reference in the TEE subsystem assured us that won't happen. But if you say slab allocations are still prone the kernel pages getting freed even after refcount then can you suggest how should we handle this better?
As otherwise it can cause very hard to debug problems if trusted OS can manipulate kernel pages that are no longer available.
We must be able to rely on the kernel callers to have the needed references before calling tee_shm_register_kernel_buf() and to keep those until after calling tee_shm_free().
And if that's your API contract, then there's no point in taking refcounts on other kinds of pages either; it's just unnecessary atomic instructions. So the right patch might be something like this:
+++ b/drivers/tee/tee_shm.c @@ -15,29 +15,11 @@ #include <linux/highmem.h> #include "tee_private.h"
I had the same diff but didn't went this way since we can't be sure that iov's are always slab backed. As far as I understood IOVs. In 'worst-case' scenario an iov can be backed by different page types too.
We're only using kvec's. Briefly, before commit 7bdee4157591 ("tee: Use iov_iter to better support shared buffer registration") we checked with is_vmalloc_addr() || is_kmap_addr(). I like Matthew's suggestion, it's nice to fix problems by deleting code. :-)
Sumit, you know the callers better. What do you think?
If we don't have a sane way to refcont registered kernel pages in TEE subsystem then yeah we have to solely rely on the client drivers to behave properly. Nevertheless, it's still within the kernel boundaries which we can rely upon.
Yes.
Cheers, Jens
On Thu, Aug 21, 2025 at 07:41:24PM +0200, Marco Felsch wrote:
Hi all,
is this issue fixed with 6.17? I ran:
git log v6.14...v6.17-rc1 drivers/tee/tee_shm.c
and saw no changes.
Care to send a proper patch regarding what Matthew proposed in this thread?
-Sumit
Regards, Marco
On 25-04-28, Jens Wiklander wrote:
On Thu, Mar 27, 2025 at 5:42 AM Sumit Garg sumit.garg@kernel.org wrote:
On Wed, Mar 26, 2025 at 02:47:46PM +0100, Jens Wiklander wrote:
On Wed, Mar 26, 2025 at 12:07 PM Marco Felsch m.felsch@pengutronix.de wrote:
On 25-03-26, Matthew Wilcox wrote:
On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote: > Skip manipulating the refcount in case of slab pages according commit > b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
This almost certainly isn't right. I know nothing about TEE, but that you are doing this indicates a problem. The hack that we put into networking should not be blindly replicated.
Why are you taking a reference on the pages to begin with? Is it copy and pasted from somewhere else, or was there actual thought put into it?
Not sure, this belongs to the TEE maintainers.
I don't know. We were getting the user pages first, so I assume we just did the same thing when we added support for kernel pages.
If it's "prevent the caller from freeing the allocation", well, it never accomplished that with slab allocations. So for callers that do kmalloc (eg setup_mm_hdr() in drivers/firmware/efi/stmm/tee_stmm_efi.c), you have to rely on them not freeing the allocation while the TEE driver has it.
It's not just about the TEE driver but rather if the TEE implementation (a trusted OS) to whom the page is registered with. We don't want the trusted OS to work on registered kernel pages if they gets free somehow in the TEE client driver. Having a reference in the TEE subsystem assured us that won't happen. But if you say slab allocations are still prone the kernel pages getting freed even after refcount then can you suggest how should we handle this better?
As otherwise it can cause very hard to debug problems if trusted OS can manipulate kernel pages that are no longer available.
We must be able to rely on the kernel callers to have the needed references before calling tee_shm_register_kernel_buf() and to keep those until after calling tee_shm_free().
And if that's your API contract, then there's no point in taking refcounts on other kinds of pages either; it's just unnecessary atomic instructions. So the right patch might be something like this:
+++ b/drivers/tee/tee_shm.c @@ -15,29 +15,11 @@ #include <linux/highmem.h> #include "tee_private.h"
I had the same diff but didn't went this way since we can't be sure that iov's are always slab backed. As far as I understood IOVs. In 'worst-case' scenario an iov can be backed by different page types too.
We're only using kvec's. Briefly, before commit 7bdee4157591 ("tee: Use iov_iter to better support shared buffer registration") we checked with is_vmalloc_addr() || is_kmap_addr(). I like Matthew's suggestion, it's nice to fix problems by deleting code. :-)
Sumit, you know the callers better. What do you think?
If we don't have a sane way to refcont registered kernel pages in TEE subsystem then yeah we have to solely rely on the client drivers to behave properly. Nevertheless, it's still within the kernel boundaries which we can rely upon.
Yes.
Cheers, Jens
On 25-08-22, Sumit Garg wrote:
On Thu, Aug 21, 2025 at 07:41:24PM +0200, Marco Felsch wrote:
Hi all,
is this issue fixed with 6.17? I ran:
git log v6.14...v6.17-rc1 drivers/tee/tee_shm.c
and saw no changes.
Care to send a proper patch regarding what Matthew proposed in this thread?
I'm still not sure if the IOVs can be backed by other allocators too because the OP-TEE API allows arbitrary sizes. Therefore my hope was that one of the OP-TEE maintainers is taking care of this problem.
I also wonder why no one spotted/reported this issue too.
Regards, Marco
-Sumit
Regards, Marco
On 25-04-28, Jens Wiklander wrote:
On Thu, Mar 27, 2025 at 5:42 AM Sumit Garg sumit.garg@kernel.org wrote:
On Wed, Mar 26, 2025 at 02:47:46PM +0100, Jens Wiklander wrote:
On Wed, Mar 26, 2025 at 12:07 PM Marco Felsch m.felsch@pengutronix.de wrote:
On 25-03-26, Matthew Wilcox wrote: > On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote: > > Skip manipulating the refcount in case of slab pages according commit > > b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page"). > > This almost certainly isn't right. I know nothing about TEE, but that > you are doing this indicates a problem. The hack that we put into > networking should not be blindly replicated. > > Why are you taking a reference on the pages to begin with? Is it copy > and pasted from somewhere else, or was there actual thought put into it?
Not sure, this belongs to the TEE maintainers.
I don't know. We were getting the user pages first, so I assume we just did the same thing when we added support for kernel pages.
> If it's "prevent the caller from freeing the allocation", well, it never > accomplished that with slab allocations. So for callers that do kmalloc > (eg setup_mm_hdr() in drivers/firmware/efi/stmm/tee_stmm_efi.c), you > have to rely on them not freeing the allocation while the TEE driver > has it.
It's not just about the TEE driver but rather if the TEE implementation (a trusted OS) to whom the page is registered with. We don't want the trusted OS to work on registered kernel pages if they gets free somehow in the TEE client driver. Having a reference in the TEE subsystem assured us that won't happen. But if you say slab allocations are still prone the kernel pages getting freed even after refcount then can you suggest how should we handle this better?
As otherwise it can cause very hard to debug problems if trusted OS can manipulate kernel pages that are no longer available.
We must be able to rely on the kernel callers to have the needed references before calling tee_shm_register_kernel_buf() and to keep those until after calling tee_shm_free().
> > And if that's your API contract, then there's no point in taking > refcounts on other kinds of pages either; it's just unnecessary atomic > instructions. So the right patch might be something like this: > > +++ b/drivers/tee/tee_shm.c > @@ -15,29 +15,11 @@ > #include <linux/highmem.h> > #include "tee_private.h"
I had the same diff but didn't went this way since we can't be sure that iov's are always slab backed. As far as I understood IOVs. In 'worst-case' scenario an iov can be backed by different page types too.
We're only using kvec's. Briefly, before commit 7bdee4157591 ("tee: Use iov_iter to better support shared buffer registration") we checked with is_vmalloc_addr() || is_kmap_addr(). I like Matthew's suggestion, it's nice to fix problems by deleting code. :-)
Sumit, you know the callers better. What do you think?
If we don't have a sane way to refcont registered kernel pages in TEE subsystem then yeah we have to solely rely on the client drivers to behave properly. Nevertheless, it's still within the kernel boundaries which we can rely upon.
Yes.
Cheers, Jens
Hi,
On 8/22/25 12:15 PM, Marco Felsch wrote:
On 25-08-22, Sumit Garg wrote:
On Thu, Aug 21, 2025 at 07:41:24PM +0200, Marco Felsch wrote:
Hi all,
is this issue fixed with 6.17? I ran:
git log v6.14...v6.17-rc1 drivers/tee/tee_shm.c
and saw no changes.
Care to send a proper patch regarding what Matthew proposed in this thread?
I'm still not sure if the IOVs can be backed by other allocators too because the OP-TEE API allows arbitrary sizes. Therefore my hope was that one of the OP-TEE maintainers is taking care of this problem.
I also wonder why no one spotted/reported this issue too.
Any updates on how to proceed on this? I've ran into the issue today with the latest kernel master when loading a sealed key blob using keyctl on a Radxa Rock5T (rk3588):
[ 29.222792] ------------[ cut here ]------------ [ 29.223213] WARNING: include/linux/mm.h:1584 at register_shm_helper+0x2b4/0x2d0, CPU#2: keyctl/262 [ 29.224005] Modules linked in: hantro_vpu v4l2_jpeg v4l2_vp9 synopsys_hdmirx panthor v4l2_h264 drm_gpuvm drm_exec fuse [ 29.224965] CPU: 2 UID: 0 PID: 262 Comm: keyctl Not tainted 6.19.0-rc8-00006-g6bd9ed02871f #2 PREEMPT [ 29.225777] Hardware name: Radxa ROCK 5T (DT) [ 29.226160] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 29.226769] pc : register_shm_helper+0x2b4/0x2d0 [ 29.227175] lr : register_shm_helper+0x178/0x2d0 [ 29.227581] sp : ffffffc0846aba70 [ 29.227872] x29: ffffffc0846aba90 x28: ffffff8107e7d600 x27: 0000000000000000 [ 29.228502] x26: 0000000000000000 x25: ffffff810449e41a x24: 0000000000000001 [ 29.229130] x23: ffffffc0846abaf0 x22: ffffffffffffffea x21: ffffff8100cf2400 [ 29.229758] x20: ffffff81014ec960 x19: ffffff81023da100 x18: 0000000085a8c61a [ 29.230387] x17: 000000000836c99b x16: 0000000018aab74d x15: 6436303939393264 [ 29.231016] x14: 6631323431303432 x13: 6466313234313034 x12: 3262373862366634 [ 29.231643] x11: 3166653364353337 x10: ffffffc086adc2f8 x9 : ffffffc0805dde38 [ 29.232272] x8 : ffffff8100c10018 x7 : 0000000000000001 x6 : 0000000000000000 [ 29.232900] x5 : ffffff8100c10010 x4 : ffffff810449e000 x3 : fffffffec4112600 [ 29.233528] x2 : 00000000000000f5 x1 : fffffffec4112600 x0 : 0000000000000281 [ 29.234157] Call trace: [ 29.234374] register_shm_helper+0x2b4/0x2d0 (P) [ 29.234782] tee_shm_register_kernel_buf+0x68/0xa0 [ 29.235205] trusted_tee_unseal+0x5c/0x150 [ 29.235570] trusted_instantiate+0x114/0x1f0 [ 29.235948] __key_instantiate_and_link+0x60/0x1c0 [ 29.236369] __key_create_or_update+0x2b8/0x458 [ 29.236769] key_create_or_update+0x18/0x28 [ 29.237138] __arm64_sys_add_key+0x138/0x230 [ 29.237515] do_el0_svc+0x70/0x100 [ 29.237819] el0_svc+0x30/0xf8 [ 29.238092] el0t_64_sync_handler+0x98/0xe0 [ 29.238462] el0t_64_sync+0x154/0x158 [ 29.238787] ---[ end trace 0000000000000000 ]---
Sincerely Sven
Regards, Marco
-Sumit
Regards, Marco
On 25-04-28, Jens Wiklander wrote:
On Thu, Mar 27, 2025 at 5:42 AM Sumit Garg sumit.garg@kernel.org wrote:
On Wed, Mar 26, 2025 at 02:47:46PM +0100, Jens Wiklander wrote:
On Wed, Mar 26, 2025 at 12:07 PM Marco Felsch m.felsch@pengutronix.de wrote: > On 25-03-26, Matthew Wilcox wrote: >> On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote: >>> Skip manipulating the refcount in case of slab pages according commit >>> b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page"). >> This almost certainly isn't right. I know nothing about TEE, but that >> you are doing this indicates a problem. The hack that we put into >> networking should not be blindly replicated. >> >> Why are you taking a reference on the pages to begin with? Is it copy >> and pasted from somewhere else, or was there actual thought put into it? > Not sure, this belongs to the TEE maintainers. I don't know. We were getting the user pages first, so I assume we just did the same thing when we added support for kernel pages.
>> If it's "prevent the caller from freeing the allocation", well, it never >> accomplished that with slab allocations. So for callers that do kmalloc >> (eg setup_mm_hdr() in drivers/firmware/efi/stmm/tee_stmm_efi.c), you >> have to rely on them not freeing the allocation while the TEE driver >> has it.
It's not just about the TEE driver but rather if the TEE implementation (a trusted OS) to whom the page is registered with. We don't want the trusted OS to work on registered kernel pages if they gets free somehow in the TEE client driver. Having a reference in the TEE subsystem assured us that won't happen. But if you say slab allocations are still prone the kernel pages getting freed even after refcount then can you suggest how should we handle this better?
As otherwise it can cause very hard to debug problems if trusted OS can manipulate kernel pages that are no longer available.
We must be able to rely on the kernel callers to have the needed references before calling tee_shm_register_kernel_buf() and to keep those until after calling tee_shm_free().
>> And if that's your API contract, then there's no point in taking >> refcounts on other kinds of pages either; it's just unnecessary atomic >> instructions. So the right patch might be something like this: >> >> +++ b/drivers/tee/tee_shm.c >> @@ -15,29 +15,11 @@ >> #include <linux/highmem.h> >> #include "tee_private.h" > I had the same diff but didn't went this way since we can't be sure that > iov's are always slab backed. As far as I understood IOVs. In > 'worst-case' scenario an iov can be backed by different page types too. We're only using kvec's. Briefly, before commit 7bdee4157591 ("tee: Use iov_iter to better support shared buffer registration") we checked with is_vmalloc_addr() || is_kmap_addr(). I like Matthew's suggestion, it's nice to fix problems by deleting code. :-)
Sumit, you know the callers better. What do you think?
If we don't have a sane way to refcont registered kernel pages in TEE subsystem then yeah we have to solely rely on the client drivers to behave properly. Nevertheless, it's still within the kernel boundaries which we can rely upon.
Yes.
Cheers, Jens
Hi Sven,
On Tue, Feb 03, 2026 at 12:55:08PM +0100, Sven Püschel wrote:
Hi,
On 8/22/25 12:15 PM, Marco Felsch wrote:
On 25-08-22, Sumit Garg wrote:
On Thu, Aug 21, 2025 at 07:41:24PM +0200, Marco Felsch wrote:
Hi all,
is this issue fixed with 6.17? I ran:
git log v6.14...v6.17-rc1 drivers/tee/tee_shm.c
and saw no changes.
Care to send a proper patch regarding what Matthew proposed in this thread?
I'm still not sure if the IOVs can be backed by other allocators too because the OP-TEE API allows arbitrary sizes. Therefore my hope was that one of the OP-TEE maintainers is taking care of this problem.
I also wonder why no one spotted/reported this issue too.
Any updates on how to proceed on this? I've ran into the issue today with the latest kernel master when loading a sealed key blob using keyctl on a Radxa Rock5T (rk3588):
Can you check if fix suggested by Matthew here [1] fixes problem for you? If it does then can you create a proper fix patch for upstream around that?
[1] https://lore.kernel.org/all/Z-Pc6C1YUqLyej3Z@casper.infradead.org/
-Sumit
[ 29.222792] ------------[ cut here ]------------ [ 29.223213] WARNING: include/linux/mm.h:1584 at register_shm_helper+0x2b4/0x2d0, CPU#2: keyctl/262 [ 29.224005] Modules linked in: hantro_vpu v4l2_jpeg v4l2_vp9 synopsys_hdmirx panthor v4l2_h264 drm_gpuvm drm_exec fuse [ 29.224965] CPU: 2 UID: 0 PID: 262 Comm: keyctl Not tainted 6.19.0-rc8-00006-g6bd9ed02871f #2 PREEMPT [ 29.225777] Hardware name: Radxa ROCK 5T (DT) [ 29.226160] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 29.226769] pc : register_shm_helper+0x2b4/0x2d0 [ 29.227175] lr : register_shm_helper+0x178/0x2d0 [ 29.227581] sp : ffffffc0846aba70 [ 29.227872] x29: ffffffc0846aba90 x28: ffffff8107e7d600 x27: 0000000000000000 [ 29.228502] x26: 0000000000000000 x25: ffffff810449e41a x24: 0000000000000001 [ 29.229130] x23: ffffffc0846abaf0 x22: ffffffffffffffea x21: ffffff8100cf2400 [ 29.229758] x20: ffffff81014ec960 x19: ffffff81023da100 x18: 0000000085a8c61a [ 29.230387] x17: 000000000836c99b x16: 0000000018aab74d x15: 6436303939393264 [ 29.231016] x14: 6631323431303432 x13: 6466313234313034 x12: 3262373862366634 [ 29.231643] x11: 3166653364353337 x10: ffffffc086adc2f8 x9 : ffffffc0805dde38 [ 29.232272] x8 : ffffff8100c10018 x7 : 0000000000000001 x6 : 0000000000000000 [ 29.232900] x5 : ffffff8100c10010 x4 : ffffff810449e000 x3 : fffffffec4112600 [ 29.233528] x2 : 00000000000000f5 x1 : fffffffec4112600 x0 : 0000000000000281 [ 29.234157] Call trace: [ 29.234374] register_shm_helper+0x2b4/0x2d0 (P) [ 29.234782] tee_shm_register_kernel_buf+0x68/0xa0 [ 29.235205] trusted_tee_unseal+0x5c/0x150 [ 29.235570] trusted_instantiate+0x114/0x1f0 [ 29.235948] __key_instantiate_and_link+0x60/0x1c0 [ 29.236369] __key_create_or_update+0x2b8/0x458 [ 29.236769] key_create_or_update+0x18/0x28 [ 29.237138] __arm64_sys_add_key+0x138/0x230 [ 29.237515] do_el0_svc+0x70/0x100 [ 29.237819] el0_svc+0x30/0xf8 [ 29.238092] el0t_64_sync_handler+0x98/0xe0 [ 29.238462] el0t_64_sync+0x154/0x158 [ 29.238787] ---[ end trace 0000000000000000 ]---
Sincerely Sven
Regards, Marco
-Sumit
Regards, Marco
On 25-04-28, Jens Wiklander wrote:
On Thu, Mar 27, 2025 at 5:42 AM Sumit Garg sumit.garg@kernel.org wrote:
On Wed, Mar 26, 2025 at 02:47:46PM +0100, Jens Wiklander wrote: > On Wed, Mar 26, 2025 at 12:07 PM Marco Felsch m.felsch@pengutronix.de wrote: > > On 25-03-26, Matthew Wilcox wrote: > > > On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote: > > > > Skip manipulating the refcount in case of slab pages according commit > > > > b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page"). > > > This almost certainly isn't right. I know nothing about TEE, but that > > > you are doing this indicates a problem. The hack that we put into > > > networking should not be blindly replicated. > > > > > > Why are you taking a reference on the pages to begin with? Is it copy > > > and pasted from somewhere else, or was there actual thought put into it? > > Not sure, this belongs to the TEE maintainers. > I don't know. We were getting the user pages first, so I assume we > just did the same thing when we added support for kernel pages. > > > > If it's "prevent the caller from freeing the allocation", well, it never > > > accomplished that with slab allocations. So for callers that do kmalloc > > > (eg setup_mm_hdr() in drivers/firmware/efi/stmm/tee_stmm_efi.c), you > > > have to rely on them not freeing the allocation while the TEE driver > > > has it. It's not just about the TEE driver but rather if the TEE implementation (a trusted OS) to whom the page is registered with. We don't want the trusted OS to work on registered kernel pages if they gets free somehow in the TEE client driver. Having a reference in the TEE subsystem assured us that won't happen. But if you say slab allocations are still prone the kernel pages getting freed even after refcount then can you suggest how should we handle this better?
As otherwise it can cause very hard to debug problems if trusted OS can manipulate kernel pages that are no longer available.
We must be able to rely on the kernel callers to have the needed references before calling tee_shm_register_kernel_buf() and to keep those until after calling tee_shm_free().
> > > And if that's your API contract, then there's no point in taking > > > refcounts on other kinds of pages either; it's just unnecessary atomic > > > instructions. So the right patch might be something like this: > > > > > > +++ b/drivers/tee/tee_shm.c > > > @@ -15,29 +15,11 @@ > > > #include <linux/highmem.h> > > > #include "tee_private.h" > > I had the same diff but didn't went this way since we can't be sure that > > iov's are always slab backed. As far as I understood IOVs. In > > 'worst-case' scenario an iov can be backed by different page types too. > We're only using kvec's. Briefly, before commit 7bdee4157591 ("tee: > Use iov_iter to better support shared buffer registration") we checked > with is_vmalloc_addr() || is_kmap_addr(). I like Matthew's suggestion, > it's nice to fix problems by deleting code. :-) > > Sumit, you know the callers better. What do you think? If we don't have a sane way to refcont registered kernel pages in TEE subsystem then yeah we have to solely rely on the client drivers to behave properly. Nevertheless, it's still within the kernel boundaries which we can rely upon.
Yes.
Cheers, Jens
Hi Sumit,
TBH: I was hoping that you will take care of this since you're marked as maintainer for the tee-trusted-key and we noticed the warning with 6.14 and still no fix available :/
However please see below for further discussion.
On 25-04-28, Jens Wiklander wrote:
On Thu, Mar 27, 2025 at 5:42 AM Sumit Garg sumit.garg@kernel.org wrote:
On Wed, Mar 26, 2025 at 02:47:46PM +0100, Jens Wiklander wrote:
On Wed, Mar 26, 2025 at 12:07 PM Marco Felsch m.felsch@pengutronix.de wrote:
On 25-03-26, Matthew Wilcox wrote:
On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote:
Skip manipulating the refcount in case of slab pages according commit b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
This almost certainly isn't right. I know nothing about TEE, but that you are doing this indicates a problem. The hack that we put into networking should not be blindly replicated.
Why are you taking a reference on the pages to begin with? Is it copy and pasted from somewhere else, or was there actual thought put into it?
Not sure, this belongs to the TEE maintainers.
I don't know. We were getting the user pages first, so I assume we just did the same thing when we added support for kernel pages.
If it's "prevent the caller from freeing the allocation", well, it never accomplished that with slab allocations. So for callers that do kmalloc (eg setup_mm_hdr() in drivers/firmware/efi/stmm/tee_stmm_efi.c), you have to rely on them not freeing the allocation while the TEE driver has it.
It's not just about the TEE driver but rather if the TEE implementation (a trusted OS) to whom the page is registered with. We don't want the trusted OS to work on registered kernel pages if they gets free somehow in the TEE client driver. Having a reference in the TEE subsystem assured us that won't happen. But if you say slab allocations are still prone the kernel pages getting freed even after refcount then can you suggest how should we handle this better?
As otherwise it can cause very hard to debug problems if trusted OS can manipulate kernel pages that are no longer available.
We must be able to rely on the kernel callers to have the needed references before calling tee_shm_register_kernel_buf() and to keep those until after calling tee_shm_free().
I checked the code once again and figured that we could drop/replace tee_shm_register_kernel_buf() with tee_shm_alloc_kernel_buf(). I don't see why a kernel driver needs to tee_shm_register_kernel_buf() in the first place, maybe this is legacy. The only users of tee_shm_register_kernel_buf() are trusted_tee.c and tee_stmm_efi.c.
+Cc the efi-stmm folks since they will be affected by this change as well.
Regards, Marco
And if that's your API contract, then there's no point in taking refcounts on other kinds of pages either; it's just unnecessary atomic instructions. So the right patch might be something like this:
+++ b/drivers/tee/tee_shm.c @@ -15,29 +15,11 @@ #include <linux/highmem.h> #include "tee_private.h"
I had the same diff but didn't went this way since we can't be sure that iov's are always slab backed. As far as I understood IOVs. In 'worst-case' scenario an iov can be backed by different page types too.
We're only using kvec's. Briefly, before commit 7bdee4157591 ("tee: Use iov_iter to better support shared buffer registration") we checked with is_vmalloc_addr() || is_kmap_addr(). I like Matthew's suggestion, it's nice to fix problems by deleting code. :-)
Sumit, you know the callers better. What do you think?
If we don't have a sane way to refcont registered kernel pages in TEE subsystem then yeah we have to solely rely on the client drivers to behave properly. Nevertheless, it's still within the kernel boundaries which we can rely upon.
Yes.
Cheers, Jens
Hi Marco,
On Thu, Feb 12, 2026 at 01:58:30PM +0100, Marco Felsch wrote:
Hi Sumit,
TBH: I was hoping that you will take care of this since you're marked as maintainer for the tee-trusted-key and we noticed the warning with 6.14 and still no fix available :/
Mathew did suggested a fix long back on which everybody agreed but didn't got enough attention from you to test and report if that fixed your issue. Since you insisted further, I have created a formal fix patch based on that here [1]. Care to test that?
[1] https://lore.kernel.org/all/20260213113317.1728769-1-sumit.garg@kernel.org/
However please see below for further discussion.
On 25-04-28, Jens Wiklander wrote:
On Thu, Mar 27, 2025 at 5:42 AM Sumit Garg sumit.garg@kernel.org wrote:
On Wed, Mar 26, 2025 at 02:47:46PM +0100, Jens Wiklander wrote:
On Wed, Mar 26, 2025 at 12:07 PM Marco Felsch m.felsch@pengutronix.de wrote:
On 25-03-26, Matthew Wilcox wrote:
On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote: > Skip manipulating the refcount in case of slab pages according commit > b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
This almost certainly isn't right. I know nothing about TEE, but that you are doing this indicates a problem. The hack that we put into networking should not be blindly replicated.
Why are you taking a reference on the pages to begin with? Is it copy and pasted from somewhere else, or was there actual thought put into it?
Not sure, this belongs to the TEE maintainers.
I don't know. We were getting the user pages first, so I assume we just did the same thing when we added support for kernel pages.
If it's "prevent the caller from freeing the allocation", well, it never accomplished that with slab allocations. So for callers that do kmalloc (eg setup_mm_hdr() in drivers/firmware/efi/stmm/tee_stmm_efi.c), you have to rely on them not freeing the allocation while the TEE driver has it.
It's not just about the TEE driver but rather if the TEE implementation (a trusted OS) to whom the page is registered with. We don't want the trusted OS to work on registered kernel pages if they gets free somehow in the TEE client driver. Having a reference in the TEE subsystem assured us that won't happen. But if you say slab allocations are still prone the kernel pages getting freed even after refcount then can you suggest how should we handle this better?
As otherwise it can cause very hard to debug problems if trusted OS can manipulate kernel pages that are no longer available.
We must be able to rely on the kernel callers to have the needed references before calling tee_shm_register_kernel_buf() and to keep those until after calling tee_shm_free().
I checked the code once again and figured that we could drop/replace tee_shm_register_kernel_buf() with tee_shm_alloc_kernel_buf(). I don't see why a kernel driver needs to tee_shm_register_kernel_buf() in the first place, maybe this is legacy. The only users of tee_shm_register_kernel_buf() are trusted_tee.c and tee_stmm_efi.c.
No it's not legacy but allows for efficient memory reuse within the kernel as to not create bounce buffers to share data with TEE.
-Sumit
+Cc the efi-stmm folks since they will be affected by this change as well.
Regards, Marco
And if that's your API contract, then there's no point in taking refcounts on other kinds of pages either; it's just unnecessary atomic instructions. So the right patch might be something like this:
+++ b/drivers/tee/tee_shm.c @@ -15,29 +15,11 @@ #include <linux/highmem.h> #include "tee_private.h"
I had the same diff but didn't went this way since we can't be sure that iov's are always slab backed. As far as I understood IOVs. In 'worst-case' scenario an iov can be backed by different page types too.
We're only using kvec's. Briefly, before commit 7bdee4157591 ("tee: Use iov_iter to better support shared buffer registration") we checked with is_vmalloc_addr() || is_kmap_addr(). I like Matthew's suggestion, it's nice to fix problems by deleting code. :-)
Sumit, you know the callers better. What do you think?
If we don't have a sane way to refcont registered kernel pages in TEE subsystem then yeah we have to solely rely on the client drivers to behave properly. Nevertheless, it's still within the kernel boundaries which we can rely upon.
Yes.
Cheers, Jens
-- #gernperDu #CallMeByMyFirstName
Pengutronix e.K. | | Steuerwalder Str. 21 | https://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Hi Sumit,
On 26-02-13, Sumit Garg wrote:
Hi Marco,
On Thu, Feb 12, 2026 at 01:58:30PM +0100, Marco Felsch wrote:
Hi Sumit,
TBH: I was hoping that you will take care of this since you're marked as maintainer for the tee-trusted-key and we noticed the warning with 6.14 and still no fix available :/
Mathew did suggested a fix long back on which everybody agreed but
You agreed. I said that the current TEE API also allows non-slabed based backed memory and therefore I don't wanted to send this patch approach and instead asked you to do so since you're the maintainer and fine with the change.
didn't got enough attention from you to test and report if that fixed
Why should it get attention from us? Maybe we do have different views of being a maintainer.
your issue. Since you insisted further, I have created a formal fix
Why is it our issue? It's everyones issue which uses the tee trusted-key driver.
patch based on that here [1]. Care to test that?
A colleague of mine is going to test it and will reply on the patch.
[1] https://lore.kernel.org/all/20260213113317.1728769-1-sumit.garg@kernel.org/
...
I checked the code once again and figured that we could drop/replace tee_shm_register_kernel_buf() with tee_shm_alloc_kernel_buf(). I don't see why a kernel driver needs to tee_shm_register_kernel_buf() in the first place, maybe this is legacy. The only users of tee_shm_register_kernel_buf() are trusted_tee.c and tee_stmm_efi.c.
No it's not legacy but allows for efficient memory reuse within the kernel as to not create bounce buffers to share data with TEE.
To be hones, there are only two driver using the API. The tee_stmm_efi driver can do the alloc during the probe(). The trusted_tee has to use a bounce buffer, yes but how often do you assume that (un)sealing and rng ops have to be done during runtime? This shouldn't be a overhead at all.
Therefore my suggestion would be still to drop the internal kernel API and only use it for userspace pages, where it could really matter.
Regards, Marco
On Fri, Feb 13, 2026 at 11:04:48PM +0100, Marco Felsch wrote:
Hi Sumit,
On 26-02-13, Sumit Garg wrote:
Hi Marco,
On Thu, Feb 12, 2026 at 01:58:30PM +0100, Marco Felsch wrote:
Hi Sumit,
TBH: I was hoping that you will take care of this since you're marked as maintainer for the tee-trusted-key and we noticed the warning with 6.14 and still no fix available :/
Mathew did suggested a fix long back on which everybody agreed but
You agreed. I said that the current TEE API also allows non-slabed based backed memory and therefore I don't wanted to send this patch approach and instead asked you to do so since you're the maintainer and fine with the change.
didn't got enough attention from you to test and report if that fixed
Why should it get attention from us? Maybe we do have different views of being a maintainer.
It's really the basic expectation I have put here which every reporter of a bug needs to say if a suggested fix works for them or not.
your issue. Since you insisted further, I have created a formal fix
Why is it our issue? It's everyones issue which uses the tee trusted-key driver.
patch based on that here [1]. Care to test that?
A colleague of mine is going to test it and will reply on the patch.
[1] https://lore.kernel.org/all/20260213113317.1728769-1-sumit.garg@kernel.org/
...
I checked the code once again and figured that we could drop/replace tee_shm_register_kernel_buf() with tee_shm_alloc_kernel_buf(). I don't see why a kernel driver needs to tee_shm_register_kernel_buf() in the first place, maybe this is legacy. The only users of tee_shm_register_kernel_buf() are trusted_tee.c and tee_stmm_efi.c.
No it's not legacy but allows for efficient memory reuse within the kernel as to not create bounce buffers to share data with TEE.
To be hones, there are only two driver using the API. The tee_stmm_efi driver can do the alloc during the probe(). The trusted_tee has to use a bounce buffer, yes but how often do you assume that (un)sealing and rng ops have to be done during runtime? This shouldn't be a overhead at all.
Therefore my suggestion would be still to drop the internal kernel API and only use it for userspace pages, where it could really matter.
I don't disagree with what you are saying here but we really need to promote efficient memory reuse for TEE clients. There will surely be more use-cases coming in future which can benefit from the flexibility to register buffer. One another kernel client being remoteproc subsystem which is already under review for this API.
-Sumit
Regards, Marco -- #gernperDu #CallMeByMyFirstName
Pengutronix e.K. | | Steuerwalder Str. 21 | https://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On 26-02-16, Sumit Garg wrote:
On Fri, Feb 13, 2026 at 11:04:48PM +0100, Marco Felsch wrote:
Hi Sumit,
On 26-02-13, Sumit Garg wrote:
Hi Marco,
On Thu, Feb 12, 2026 at 01:58:30PM +0100, Marco Felsch wrote:
Hi Sumit,
TBH: I was hoping that you will take care of this since you're marked as maintainer for the tee-trusted-key and we noticed the warning with 6.14 and still no fix available :/
Mathew did suggested a fix long back on which everybody agreed but
You agreed. I said that the current TEE API also allows non-slabed based backed memory and therefore I don't wanted to send this patch approach and instead asked you to do so since you're the maintainer and fine with the change.
didn't got enough attention from you to test and report if that fixed
Why should it get attention from us? Maybe we do have different views of being a maintainer.
It's really the basic expectation I have put here which every reporter of a bug needs to say if a suggested fix works for them or not.
your issue. Since you insisted further, I have created a formal fix
Why is it our issue? It's everyones issue which uses the tee trusted-key driver.
patch based on that here [1]. Care to test that?
A colleague of mine is going to test it and will reply on the patch.
[1] https://lore.kernel.org/all/20260213113317.1728769-1-sumit.garg@kernel.org/
...
I checked the code once again and figured that we could drop/replace tee_shm_register_kernel_buf() with tee_shm_alloc_kernel_buf(). I don't see why a kernel driver needs to tee_shm_register_kernel_buf() in the first place, maybe this is legacy. The only users of tee_shm_register_kernel_buf() are trusted_tee.c and tee_stmm_efi.c.
No it's not legacy but allows for efficient memory reuse within the kernel as to not create bounce buffers to share data with TEE.
To be hones, there are only two driver using the API. The tee_stmm_efi driver can do the alloc during the probe(). The trusted_tee has to use a bounce buffer, yes but how often do you assume that (un)sealing and rng ops have to be done during runtime? This shouldn't be a overhead at all.
Therefore my suggestion would be still to drop the internal kernel API and only use it for userspace pages, where it could really matter.
I don't disagree with what you are saying here but we really need to promote efficient memory reuse for TEE clients. There will surely be more use-cases coming in future which can benefit from the flexibility to register buffer. One another kernel client being remoteproc subsystem which is already under review for this API.
I see, thanks for the pointer. I'm really curious why STM didn't reported the warning stacktrace, since they should trigger it too.
Regards, Marco
-Sumit
Regards, Marco -- #gernperDu #CallMeByMyFirstName
Pengutronix e.K. | | Steuerwalder Str. 21 | https://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
op-tee@lists.trustedfirmware.org
-
Jens Wiklander -
Marco Felsch -
Matthew Wilcox -
Sumit Garg -
Sven Püschel