| Message ID | 20220722022416.137548-3-mfo@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce "sysctl:" module aliases | expand |
On Fri, Jul 22, 2022 at 11:24 AM Mauricio Faria de Oliveira <mfo@canonical.com> wrote: > > 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 <mfo@canonical.com> > --- > 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] > + 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 >
On Tue, Jul 26, 2022 at 6:20 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Fri, Jul 22, 2022 at 11:24 AM Mauricio Faria de Oliveira > <mfo@canonical.com> wrote: > > > > 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 <mfo@canonical.com> > > --- > > 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! > > > > > > > + 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
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); + 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]); } }
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 <mfo@canonical.com> --- scripts/mod/modpost.c | 61 ++++++++++++++++--------------------------- 1 file changed, 23 insertions(+), 38 deletions(-)