| Message ID | 20190611044747.44839-3-andriin@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | BTF-defined BPF map definitions | expand |
On Mon, Jun 10, 2019 at 9:49 PM Andrii Nakryiko <andriin@fb.com> wrote: > > As a preparation for adding BTF-based BPF map loading, extract .BTF and > .BTF.ext loading logic. Also simplify error handling in > bpf_object__elf_collect() by returning early, as there is no common > clean up to be done. > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > tools/lib/bpf/libbpf.c | 137 ++++++++++++++++++++++------------------- > 1 file changed, 75 insertions(+), 62 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index ba89d9727137..9e39a0a33aeb 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -1078,6 +1078,58 @@ static void bpf_object__sanitize_btf_ext(struct bpf_object *obj) > } > } > > +static int bpf_object__load_btf(struct bpf_object *obj, > + Elf_Data *btf_data, > + Elf_Data *btf_ext_data) > +{ > + int err = 0; > + > + if (btf_data) { > + obj->btf = btf__new(btf_data->d_buf, btf_data->d_size); > + if (IS_ERR(obj->btf)) { > + pr_warning("Error loading ELF section %s: %d.\n", > + BTF_ELF_SEC, err); > + goto out; If we goto out here, we will return 0. > + } > + err = btf__finalize_data(obj, obj->btf); > + if (err) { > + pr_warning("Error finalizing %s: %d.\n", > + BTF_ELF_SEC, err); > + goto out; > + } > + bpf_object__sanitize_btf(obj); > + err = btf__load(obj->btf); > + if (err) { > + pr_warning("Error loading %s into kernel: %d.\n", > + BTF_ELF_SEC, err); > + goto out; > + } > + } > + if (btf_ext_data) { > + if (!obj->btf) { > + pr_debug("Ignore ELF section %s because its depending ELF section %s is not found.\n", > + BTF_EXT_ELF_SEC, BTF_ELF_SEC); > + goto out; We will also return 0 when goto out here. > + } > + obj->btf_ext = btf_ext__new(btf_ext_data->d_buf, > + btf_ext_data->d_size); > + if (IS_ERR(obj->btf_ext)) { > + pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", > + BTF_EXT_ELF_SEC, PTR_ERR(obj->btf_ext)); > + obj->btf_ext = NULL; > + goto out; And, here. And we will not free obj->btf. > + } > + bpf_object__sanitize_btf_ext(obj); > + } > +out: > + if (err || IS_ERR(obj->btf)) { > + if (!IS_ERR_OR_NULL(obj->btf)) > + btf__free(obj->btf); > + obj->btf = NULL; > + } > + return 0; > +} > + > static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > { > Elf *elf = obj->efile.elf; > @@ -1102,24 +1154,21 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > if (gelf_getshdr(scn, &sh) != &sh) { > pr_warning("failed to get section(%d) header from %s\n", > idx, obj->path); > - err = -LIBBPF_ERRNO__FORMAT; > - goto out; > + return -LIBBPF_ERRNO__FORMAT; > } > > name = elf_strptr(elf, ep->e_shstrndx, sh.sh_name); > if (!name) { > pr_warning("failed to get section(%d) name from %s\n", > idx, obj->path); > - err = -LIBBPF_ERRNO__FORMAT; > - goto out; > + return -LIBBPF_ERRNO__FORMAT; > } > > data = elf_getdata(scn, 0); > if (!data) { > pr_warning("failed to get section(%d) data from %s(%s)\n", > idx, name, obj->path); > - err = -LIBBPF_ERRNO__FORMAT; > - goto out; > + return -LIBBPF_ERRNO__FORMAT; > } > pr_debug("section(%d) %s, size %ld, link %d, flags %lx, type=%d\n", > idx, name, (unsigned long)data->d_size, > @@ -1130,10 +1179,14 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > err = bpf_object__init_license(obj, > data->d_buf, > data->d_size); > + if (err) > + return err; > } else if (strcmp(name, "version") == 0) { > err = bpf_object__init_kversion(obj, > data->d_buf, > data->d_size); > + if (err) > + return err; > } else if (strcmp(name, "maps") == 0) { > obj->efile.maps_shndx = idx; > } else if (strcmp(name, BTF_ELF_SEC) == 0) { > @@ -1144,11 +1197,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > if (obj->efile.symbols) { > pr_warning("bpf: multiple SYMTAB in %s\n", > obj->path); > - err = -LIBBPF_ERRNO__FORMAT; > - } else { > - obj->efile.symbols = data; > - obj->efile.strtabidx = sh.sh_link; > + return -LIBBPF_ERRNO__FORMAT; > } > + obj->efile.symbols = data; > + obj->efile.strtabidx = sh.sh_link; > } else if (sh.sh_type == SHT_PROGBITS && data->d_size > 0) { > if (sh.sh_flags & SHF_EXECINSTR) { > if (strcmp(name, ".text") == 0) > @@ -1162,6 +1214,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > pr_warning("failed to alloc program %s (%s): %s", > name, obj->path, cp); > + return err; > } > } else if (strcmp(name, ".data") == 0) { > obj->efile.data = data; > @@ -1173,8 +1226,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > pr_debug("skip section(%d) %s\n", idx, name); > } > } else if (sh.sh_type == SHT_REL) { > + int nr_reloc = obj->efile.nr_reloc; > void *reloc = obj->efile.reloc; > - int nr_reloc = obj->efile.nr_reloc + 1; > int sec = sh.sh_info; /* points to other section */ > > /* Only do relo for section with exec instructions */ > @@ -1184,79 +1237,39 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > continue; > } > > - reloc = reallocarray(reloc, nr_reloc, > + reloc = reallocarray(reloc, nr_reloc + 1, > sizeof(*obj->efile.reloc)); > if (!reloc) { > pr_warning("realloc failed\n"); > - err = -ENOMEM; > - } else { > - int n = nr_reloc - 1; > + return -ENOMEM; > + } > > - obj->efile.reloc = reloc; > - obj->efile.nr_reloc = nr_reloc; > + obj->efile.reloc = reloc; > + obj->efile.nr_reloc++; > > - obj->efile.reloc[n].shdr = sh; > - obj->efile.reloc[n].data = data; > - } > + obj->efile.reloc[nr_reloc].shdr = sh; > + obj->efile.reloc[nr_reloc].data = data; > } else if (sh.sh_type == SHT_NOBITS && strcmp(name, ".bss") == 0) { > obj->efile.bss = data; > obj->efile.bss_shndx = idx; > } else { > pr_debug("skip section(%d) %s\n", idx, name); > } > - if (err) > - goto out; > } > > if (!obj->efile.strtabidx || obj->efile.strtabidx >= idx) { > pr_warning("Corrupted ELF file: index of strtab invalid\n"); > return -LIBBPF_ERRNO__FORMAT; > } > - if (btf_data) { > - obj->btf = btf__new(btf_data->d_buf, btf_data->d_size); > - if (IS_ERR(obj->btf)) { > - pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", > - BTF_ELF_SEC, PTR_ERR(obj->btf)); > - obj->btf = NULL; > - } else { > - err = btf__finalize_data(obj, obj->btf); > - if (!err) { > - bpf_object__sanitize_btf(obj); > - err = btf__load(obj->btf); > - } > - if (err) { > - pr_warning("Error finalizing and loading %s into kernel: %d. Ignored and continue.\n", > - BTF_ELF_SEC, err); > - btf__free(obj->btf); > - obj->btf = NULL; > - err = 0; > - } > - } > - } > - if (btf_ext_data) { > - if (!obj->btf) { > - pr_debug("Ignore ELF section %s because its depending ELF section %s is not found.\n", > - BTF_EXT_ELF_SEC, BTF_ELF_SEC); > - } else { > - obj->btf_ext = btf_ext__new(btf_ext_data->d_buf, > - btf_ext_data->d_size); > - if (IS_ERR(obj->btf_ext)) { > - pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", > - BTF_EXT_ELF_SEC, > - PTR_ERR(obj->btf_ext)); > - obj->btf_ext = NULL; > - } else { > - bpf_object__sanitize_btf_ext(obj); > - } > - } > - } > + err = bpf_object__load_btf(obj, btf_data, btf_ext_data); > + if (err) > + return err; > if (bpf_object__has_maps(obj)) { > err = bpf_object__init_maps(obj, flags); > if (err) > - goto out; > + return err; > } > err = bpf_object__init_prog_names(obj); > -out: > return err; > } > > -- > 2.17.1 >
On Sat, Jun 15, 2019 at 1:25 PM Song Liu <liu.song.a23@gmail.com> wrote: > > On Mon, Jun 10, 2019 at 9:49 PM Andrii Nakryiko <andriin@fb.com> wrote: > > > > As a preparation for adding BTF-based BPF map loading, extract .BTF and > > .BTF.ext loading logic. Also simplify error handling in > > bpf_object__elf_collect() by returning early, as there is no common > > clean up to be done. Maybe separate bpf_object__load_btf() and other cleanup in two patches? (I won't complain if you keep them together) Thanks, Song > > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > --- > > tools/lib/bpf/libbpf.c | 137 ++++++++++++++++++++++------------------- > > 1 file changed, 75 insertions(+), 62 deletions(-) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index ba89d9727137..9e39a0a33aeb 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -1078,6 +1078,58 @@ static void bpf_object__sanitize_btf_ext(struct bpf_object *obj) > > } > > } > > > > +static int bpf_object__load_btf(struct bpf_object *obj, > > + Elf_Data *btf_data, > > + Elf_Data *btf_ext_data) > > +{ > > + int err = 0; > > + > > + if (btf_data) { > > + obj->btf = btf__new(btf_data->d_buf, btf_data->d_size); > > + if (IS_ERR(obj->btf)) { > > + pr_warning("Error loading ELF section %s: %d.\n", > > + BTF_ELF_SEC, err); > > + goto out; > > If we goto out here, we will return 0. > > > + } > > + err = btf__finalize_data(obj, obj->btf); > > + if (err) { > > + pr_warning("Error finalizing %s: %d.\n", > > + BTF_ELF_SEC, err); > > + goto out; > > + } > > + bpf_object__sanitize_btf(obj); > > + err = btf__load(obj->btf); > > + if (err) { > > + pr_warning("Error loading %s into kernel: %d.\n", > > + BTF_ELF_SEC, err); > > + goto out; > > + } > > + } > > + if (btf_ext_data) { > > + if (!obj->btf) { > > + pr_debug("Ignore ELF section %s because its depending ELF section %s is not found.\n", > > + BTF_EXT_ELF_SEC, BTF_ELF_SEC); > > + goto out; > > We will also return 0 when goto out here. > > > + } > > + obj->btf_ext = btf_ext__new(btf_ext_data->d_buf, > > + btf_ext_data->d_size); > > + if (IS_ERR(obj->btf_ext)) { > > + pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", > > + BTF_EXT_ELF_SEC, PTR_ERR(obj->btf_ext)); > > + obj->btf_ext = NULL; > > + goto out; > And, here. And we will not free obj->btf. > > > + } > > + bpf_object__sanitize_btf_ext(obj); > > + } > > +out: > > + if (err || IS_ERR(obj->btf)) { > > + if (!IS_ERR_OR_NULL(obj->btf)) > > + btf__free(obj->btf); > > + obj->btf = NULL; > > + } > > + return 0; > > +} > > + > > static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > { > > Elf *elf = obj->efile.elf; > > @@ -1102,24 +1154,21 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > if (gelf_getshdr(scn, &sh) != &sh) { > > pr_warning("failed to get section(%d) header from %s\n", > > idx, obj->path); > > - err = -LIBBPF_ERRNO__FORMAT; > > - goto out; > > + return -LIBBPF_ERRNO__FORMAT; > > } > > > > name = elf_strptr(elf, ep->e_shstrndx, sh.sh_name); > > if (!name) { > > pr_warning("failed to get section(%d) name from %s\n", > > idx, obj->path); > > - err = -LIBBPF_ERRNO__FORMAT; > > - goto out; > > + return -LIBBPF_ERRNO__FORMAT; > > } > > > > data = elf_getdata(scn, 0); > > if (!data) { > > pr_warning("failed to get section(%d) data from %s(%s)\n", > > idx, name, obj->path); > > - err = -LIBBPF_ERRNO__FORMAT; > > - goto out; > > + return -LIBBPF_ERRNO__FORMAT; > > } > > pr_debug("section(%d) %s, size %ld, link %d, flags %lx, type=%d\n", > > idx, name, (unsigned long)data->d_size, > > @@ -1130,10 +1179,14 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > err = bpf_object__init_license(obj, > > data->d_buf, > > data->d_size); > > + if (err) > > + return err; > > } else if (strcmp(name, "version") == 0) { > > err = bpf_object__init_kversion(obj, > > data->d_buf, > > data->d_size); > > + if (err) > > + return err; > > } else if (strcmp(name, "maps") == 0) { > > obj->efile.maps_shndx = idx; > > } else if (strcmp(name, BTF_ELF_SEC) == 0) { > > @@ -1144,11 +1197,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > if (obj->efile.symbols) { > > pr_warning("bpf: multiple SYMTAB in %s\n", > > obj->path); > > - err = -LIBBPF_ERRNO__FORMAT; > > - } else { > > - obj->efile.symbols = data; > > - obj->efile.strtabidx = sh.sh_link; > > + return -LIBBPF_ERRNO__FORMAT; > > } > > + obj->efile.symbols = data; > > + obj->efile.strtabidx = sh.sh_link; > > } else if (sh.sh_type == SHT_PROGBITS && data->d_size > 0) { > > if (sh.sh_flags & SHF_EXECINSTR) { > > if (strcmp(name, ".text") == 0) > > @@ -1162,6 +1214,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > > > pr_warning("failed to alloc program %s (%s): %s", > > name, obj->path, cp); > > + return err; > > } > > } else if (strcmp(name, ".data") == 0) { > > obj->efile.data = data; > > @@ -1173,8 +1226,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > pr_debug("skip section(%d) %s\n", idx, name); > > } > > } else if (sh.sh_type == SHT_REL) { > > + int nr_reloc = obj->efile.nr_reloc; > > void *reloc = obj->efile.reloc; > > - int nr_reloc = obj->efile.nr_reloc + 1; > > int sec = sh.sh_info; /* points to other section */ > > > > /* Only do relo for section with exec instructions */ > > @@ -1184,79 +1237,39 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > continue; > > } > > > > - reloc = reallocarray(reloc, nr_reloc, > > + reloc = reallocarray(reloc, nr_reloc + 1, > > sizeof(*obj->efile.reloc)); > > if (!reloc) { > > pr_warning("realloc failed\n"); > > - err = -ENOMEM; > > - } else { > > - int n = nr_reloc - 1; > > + return -ENOMEM; > > + } > > > > - obj->efile.reloc = reloc; > > - obj->efile.nr_reloc = nr_reloc; > > + obj->efile.reloc = reloc; > > + obj->efile.nr_reloc++; > > > > - obj->efile.reloc[n].shdr = sh; > > - obj->efile.reloc[n].data = data; > > - } > > + obj->efile.reloc[nr_reloc].shdr = sh; > > + obj->efile.reloc[nr_reloc].data = data; > > } else if (sh.sh_type == SHT_NOBITS && strcmp(name, ".bss") == 0) { > > obj->efile.bss = data; > > obj->efile.bss_shndx = idx; > > } else { > > pr_debug("skip section(%d) %s\n", idx, name); > > } > > - if (err) > > - goto out; > > } > > > > if (!obj->efile.strtabidx || obj->efile.strtabidx >= idx) { > > pr_warning("Corrupted ELF file: index of strtab invalid\n"); > > return -LIBBPF_ERRNO__FORMAT; > > } > > - if (btf_data) { > > - obj->btf = btf__new(btf_data->d_buf, btf_data->d_size); > > - if (IS_ERR(obj->btf)) { > > - pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", > > - BTF_ELF_SEC, PTR_ERR(obj->btf)); > > - obj->btf = NULL; > > - } else { > > - err = btf__finalize_data(obj, obj->btf); > > - if (!err) { > > - bpf_object__sanitize_btf(obj); > > - err = btf__load(obj->btf); > > - } > > - if (err) { > > - pr_warning("Error finalizing and loading %s into kernel: %d. Ignored and continue.\n", > > - BTF_ELF_SEC, err); > > - btf__free(obj->btf); > > - obj->btf = NULL; > > - err = 0; > > - } > > - } > > - } > > - if (btf_ext_data) { > > - if (!obj->btf) { > > - pr_debug("Ignore ELF section %s because its depending ELF section %s is not found.\n", > > - BTF_EXT_ELF_SEC, BTF_ELF_SEC); > > - } else { > > - obj->btf_ext = btf_ext__new(btf_ext_data->d_buf, > > - btf_ext_data->d_size); > > - if (IS_ERR(obj->btf_ext)) { > > - pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", > > - BTF_EXT_ELF_SEC, > > - PTR_ERR(obj->btf_ext)); > > - obj->btf_ext = NULL; > > - } else { > > - bpf_object__sanitize_btf_ext(obj); > > - } > > - } > > - } > > + err = bpf_object__load_btf(obj, btf_data, btf_ext_data); > > + if (err) > > + return err; > > if (bpf_object__has_maps(obj)) { > > err = bpf_object__init_maps(obj, flags); > > if (err) > > - goto out; > > + return err; > > } > > err = bpf_object__init_prog_names(obj); > > -out: > > return err; > > } > > > > -- > > 2.17.1 > >
On Sat, Jun 15, 2019 at 1:26 PM Song Liu <liu.song.a23@gmail.com> wrote: > > On Mon, Jun 10, 2019 at 9:49 PM Andrii Nakryiko <andriin@fb.com> wrote: > > > > As a preparation for adding BTF-based BPF map loading, extract .BTF and > > .BTF.ext loading logic. Also simplify error handling in > > bpf_object__elf_collect() by returning early, as there is no common > > clean up to be done. > > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > --- > > tools/lib/bpf/libbpf.c | 137 ++++++++++++++++++++++------------------- > > 1 file changed, 75 insertions(+), 62 deletions(-) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index ba89d9727137..9e39a0a33aeb 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -1078,6 +1078,58 @@ static void bpf_object__sanitize_btf_ext(struct bpf_object *obj) > > } > > } > > > > +static int bpf_object__load_btf(struct bpf_object *obj, > > + Elf_Data *btf_data, > > + Elf_Data *btf_ext_data) > > +{ > > + int err = 0; > > + > > + if (btf_data) { > > + obj->btf = btf__new(btf_data->d_buf, btf_data->d_size); > > + if (IS_ERR(obj->btf)) { > > + pr_warning("Error loading ELF section %s: %d.\n", > > + BTF_ELF_SEC, err); > > + goto out; > > If we goto out here, we will return 0. Yes, it's intentional. BTF is treated as optional, so if we fail to load it, libbpf will emit warning, but will proceed as nothing happened and no BTF was supposed to be loaded. > > > + } > > + err = btf__finalize_data(obj, obj->btf); > > + if (err) { > > + pr_warning("Error finalizing %s: %d.\n", > > + BTF_ELF_SEC, err); > > + goto out; > > + } > > + bpf_object__sanitize_btf(obj); > > + err = btf__load(obj->btf); > > + if (err) { > > + pr_warning("Error loading %s into kernel: %d.\n", > > + BTF_ELF_SEC, err); > > + goto out; > > + } > > + } > > + if (btf_ext_data) { > > + if (!obj->btf) { > > + pr_debug("Ignore ELF section %s because its depending ELF section %s is not found.\n", > > + BTF_EXT_ELF_SEC, BTF_ELF_SEC); > > + goto out; > > We will also return 0 when goto out here. See above, it's original behavior of libbpf. > > > + } > > + obj->btf_ext = btf_ext__new(btf_ext_data->d_buf, > > + btf_ext_data->d_size); > > + if (IS_ERR(obj->btf_ext)) { > > + pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", > > + BTF_EXT_ELF_SEC, PTR_ERR(obj->btf_ext)); > > + obj->btf_ext = NULL; > > + goto out; > And, here. And we will not free obj->btf. This is situation in which we successfully loaded .BTF, but failed to load .BTF.ext. In that case we'll warn about .BTF.ext, but will drop it and continue with .BTF only. > > > + } > > + bpf_object__sanitize_btf_ext(obj); > > + } > > +out: > > + if (err || IS_ERR(obj->btf)) { > > + if (!IS_ERR_OR_NULL(obj->btf)) > > + btf__free(obj->btf); > > + obj->btf = NULL; > > + } > > + return 0; > > +} > > + > > static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > { > > Elf *elf = obj->efile.elf; > > @@ -1102,24 +1154,21 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > if (gelf_getshdr(scn, &sh) != &sh) { > > pr_warning("failed to get section(%d) header from %s\n", > > idx, obj->path); > > - err = -LIBBPF_ERRNO__FORMAT; > > - goto out; > > + return -LIBBPF_ERRNO__FORMAT; > > } > > > > name = elf_strptr(elf, ep->e_shstrndx, sh.sh_name); > > if (!name) { > > pr_warning("failed to get section(%d) name from %s\n", > > idx, obj->path); > > - err = -LIBBPF_ERRNO__FORMAT; > > - goto out; > > + return -LIBBPF_ERRNO__FORMAT; > > } > > > > data = elf_getdata(scn, 0); > > if (!data) { > > pr_warning("failed to get section(%d) data from %s(%s)\n", > > idx, name, obj->path); > > - err = -LIBBPF_ERRNO__FORMAT; > > - goto out; > > + return -LIBBPF_ERRNO__FORMAT; > > } > > pr_debug("section(%d) %s, size %ld, link %d, flags %lx, type=%d\n", > > idx, name, (unsigned long)data->d_size, > > @@ -1130,10 +1179,14 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > err = bpf_object__init_license(obj, > > data->d_buf, > > data->d_size); > > + if (err) > > + return err; > > } else if (strcmp(name, "version") == 0) { > > err = bpf_object__init_kversion(obj, > > data->d_buf, > > data->d_size); > > + if (err) > > + return err; > > } else if (strcmp(name, "maps") == 0) { > > obj->efile.maps_shndx = idx; > > } else if (strcmp(name, BTF_ELF_SEC) == 0) { > > @@ -1144,11 +1197,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > if (obj->efile.symbols) { > > pr_warning("bpf: multiple SYMTAB in %s\n", > > obj->path); > > - err = -LIBBPF_ERRNO__FORMAT; > > - } else { > > - obj->efile.symbols = data; > > - obj->efile.strtabidx = sh.sh_link; > > + return -LIBBPF_ERRNO__FORMAT; > > } > > + obj->efile.symbols = data; > > + obj->efile.strtabidx = sh.sh_link; > > } else if (sh.sh_type == SHT_PROGBITS && data->d_size > 0) { > > if (sh.sh_flags & SHF_EXECINSTR) { > > if (strcmp(name, ".text") == 0) > > @@ -1162,6 +1214,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > > > pr_warning("failed to alloc program %s (%s): %s", > > name, obj->path, cp); > > + return err; > > } > > } else if (strcmp(name, ".data") == 0) { > > obj->efile.data = data; > > @@ -1173,8 +1226,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > pr_debug("skip section(%d) %s\n", idx, name); > > } > > } else if (sh.sh_type == SHT_REL) { > > + int nr_reloc = obj->efile.nr_reloc; > > void *reloc = obj->efile.reloc; > > - int nr_reloc = obj->efile.nr_reloc + 1; > > int sec = sh.sh_info; /* points to other section */ > > > > /* Only do relo for section with exec instructions */ > > @@ -1184,79 +1237,39 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) > > continue; > > } > > > > - reloc = reallocarray(reloc, nr_reloc, > > + reloc = reallocarray(reloc, nr_reloc + 1, > > sizeof(*obj->efile.reloc)); > > if (!reloc) { > > pr_warning("realloc failed\n"); > > - err = -ENOMEM; > > - } else { > > - int n = nr_reloc - 1; > > + return -ENOMEM; > > + } > > > > - obj->efile.reloc = reloc; > > - obj->efile.nr_reloc = nr_reloc; > > + obj->efile.reloc = reloc; > > + obj->efile.nr_reloc++; > > > > - obj->efile.reloc[n].shdr = sh; > > - obj->efile.reloc[n].data = data; > > - } > > + obj->efile.reloc[nr_reloc].shdr = sh; > > + obj->efile.reloc[nr_reloc].data = data; > > } else if (sh.sh_type == SHT_NOBITS && strcmp(name, ".bss") == 0) { > > obj->efile.bss = data; > > obj->efile.bss_shndx = idx; > > } else { > > pr_debug("skip section(%d) %s\n", idx, name); > > } > > - if (err) > > - goto out; > > } > > > > if (!obj->efile.strtabidx || obj->efile.strtabidx >= idx) { > > pr_warning("Corrupted ELF file: index of strtab invalid\n"); > > return -LIBBPF_ERRNO__FORMAT; > > } > > - if (btf_data) { > > - obj->btf = btf__new(btf_data->d_buf, btf_data->d_size); > > - if (IS_ERR(obj->btf)) { > > - pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", > > - BTF_ELF_SEC, PTR_ERR(obj->btf)); > > - obj->btf = NULL; > > - } else { > > - err = btf__finalize_data(obj, obj->btf); > > - if (!err) { > > - bpf_object__sanitize_btf(obj); > > - err = btf__load(obj->btf); > > - } > > - if (err) { > > - pr_warning("Error finalizing and loading %s into kernel: %d. Ignored and continue.\n", > > - BTF_ELF_SEC, err); > > - btf__free(obj->btf); > > - obj->btf = NULL; > > - err = 0; > > - } > > - } > > - } > > - if (btf_ext_data) { > > - if (!obj->btf) { > > - pr_debug("Ignore ELF section %s because its depending ELF section %s is not found.\n", > > - BTF_EXT_ELF_SEC, BTF_ELF_SEC); > > - } else { > > - obj->btf_ext = btf_ext__new(btf_ext_data->d_buf, > > - btf_ext_data->d_size); > > - if (IS_ERR(obj->btf_ext)) { > > - pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", > > - BTF_EXT_ELF_SEC, > > - PTR_ERR(obj->btf_ext)); > > - obj->btf_ext = NULL; > > - } else { > > - bpf_object__sanitize_btf_ext(obj); > > - } > > - } > > - } > > + err = bpf_object__load_btf(obj, btf_data, btf_ext_data); > > + if (err) > > + return err; > > if (bpf_object__has_maps(obj)) { > > err = bpf_object__init_maps(obj, flags); > > if (err) > > - goto out; > > + return err; > > } > > err = bpf_object__init_prog_names(obj); > > -out: > > return err; > > } > > > > -- > > 2.17.1 > >
On Sat, Jun 15, 2019 at 1:28 PM Song Liu <liu.song.a23@gmail.com> wrote: > > On Sat, Jun 15, 2019 at 1:25 PM Song Liu <liu.song.a23@gmail.com> wrote: > > > > On Mon, Jun 10, 2019 at 9:49 PM Andrii Nakryiko <andriin@fb.com> wrote: > > > > > > As a preparation for adding BTF-based BPF map loading, extract .BTF and > > > .BTF.ext loading logic. Also simplify error handling in > > > bpf_object__elf_collect() by returning early, as there is no common > > > clean up to be done. > > Maybe separate bpf_object__load_btf() and other cleanup in two patches? > (I won't complain if you keep them together) Split into two. > > Thanks, > Song > > > > > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > > --- > > > tools/lib/bpf/libbpf.c | 137 ++++++++++++++++++++++------------------- > > > 1 file changed, 75 insertions(+), 62 deletions(-) <snip>
> On Jun 17, 2019, at 10:24 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Sat, Jun 15, 2019 at 1:26 PM Song Liu <liu.song.a23@gmail.com> wrote: >> >> On Mon, Jun 10, 2019 at 9:49 PM Andrii Nakryiko <andriin@fb.com> wrote: >>> >>> As a preparation for adding BTF-based BPF map loading, extract .BTF and >>> .BTF.ext loading logic. Also simplify error handling in >>> bpf_object__elf_collect() by returning early, as there is no common >>> clean up to be done. >>> >>> Signed-off-by: Andrii Nakryiko <andriin@fb.com> >>> --- >>> tools/lib/bpf/libbpf.c | 137 ++++++++++++++++++++++------------------- >>> 1 file changed, 75 insertions(+), 62 deletions(-) >>> >>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >>> index ba89d9727137..9e39a0a33aeb 100644 >>> --- a/tools/lib/bpf/libbpf.c >>> +++ b/tools/lib/bpf/libbpf.c >>> @@ -1078,6 +1078,58 @@ static void bpf_object__sanitize_btf_ext(struct bpf_object *obj) >>> } >>> } >>> >>> +static int bpf_object__load_btf(struct bpf_object *obj, >>> + Elf_Data *btf_data, >>> + Elf_Data *btf_ext_data) >>> +{ >>> + int err = 0; >>> + >>> + if (btf_data) { >>> + obj->btf = btf__new(btf_data->d_buf, btf_data->d_size); >>> + if (IS_ERR(obj->btf)) { >>> + pr_warning("Error loading ELF section %s: %d.\n", >>> + BTF_ELF_SEC, err); >>> + goto out; >> >> If we goto out here, we will return 0. > > > Yes, it's intentional. BTF is treated as optional, so if we fail to > load it, libbpf will emit warning, but will proceed as nothing > happened and no BTF was supposed to be loaded. > >> >>> + } >>> + err = btf__finalize_data(obj, obj->btf); >>> + if (err) { >>> + pr_warning("Error finalizing %s: %d.\n", >>> + BTF_ELF_SEC, err); >>> + goto out; >>> + } >>> + bpf_object__sanitize_btf(obj); >>> + err = btf__load(obj->btf); >>> + if (err) { >>> + pr_warning("Error loading %s into kernel: %d.\n", >>> + BTF_ELF_SEC, err); >>> + goto out; >>> + } >>> + } >>> + if (btf_ext_data) { >>> + if (!obj->btf) { >>> + pr_debug("Ignore ELF section %s because its depending ELF section %s is not found.\n", >>> + BTF_EXT_ELF_SEC, BTF_ELF_SEC); >>> + goto out; >> >> We will also return 0 when goto out here. > > > See above, it's original behavior of libbpf. > >> >>> + } >>> + obj->btf_ext = btf_ext__new(btf_ext_data->d_buf, >>> + btf_ext_data->d_size); >>> + if (IS_ERR(obj->btf_ext)) { >>> + pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", >>> + BTF_EXT_ELF_SEC, PTR_ERR(obj->btf_ext)); >>> + obj->btf_ext = NULL; >>> + goto out; >> And, here. And we will not free obj->btf. > > This is situation in which we successfully loaded .BTF, but failed to > load .BTF.ext. In that case we'll warn about .BTF.ext, but will drop > it and continue with .BTF only. > Yeah, that makes sense. Shall we let bpf_object__load_btf() return void? Since it always returns 0? Thanks, Song <snip>
On Mon, Jun 17, 2019 at 11:07 AM Song Liu <songliubraving@fb.com> wrote: > > > > > On Jun 17, 2019, at 10:24 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > On Sat, Jun 15, 2019 at 1:26 PM Song Liu <liu.song.a23@gmail.com> wrote: > >> > >> On Mon, Jun 10, 2019 at 9:49 PM Andrii Nakryiko <andriin@fb.com> wrote: > >>> > >>> As a preparation for adding BTF-based BPF map loading, extract .BTF and > >>> .BTF.ext loading logic. Also simplify error handling in > >>> bpf_object__elf_collect() by returning early, as there is no common > >>> clean up to be done. > >>> > >>> Signed-off-by: Andrii Nakryiko <andriin@fb.com> > >>> --- > >>> tools/lib/bpf/libbpf.c | 137 ++++++++++++++++++++++------------------- > >>> 1 file changed, 75 insertions(+), 62 deletions(-) > >>> > >>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >>> index ba89d9727137..9e39a0a33aeb 100644 > >>> --- a/tools/lib/bpf/libbpf.c > >>> +++ b/tools/lib/bpf/libbpf.c > >>> @@ -1078,6 +1078,58 @@ static void bpf_object__sanitize_btf_ext(struct bpf_object *obj) > >>> } > >>> } > >>> > >>> +static int bpf_object__load_btf(struct bpf_object *obj, > >>> + Elf_Data *btf_data, > >>> + Elf_Data *btf_ext_data) > >>> +{ > >>> + int err = 0; > >>> + > >>> + if (btf_data) { > >>> + obj->btf = btf__new(btf_data->d_buf, btf_data->d_size); > >>> + if (IS_ERR(obj->btf)) { > >>> + pr_warning("Error loading ELF section %s: %d.\n", > >>> + BTF_ELF_SEC, err); > >>> + goto out; > >> > >> If we goto out here, we will return 0. > > > > > > Yes, it's intentional. BTF is treated as optional, so if we fail to > > load it, libbpf will emit warning, but will proceed as nothing > > happened and no BTF was supposed to be loaded. > > > >> > >>> + } > >>> + err = btf__finalize_data(obj, obj->btf); > >>> + if (err) { > >>> + pr_warning("Error finalizing %s: %d.\n", > >>> + BTF_ELF_SEC, err); > >>> + goto out; > >>> + } > >>> + bpf_object__sanitize_btf(obj); > >>> + err = btf__load(obj->btf); > >>> + if (err) { > >>> + pr_warning("Error loading %s into kernel: %d.\n", > >>> + BTF_ELF_SEC, err); > >>> + goto out; > >>> + } > >>> + } > >>> + if (btf_ext_data) { > >>> + if (!obj->btf) { > >>> + pr_debug("Ignore ELF section %s because its depending ELF section %s is not found.\n", > >>> + BTF_EXT_ELF_SEC, BTF_ELF_SEC); > >>> + goto out; > >> > >> We will also return 0 when goto out here. > > > > > > See above, it's original behavior of libbpf. > > > >> > >>> + } > >>> + obj->btf_ext = btf_ext__new(btf_ext_data->d_buf, > >>> + btf_ext_data->d_size); > >>> + if (IS_ERR(obj->btf_ext)) { > >>> + pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", > >>> + BTF_EXT_ELF_SEC, PTR_ERR(obj->btf_ext)); > >>> + obj->btf_ext = NULL; > >>> + goto out; > >> And, here. And we will not free obj->btf. > > > > This is situation in which we successfully loaded .BTF, but failed to > > load .BTF.ext. In that case we'll warn about .BTF.ext, but will drop > > it and continue with .BTF only. > > > > Yeah, that makes sense. > > Shall we let bpf_object__load_btf() return void? Since it always > returns 0? This is split into bpf_object__init_btf and bpf_object__sanitize_and_load_btf in patch #6, both of which can return errors. So probably not worth changing just for one patch. > > Thanks, > Song > > <snip> >
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index ba89d9727137..9e39a0a33aeb 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -1078,6 +1078,58 @@ static void bpf_object__sanitize_btf_ext(struct bpf_object *obj) } } +static int bpf_object__load_btf(struct bpf_object *obj, + Elf_Data *btf_data, + Elf_Data *btf_ext_data) +{ + int err = 0; + + if (btf_data) { + obj->btf = btf__new(btf_data->d_buf, btf_data->d_size); + if (IS_ERR(obj->btf)) { + pr_warning("Error loading ELF section %s: %d.\n", + BTF_ELF_SEC, err); + goto out; + } + err = btf__finalize_data(obj, obj->btf); + if (err) { + pr_warning("Error finalizing %s: %d.\n", + BTF_ELF_SEC, err); + goto out; + } + bpf_object__sanitize_btf(obj); + err = btf__load(obj->btf); + if (err) { + pr_warning("Error loading %s into kernel: %d.\n", + BTF_ELF_SEC, err); + goto out; + } + } + if (btf_ext_data) { + if (!obj->btf) { + pr_debug("Ignore ELF section %s because its depending ELF section %s is not found.\n", + BTF_EXT_ELF_SEC, BTF_ELF_SEC); + goto out; + } + obj->btf_ext = btf_ext__new(btf_ext_data->d_buf, + btf_ext_data->d_size); + if (IS_ERR(obj->btf_ext)) { + pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", + BTF_EXT_ELF_SEC, PTR_ERR(obj->btf_ext)); + obj->btf_ext = NULL; + goto out; + } + bpf_object__sanitize_btf_ext(obj); + } +out: + if (err || IS_ERR(obj->btf)) { + if (!IS_ERR_OR_NULL(obj->btf)) + btf__free(obj->btf); + obj->btf = NULL; + } + return 0; +} + static int bpf_object__elf_collect(struct bpf_object *obj, int flags) { Elf *elf = obj->efile.elf; @@ -1102,24 +1154,21 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) if (gelf_getshdr(scn, &sh) != &sh) { pr_warning("failed to get section(%d) header from %s\n", idx, obj->path); - err = -LIBBPF_ERRNO__FORMAT; - goto out; + return -LIBBPF_ERRNO__FORMAT; } name = elf_strptr(elf, ep->e_shstrndx, sh.sh_name); if (!name) { pr_warning("failed to get section(%d) name from %s\n", idx, obj->path); - err = -LIBBPF_ERRNO__FORMAT; - goto out; + return -LIBBPF_ERRNO__FORMAT; } data = elf_getdata(scn, 0); if (!data) { pr_warning("failed to get section(%d) data from %s(%s)\n", idx, name, obj->path); - err = -LIBBPF_ERRNO__FORMAT; - goto out; + return -LIBBPF_ERRNO__FORMAT; } pr_debug("section(%d) %s, size %ld, link %d, flags %lx, type=%d\n", idx, name, (unsigned long)data->d_size, @@ -1130,10 +1179,14 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) err = bpf_object__init_license(obj, data->d_buf, data->d_size); + if (err) + return err; } else if (strcmp(name, "version") == 0) { err = bpf_object__init_kversion(obj, data->d_buf, data->d_size); + if (err) + return err; } else if (strcmp(name, "maps") == 0) { obj->efile.maps_shndx = idx; } else if (strcmp(name, BTF_ELF_SEC) == 0) { @@ -1144,11 +1197,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) if (obj->efile.symbols) { pr_warning("bpf: multiple SYMTAB in %s\n", obj->path); - err = -LIBBPF_ERRNO__FORMAT; - } else { - obj->efile.symbols = data; - obj->efile.strtabidx = sh.sh_link; + return -LIBBPF_ERRNO__FORMAT; } + obj->efile.symbols = data; + obj->efile.strtabidx = sh.sh_link; } else if (sh.sh_type == SHT_PROGBITS && data->d_size > 0) { if (sh.sh_flags & SHF_EXECINSTR) { if (strcmp(name, ".text") == 0) @@ -1162,6 +1214,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) pr_warning("failed to alloc program %s (%s): %s", name, obj->path, cp); + return err; } } else if (strcmp(name, ".data") == 0) { obj->efile.data = data; @@ -1173,8 +1226,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) pr_debug("skip section(%d) %s\n", idx, name); } } else if (sh.sh_type == SHT_REL) { + int nr_reloc = obj->efile.nr_reloc; void *reloc = obj->efile.reloc; - int nr_reloc = obj->efile.nr_reloc + 1; int sec = sh.sh_info; /* points to other section */ /* Only do relo for section with exec instructions */ @@ -1184,79 +1237,39 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags) continue; } - reloc = reallocarray(reloc, nr_reloc, + reloc = reallocarray(reloc, nr_reloc + 1, sizeof(*obj->efile.reloc)); if (!reloc) { pr_warning("realloc failed\n"); - err = -ENOMEM; - } else { - int n = nr_reloc - 1; + return -ENOMEM; + } - obj->efile.reloc = reloc; - obj->efile.nr_reloc = nr_reloc; + obj->efile.reloc = reloc; + obj->efile.nr_reloc++; - obj->efile.reloc[n].shdr = sh; - obj->efile.reloc[n].data = data; - } + obj->efile.reloc[nr_reloc].shdr = sh; + obj->efile.reloc[nr_reloc].data = data; } else if (sh.sh_type == SHT_NOBITS && strcmp(name, ".bss") == 0) { obj->efile.bss = data; obj->efile.bss_shndx = idx; } else { pr_debug("skip section(%d) %s\n", idx, name); } - if (err) - goto out; } if (!obj->efile.strtabidx || obj->efile.strtabidx >= idx) { pr_warning("Corrupted ELF file: index of strtab invalid\n"); return -LIBBPF_ERRNO__FORMAT; } - if (btf_data) { - obj->btf = btf__new(btf_data->d_buf, btf_data->d_size); - if (IS_ERR(obj->btf)) { - pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", - BTF_ELF_SEC, PTR_ERR(obj->btf)); - obj->btf = NULL; - } else { - err = btf__finalize_data(obj, obj->btf); - if (!err) { - bpf_object__sanitize_btf(obj); - err = btf__load(obj->btf); - } - if (err) { - pr_warning("Error finalizing and loading %s into kernel: %d. Ignored and continue.\n", - BTF_ELF_SEC, err); - btf__free(obj->btf); - obj->btf = NULL; - err = 0; - } - } - } - if (btf_ext_data) { - if (!obj->btf) { - pr_debug("Ignore ELF section %s because its depending ELF section %s is not found.\n", - BTF_EXT_ELF_SEC, BTF_ELF_SEC); - } else { - obj->btf_ext = btf_ext__new(btf_ext_data->d_buf, - btf_ext_data->d_size); - if (IS_ERR(obj->btf_ext)) { - pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", - BTF_EXT_ELF_SEC, - PTR_ERR(obj->btf_ext)); - obj->btf_ext = NULL; - } else { - bpf_object__sanitize_btf_ext(obj); - } - } - } + err = bpf_object__load_btf(obj, btf_data, btf_ext_data); + if (err) + return err; if (bpf_object__has_maps(obj)) { err = bpf_object__init_maps(obj, flags); if (err) - goto out; + return err; } err = bpf_object__init_prog_names(obj); -out: return err; }
As a preparation for adding BTF-based BPF map loading, extract .BTF and .BTF.ext loading logic. Also simplify error handling in bpf_object__elf_collect() by returning early, as there is no common clean up to be done. Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- tools/lib/bpf/libbpf.c | 137 ++++++++++++++++++++++------------------- 1 file changed, 75 insertions(+), 62 deletions(-)