Message-ID: <c405a81e-3a6c-47ec-8069-65ae95c1572a@pengutronix.de>
Date: Tue, 3 Feb 2026 12:55:08 +0100
From: Sven Püschel <s.pueschel@...gutronix.de>
To: Marco Felsch <m.felsch@...gutronix.de>, Sumit Garg <sumit.garg@...nel.org>
Cc: linux-kernel@...r.kernel.org, Matthew Wilcox <willy@...radead.org>,
 op-tee@...ts.trustedfirmware.org, kernel@...gutronix.de,
 akpm@...ux-foundation.org, Jens Wiklander <jens.wiklander@...aro.org>,
 vbabka@...e.cz
Subject: Re: [PATCH v2] tee: shm: fix slab page refcounting

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@...nel.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@...gutronix.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
>

Powered by blists - more mailing lists