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

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

[Shuyi Cheng]

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

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

[Shuyi Cheng]

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 <chengshuyi@linux.alibaba.com>
---
 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

[PATCH bpf-next v3 2/2] libbpf: Fix the possible memory leak caused by obj->kconfig

[Shuyi Cheng]

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 <chengshuyi@linux.alibaba.com>
---
 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

Re: [PATCH bpf-next v3 2/2] libbpf: Fix the possible memory leak caused by obj->kconfig

[Dan Carpenter]

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

Re: [PATCH bpf-next v3 2/2] libbpf: Fix the possible memory leak caused by obj->kconfig

[Shuyi Cheng]

On 7/10/21 10:42 PM, Dan Carpenter wrote:
> 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.

Agree, Thanks.

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

regards,
Shuyi

> 
> regards,
> dan carpenter
> 

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

[Andrii Nakryiko]

On Thu, Jul 8, 2021 at 7:48 PM Shuyi Cheng <chengshuyi@linux.alibaba.com> wrote:
>
> 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 <chengshuyi@linux.alibaba.com>
> ---
>  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).
> +        */

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

> +       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;
> +}

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

> +
>  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;
> +               }
> +       }

this part looks good

>
>         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);

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).

> @@ -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;

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

>
>         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).

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
>

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

[Andrii Nakryiko]

On Thu, Jul 8, 2021 at 7:48 PM Shuyi Cheng <chengshuyi@linux.alibaba.com> 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.


> 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
>