* Re: [PATCH v2] tee: shm: fix slab page refcounting
@ 2025-04-28 12:48 ` Jens Wiklander
0 siblings, 0 replies; 34+ messages in thread
From: Jens Wiklander @ 2025-04-28 12:48 UTC (permalink / raw)
To: Sumit Garg
Cc: Matthew Wilcox, Marco Felsch, vbabka, akpm, kernel, op-tee,
linux-kernel
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2] tee: shm: fix slab page refcounting
2025-04-28 12:48 ` Jens Wiklander
(?)
@ 2025-08-21 17:41 ` Marco Felsch
2025-08-22 8:52 ` Sumit Garg
-1 siblings, 1 reply; 34+ messages in thread
From: Marco Felsch @ 2025-08-21 17:41 UTC (permalink / raw)
To: Jens Wiklander
Cc: Sumit Garg, Matthew Wilcox, vbabka, akpm, kernel, op-tee,
linux-kernel
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2] tee: shm: fix slab page refcounting
2025-08-21 17:41 ` Marco Felsch
@ 2025-08-22 8:52 ` Sumit Garg
0 siblings, 0 replies; 34+ messages in thread
From: Sumit Garg via OP-TEE @ 2025-08-22 8:52 UTC (permalink / raw)
To: Marco Felsch; +Cc: Matthew Wilcox, vbabka, akpm, kernel, op-tee, linux-kernel
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2] tee: shm: fix slab page refcounting
@ 2025-08-22 8:52 ` Sumit Garg
0 siblings, 0 replies; 34+ messages in thread
From: Sumit Garg @ 2025-08-22 8:52 UTC (permalink / raw)
To: Marco Felsch
Cc: Jens Wiklander, Matthew Wilcox, vbabka, akpm, kernel, op-tee,
linux-kernel
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2] tee: shm: fix slab page refcounting
2025-08-22 8:52 ` Sumit Garg
@ 2025-08-22 10:15 ` Marco Felsch
-1 siblings, 0 replies; 34+ messages in thread
From: Marco Felsch @ 2025-08-22 10:15 UTC (permalink / raw)
To: Sumit Garg; +Cc: Matthew Wilcox, vbabka, akpm, kernel, op-tee, linux-kernel
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2] tee: shm: fix slab page refcounting
@ 2025-08-22 10:15 ` Marco Felsch
0 siblings, 0 replies; 34+ messages in thread
From: Marco Felsch @ 2025-08-22 10:15 UTC (permalink / raw)
To: Sumit Garg
Cc: Jens Wiklander, Matthew Wilcox, vbabka, akpm, kernel, op-tee,
linux-kernel
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2] tee: shm: fix slab page refcounting
2025-08-22 10:15 ` Marco Felsch
@ 2026-02-03 11:55 ` Sven Püschel
-1 siblings, 0 replies; 34+ messages in thread
From: Sven Püschel @ 2026-02-03 11:55 UTC (permalink / raw)
To: Marco Felsch, Sumit Garg
Cc: linux-kernel, Matthew Wilcox, op-tee, kernel, akpm, vbabka
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
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2] tee: shm: fix slab page refcounting
@ 2026-02-03 11:55 ` Sven Püschel
0 siblings, 0 replies; 34+ messages in thread
From: Sven Püschel @ 2026-02-03 11:55 UTC (permalink / raw)
To: Marco Felsch, Sumit Garg
Cc: linux-kernel, Matthew Wilcox, op-tee, kernel, akpm,
Jens Wiklander, vbabka
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
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2] tee: shm: fix slab page refcounting
2026-02-03 11:55 ` Sven Püschel
@ 2026-02-03 12:06 ` Sumit Garg
-1 siblings, 0 replies; 34+ messages in thread
From: Sumit Garg via OP-TEE @ 2026-02-03 12:06 UTC (permalink / raw)
To: Sven Püschel
Cc: Marco Felsch, linux-kernel, Matthew Wilcox, op-tee, kernel, akpm,
vbabka
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
> >
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2] tee: shm: fix slab page refcounting
@ 2026-02-03 12:06 ` Sumit Garg
0 siblings, 0 replies; 34+ messages in thread
From: Sumit Garg @ 2026-02-03 12:06 UTC (permalink / raw)
To: Sven Püschel
Cc: Marco Felsch, linux-kernel, Matthew Wilcox, op-tee, kernel, akpm,
Jens Wiklander, vbabka
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
> >
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2] tee: shm: fix slab page refcounting
2025-04-28 12:48 ` Jens Wiklander
(?)
(?)
@ 2026-02-12 12:58 ` Marco Felsch
2026-02-13 11:41 ` Sumit Garg via OP-TEE
-1 siblings, 1 reply; 34+ messages in thread
From: Marco Felsch @ 2026-02-12 12:58 UTC (permalink / raw)
To: Jens Wiklander
Cc: Sumit Garg, Matthew Wilcox, vbabka, akpm, kernel, op-tee,
linux-kernel, linux-efi, linux-stm32, linux-arm-kernel,
mcoquelin.stm32, alexandre.torgue, ilias.apalodimas, jan.kiszka,
masahisa.kojima
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
--
#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 |
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2] tee: shm: fix slab page refcounting
2026-02-12 12:58 ` Marco Felsch
@ 2026-02-13 11:41 ` Sumit Garg via OP-TEE
0 siblings, 0 replies; 34+ messages in thread
From: Sumit Garg @ 2026-02-13 11:41 UTC (permalink / raw)
To: Marco Felsch
Cc: Jens Wiklander, Matthew Wilcox, vbabka, akpm, kernel, op-tee,
linux-kernel, linux-efi, linux-stm32, linux-arm-kernel,
mcoquelin.stm32, alexandre.torgue, ilias.apalodimas, jan.kiszka,
masahisa.kojima
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 |
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2] tee: shm: fix slab page refcounting
@ 2026-02-13 11:41 ` Sumit Garg via OP-TEE
0 siblings, 0 replies; 34+ messages in thread
From: Sumit Garg via OP-TEE @ 2026-02-13 11:41 UTC (permalink / raw)
To: Marco Felsch
Cc: Matthew Wilcox, vbabka, akpm, kernel, op-tee, linux-kernel,
linux-efi, linux-stm32, linux-arm-kernel, mcoquelin.stm32,
alexandre.torgue, ilias.apalodimas, jan.kiszka, masahisa.kojima
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 |
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v2] tee: shm: fix slab page refcounting
2026-02-13 11:41 ` Sumit Garg via OP-TEE
@ 2026-02-13 22:04 ` Marco Felsch
-1 siblings, 0 replies; 34+ messages in thread
From: Marco Felsch @ 2026-02-13 22:04 UTC (permalink / raw)
To: Sumit Garg
Cc: Jens Wiklander, Matthew Wilcox, vbabka, akpm, kernel, op-tee,
linux-kernel, linux-efi, linux-stm32, linux-arm-kernel,
mcoquelin.stm32, alexandre.torgue, ilias.apalodimas, jan.kiszka,
masahisa.kojima, spu
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
--
#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 |
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2] tee: shm: fix slab page refcounting
@ 2026-02-13 22:04 ` Marco Felsch
0 siblings, 0 replies; 34+ messages in thread
From: Marco Felsch @ 2026-02-13 22:04 UTC (permalink / raw)
To: Sumit Garg
Cc: Matthew Wilcox, vbabka, akpm, kernel, op-tee, linux-kernel,
linux-efi, linux-stm32, linux-arm-kernel, mcoquelin.stm32,
alexandre.torgue, ilias.apalodimas, jan.kiszka, masahisa.kojima,
spu
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
--
#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 |
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2] tee: shm: fix slab page refcounting
2026-02-13 22:04 ` Marco Felsch
@ 2026-02-16 6:28 ` Sumit Garg via OP-TEE
-1 siblings, 0 replies; 34+ messages in thread
From: Sumit Garg @ 2026-02-16 6:28 UTC (permalink / raw)
To: Marco Felsch
Cc: Jens Wiklander, Matthew Wilcox, vbabka, akpm, kernel, op-tee,
linux-kernel, linux-efi, linux-stm32, linux-arm-kernel,
mcoquelin.stm32, alexandre.torgue, ilias.apalodimas, jan.kiszka,
masahisa.kojima, spu
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 |
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2] tee: shm: fix slab page refcounting
@ 2026-02-16 6:28 ` Sumit Garg via OP-TEE
0 siblings, 0 replies; 34+ messages in thread
From: Sumit Garg via OP-TEE @ 2026-02-16 6:28 UTC (permalink / raw)
To: Marco Felsch
Cc: Matthew Wilcox, vbabka, akpm, kernel, op-tee, linux-kernel,
linux-efi, linux-stm32, linux-arm-kernel, mcoquelin.stm32,
alexandre.torgue, ilias.apalodimas, jan.kiszka, masahisa.kojima,
spu
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 |
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2] tee: shm: fix slab page refcounting
2026-02-16 6:28 ` Sumit Garg via OP-TEE
@ 2026-02-16 8:28 ` Marco Felsch
-1 siblings, 0 replies; 34+ messages in thread
From: Marco Felsch @ 2026-02-16 8:28 UTC (permalink / raw)
To: Sumit Garg
Cc: Jens Wiklander, Matthew Wilcox, vbabka, akpm, kernel, op-tee,
linux-kernel, linux-efi, linux-stm32, linux-arm-kernel,
mcoquelin.stm32, alexandre.torgue, ilias.apalodimas, jan.kiszka,
masahisa.kojima, spu
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 |
>
--
#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 |
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2] tee: shm: fix slab page refcounting
@ 2026-02-16 8:28 ` Marco Felsch
0 siblings, 0 replies; 34+ messages in thread
From: Marco Felsch @ 2026-02-16 8:28 UTC (permalink / raw)
To: Sumit Garg
Cc: Matthew Wilcox, vbabka, akpm, kernel, op-tee, linux-kernel,
linux-efi, linux-stm32, linux-arm-kernel, mcoquelin.stm32,
alexandre.torgue, ilias.apalodimas, jan.kiszka, masahisa.kojima,
spu
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 |
>
--
#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 |
^ permalink raw reply [flat|nested] 34+ messages in thread