Re: [PATCH v2] module/kallsyms: Filter out local mapping symbols during module load
Huacai Chen <[email protected]>
| Newsgroups | org.kernel.vger.linux-modules,dev.linux.lists.loongarch,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <CAAhV-H5qN2=LEbVS+hd5Uohq6OKHgkxEWLg4GNukYFBmjEABHg@mail.gmail.com> |
Hi, Petr, On Tue, Aug 11, 2026 at 8:20 PM Petr Pavlu <[email protected]> wrote: > > On 8/11/26 9:15 AM, Tiezhu Yang wrote: > > The compiler toolchains generate internal local labels on certain > > architectures (such as LoongArch) for optimizations and relocations. > > > > While these local labels are filtered out during runtime lookups in > > find_kallsyms_symbol(), they still leak into the permanent symbol > > tables of loaded modules, because layout_symtab() and add_kallsyms() > > do not check for the mapping symbols during layout generation. > > > > Consequently, tracing tools like bpftrace resolve identical addresses > > into confusing local labels instead of actual clear C function names. > > > > Fix this by adding is_mapping_symbol() checks directly into the symbol > > tracking loops of layout_symtab() and add_kallsyms(). This prevents the > > mapping symbols from entering the module's memory symbol arrays at load > > time. > > This optimization looks sensible to me. Kallsyms in vmlinux also > normally doesn't include information about local labels (.L<xyz>) and > mapping symbols ($a, $d, ...), so it should be ok not to track them in > modules either. > > Note that the patch description mixes up these two symbol categories, > presumably because the existing is_mapping_symbol() function conflates > them as well. > > > > > For livepatch modules, core_kallsyms.symtab must remain a complete copy > > of the original ELF symbol table to prevent delayed relocation failures. > > Therefore, enforce is_livepatch_module() checking to skip the filtering > > for livepatch modules. > > > > Reproduce steps: > > > > 1. Set up a LoongArch VM with "-accel kvm": > > > > $ sudo qemu-system-loongarch64 -serial stdio \ > > -machine virt -cpu la464 -smp 4 -m 4G \ > > -bios /usr/share/edk2/loongarch64/QEMU_EFI.fd \ > > -nodefaults -no-reboot -nographic -accel kvm > > > > 2. Use bpftrace to capture kstack when vCPU is scheduled out: > > > > $ cat trace_sched.bt > > kprobe:kvm:kvm_sched_out > > { > > if (pid == $1) { > > print(kstack()); > > } > > } > > > > $ sudo bpftrace trace_sched.bt `pgrep -o qemu-system` > > > > Test results: > > > > 1. Before this patch (Confusing stack with local labels): > > > > kvm_sched_out+0 > > __schedule+1584 > > schedule+48 > > .LVL3767+36 > > .LVL3801+8 > > .LVL285+12 > > .LVL425+44 > > .LVL1419+20 > > kvm_exc_entry+260 > > The description should mention that this change affects what appears in > /proc/kallsyms, as that is the important part. bpftrace is only one user > of this data and apparently gets confused because it does not apply any > filtering of its own, unlike perf which has is_ignored_kernel_symbol(). > > > > > 2. After this patch (Accurate stack with function names): > > > > kvm_sched_out+0 > > __schedule+1584 > > schedule+48 > > kvm_vcpu_block+112 > > kvm_vcpu_halt+104 > > kvm_emu_idle+284 > > kvm_handle_gspr+1316 > > kvm_handle_exit+456 > > kvm_exc_entry+260 > > > > Signed-off-by: Tiezhu Yang <[email protected]> > > --- > > v2: Fix livepatch support as pointed out by Sashiko bot. > > > > kernel/module/kallsyms.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c > > index f23126d804b2..aece7aa49dd4 100644 > > --- a/kernel/module/kallsyms.c > > +++ b/kernel/module/kallsyms.c > > @@ -130,6 +130,10 @@ void layout_symtab(struct module *mod, struct load_info *info) > > > > /* Compute total space required for the core symbols' strtab. */ > > for (ndst = i = 0; i < nsrc; i++) { > > + if (!is_livepatch_module(mod) && > > + is_mapping_symbol(&info->strtab[src[i].st_name])) > > + continue; > > + > > if (i == 0 || is_livepatch_module(mod) || > > is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum, > > info->index.pcpu)) { > > @@ -198,6 +202,11 @@ void add_kallsyms(struct module *mod, const struct load_info *info) > > src = kallsyms->symtab; > > for (ndst = i = 0; i < kallsyms->num_symtab; i++) { > > kallsyms->typetab[i] = elf_type(src + i, info); > > + > > + if (!is_livepatch_module(mod) && > > + is_mapping_symbol(&kallsyms->strtab[src[i].st_name])) > > + continue; > > + > > if (i == 0 || is_livepatch_module(mod) || > > is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum, > > info->index.pcpu)) { > > It is better to avoid duplicating the same logic in layout_symtab() and > add_kallsyms(). The function is_core_symbol() can be renamed to > something like is_kept_symbol() and can be solely responsible for > deciding what to keep. I'm sorry but I think Tiezhu's patch is simpler and cleaner. :) Huacai > > The following prototype shows my suggestion: > > diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c > index 0fc11e45df9b..c86879cc5b66 100644 > --- a/kernel/module/kallsyms.c > +++ b/kernel/module/kallsyms.c > @@ -75,23 +75,29 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info) > return '?'; > } > > -static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs, > - unsigned int shnum, unsigned int pcpundx) > +static bool is_kept_symbol(struct module *mod, const struct load_info *info, > + unsigned int symnum, const Elf_Sym *src) > { > const Elf_Shdr *sec; > enum mod_mem_type type; > > + if (symnum == 0 || is_livepatch_module(mod)) > + return true; > + > if (src->st_shndx == SHN_UNDEF || > - src->st_shndx >= shnum || > + src->st_shndx >= info->hdr->e_shnum || > !src->st_name) > return false; > > + if (is_mapping_symbol(&info->strtab[src->st_name])) > + return false; > + > #ifdef CONFIG_KALLSYMS_ALL > - if (src->st_shndx == pcpundx) > + if (src->st_shndx == info->index.pcpu) > return true; > #endif > > - sec = sechdrs + src->st_shndx; > + sec = info->sechdrs + src->st_shndx; > type = sec->sh_entsize >> SH_ENTSIZE_TYPE_SHIFT; > if (!(sec->sh_flags & SHF_ALLOC) > #ifndef CONFIG_KALLSYMS_ALL > @@ -130,12 +136,11 @@ void layout_symtab(struct module *mod, struct load_info *info) > > /* Compute total space required for the core symbols' strtab. */ > for (ndst = i = 0; i < nsrc; i++) { > - if (i == 0 || is_livepatch_module(mod) || > - is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum, > - info->index.pcpu)) { > - strtab_size += strlen(&info->strtab[src[i].st_name]) + 1; > - ndst++; > - } > + if (!is_kept_symbol(mod, info, i, src + i)) > + continue; > + > + strtab_size += strlen(&info->strtab[src[i].st_name]) + 1; > + ndst++; > } > > /* Append room for core symbols at end of core part. */ > @@ -197,23 +202,22 @@ void add_kallsyms(struct module *mod, const struct load_info *info) > strtab_size = info->core_typeoffs - info->stroffs; > src = kallsyms->symtab; > for (ndst = i = 0; i < kallsyms->num_symtab; i++) { > + ssize_t ret; > + > kallsyms->typetab[i] = elf_type(src + i, info); > - if (i == 0 || is_livepatch_module(mod) || > - is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum, > - info->index.pcpu)) { > - ssize_t ret; > - > - mod->core_kallsyms.typetab[ndst] = > - kallsyms->typetab[i]; > - dst[ndst] = src[i]; > - dst[ndst++].st_name = s - mod->core_kallsyms.strtab; > - ret = strscpy(s, &kallsyms->strtab[src[i].st_name], > - strtab_size); > - if (ret < 0) > - break; > - s += ret + 1; > - strtab_size -= ret + 1; > - } > + > + if (!is_kept_symbol(mod, info, i, src + i)) > + continue; > + > + mod->core_kallsyms.typetab[ndst] = kallsyms->typetab[i]; > + dst[ndst] = src[i]; > + dst[ndst++].st_name = s - mod->core_kallsyms.strtab; > + ret = strscpy(s, &kallsyms->strtab[src[i].st_name], > + strtab_size); > + if (ret < 0) > + break; > + s += ret + 1; > + strtab_size -= ret + 1; > } > > /* Set up to point into init section. */ > > -- > Thanks, > Petr >