Re: [PATCH v2 bpf-next 07/17] libbpf: factor out symtab and relos sanity checks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;
> > -                     }
> [...]



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux