Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230521160426.1881124-1-masahiroy@kernel.org> <20230521160426.1881124-3-masahiroy@kernel.org>
In-Reply-To: <20230521160426.1881124-3-masahiroy@kernel.org>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Mon, 22 May 2023 10:56:20 -0700
Message-ID: <CAKwvOd=B+dKWjxD-K-8btROvywp_Nei=CREeYZdCvKSGuHHJOA@mail.gmail.com>
Subject: Re: [PATCH v6 02/20] modpost: fix section mismatch message for R_ARM_ABS32
To:     Masahiro Yamada <masahiroy@kernel.org>
Cc:     linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org,
        Nathan Chancellor <nathan@kernel.org>,
        Nicolas Schier <nicolas@fjasle.eu>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        Fangrui Song <maskray@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

+ linux-arm-kernel and some folks who might know another idea.

On Sun, May 21, 2023 at 9:05=E2=80=AFAM Masahiro Yamada <masahiroy@kernel.o=
rg> wrote:
>
> addend_arm_rel() processes R_ARM_ABS32 in a wrong way.
>
> Here, simple test code.
>
>   [test code 1]
>
>     #include <linux/init.h>
>
>     int __initdata foo;
>     int get_foo(int x) { return foo; }
>
> If you compile it with ARM versatile_defconfig, modpost will show the
> symbol name, (unknown).
>
>   WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (se=
ction: .text) -> (unknown) (section: .init.data)
>
> If you compile it for other architectures, modpost will show the correct
> symbol name.
>
>   WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (se=
ction: .text) -> foo (section: .init.data)
>
> For R_ARM_ABS32, addend_arm_rel() sets r->r_addend to a wrong value.
>
> I just mimicked the code in arch/arm/kernel/module.c.
>
> However, there is more difficulty for ARM.
>
> Here, test code.
>
>   [test code 2]
>
>     #include <linux/init.h>
>
>     int __initdata foo;
>     int get_foo(int x) { return foo; }
>
>     int __initdata bar;
>     int get_bar(int x) { return bar; }
>
> With this commit applied, modpost will show the following messages
> for ARM versatile_defconfig:
>
>   WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (se=
ction: .text) -> foo (section: .init.data)
>   WARNING: modpost: vmlinux.o: section mismatch in reference: get_bar (se=
ction: .text) -> foo (section: .init.data)
>
> The reference from 'get_bar' to 'foo' seems wrong.
>
> I have no solution for this because it is true in assembly level.
>
> In the following output, relocation at 0x1c is no longer associated
> with 'bar'. The two relocation entries point to the same symbol, and
> the offset to 'bar' is encoded in the instruction 'r0, [r3, #4]'.
>
>   Disassembly of section .text:
>
>   00000000 <get_foo>:
>      0: e59f3004          ldr     r3, [pc, #4]   @ c <get_foo+0xc>
>      4: e5930000          ldr     r0, [r3]
>      8: e12fff1e          bx      lr
>      c: 00000000          .word   0x00000000
>
>   00000010 <get_bar>:
>     10: e59f3004          ldr     r3, [pc, #4]   @ 1c <get_bar+0xc>
>     14: e5930004          ldr     r0, [r3, #4]
>     18: e12fff1e          bx      lr
>     1c: 00000000          .word   0x00000000
>
>   Relocation section '.rel.text' at offset 0x244 contains 2 entries:
>    Offset     Info    Type            Sym.Value  Sym. Name
>   0000000c  00000c02 R_ARM_ABS32       00000000   .init.data
>   0000001c  00000c02 R_ARM_ABS32       00000000   .init.data
>
> When find_elf_symbol() gets into a situation where relsym->st_name is
> zero, there is no guarantee to get the symbol name as written in C.
>
> I am keeping the current logic because it is useful in many architectures=
,
> but the symbol name is not always correct depending on the optimization
> of the relocation. I left some comments in find_tosym().
>
> Fixes: 56a974fa2d59 ("kbuild: make better section mismatch reports on arm=
")
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> Changes in v6:
>  - More detailed commit log
>
>  scripts/mod/modpost.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 71de14544432..34fbbd85bfde 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1124,6 +1124,10 @@ static Elf_Sym *find_tosym(struct elf_info *elf, E=
lf64_Sword addr,
>         if (relsym->st_name !=3D 0)
>                 return relsym;
>
> +       /*
> +        * Strive to find a better symbol name, but the resulting name do=
es not
> +        * always match the symbol referenced in the original code.
> +        */
>         relsym_secindex =3D get_secindex(elf, relsym);
>         for (sym =3D elf->symtab_start; sym < elf->symtab_stop; sym++) {
>                 if (get_secindex(elf, sym) !=3D relsym_secindex)
> @@ -1306,12 +1310,12 @@ static int addend_386_rel(struct elf_info *elf, E=
lf_Shdr *sechdr, Elf_Rela *r)
>  static int addend_arm_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Re=
la *r)
>  {
>         unsigned int r_typ =3D ELF_R_TYPE(r->r_info);
> +       Elf_Sym *sym =3D elf->symtab_start + ELF_R_SYM(r->r_info);
> +       unsigned int inst =3D TO_NATIVE(*reloc_location(elf, sechdr, r));
>
>         switch (r_typ) {
>         case R_ARM_ABS32:
> -               /* From ARM ABI: (S + A) | T */
> -               r->r_addend =3D (int)(long)
> -                             (elf->symtab_start + ELF_R_SYM(r->r_info));
> +               r->r_addend =3D inst + sym->st_value;
>                 break;
>         case R_ARM_PC24:
>         case R_ARM_CALL:
> --
> 2.39.2
>


--=20
Thanks,
~Nick Desaulniers