Hi Kazu & Lianbo, On Mon, Dec 9, 2024 at 2:43 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> wrote: > > On 2024/12/06 17:59, lijiang wrote: > > Hi, Kazu > > Thank you for the fix. > > On Thu, Dec 5, 2024 at 5:10 PM <devel-request@xxxxxxxxxxxxxxxxxxxxxxxxxxx <mailto:devel-request@xxxxxxxxxxxxxxxxxxxxxxxxxxx>> wrote: > > > > Date: Thu, 5 Dec 2024 01:30:34 +0000 > > From: HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx>> > > Subject: [PATCH] Fix infinite loop during module > > symbols initialization > > To: "devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx <mailto:devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx>" > > <devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx <mailto:devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx>> > > Message-ID: <1733362232-30303-1-git-send-email-k-hagio-ab@xxxxxxx <mailto:1733362232-30303-1-git-send-email-k-hagio-ab@xxxxxxx>> > > Content-Type: text/plain; charset="iso-2022-jp" > > > > From: Kazuhito Hagio <k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx>> > > > > An infinite loop occured in mod_symname_hash_install() during module > > symbols initialization on 6.13-rc1 kernel. > > > > Not sure which kernel patch caused this (probably execmem patches), a > > gap region between a module text and another module's one has gone, i.e. > > they became consecutive. In this case, as we set a module's pseudo end > > symbol to "base + size" value, it's equal to another module's pseudo > > start symbol. This and qsort() for st->ext_module_symbtable cause > > region overlaps like this: > > > > sp sp->value sp->name > > 823ac68 ffffffffc0800000 _MODULE_TEXT_START_dm_mod > > ... > > 8242080 ffffffffc0814ad0 __pfx_dm_devt_from_path > > 82420a8 ffffffffc0814ae0 dm_devt_from_path > > 82420d0 ffffffffc0815000 _MODULE_TEXT_START_libcrc32c << same value > > 82420f8 ffffffffc0815000 __pfx_crc32c << and > > 8242120 ffffffffc0815000 _MODULE_TEXT_END_dm_mod << overlap > > > > > > Strange, this looks like a kernel module issue? > > I don't think so, as written above, it should be due to the change of > memory placement in kernel and crash's current implementation. > > > > > 8242148 ffffffffc0815010 crc32c > > 8242170 ffffffffc0815080 __pfx_libcrc32c_mod_fini > > ... > > 8242238 ffffffffc0816000 _MODULE_TEXT_END_libcrc32c > > > > As a result, mod_symtable_hash_install_range() can add a module symbol > > two times, which makes a self-loop. This causes an infinite loop in > > mod_symname_hash_install(). > > > > Core was generated by `crash'. > > #0 mod_symname_hash_install (spn=0x75e6bc0) at symbols.c:1236 > > 1236 if (!sp->name_hash_next || > > (gdb) p sp I can also reproduce this issue on 6.13.0-rc2 machine. The mod_symname_hash_install() function missed the case if one spn was installed 2 more times, which as a result, will cause the infinite loop. The reason for installing 2 more times is because the addresses overlap between 2 kernel modules. See the following: crash> sym -l ffffffffc0800000 MODULE TEXT START: qemu_fw_cfg ffffffffc0800000 (t) __pfx_fw_cfg_sysfs_attr_show ffffffffc0800010 (t) fw_cfg_sysfs_attr_show ... ffffffffc0801000 MODULE TEXT START: serio_raw ffffffffc0801000 (t) __pfx_serio_raw_poll ffffffffc0801000 MODULE TEXT END: qemu_fw_cfg ... ffffffffc0802000 MODULE TEXT END: serio_raw For module qemu_fw_cfg, 3 symbols of ffffffffc0801000 will be installed. Then for module serio_raw, 3 symbols of ffffffffc0801000 will be reinstalled. Due to the lack of repeat symbol check, there will be symbol reinstall, and infinite loop: sp->name_hash_next = spn; <<----- name_hash_next point to itself if sp==spn I think Kazu's "base + size - 1" solution is reasonable, this will let qemu_fw_cfg install range from ffffffffc0800000 to (ffffffffc0801000-1), so no overlap for 2 modules. In addition, I'd like to add the following code as well, to skip if a symbol has already been installed: diff --git a/symbols.c b/symbols.c index a26bdc1..e1a62bb 100644 --- a/symbols.c +++ b/symbols.c @@ -1233,6 +1233,9 @@ mod_symname_hash_install(struct syment *spn) return; } for (; sp; sp = sp->name_hash_next) { + if (spn == sp) + return; + if (!sp->name_hash_next || spn->value < sp->name_hash_next->value) { spn->name_hash_next = sp->name_hash_next; What do you think? Thanks, Tao Liu > > $1 = (struct syment *) 0x75e6670 > > (gdb) p sp->name > > $2 = 0x7c03573 "_MODULE_TEXT_START_libcrc32c" > > (gdb) p sp->name_hash_next > > $3 = (struct syment *) 0x75e6670 << self-loop > > > > To avoid this, fix module pseudo end symbols to "base + size - 1". > > > > So far I haven't thought of a better solution yet. For the patch: Ack. > > Thank you for your review. > > Thanks, > Kazu > > > > > Thanks > > Lianbo > > > > Signed-off-by: Kazuhito Hagio <k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx>> > > --- > > symbols.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/symbols.c b/symbols.c > > index a26bdc1a3461..0fe5d2951c7e 100644 > > --- a/symbols.c > > +++ b/symbols.c > > @@ -2196,7 +2196,7 @@ store_module_symbols_6_4(ulong total, int mods_installed) > > if (!lm->mem[t].size) > > continue; > > > > - st->ext_module_symtable[mcnt].value = lm->mem[t].base + lm->mem[t].size; > > + st->ext_module_symtable[mcnt].value = lm->mem[t].base + lm->mem[t].size - 1; > > > > st->ext_module_symtable[mcnt].type = 'm'; > > st->ext_module_symtable[mcnt].flags |= MODULE_SYMBOL; > > sprintf(buf2, "%s%s", module_tag[t].end, mod_name); > > -- > > 2.31.1 > > > -- > Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx > To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx > https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/ > Contribution Guidelines: https://github.com/crash-utility/crash/wiki -- Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/ Contribution Guidelines: https://github.com/crash-utility/crash/wiki
