Re: [PATCH v2] tee: shm: fix slab page refcounting
From: Sven Püschel
Date: Tue Feb 03 2026 - 06:55:24 EST
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:I'm still not sure if the IOVs can be backed by other allocators too
Hi all,Care to send a proper patch regarding what Matthew proposed in this
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.
thread?
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@xxxxxxxxxx> wrote:
On Wed, Mar 26, 2025 at 02:47:46PM +0100, Jens Wiklander wrote:We must be able to rely on the kernel callers to have the needed
On Wed, Mar 26, 2025 at 12:07 PM Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote:It's not just about the TEE driver but rather if the TEE implementation
On 25-03-26, Matthew Wilcox wrote:I don't know. We were getting the user pages first, so I assume we
On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote:Not sure, this belongs to the TEE maintainers.
Skip manipulating the refcount in case of slab pages according commitThis almost certainly isn't right. I know nothing about TEE, but that
b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
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?
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.
(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.
references before calling tee_shm_register_kernel_buf() and to keep
those until after calling tee_shm_free().
Yes.If we don't have a sane way to refcont registered kernel pages in TEEWe're only using kvec's. Briefly, before commit 7bdee4157591 ("tee:And if that's your API contract, then there's no point in takingI had the same diff but didn't went this way since we can't be sure that
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"
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.
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?
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.
Cheers,
Jens