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: <20220722022416.137548-1-mfo@canonical.com> <20220722022416.137548-3-mfo@canonical.com>
 <CAK7LNARTN55RE18vXjMuAUO37kMeawBD4G=UcS6_j7U0asCZEA@mail.gmail.com>
In-Reply-To: <CAK7LNARTN55RE18vXjMuAUO37kMeawBD4G=UcS6_j7U0asCZEA@mail.gmail.com>
From:   Mauricio Faria de Oliveira <mfo@canonical.com>
Date:   Wed, 27 Jul 2022 14:10:18 -0300
Message-ID: <CAO9xwp2GpYHSNnvoXze=ye3OZoOxuoBv40BS+rv4UCdV=O_oHg@mail.gmail.com>
Subject: Re: [RFC PATCH 2/6] modpost: deduplicate section_rel[a]()
To:     Masahiro Yamada <masahiroy@kernel.org>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-modules <linux-modules@vger.kernel.org>,
        Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
        Linux FS-devel Mailing List <linux-fsdevel@vger.kernel.org>,
        Michal Marek <michal.lkml@markovi.net>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Iurii Zaikin <yzaikin@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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