Re: [PATCH v2] tee: shm: fix slab page refcounting

Re: [PATCH v2] tee: shm: fix slab page refcounting

[Marco Felsch]

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    |

Re: [PATCH v2] tee: shm: fix slab page refcounting

[Sumit Garg]

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    |

Re: [PATCH v2] tee: shm: fix slab page refcounting

[Marco Felsch]

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    |

Re: [PATCH v2] tee: shm: fix slab page refcounting

[Sumit Garg]

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    |

Re: [PATCH v2] tee: shm: fix slab page refcounting

[Marco Felsch]

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    |