[PATCH v2 bpf-next 0/2] tools/btf: extends libbpf APIs to work with btf w/o kernel

[PATCH v2 bpf-next 0/2] tools/btf: extends libbpf APIs to work with btf w/o kernel

[Andrii Nakryiko]

This patchset changes existing btf__new() API call to only load and initialize
struct btf, while exposing new btf__load() API to attempt to load and validate
BTF in kernel. It also adds ability to copy raw BTF data out of struct btf for
further processing by external applications.

This makes utilizing libbpf's APIs that don't require kernel facilities (e.g.,
btf_dedup) simpler and more natural from external application.

v1->v2:
- btf_load() returns just error, not fd
- fix ordering in libbpf.map

Andrii Nakryiko (2):
  btf: separate btf creation and loading
  btf: expose API to work with raw btf data

 tools/lib/bpf/btf.c      | 63 +++++++++++++++++++++++++---------------
 tools/lib/bpf/btf.h      |  3 ++
 tools/lib/bpf/libbpf.c   |  2 +-
 tools/lib/bpf/libbpf.map |  3 ++
 4 files changed, 46 insertions(+), 25 deletions(-)

-- 
2.17.1

[PATCH v2 bpf-next 1/2] btf: separate btf creation and loading

[Andrii Nakryiko]

This change splits out previous btf__new functionality of constructing
struct btf and loading it into kernel into two:
- btf__new() just creates and initializes struct btf
- btf__load() attempts to load existing struct btf into kernel

btf__free will still close BTF fd, if it was ever loaded successfully
into kernel.

This change allows users of libbpf to manipulate BTF using its API,
without the need to unnecessarily load it into kernel.

One of the intended use cases is pahole using libbpf to do DWARF to BTF
conversion and deduplication using libbpf, while handling ELF sections
overwrites and other concerns on its own.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Acked-by: Song Liu <songliubraving@fb.com>
---
 tools/lib/bpf/btf.c      | 53 ++++++++++++++++++++++------------------
 tools/lib/bpf/btf.h      |  1 +
 tools/lib/bpf/libbpf.c   |  2 +-
 tools/lib/bpf/libbpf.map |  1 +
 4 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 4949f8840bda..1c2ba7182400 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -366,8 +366,6 @@ void btf__free(struct btf *btf)
 
 struct btf *btf__new(__u8 *data, __u32 size)
 {
-	__u32 log_buf_size = 0;
-	char *log_buf = NULL;
 	struct btf *btf;
 	int err;
 
@@ -377,15 +375,6 @@ struct btf *btf__new(__u8 *data, __u32 size)
 
 	btf->fd = -1;
 
-	log_buf = malloc(BPF_LOG_BUF_SIZE);
-	if (!log_buf) {
-		err = -ENOMEM;
-		goto done;
-	}
-
-	*log_buf = 0;
-	log_buf_size = BPF_LOG_BUF_SIZE;
-
 	btf->data = malloc(size);
 	if (!btf->data) {
 		err = -ENOMEM;
@@ -395,17 +384,6 @@ struct btf *btf__new(__u8 *data, __u32 size)
 	memcpy(btf->data, data, size);
 	btf->data_size = size;
 
-	btf->fd = bpf_load_btf(btf->data, btf->data_size,
-			       log_buf, log_buf_size, false);
-
-	if (btf->fd == -1) {
-		err = -errno;
-		pr_warning("Error loading BTF: %s(%d)\n", strerror(errno), errno);
-		if (log_buf && *log_buf)
-			pr_warning("%s\n", log_buf);
-		goto done;
-	}
-
 	err = btf_parse_hdr(btf);
 	if (err)
 		goto done;
@@ -417,8 +395,6 @@ struct btf *btf__new(__u8 *data, __u32 size)
 	err = btf_parse_type_sec(btf);
 
 done:
-	free(log_buf);
-
 	if (err) {
 		btf__free(btf);
 		return ERR_PTR(err);
@@ -427,6 +403,35 @@ struct btf *btf__new(__u8 *data, __u32 size)
 	return btf;
 }
 
+int btf__load(struct btf* btf) {
+	__u32 log_buf_size = BPF_LOG_BUF_SIZE;
+	char *log_buf = NULL;
+	int err = 0;
+
+	if (btf->fd >= 0)
+		return -EEXIST;
+
+	log_buf = malloc(log_buf_size);
+	if (!log_buf)
+		return -ENOMEM;
+
+	*log_buf = 0;
+
+	btf->fd = bpf_load_btf(btf->data, btf->data_size,
+			       log_buf, log_buf_size, false);
+	if (btf->fd < 0) {
+		err = -errno;
+		pr_warning("Error loading BTF: %s(%d)\n", strerror(errno), errno);
+		if (*log_buf)
+			pr_warning("%s\n", log_buf);
+		goto done;
+	}
+
+done:
+	free(log_buf);
+	return err;
+}
+
 int btf__fd(const struct btf *btf)
 {
 	return btf->fd;
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 25a9d2db035d..e8410887f93a 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -57,6 +57,7 @@ struct btf_ext_header {
 
 LIBBPF_API void btf__free(struct btf *btf);
 LIBBPF_API struct btf *btf__new(__u8 *data, __u32 size);
+LIBBPF_API int btf__load(struct btf* btf);
 LIBBPF_API __s32 btf__find_by_name(const struct btf *btf,
 				   const char *type_name);
 LIBBPF_API __u32 btf__get_nr_types(const struct btf *btf);
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 47969aa0faf8..ff86a43a4336 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -835,7 +835,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
 			obj->efile.maps_shndx = idx;
 		else if (strcmp(name, BTF_ELF_SEC) == 0) {
 			obj->btf = btf__new(data->d_buf, data->d_size);
-			if (IS_ERR(obj->btf)) {
+			if (IS_ERR(obj->btf) || btf__load(obj->btf)) {
 				pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n",
 					   BTF_ELF_SEC, PTR_ERR(obj->btf));
 				obj->btf = NULL;
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 89c1149e32ee..f5372df143f4 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -137,6 +137,7 @@ LIBBPF_0.0.2 {
 		btf__get_map_kv_tids;
 		btf__get_nr_types;
 		btf__get_strings;
+		btf__load;
 		btf_ext__free;
 		btf_ext__func_info_rec_size;
 		btf_ext__line_info_rec_size;
-- 
2.17.1

Re: [PATCH v2 bpf-next 1/2] btf: separate btf creation and loading

[Alexei Starovoitov]

On Tue, Feb 05, 2019 at 04:29:48PM -0800, Andrii Nakryiko wrote:
> This change splits out previous btf__new functionality of constructing
> struct btf and loading it into kernel into two:
> - btf__new() just creates and initializes struct btf
> - btf__load() attempts to load existing struct btf into kernel
> 
> btf__free will still close BTF fd, if it was ever loaded successfully
> into kernel.
> 
> This change allows users of libbpf to manipulate BTF using its API,
> without the need to unnecessarily load it into kernel.
> 
> One of the intended use cases is pahole using libbpf to do DWARF to BTF
> conversion and deduplication using libbpf, while handling ELF sections
> overwrites and other concerns on its own.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> Acked-by: Song Liu <songliubraving@fb.com>

should it be 
Fixes: 2d3feca8c44f ("bpf: btf: print map dump and lookup with btf info")
?
Even back then btf__new() was doing the loading.
So that btf support in bpftool was silently double loading btf.

Re: [PATCH v2 bpf-next 1/2] btf: separate btf creation and loading

[Andrii Nakryiko]

On Tue, Feb 5, 2019 at 7:03 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Feb 05, 2019 at 04:29:48PM -0800, Andrii Nakryiko wrote:
> > This change splits out previous btf__new functionality of constructing
> > struct btf and loading it into kernel into two:
> > - btf__new() just creates and initializes struct btf
> > - btf__load() attempts to load existing struct btf into kernel
> >
> > btf__free will still close BTF fd, if it was ever loaded successfully
> > into kernel.
> >
> > This change allows users of libbpf to manipulate BTF using its API,
> > without the need to unnecessarily load it into kernel.
> >
> > One of the intended use cases is pahole using libbpf to do DWARF to BTF
> > conversion and deduplication using libbpf, while handling ELF sections
> > overwrites and other concerns on its own.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > Acked-by: Song Liu <songliubraving@fb.com>
>
> should it be
> Fixes: 2d3feca8c44f ("bpf: btf: print map dump and lookup with btf info")
> ?
> Even back then btf__new() was doing the loading.
> So that btf support in bpftool was silently double loading btf.
>

ok, will add that to commit message

[PATCH v2 bpf-next 2/2] btf: expose API to work with raw btf data

[Andrii Nakryiko]

This patch exposes two new APIs btf__get_raw_data_size() and
btf__get_raw_data() that allows to get a copy of raw BTF data out of
struct btf. This is useful for external programs that need to manipulate
raw data, e.g., pahole using btf__dedup() to deduplicate BTF type info
and then writing it back to file.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Acked-by: Song Liu <songliubraving@fb.com>
---
 tools/lib/bpf/btf.c      | 10 ++++++++++
 tools/lib/bpf/btf.h      |  2 ++
 tools/lib/bpf/libbpf.map |  2 ++
 3 files changed, 14 insertions(+)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 1c2ba7182400..34bfb3641aac 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -437,6 +437,16 @@ int btf__fd(const struct btf *btf)
 	return btf->fd;
 }
 
+__u32 btf__get_raw_data_size(const struct btf *btf)
+{
+	return btf->data_size;
+}
+
+void btf__get_raw_data(const struct btf *btf, char *data)
+{
+	memcpy(data, btf->data, btf->data_size);
+}
+
 void btf__get_strings(const struct btf *btf, const char **strings,
 		      __u32 *str_len)
 {
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index e8410887f93a..d46f680b9416 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -66,6 +66,8 @@ LIBBPF_API const struct btf_type *btf__type_by_id(const struct btf *btf,
 LIBBPF_API __s64 btf__resolve_size(const struct btf *btf, __u32 type_id);
 LIBBPF_API int btf__resolve_type(const struct btf *btf, __u32 type_id);
 LIBBPF_API int btf__fd(const struct btf *btf);
+LIBBPF_API __u32 btf__get_raw_data_size(const struct btf *btf);
+LIBBPF_API void btf__get_raw_data(const struct btf *btf, char *data);
 LIBBPF_API void btf__get_strings(const struct btf *btf, const char **strings,
 				 __u32 *str_len);
 LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index f5372df143f4..0ebbee13a3cd 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -136,6 +136,8 @@ LIBBPF_0.0.2 {
 		btf__dedup;
 		btf__get_map_kv_tids;
 		btf__get_nr_types;
+		btf__get_raw_data;
+		btf__get_raw_data_size;
 		btf__get_strings;
 		btf__load;
 		btf_ext__free;
-- 
2.17.1

Re: [PATCH v2 bpf-next 2/2] btf: expose API to work with raw btf data

[Alexei Starovoitov]

On Tue, Feb 05, 2019 at 04:29:49PM -0800, Andrii Nakryiko wrote:
> This patch exposes two new APIs btf__get_raw_data_size() and
> btf__get_raw_data() that allows to get a copy of raw BTF data out of
> struct btf. This is useful for external programs that need to manipulate
> raw data, e.g., pahole using btf__dedup() to deduplicate BTF type info
> and then writing it back to file.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> Acked-by: Song Liu <songliubraving@fb.com>
> ---
>  tools/lib/bpf/btf.c      | 10 ++++++++++
>  tools/lib/bpf/btf.h      |  2 ++
>  tools/lib/bpf/libbpf.map |  2 ++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 1c2ba7182400..34bfb3641aac 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -437,6 +437,16 @@ int btf__fd(const struct btf *btf)
>  	return btf->fd;
>  }
>  
> +__u32 btf__get_raw_data_size(const struct btf *btf)
> +{
> +	return btf->data_size;
> +}
> +
> +void btf__get_raw_data(const struct btf *btf, char *data)
> +{
> +	memcpy(data, btf->data, btf->data_size);
> +}

I cannot think of any other way to use this api,
but to call btf__get_raw_data_size() first,
then malloc that much memory and then call btf__get_raw_data()
to store btf into it.

If so, may be api should be single call that allocates, copies,
and returns pointer to new mem and its size?
Probably less error prone?

Re: [PATCH v2 bpf-next 2/2] btf: expose API to work with raw btf data

[Andrii Nakryiko]

On Tue, Feb 5, 2019 at 7:07 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Feb 05, 2019 at 04:29:49PM -0800, Andrii Nakryiko wrote:
> > This patch exposes two new APIs btf__get_raw_data_size() and
> > btf__get_raw_data() that allows to get a copy of raw BTF data out of
> > struct btf. This is useful for external programs that need to manipulate
> > raw data, e.g., pahole using btf__dedup() to deduplicate BTF type info
> > and then writing it back to file.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > Acked-by: Song Liu <songliubraving@fb.com>
> > ---
> >  tools/lib/bpf/btf.c      | 10 ++++++++++
> >  tools/lib/bpf/btf.h      |  2 ++
> >  tools/lib/bpf/libbpf.map |  2 ++
> >  3 files changed, 14 insertions(+)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index 1c2ba7182400..34bfb3641aac 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -437,6 +437,16 @@ int btf__fd(const struct btf *btf)
> >       return btf->fd;
> >  }
> >
> > +__u32 btf__get_raw_data_size(const struct btf *btf)
> > +{
> > +     return btf->data_size;
> > +}
> > +
> > +void btf__get_raw_data(const struct btf *btf, char *data)
> > +{
> > +     memcpy(data, btf->data, btf->data_size);
> > +}
>
> I cannot think of any other way to use this api,
> but to call btf__get_raw_data_size() first,
> then malloc that much memory and then call btf__get_raw_data()
> to store btf into it.
>
> If so, may be api should be single call that allocates, copies,
> and returns pointer to new mem and its size?
> Probably less error prone?
>

I don't have strong preference, but providing pointer to allocated memory
seems more flexible and allows more clever/optimal use of memory from caller
side. E.g., instead of doing two mallocs, you can imagine doing something
like:

int max_size = max(btf__get_raw_data_size(btf),
                   btf_ext__get_raw_data_size(btf_ext));
char *m = malloc(max_size);
btf__get_raw_data(btf, m);
dump_btf_section_to_file(m, some_file);
btf_ext__get_raw_data(btf_ext, m);
dump_btf_ext_section_to_file(m, some_file);
free(m);

Also, pointer to memory could be mmap()'ed file, for instance. In general,
for a library it might be a good thing to not be prescriptive as to how one
gets that piece of memory.

If those examples are not convincing enough, I'm happy to go with single
btf__get_raw_data() call doing allocation and returning pointer.

Re: [PATCH v2 bpf-next 2/2] btf: expose API to work with raw btf data

[Alexei Starovoitov]

On Tue, Feb 05, 2019 at 09:46:14PM -0800, Andrii Nakryiko wrote:
> On Tue, Feb 5, 2019 at 7:07 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Feb 05, 2019 at 04:29:49PM -0800, Andrii Nakryiko wrote:
> > > This patch exposes two new APIs btf__get_raw_data_size() and
> > > btf__get_raw_data() that allows to get a copy of raw BTF data out of
> > > struct btf. This is useful for external programs that need to manipulate
> > > raw data, e.g., pahole using btf__dedup() to deduplicate BTF type info
> > > and then writing it back to file.
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > Acked-by: Song Liu <songliubraving@fb.com>
> > > ---
> > >  tools/lib/bpf/btf.c      | 10 ++++++++++
> > >  tools/lib/bpf/btf.h      |  2 ++
> > >  tools/lib/bpf/libbpf.map |  2 ++
> > >  3 files changed, 14 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > > index 1c2ba7182400..34bfb3641aac 100644
> > > --- a/tools/lib/bpf/btf.c
> > > +++ b/tools/lib/bpf/btf.c
> > > @@ -437,6 +437,16 @@ int btf__fd(const struct btf *btf)
> > >       return btf->fd;
> > >  }
> > >
> > > +__u32 btf__get_raw_data_size(const struct btf *btf)
> > > +{
> > > +     return btf->data_size;
> > > +}
> > > +
> > > +void btf__get_raw_data(const struct btf *btf, char *data)
> > > +{
> > > +     memcpy(data, btf->data, btf->data_size);
> > > +}
> >
> > I cannot think of any other way to use this api,
> > but to call btf__get_raw_data_size() first,
> > then malloc that much memory and then call btf__get_raw_data()
> > to store btf into it.
> >
> > If so, may be api should be single call that allocates, copies,
> > and returns pointer to new mem and its size?
> > Probably less error prone?
> >
> 
> I don't have strong preference, but providing pointer to allocated memory
> seems more flexible and allows more clever/optimal use of memory from caller
> side. E.g., instead of doing two mallocs, you can imagine doing something
> like:
> 
> int max_size = max(btf__get_raw_data_size(btf),
>                    btf_ext__get_raw_data_size(btf_ext));
> char *m = malloc(max_size);
> btf__get_raw_data(btf, m);
> dump_btf_section_to_file(m, some_file);
> btf_ext__get_raw_data(btf_ext, m);
> dump_btf_ext_section_to_file(m, some_file);
> free(m);
> 
> Also, pointer to memory could be mmap()'ed file, for instance. In general,
> for a library it might be a good thing to not be prescriptive as to how one
> gets that piece of memory.

Plausible, but I'd like to see pahole patches to be convinced ;)

Re: [PATCH v2 bpf-next 2/2] btf: expose API to work with raw btf data

[Andrii Nakryiko]

On Tue, Feb 5, 2019 at 10:25 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Feb 05, 2019 at 09:46:14PM -0800, Andrii Nakryiko wrote:
> > On Tue, Feb 5, 2019 at 7:07 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Tue, Feb 05, 2019 at 04:29:49PM -0800, Andrii Nakryiko wrote:
> > > > This patch exposes two new APIs btf__get_raw_data_size() and
> > > > btf__get_raw_data() that allows to get a copy of raw BTF data out of
> > > > struct btf. This is useful for external programs that need to manipulate
> > > > raw data, e.g., pahole using btf__dedup() to deduplicate BTF type info
> > > > and then writing it back to file.
> > > >
> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > > Acked-by: Song Liu <songliubraving@fb.com>
> > > > ---
> > > >  tools/lib/bpf/btf.c      | 10 ++++++++++
> > > >  tools/lib/bpf/btf.h      |  2 ++
> > > >  tools/lib/bpf/libbpf.map |  2 ++
> > > >  3 files changed, 14 insertions(+)
> > > >
> > > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > > > index 1c2ba7182400..34bfb3641aac 100644
> > > > --- a/tools/lib/bpf/btf.c
> > > > +++ b/tools/lib/bpf/btf.c
> > > > @@ -437,6 +437,16 @@ int btf__fd(const struct btf *btf)
> > > >       return btf->fd;
> > > >  }
> > > >
> > > > +__u32 btf__get_raw_data_size(const struct btf *btf)
> > > > +{
> > > > +     return btf->data_size;
> > > > +}
> > > > +
> > > > +void btf__get_raw_data(const struct btf *btf, char *data)
> > > > +{
> > > > +     memcpy(data, btf->data, btf->data_size);
> > > > +}
> > >
> > > I cannot think of any other way to use this api,
> > > but to call btf__get_raw_data_size() first,
> > > then malloc that much memory and then call btf__get_raw_data()
> > > to store btf into it.
> > >
> > > If so, may be api should be single call that allocates, copies,
> > > and returns pointer to new mem and its size?
> > > Probably less error prone?
> > >
> >
> > I don't have strong preference, but providing pointer to allocated memory
> > seems more flexible and allows more clever/optimal use of memory from caller
> > side. E.g., instead of doing two mallocs, you can imagine doing something
> > like:
> >
> > int max_size = max(btf__get_raw_data_size(btf),
> >                    btf_ext__get_raw_data_size(btf_ext));
> > char *m = malloc(max_size);
> > btf__get_raw_data(btf, m);
> > dump_btf_section_to_file(m, some_file);
> > btf_ext__get_raw_data(btf_ext, m);
> > dump_btf_ext_section_to_file(m, some_file);
> > free(m);
> >
> > Also, pointer to memory could be mmap()'ed file, for instance. In general,
> > for a library it might be a good thing to not be prescriptive as to how one
> > gets that piece of memory.
>
> Plausible, but I'd like to see pahole patches to be convinced ;)
>

Here's a summary of proposed ways to expose raw data through new api,
with pros/cons.

1. Originally proposed two functions. `int btf__get_raw_data_size()`
to get size, `void btf__get_raw_data(void* buf)` to write raw data to
a provided buf.

Pros:
  - allows maximal flexibility for users of this API. They can manage
memory as it's convenient for them (e.g., reusing same buffer for
multiple btf and btf_ext raw data).
  - allows using mmap()'ed memory, as allocation and memory ownership
is delegated to user

Cons:
  - has potential of buffer overflows, if user doesn't provide big enough buffer


2. Alexei's proposal to combine getting size in single function that
internally allocates new memory buffer, copies data and returns it to
users to use and later free.

Pros:
  - one less API function
  - more straightforward usage, it's hard to misuse it (except for
memory leaking, if memory is not freed)

Cons:
  - always allocated for each call
  - least flexible approach, doesn't allow caller to manage memory,
prevents any kind of direct write to mmap()'ed file

3. Daniel proposed realloc-like approach, where caller optionally
provides memory buffer, but we always call realloc() internally to
ensure we have long enough buffer.

Pros:
  - allows callers to provide their memory buffer (similar to approach
#1, but see cons below)
  - prevents user error with providing too small buffer (similar to approach #2)

Cons:
  - realloc expects that memory was allocated by previous malloc()
call, so caller can't allocate bigger chunk of memory and provide
pointer inside that area (behavior is undefined in that case). This
requirement is not immediately obvious, so this approach feels more
error prone than either of approach #1 and #2
  - still doesn't allow mmap()'ed usage, again due to realloc()'s requirements


Approach #3 looks most subtly-error-prone, as it's too easy to just
provide pointer that's not at the beginning of malloc()'ed memory, but
this might not be detected immediately, and could potentially lead to
silent memory corruption.

I'd still go with approach #1 as it provides least restrictive API,
even though approach #2 will provide marginally better usability for
common cases.

Alexei, Daniel, which approach you'd prefer in the end after
considering all pros and cons?

Re: [PATCH v2 bpf-next 2/2] btf: expose API to work with raw btf data

[Andrii Nakryiko]

On Thu, Feb 7, 2019 at 11:21 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Feb 5, 2019 at 10:25 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Feb 05, 2019 at 09:46:14PM -0800, Andrii Nakryiko wrote:
> > > On Tue, Feb 5, 2019 at 7:07 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Tue, Feb 05, 2019 at 04:29:49PM -0800, Andrii Nakryiko wrote:
> > > > > This patch exposes two new APIs btf__get_raw_data_size() and
> > > > > btf__get_raw_data() that allows to get a copy of raw BTF data out of
> > > > > struct btf. This is useful for external programs that need to manipulate
> > > > > raw data, e.g., pahole using btf__dedup() to deduplicate BTF type info
> > > > > and then writing it back to file.
> > > > >
> > > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > > > Acked-by: Song Liu <songliubraving@fb.com>
> > > > > ---
> > > > >  tools/lib/bpf/btf.c      | 10 ++++++++++
> > > > >  tools/lib/bpf/btf.h      |  2 ++
> > > > >  tools/lib/bpf/libbpf.map |  2 ++
> > > > >  3 files changed, 14 insertions(+)
> > > > >
> > > > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > > > > index 1c2ba7182400..34bfb3641aac 100644
> > > > > --- a/tools/lib/bpf/btf.c
> > > > > +++ b/tools/lib/bpf/btf.c
> > > > > @@ -437,6 +437,16 @@ int btf__fd(const struct btf *btf)
> > > > >       return btf->fd;
> > > > >  }
> > > > >
> > > > > +__u32 btf__get_raw_data_size(const struct btf *btf)
> > > > > +{
> > > > > +     return btf->data_size;
> > > > > +}
> > > > > +
> > > > > +void btf__get_raw_data(const struct btf *btf, char *data)
> > > > > +{
> > > > > +     memcpy(data, btf->data, btf->data_size);
> > > > > +}
> > > >
> > > > I cannot think of any other way to use this api,
> > > > but to call btf__get_raw_data_size() first,
> > > > then malloc that much memory and then call btf__get_raw_data()
> > > > to store btf into it.
> > > >
> > > > If so, may be api should be single call that allocates, copies,
> > > > and returns pointer to new mem and its size?
> > > > Probably less error prone?
> > > >
> > >
> > > I don't have strong preference, but providing pointer to allocated memory
> > > seems more flexible and allows more clever/optimal use of memory from caller
> > > side. E.g., instead of doing two mallocs, you can imagine doing something
> > > like:
> > >
> > > int max_size = max(btf__get_raw_data_size(btf),
> > >                    btf_ext__get_raw_data_size(btf_ext));
> > > char *m = malloc(max_size);
> > > btf__get_raw_data(btf, m);
> > > dump_btf_section_to_file(m, some_file);
> > > btf_ext__get_raw_data(btf_ext, m);
> > > dump_btf_ext_section_to_file(m, some_file);
> > > free(m);
> > >
> > > Also, pointer to memory could be mmap()'ed file, for instance. In general,
> > > for a library it might be a good thing to not be prescriptive as to how one
> > > gets that piece of memory.
> >
> > Plausible, but I'd like to see pahole patches to be convinced ;)
> >
>
> Here's a summary of proposed ways to expose raw data through new api,
> with pros/cons.
>
> 1. Originally proposed two functions. `int btf__get_raw_data_size()`
> to get size, `void btf__get_raw_data(void* buf)` to write raw data to
> a provided buf.
>
> Pros:
>   - allows maximal flexibility for users of this API. They can manage
> memory as it's convenient for them (e.g., reusing same buffer for
> multiple btf and btf_ext raw data).
>   - allows using mmap()'ed memory, as allocation and memory ownership
> is delegated to user
>
> Cons:
>   - has potential of buffer overflows, if user doesn't provide big enough buffer
>
>
> 2. Alexei's proposal to combine getting size in single function that
> internally allocates new memory buffer, copies data and returns it to
> users to use and later free.
>
> Pros:
>   - one less API function
>   - more straightforward usage, it's hard to misuse it (except for
> memory leaking, if memory is not freed)
>
> Cons:
>   - always allocated for each call
>   - least flexible approach, doesn't allow caller to manage memory,
> prevents any kind of direct write to mmap()'ed file
>
> 3. Daniel proposed realloc-like approach, where caller optionally
> provides memory buffer, but we always call realloc() internally to
> ensure we have long enough buffer.
>
> Pros:
>   - allows callers to provide their memory buffer (similar to approach
> #1, but see cons below)
>   - prevents user error with providing too small buffer (similar to approach #2)
>
> Cons:
>   - realloc expects that memory was allocated by previous malloc()
> call, so caller can't allocate bigger chunk of memory and provide
> pointer inside that area (behavior is undefined in that case). This
> requirement is not immediately obvious, so this approach feels more
> error prone than either of approach #1 and #2
>   - still doesn't allow mmap()'ed usage, again due to realloc()'s requirements
>

There is actually approach #4 - just return const void* to an internal
memory buffer. This is trivial for struct btf, will require just
slight changes for struct btf_ext, but it puts all the control in
user's hands without imposing any unnecessary allocations. This
approach seems to provide best of all approaches with no downsides.

>
> Approach #3 looks most subtly-error-prone, as it's too easy to just
> provide pointer that's not at the beginning of malloc()'ed memory, but
> this might not be detected immediately, and could potentially lead to
> silent memory corruption.
>
> I'd still go with approach #1 as it provides least restrictive API,
> even though approach #2 will provide marginally better usability for
> common cases.
>
> Alexei, Daniel, which approach you'd prefer in the end after
> considering all pros and cons?