Weinan Liu <wnliu@xxxxxxxxxx> writes: >> After some debugging this is what I found: >> >> devtmpfsd() calls devtmpfs_work_loop() which is marked '__noreturn' and has an >> infinite loop. The compiler puts the `bl` to devtmpfs_work_loop() as the the >> last instruction in devtmpfsd() and therefore on entry to devtmpfs_work_loop(), >> LR points to an instruction beyond devtmpfsd() and this consfuses the unwinder. >> >> ffff800080d9a070 <devtmpfsd>: >> ffff800080d9a070: d503201f nop >> ffff800080d9a074: d503201f nop >> ffff800080d9a078: d503233f paciasp >> ffff800080d9a07c: a9be7bfd stp x29, x30, [sp, #-32]! >> ffff800080d9a080: 910003fd mov x29, sp >> ffff800080d9a084: f9000bf3 str x19, [sp, #16] >> ffff800080d9a088: 943378e8 bl ffff800081a78428 <devtmpfs_setup> >> ffff800080d9a08c: 90006ca1 adrp x1, ffff800081b2e000 <unique_processor_ids+0x3758> >> ffff800080d9a090: 2a0003f3 mov w19, w0 >> ffff800080d9a094: 912de021 add x1, x1, #0xb78 >> ffff800080d9a098: 91002020 add x0, x1, #0x8 >> ffff800080d9a09c: 97cd2a43 bl ffff8000800e49a8 <complete> >> ffff800080d9a0a0: 340000d3 cbz w19, ffff800080d9a0b8 <devtmpfsd+0x48> >> ffff800080d9a0a4: 2a1303e0 mov w0, w19 >> ffff800080d9a0a8: f9400bf3 ldr x19, [sp, #16] >> ffff800080d9a0ac: a8c27bfd ldp x29, x30, [sp], #32 >> ffff800080d9a0b0: d50323bf autiasp >> ffff800080d9a0b4: d65f03c0 ret >> ffff800080d9a0b8: 97f06526 bl ffff8000809b3550 <devtmpfs_work_loop> >> ffff800080d9a0bc: 00000000 udf #0 >> ffff800080d9a0c0: d503201f nop >> ffff800080d9a0c4: d503201f nop >> >> find_fde() got pc=0xffff800080d9a0bc which is not in [sfde_func_start_address, sfde_func_size) >> >> output for readelf --sframe for devtmpfsd() >> >> func idx [51825]: pc = 0xffff800080d9a070, size = 76 bytes >> STARTPC CFA FP RA >> ffff800080d9a070 sp+0 u u >> ffff800080d9a07c sp+0 u u[s] >> ffff800080d9a080 sp+32 c-32 c-24[s] >> ffff800080d9a0b0 sp+0 u u[s] >> ffff800080d9a0b4 sp+0 u u >> ffff800080d9a0b8 sp+32 c-32 c-24[s] >> >> The unwinder and all the related infra is assuming that the return address >> will be part of a valid function which is not the case here. >> >> I am not sure which component needs to be fixed here, but the following >> patch(which is a hack) fixes the issue by considering the return address as >> part of the function descriptor entry. >> >> -- 8< -- >> >> diff --git a/kernel/sframe_lookup.c b/kernel/sframe_lookup.c >> index 846f1da95..28bec5064 100644 >> --- a/kernel/sframe_lookup.c >> +++ b/kernel/sframe_lookup.c >> @@ -82,7 +82,7 @@ static struct sframe_fde *find_fde(const struct sframe_table *tbl, unsigned long >> if (f >= tbl->sfhdr_p->num_fdes || f < 0) >> return NULL; >> fdep = tbl->fde_p + f; >> - if (ip < fdep->start_addr || ip >= fdep->start_addr + fdep->size) >> + if (ip < fdep->start_addr || ip > fdep->start_addr + fdep->size) >> return NULL; >> >> return fdep; >> @@ -106,7 +106,7 @@ static int find_fre(const struct sframe_table *tbl, unsigned long pc, >> else >> ip_off = (int32_t)(pc - (unsigned long)tbl->sfhdr_p) - fdep->start_addr; >> >> - if (ip_off < 0 || ip_off >= fdep->size) >> + if (ip_off < 0 || ip_off > fdep->size) >> return -EINVAL; >> >> /* >> >> -- >8 -- >> >> Thanks, >> Puranjay > > Thank you for reporting this issue. > I just found out that Josh also intentionally uses '>' instead of '>=' for the same reason > https://lore.kernel.org/lkml/20250122225257.h64ftfnorofe7cb4@jpoimboe/T/#m6d70a20ed9f5b3bbe5b24b24b8c5dcc603a79101 > > QQ, do we need to care the stacktrace after '__noreturn' function? Yes, I think we should, but others people could add more to this. I have been testing this series with Kpatch and created a PR that works with this unwinder: https://github.com/dynup/kpatch/pull/1439 For the modules, I think we need per module sframe tables that are initialised when the module is loaded. And the unwinder should use the module specific table if the IP is in a module's code. Have you already started working on it? if not I would like to help and work on that. Thanks, Puranjay
Attachment:
signature.asc
Description: PGP signature
