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

On Thu, Apr 22, 2021 at 9:06 AM Yonghong Song [off-list ref] wrote:

On 4/16/21 1:23 PM, Andrii Nakryiko wrote:

quoted

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@kernel.org>

Ack with a minor suggestion below.
Acked-by: Yonghong Song <redacted>

quoted

---
  tools/lib/bpf/linker.c | 233 ++++++++++++++++++++++-------------------
  1 file changed, 127 insertions(+), 106 deletions(-)

[...]

quoted

-                     /* .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.

quoted

+             }
+     }

-                     /* 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;
-                     }

[...]