Re: [Crash-utility] [PATCH CRASH] Fix module section load address when sh_addr != 0

Hi Stephen,

Thanks for reporting the issue and patch. Please check if I understood
you correctly. The correct output you are expecting in your case is:

    crash> sym ata_dummy_port_ops
    ffffffffc0a71580 (?) ata_dummy_port_ops [libata]
    crash> mod -s libata
         MODULE       NAME                      TEXT_BASE         SIZE
 OBJECT FILE
    ffffffffc0a7b640  libata                 ffffffffc0a47000   520192
    /usr/lib/debug/lib/modules/6.12.0-0.11.8.el9uek.x86_64/kernel/drivers/ata/libata.ko.debug
    crash> sym ata_dummy_port_ops
    ffffffffc0a71580 (B) ata_dummy_port_ops [libata]
    crash> p/x &ata_dummy_port_ops
    $1 = 0xffffffffc0a6fe80 <<-------- should be 0xffffffffc0a71580,
same as sym's output, right?

If that is the case, then after applied your patch, the issue still
exists on my machine:

crash> sym fuse_miscdevice
ffffffffc05f7fe0 (?) fuse_miscdevice [fuse]
crash> mod -s fuse
     MODULE       NAME                             TEXT_BASE
SIZE  OBJECT FILE
ffffffffc05f8dc0  fuse                          ffffffffc05da000
233472  /usr/lib/debug/lib/modules/6.11.5-300.fc41.x86_64/kernel/fs/fuse/fuse.ko.debug
crash> sym fuse_miscdevice
ffffffffc05f7fe0 (b) fuse_miscdevice [fuse]
crash> p/x &fuse_miscdevice
$1 = 0xffffffffc05daf20 << ------ unmatch with the previous value.

The .data section of fuse.ko.debug have non-zero address:

$ readelf -S /usr/lib/debug/lib/modules/6.11.5-300.fc41.x86_64/kernel/fs/fuse/fuse.ko.debug
Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
...
[50] .data             NOBITS           0000000000000e20  00000100
       000000000000080f  0000000000000000  WA       0     0     32

Could you please re-check your patch for this, or is there something
I'm missing?

Thanks,
Tao Liu

On Wed, Apr 2, 2025 at 11:41 AM Stephen Brennan
<[email protected]> wrote:
>
> A user reported that crash was reporting the address for certain module
> variables incorrectly. I was able to track it down specifically to
> variables which were located in the .data section of a kernel module.
> While the "sym" command gave the correct value, printing the address of
> the variable or expressions based on it with "p" would give an incorrect
> value. For example, the variable "ata_dummy_port_ops" variable is
> included in the .data section of libata.ko when built as a module:
>
>     $ sudo grep '\bata_dummy_port_ops\b' /proc/kallsyms
>     ffffffffc0a71580 d ata_dummy_port_ops   [libata]
>     $ sudo crash /usr/lib/debug/lib/modules/$(uname -r)/vmlinux /proc/kcore
>     crash> sym ata_dummy_port_ops
>     ffffffffc0a71580 (?) ata_dummy_port_ops [libata]
>     crash> mod -s libata
>          MODULE       NAME                      TEXT_BASE         SIZE  OBJECT FILE
>     ffffffffc0a7b640  libata                 ffffffffc0a47000   520192
>     /usr/lib/debug/lib/modules/6.12.0-0.11.8.el9uek.x86_64/kernel/drivers/ata/libata.ko.debug
>     crash> sym ata_dummy_port_ops
>     ffffffffc0a71580 (B) ata_dummy_port_ops [libata]
>     crash> p/x &ata_dummy_port_ops
>     $1 = 0xffffffffc0a6fe80
>
> The symbol value (from kallsyms) is correct, but its address provided by
> GDB is incorrect. It turns out that the .data section has an sh_addr
> which is non-zero. The result of this is that calculate_load_order_6_4()
> incorrectly calculates the base address for the .data section. This
> patch fixes the base address which is later provided to GDB via
> add-symbol-file.
>
> The impact here is interesting. Only variables within sections that have
> a non-zero sh_addr are impacted. It turns out that this is relatively
> common since Linux kernel commit 22d407b164ff7 ("lib: add allocation
> tagging support for memory allocation profiling"), which was merged in
> 6.10. That commit added an entry to the scripts/module.lds.S linker
> script, without specifying a base address of zero. I believe that is the
> reason for the non-zero base addresses.
>
> I was able to verify that, in addition to the Oracle Linux kernel where
> we initially noticed the issue, kernel modules on Arch Linux and Fedora
> also have non-zero .data sh_addr values. This is likely the case for
> most non-clang kernels since 6.10, but those were the only two distros I
> checked. While my reading of the module.lds.S seems to indicate that
> kernels built with CONFIG_LTO_CLANG=y should also have non-zero .data,
> .bss, and .rodata section addresses, I haven't been able to reproduce
> this with clang LTO kernels. Regardless, crash should properly handle
> non-zero sh_addr since it exists in the real world now.
>
> The core of the issue is that the symbol value returned by BFD includes
> the sh_addr of the section containing the symbol. For example, suppose
> a symbol with address 0 is located within a section with virtual address
> 0xa00. Then, the resulting symbol value will be 0xa00, not 0.
>
> calculate_load_order_6_4() computes the base address of each section by
> using a kallsyms symbol known to be within that section, and then
> subtracting the value of the symbol from the object file. This
> implicitly assumes that the section sh_addr is zero, and thus the symbol
> value is just an offset. To fix the computation, add in the section base
> address, to account for cases where it is non-zero.
>
> Signed-off-by: Stephen Brennan <[email protected]>
> ---
>  symbols.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/symbols.c b/symbols.c
> index 5adbc30..e30fafe 100644
> --- a/symbols.c
> +++ b/symbols.c
> @@ -12808,6 +12808,7 @@ calculate_load_order_6_4(struct load_module *lm, bfd *bfd, int dynamic,
>         asymbol *store;
>         asymbol *sym;
>         symbol_info syminfo;
> +       bfd_vma secaddr;
>         char *secname;
>         int i, t;
>
> @@ -12860,6 +12861,7 @@ calculate_load_order_6_4(struct load_module *lm, bfd *bfd, int dynamic,
>                                 }
>                                 if (strcmp(syminfo.name, s1->name) == 0) {
>                                         secname = (char *)bfd_section_name(sym->section);
> +                                       secaddr = bfd_section_vma(sym->section);
>                                         break;
>                                 }
>
> @@ -12890,14 +12892,14 @@ calculate_load_order_6_4(struct load_module *lm, bfd *bfd, int dynamic,
>                         }
>
>                         /* Update the offset information for the section */
> -                       sec_start = s1->value - syminfo.value;
> +                       sec_start = s1->value - syminfo.value + secaddr;
>                         /* keep the address instead of offset */
>                         lm->mod_section_data[i].addr = sec_start;
>                         lm->mod_section_data[i].flags |= SEC_FOUND;
>
>                         if (CRASHDEBUG(2))
> -                               fprintf(fp, "update sec offset sym %s @ %lx  val %lx  section %s\n",
> -                                       s1->name, s1->value, (ulong)syminfo.value, secname);
> +                               fprintf(fp, "update sec offset sym %s @ %lx  val %lx  section %s @ %lx\n",
> +                                       s1->name, s1->value, (ulong)syminfo.value, secname, secaddr);
>
>                         if (strcmp(secname, ".text") == 0)
>                                 lm->mod_text_start = sec_start;
> --
> 2.43.5
> --
> Crash-utility mailing list -- [email protected]
> To unsubscribe send an email to [email protected]
> https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
> Contribution Guidelines: https://github.com/crash-utility/crash/wiki