On Tue, Jul 26, 2022 at 6:20 AM Masahiro Yamada [off-list ref] wrote:

On Fri, Jul 22, 2022 at 11:24 AM Mauricio Faria de Oliveira
[off-list ref] wrote:

quoted

Now both functions are almost identical, and we can again generalize
the relocation types Elf_Rela/Elf_Rel with Elf_Rela, and handle some
differences with conditionals on section header type (SHT_RELA/REL).

The important bit is to make sure the loop increment uses the right
size for pointer arithmethic.

The original reason for split functions to make program logic easier
to follow; commit 5b24c0715fc4 ("kbuild: code refactoring in modpost").

Hopefully these 2 commits may help improving that, without an impact
in understanding the code due to generalization of relocation types.

Signed-off-by: Mauricio Faria de Oliveira <redacted>
---
 scripts/mod/modpost.c | 61 ++++++++++++++++---------------------------
 1 file changed, 23 insertions(+), 38 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 4c1038dccae0..d1ed67fa290b 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c

@@ -1794,63 +1794,49 @@ static int get_relx_sym(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *rela,
        return 0;
 }

-static void section_rela(const char *modname, struct elf_info *elf,
+/* The caller must ensure sechdr->sh_type == SHT_RELA or SHT_REL. */
+static void section_relx(const char *modname, struct elf_info *elf,
                         Elf_Shdr *sechdr)
 {
        Elf_Sym  *sym;
-       Elf_Rela *rela;
+       Elf_Rela *relx; /* access .r_addend in SHT_RELA _only_! */
        Elf_Rela r;
+       size_t relx_size;
        const char *fromsec;

        Elf_Rela *start = (void *)elf->hdr + sechdr->sh_offset;
        Elf_Rela *stop  = (void *)start + sechdr->sh_size;

        fromsec = sech_name(elf, sechdr);
-       fromsec += strlen(".rela");
+       if (sechdr->sh_type == SHT_RELA) {
+               relx_size = sizeof(Elf_Rela);
+               fromsec += strlen(".rela");
+       } else if (sechdr->sh_type == SHT_REL) {
+               relx_size = sizeof(Elf_Rel);
+               fromsec += strlen(".rel");
+       } else {
+               error("%s: [%s.ko] not relocation section\n", fromsec, modname);

Nit.

modname already contains the suffix  ".o".

For vmlinux, the error message will print like this:
[vmlinux.o.ko]

Oops, I missed the '.o' suffix difference between modname and mod->name.

If it's OK, I just removed '.ko' as it's simpler than plumbing 'mod' in
(i.e., for  "[%s%s]" with mod->name, mod->is_vmlinux ? "" : ".ko" ).

And just noting for myself/other readers:

Similar calls in do_sysctl_{entry,table}() with '.ko' are OK because their
modname is mod->name (without '.o' suffix), and shouldn't run for vmlinux,
just modules (MODULE_SYSCTL_TABLE is defined if MODULE is defined).

Thanks!

quoted

+               return;
+       }
+
        /* if from section (name) is know good then skip it */
        if (match(fromsec, section_white_list))
                return;

-       for (rela = start; rela < stop; rela++) {
-               if (get_relx_sym(elf, sechdr, rela, &r, &sym))
+       for (relx = start; relx < stop; relx = (void *)relx + relx_size) {
+               if (get_relx_sym(elf, sechdr, relx, &r, &sym))
                        continue;

                switch (elf->hdr->e_machine) {
                case EM_RISCV:
-                       if (!strcmp("__ex_table", fromsec) &&
+                       if (sechdr->sh_type == SHT_RELA &&
+                           !strcmp("__ex_table", fromsec) &&
                            ELF_R_TYPE(r.r_info) == R_RISCV_SUB32)
                                continue;
                        break;
                }

-               if (is_second_extable_reloc(start, rela, fromsec))
-                       find_extable_entry_size(fromsec, &r);
-               check_section_mismatch(modname, elf, &r, sym, fromsec);
-       }
-}
-
-static void section_rel(const char *modname, struct elf_info *elf,
-                       Elf_Shdr *sechdr)
-{
-       Elf_Sym *sym;
-       Elf_Rel *rel;
-       Elf_Rela r;
-       const char *fromsec;
-
-       Elf_Rel *start = (void *)elf->hdr + sechdr->sh_offset;
-       Elf_Rel *stop  = (void *)start + sechdr->sh_size;
-
-       fromsec = sech_name(elf, sechdr);
-       fromsec += strlen(".rel");
-       /* if from section (name) is know good then skip it */
-       if (match(fromsec, section_white_list))
-               return;
-
-       for (rel = start; rel < stop; rel++) {
-               if (get_relx_sym(elf, sechdr, (Elf_Rela *)rel, &r, &sym)
-                       continue;
-
-               if (is_second_extable_reloc(start, rel, fromsec))
+               if (is_second_extable_reloc(start, relx, fromsec))
                        find_extable_entry_size(fromsec, &r);
                check_section_mismatch(modname, elf, &r, sym, fromsec);
        }

@@ -1877,10 +1863,9 @@ static void check_sec_ref(const char *modname, struct elf_info *elf)
        for (i = 0; i < elf->num_sections; i++) {
                check_section(modname, elf, &elf->sechdrs[i]);
                /* We want to process only relocation sections and not .init */
-               if (sechdrs[i].sh_type == SHT_RELA)
-                       section_rela(modname, elf, &elf->sechdrs[i]);
-               else if (sechdrs[i].sh_type == SHT_REL)
-                       section_rel(modname, elf, &elf->sechdrs[i]);
+               if (sechdrs[i].sh_type == SHT_RELA ||
+                   sechdrs[i].sh_type == SHT_REL)
+                       section_relx(modname, elf, &elf->sechdrs[i]);
        }
 }

--

2.25.1

--
Best Regards
Masahiro Yamada

--
Mauricio Faria de Oliveira