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 <[email protected]>
---
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);
+ 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 Fri, Jul 22, 2022 at 11:24 AM Mauricio Faria de Oliveira
<[email protected]> 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 <[email protected]>
> ---
> 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
>
--
Best Regards
Masahiro Yamada
On Tue, Jul 26, 2022 at 6:20 AM Masahiro Yamada <[email protected]> wrote:
>
> On Fri, Jul 22, 2022 at 11:24 AM Mauricio Faria de Oliveira
> <[email protected]> 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 <[email protected]>
> > ---
> > 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