[PATCH bpf-next v3 0/2] libbpf: Introduce 'btf_custom_path' to 'bpf_obj_open_opts'

Patch 1: Add 'btf_custom_path' to 'bpf_obj_open_opts', allow developers 
to use custom btf to perform CO-RE relocation.

Patch 2: Fixed the memory leak problem pointed out by Andrii.

Changelog:
----------

v2: https://lore.kernel.org/bpf/CAEf4Bza_ua+tjxdhyy4nZ8Boeo+scipWmr_1xM1pC6N5wyuhAA@mail.gmail.com/T/#mf9cf86ae0ffa96180ac29e4fd12697eb70eccd0f
v2->v3:

--- Load the BTF specified by btf_custom_path to btf_vmlinux_override 
    instead of btf_bmlinux.
--- Fix the memory leak that may be introduced by the second version 
    of the patch.
--- Add a new patch to fix the possible memory leak caused by 
    obj->kconfig.

v1: https://lore.kernel.org/bpf/CAEf4BzaGjEC4t1OefDo11pj2-HfNy0BLhs_G2UREjRNTmb2u=A@mail.gmail.com/t/#m4d9f7c6761fbd2b436b5dfe491cd864b70225804
v1->v2:
-- Change custom_btf_path to btf_custom_path.
-- If the length of btf_custom_path of bpf_obj_open_opts is too long, 
   return ERR_PTR(-ENAMETOOLONG).
-- Add `custom BTF is in addition to vmlinux BTF`
   with btf_custom_path field.

Shuyi Cheng (2):
  libbpf: Introduce 'btf_custom_path' to 'bpf_obj_open_opts'
  libbpf: Fix the possible memory leak caused by obj->kconfig

 tools/lib/bpf/libbpf.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++----
 tools/lib/bpf/libbpf.h |  6 +++++-
 2 files changed, 53 insertions(+), 5 deletions(-)

-- 
1.8.3.1

btf_custom_path allows developers to load custom BTF, and subsequent 
CO-RE will use custom BTF for relocation.

Learn from Andrii's comments in [0], add the btf_custom_path parameter
to bpf_obj_open_opts, you can directly use the skeleton's
<objname>_bpf__open_opts function to pass in the btf_custom_path
parameter.

Prior to this, there was also a developer who provided a patch with
similar functions. It is a pity that the follow-up did not continue to
advance. See [1].

	[0]https://lore.kernel.org/bpf/CAEf4BzbJZLjNoiK8_VfeVg_Vrg=9iYFv+po-38SMe=UzwDKJ=Q@mail.gmail.com/#t
	[1]https://yhbt.net/lore/all/CAEf4Bzbgw49w2PtowsrzKQNcxD4fZRE6AKByX-5-dMo-+oWHHA@mail.gmail.com/

Signed-off-by: Shuyi Cheng <redacted>
---
 tools/lib/bpf/libbpf.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 tools/lib/bpf/libbpf.h |  6 +++++-
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 1e04ce7..6702b7f 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -498,6 +498,10 @@ struct bpf_object {
 	 * it at load time.
 	 */
 	struct btf *btf_vmlinux;
+	/* custom BTF is in addition to vmlinux BTF (i.e., Use the CO-RE
+	 * feature in the old kernel).
+	 */
+	char *btf_custom_path;
 	/* vmlinux BTF override for CO-RE relocations */
 	struct btf *btf_vmlinux_override;
 	/* Lazily initialized kernel module BTFs */

@@ -2668,6 +2672,27 @@ static bool obj_needs_vmlinux_btf(const struct bpf_object *obj)
 	return false;
 }
 
+static int bpf_object__load_override_btf(struct bpf_object *obj)
+{
+	int err;
+
+	if (obj->btf_vmlinux_override)
+		return 0;
+
+	if (!obj->btf_custom_path)
+		return 0;
+
+	obj->btf_vmlinux_override = btf__parse(obj->btf_custom_path, NULL);
+	err = libbpf_get_error(obj->btf_vmlinux_override);
+	pr_debug("loading custom BTF '%s': %d\n", obj->btf_custom_path, err);
+	if (err) {
+		pr_warn("failed to parse custom BTF\n");
+		obj->btf_vmlinux_override = NULL;
+	}
+
+	return err;
+}
+
 static int bpf_object__load_vmlinux_btf(struct bpf_object *obj, bool force)
 {
 	int err;

@@ -7554,7 +7579,7 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
 __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
 		   const struct bpf_object_open_opts *opts)
 {
-	const char *obj_name, *kconfig;
+	const char *obj_name, *kconfig, *btf_tmp_path;
 	struct bpf_program *prog;
 	struct bpf_object *obj;
 	char tmp_name[64];

@@ -7584,6 +7609,19 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
 	obj = bpf_object__new(path, obj_buf, obj_buf_sz, obj_name);
 	if (IS_ERR(obj))
 		return obj;
+
+	btf_tmp_path = OPTS_GET(opts, btf_custom_path, NULL);
+	if (btf_tmp_path) {
+		if (strlen(btf_tmp_path) >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto out;
+		}
+		obj->btf_custom_path = strdup(btf_tmp_path);
+		if (!obj->btf_custom_path) {
+			err = -ENOMEM;
+			goto out;
+		}
+	}
 
 	kconfig = OPTS_GET(opts, kconfig, NULL);
 	if (kconfig) {

@@ -8049,6 +8087,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
 		bpf_gen__init(obj->gen_loader, attr->log_level);
 
 	err = bpf_object__probe_loading(obj);
+	err = err ? : bpf_object__load_override_btf(obj);
 	err = err ? : bpf_object__load_vmlinux_btf(obj, false);
 	err = err ? : bpf_object__resolve_externs(obj, obj->kconfig);
 	err = err ? : bpf_object__sanitize_and_load_btf(obj);

@@ -8075,9 +8114,11 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
 	}
 	free(obj->btf_modules);
 
-	/* clean up vmlinux BTF */
+	/* clean up vmlinux BTF and custom BTF*/
 	btf__free(obj->btf_vmlinux);
 	obj->btf_vmlinux = NULL;
+	btf__free(obj->btf_vmlinux_override);
+	obj->btf_vmlinux_override = NULL;
 
 	obj->loaded = true; /* doesn't matter if successfully or not */
 

@@ -8702,6 +8743,7 @@ void bpf_object__close(struct bpf_object *obj)
 	for (i = 0; i < obj->nr_maps; i++)
 		bpf_map__destroy(&obj->maps[i]);
 
+	zfree(&obj->btf_custom_path);
 	zfree(&obj->kconfig);
 	zfree(&obj->externs);
 	obj->nr_extern = 0;

diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 6e61342..5002d1f 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h

@@ -94,8 +94,12 @@ struct bpf_object_open_opts {
 	 * system Kconfig for CONFIG_xxx externs.
 	 */
 	const char *kconfig;
+	/* custom BTF is in addition to vmlinux BTF (i.e., Use the CO-RE
+	 * feature in the old kernel).
+	 */
+	char *btf_custom_path;
 };
-#define bpf_object_open_opts__last_field kconfig
+#define bpf_object_open_opts__last_field btf_custom_path
 
 LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
 LIBBPF_API struct bpf_object *

-- 
1.8.3.1

When obj->kconfig is NULL, ERR_PTR(-ENOMEM) should not be returned
directly, err=-ENOMEM should be set, and then goto out.

Signed-off-by: Shuyi Cheng <redacted>
---
 tools/lib/bpf/libbpf.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 6702b7f..5e550e7 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -7626,8 +7626,10 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
 	kconfig = OPTS_GET(opts, kconfig, NULL);
 	if (kconfig) {
 		obj->kconfig = strdup(kconfig);
-		if (!obj->kconfig)
-			return ERR_PTR(-ENOMEM);
+		if (!obj->kconfig) {
+			err = -ENOMEM;
+			goto out;
+		}
 	}
 
 	err = bpf_object__elf_init(obj);

-- 
1.8.3.1

On Fri, Jul 09, 2021 at 10:47:53AM +0800, Shuyi Cheng wrote:

When obj->kconfig is NULL, ERR_PTR(-ENOMEM) should not be returned
directly, err=-ENOMEM should be set, and then goto out.

The commit message needs to say what the problem is that the patch is
fixing.  Here is a better commit message:

[PATCH bpf-next v3 2/2] libbpf: Fix the possible memory leak on error

If the strdup() fails then we need to call bpf_object__close(obj) to
avoid a resource leak.

Add a Fixes tag as well.

regards,
dan carpenter

On 7/10/21 10:42 PM, Dan Carpenter wrote:

On Fri, Jul 09, 2021 at 10:47:53AM +0800, Shuyi Cheng wrote:

quoted

When obj->kconfig is NULL, ERR_PTR(-ENOMEM) should not be returned
directly, err=-ENOMEM should be set, and then goto out.

The commit message needs to say what the problem is that the patch is
fixing.  Here is a better commit message:

[PATCH bpf-next v3 2/2] libbpf: Fix the possible memory leak on error

If the strdup() fails then we need to call bpf_object__close(obj) to
avoid a resource leak.

Add a Fixes tag as well.

Agree, Thanks.

After Andrii reviews the patch, I will resend a new patch.

regards,
Shuyi

regards,
dan carpenter

On Thu, Jul 8, 2021 at 7:48 PM Shuyi Cheng [off-list ref] wrote:

nit: "path to custom BTF used for CO-RE relocations" as a description?

see below, I don't think we need this function

this part looks good

btf_vmlinux_override is already working properly, so all you need to
do here (once you remembered btf_custom_path) is to pass it to
bpf_object__relocate. All the rest is taken care of, so you don't need
to add extra cleanup (except for zfree(btf_custom_path)).

        err = err ? : bpf_object__load_vmlinux_btf(obj, false);
        err = err ? : bpf_object__resolve_externs(obj, obj->kconfig);
        err = err ? : bpf_object__sanitize_and_load_btf(obj);

so few lines below this code, after bpf_object__create_maps(obj):

err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ?:
attr->target_btf_path);

This way open_opts->btf_custom_path serves as an override for
load_attr's target_btf_path (which we are going to deprecate anyways).

The only remaining thing is to make sure that
bpf_object__load_vmlinux_btf() won't attempt to load real vmlinux BTF
*just for CO-RE*. So in  obj_needs_vmlinux_btf() make sure to not
return true for CO-RE relocations if custom_btf_path is specified (see
Vamsi's patch which has this logic already).

this shouldn't be necessary, bpf_object__relocate_core() handled this

Instead of "Use the CO-RE feature in the old kernel", let's point out
explicitly that this custom BTF is used *only* for CO-RE, and any
other feature that relies on kernel BTF will still need actual vmlinux
BTF. Something like this:

/* Path to the custom BTF to be used for BPF CO-RE relocations.
 * This custom BTF completely replaces the use of vmlinux BTF
 * for the purpose of CO-RE relocations.
 * NOTE: any other BPF feature (e.g., fentry/fexit programs,
 * struct_ops, etc) will need actual kernel BTF at /sys/kernel/btf/vmlinux.
 */

I think this will make it much clearer what's the intended purpose here.

+        */
+       char *btf_custom_path;
 };
-#define bpf_object_open_opts__last_field kconfig
+#define bpf_object_open_opts__last_field btf_custom_path

 LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
 LIBBPF_API struct bpf_object *
--
1.8.3.1

On Thu, Jul 8, 2021 at 7:48 PM Shuyi Cheng [off-list ref] wrote:

Patch 1: Add 'btf_custom_path' to 'bpf_obj_open_opts', allow developers
to use custom btf to perform CO-RE relocation.

Patch 2: Fixed the memory leak problem pointed out by Andrii.

Please note that the cover letter should have a high-level overview of
what the set of patches you are sending is doing, not just a
changelog. So in this case, as an example for the future
contributions, I'd write something like this:

This patch set adds the ability to point to a custom BTF for the
purposes of BPF CO-RE relocations. This is useful for using BPF CO-RE
on old kernels that don't yet natively support kernel (vmlinux) BTF
and thus libbpf needs application's help in locating kernel BTF
generated separately from the kernel itself. This was already possible
to do through bpf_object__load's attribute struct, but that makes it
inconvenient to use with BPF skeleton, which only allows to specify
bpf_object_open_opts during the open step. Thus, add the ability to
override vmlinux BTF at open time.

Patch #1 adds libbpf changes. Patch #2 fixes pre-existing memory leak
detected during the code review. Patch #3 switches existing selftests
to using open_opts for custom BTF.

BTW, see the description above about selftests (fictional patch #3).
Please update selftests core_autosize.c and core_reloc.c to use the
new functionality instead of load_attr.target_btf_path. It's a general
rule to always add a new test or update existing test to utilize newly
added functionality. That way we can know that it actually works as
expected.