On Thu, Apr 22, 2021 at 9:06 AM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 4/16/21 1:23 PM, Andrii Nakryiko wrote:
> > Factor out logic for sanity checking SHT_SYMTAB and SHT_REL sections into
> > separate sections. They are already quite extensive and are suffering from too
> > deep indentation. Subsequent changes will extend SYMTAB sanity checking
> > further, so it's better to factor each into a separate function.
> >
> > No functional changes are intended.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> Ack with a minor suggestion below.
> Acked-by: Yonghong Song <yhs@xxxxxx>
>
> > ---
> > tools/lib/bpf/linker.c | 233 ++++++++++++++++++++++-------------------
> > 1 file changed, 127 insertions(+), 106 deletions(-)
> >
[...]
> >
> > - /* .rel<secname> -> <secname> pattern is followed */
> > - if (strncmp(sec->sec_name, ".rel", sizeof(".rel") - 1) != 0
> > - || strcmp(sec->sec_name + sizeof(".rel") - 1, link_sec->sec_name) != 0) {
> > - pr_warn("ELF relo section #%zu name has invalid name in %s\n",
> > - sec->sec_idx, obj->filename);
> > + n = sec->shdr->sh_size / sec->shdr->sh_entsize;
> > + sym = sec->data->d_buf;
> > + for (i = 0; i < n; i++, sym++) {
> > + if (sym->st_shndx
> > + && sym->st_shndx < SHN_LORESERVE
> > + && sym->st_shndx >= obj->sec_cnt) {
> > + pr_warn("ELF sym #%d in section #%zu points to missing section #%zu in %s\n",
> > + i, sec->sec_idx, (size_t)sym->st_shndx, obj->filename);
> > + return -EINVAL;
> > + }
> > + if (ELF64_ST_TYPE(sym->st_info) == STT_SECTION) {
> > + if (sym->st_value != 0)
> > return -EINVAL;
> > - }
> > + continue;
>
> I think this "continue" is not necessary.
>
yes, but I wanted to make it clear that there should be no more logic
handling STT_SECTION afterwards, if/when we extend the logic of this
loop. We have similar patterns with goto to keep logic more modular
and easily extensible:
if (something) {
err = -EINVAL;
goto err_out;
}
err_out:
return err;
So let's keep it as is.
> > + }
> > + }
> >
> > - /* don't further validate relocations for ignored sections */
> > - if (link_sec->skipped)
> > - break;
> > + return 0;
> > +}
> >
> > - /* relocatable section is data or instructions */
> > - if (link_sec->shdr->sh_type != SHT_PROGBITS
> > - && link_sec->shdr->sh_type != SHT_NOBITS) {
> > - pr_warn("ELF relo section #%zu points to invalid section #%zu in %s\n",
> > - sec->sec_idx, (size_t)sec->shdr->sh_info, obj->filename);
> > - return -EINVAL;
> > - }
> [...]
