[PATCH v2 bpf-next 1/2] libbpf: Support triple-underscore flavors for kfunc relocation

[PATCH v2 bpf-next 1/2] libbpf: Support triple-underscore flavors for kfunc relocation

[Dave Marchevsky]

The function signature of kfuncs can change at any time due to their
intentional lack of stability guarantees. As kfuncs become more widely
used, BPF program writers will need facilities to support calling
different versions of a kfunc from a single BPF object. Consider this
simplified example based on a real scenario we ran into at Meta:

  /* initial kfunc signature */
  int some_kfunc(void *ptr)

  /* Oops, we need to add some flag to modify behavior. No problem,
    change the kfunc. flags = 0 retains original behavior */
  int some_kfunc(void *ptr, long flags)

If the initial version of the kfunc is deployed on some portion of the
fleet and the new version on the rest, a fleetwide service that uses
some_kfunc will currently need to load different BPF programs depending
on which some_kfunc is available.

Luckily CO-RE provides a facility to solve a very similar problem,
struct definition changes, by allowing program writers to declare
my_struct___old and my_struct___new, with ___suffix being considered a
'flavor' of the non-suffixed name and being ignored by
bpf_core_type_exists and similar calls.

This patch extends the 'flavor' facility to the kfunc extern
relocation process. BPF program writers can now declare

  extern int some_kfunc___old(void *ptr)
  extern int some_kfunc___new(void *ptr, int flags)

then test which version of the kfunc exists with bpf_ksym_exists.
Relocation and verifier's dead code elimination will work in concert as
expected, allowing this pattern:

  if (bpf_ksym_exists(some_kfunc___old))
    some_kfunc___old(ptr);
  else
    some_kfunc___new(ptr, 0);

Changelog:

v1 -> v2: https://lore.kernel.org/bpf/20230811201346.3240403-1-davemarchevsky@fb.com/
  * No need to check obj->externs[i].essent_name before zfree (Jiri)
  * Use strndup instead of replicating same functionality (Jiri)
  * Properly handle memory allocation falure (Stanislav)

Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
---
 tools/lib/bpf/libbpf.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b14a4376a86e..8899abc04b8c 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -550,6 +550,7 @@ struct extern_desc {
 	int btf_id;
 	int sec_btf_id;
 	const char *name;
+	char *essent_name;
 	bool is_set;
 	bool is_weak;
 	union {
@@ -3770,6 +3771,7 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
 	struct extern_desc *ext;
 	int i, n, off, dummy_var_btf_id;
 	const char *ext_name, *sec_name;
+	size_t ext_essent_len;
 	Elf_Scn *scn;
 	Elf64_Shdr *sh;
 
@@ -3819,6 +3821,14 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
 		ext->sym_idx = i;
 		ext->is_weak = ELF64_ST_BIND(sym->st_info) == STB_WEAK;
 
+		ext_essent_len = bpf_core_essential_name_len(ext->name);
+		ext->essent_name = NULL;
+		if (ext_essent_len != strlen(ext->name)) {
+			ext->essent_name = strndup(ext->name, ext_essent_len);
+			if (!ext->essent_name)
+				return -ENOMEM;
+		}
+
 		ext->sec_btf_id = find_extern_sec_btf_id(obj->btf, ext->btf_id);
 		if (ext->sec_btf_id <= 0) {
 			pr_warn("failed to find BTF for extern '%s' [%d] section: %d\n",
@@ -7624,7 +7634,8 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
 
 	local_func_proto_id = ext->ksym.type_id;
 
-	kfunc_id = find_ksym_btf_id(obj, ext->name, BTF_KIND_FUNC, &kern_btf, &mod_btf);
+	kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
+				    &mod_btf);
 	if (kfunc_id < 0) {
 		if (kfunc_id == -ESRCH && ext->is_weak)
 			return 0;
@@ -7642,6 +7653,9 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
 		pr_warn("extern (func ksym) '%s': func_proto [%d] incompatible with %s [%d]\n",
 			ext->name, local_func_proto_id,
 			mod_btf ? mod_btf->name : "vmlinux", kfunc_proto_id);
+
+		if (ext->is_weak)
+			return 0;
 		return -EINVAL;
 	}
 
@@ -8370,6 +8384,10 @@ void bpf_object__close(struct bpf_object *obj)
 
 	zfree(&obj->btf_custom_path);
 	zfree(&obj->kconfig);
+
+	for (i = 0; i < obj->nr_extern; i++)
+		zfree(&obj->externs[i].essent_name);
+
 	zfree(&obj->externs);
 	obj->nr_extern = 0;
 
-- 
2.34.1

[PATCH v2 bpf-next 2/2] selftests/bpf: Add CO-RE relocs kfunc flavors tests

[Dave Marchevsky]

This patch adds selftests that exercise kfunc flavor relocation
functionality added in the previous patch. The actual kfunc defined in
kernel/bpf/helpers.c is

  struct task_struct *bpf_task_acquire(struct task_struct *p)

The following relocation behaviors are checked:

  struct task_struct *bpf_task_acquire___one(struct task_struct *name)
    * Should succeed despite differing param name

  struct task_struct *bpf_task_acquire___two(struct task_struct *p, void *ctx)
    * Should fail because there is no two-param bpf_task_acquire

  struct task_struct *bpf_task_acquire___three(void *ctx)
    * Should fail because, despite vmlinux's bpf_task_acquire having one param,
      the types don't match

Changelog:
v1 -> v2: https://lore.kernel.org/bpf/20230811201346.3240403-2-davemarchevsky@fb.com/
  * Change comment on bpf_task_acquire___two to more accurately reflect
    that it fails in same codepath as bpf_task_acquire___three, and to
    not mention dead code elimination as thats an implementation detail
    (Yonghong)

Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
---
 .../selftests/bpf/prog_tests/task_kfunc.c     |  1 +
 .../selftests/bpf/progs/task_kfunc_success.c  | 37 +++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
index 740d5f644b40..99abb0350154 100644
--- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
+++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
@@ -79,6 +79,7 @@ static const char * const success_tests[] = {
 	"test_task_from_pid_current",
 	"test_task_from_pid_invalid",
 	"task_kfunc_acquire_trusted_walked",
+	"test_task_kfunc_flavor_relo",
 };
 
 void test_task_kfunc(void)
diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_success.c b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
index b09371bba204..ffbe3ff72639 100644
--- a/tools/testing/selftests/bpf/progs/task_kfunc_success.c
+++ b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
@@ -18,6 +18,13 @@ int err, pid;
  */
 
 struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak;
+
+struct task_struct *bpf_task_acquire___one(struct task_struct *task) __ksym __weak;
+/* The two-param bpf_task_acquire doesn't exist */
+struct task_struct *bpf_task_acquire___two(struct task_struct *p, void *ctx) __ksym __weak;
+/* Incorrect type for first param */
+struct task_struct *bpf_task_acquire___three(void *ctx) __ksym __weak;
+
 void invalid_kfunc(void) __ksym __weak;
 void bpf_testmod_test_mod_kfunc(int i) __ksym __weak;
 
@@ -55,6 +62,36 @@ static int test_acquire_release(struct task_struct *task)
 	return 0;
 }
 
+SEC("tp_btf/task_newtask")
+int BPF_PROG(test_task_kfunc_flavor_relo, struct task_struct *task, u64 clone_flags)
+{
+	struct task_struct *acquired = NULL;
+	int fake_ctx = 42;
+
+	if (bpf_ksym_exists(bpf_task_acquire___one)) {
+		acquired = bpf_task_acquire___one(task);
+	} else if (bpf_ksym_exists(bpf_task_acquire___two)) {
+		/* Here, bpf_object__resolve_ksym_func_btf_id's find_ksym_btf_id
+		 * call will find vmlinux's bpf_task_acquire, but subsequent
+		 * bpf_core_types_are_compat will fail
+		 */
+		acquired = bpf_task_acquire___two(task, &fake_ctx);
+		err = 3;
+		return 0;
+	} else if (bpf_ksym_exists(bpf_task_acquire___three)) {
+		/* bpf_core_types_are_compat will fail similarly to above case */
+		acquired = bpf_task_acquire___three(&fake_ctx);
+		err = 4;
+		return 0;
+	}
+
+	if (acquired)
+		bpf_task_release(acquired);
+	else
+		err = 5;
+	return 0;
+}
+
 SEC("tp_btf/task_newtask")
 int BPF_PROG(test_task_acquire_release_argument, struct task_struct *task, u64 clone_flags)
 {
-- 
2.34.1

Re: [PATCH v2 bpf-next 1/2] libbpf: Support triple-underscore flavors for kfunc relocation

[David Vernet]

On Wed, Aug 16, 2023 at 09:58:12AM -0700, Dave Marchevsky wrote:
> The function signature of kfuncs can change at any time due to their
> intentional lack of stability guarantees. As kfuncs become more widely
> used, BPF program writers will need facilities to support calling
> different versions of a kfunc from a single BPF object. Consider this
> simplified example based on a real scenario we ran into at Meta:
> 
>   /* initial kfunc signature */
>   int some_kfunc(void *ptr)
> 
>   /* Oops, we need to add some flag to modify behavior. No problem,
>     change the kfunc. flags = 0 retains original behavior */
>   int some_kfunc(void *ptr, long flags)
> 
> If the initial version of the kfunc is deployed on some portion of the
> fleet and the new version on the rest, a fleetwide service that uses
> some_kfunc will currently need to load different BPF programs depending
> on which some_kfunc is available.
> 
> Luckily CO-RE provides a facility to solve a very similar problem,
> struct definition changes, by allowing program writers to declare
> my_struct___old and my_struct___new, with ___suffix being considered a
> 'flavor' of the non-suffixed name and being ignored by
> bpf_core_type_exists and similar calls.
> 
> This patch extends the 'flavor' facility to the kfunc extern
> relocation process. BPF program writers can now declare
> 
>   extern int some_kfunc___old(void *ptr)
>   extern int some_kfunc___new(void *ptr, int flags)
> 
> then test which version of the kfunc exists with bpf_ksym_exists.
> Relocation and verifier's dead code elimination will work in concert as
> expected, allowing this pattern:
> 
>   if (bpf_ksym_exists(some_kfunc___old))
>     some_kfunc___old(ptr);
>   else
>     some_kfunc___new(ptr, 0);
> 
> Changelog:
> 
> v1 -> v2: https://lore.kernel.org/bpf/20230811201346.3240403-1-davemarchevsky@fb.com/
>   * No need to check obj->externs[i].essent_name before zfree (Jiri)
>   * Use strndup instead of replicating same functionality (Jiri)
>   * Properly handle memory allocation falure (Stanislav)
> 
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
>  tools/lib/bpf/libbpf.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b14a4376a86e..8899abc04b8c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -550,6 +550,7 @@ struct extern_desc {
>  	int btf_id;
>  	int sec_btf_id;
>  	const char *name;
> +	char *essent_name;
>  	bool is_set;
>  	bool is_weak;
>  	union {
> @@ -3770,6 +3771,7 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
>  	struct extern_desc *ext;
>  	int i, n, off, dummy_var_btf_id;
>  	const char *ext_name, *sec_name;
> +	size_t ext_essent_len;
>  	Elf_Scn *scn;
>  	Elf64_Shdr *sh;
>  
> @@ -3819,6 +3821,14 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
>  		ext->sym_idx = i;
>  		ext->is_weak = ELF64_ST_BIND(sym->st_info) == STB_WEAK;
>  
> +		ext_essent_len = bpf_core_essential_name_len(ext->name);
> +		ext->essent_name = NULL;
> +		if (ext_essent_len != strlen(ext->name)) {
> +			ext->essent_name = strndup(ext->name, ext_essent_len);
> +			if (!ext->essent_name)
> +				return -ENOMEM;
> +		}
> +
>  		ext->sec_btf_id = find_extern_sec_btf_id(obj->btf, ext->btf_id);
>  		if (ext->sec_btf_id <= 0) {
>  			pr_warn("failed to find BTF for extern '%s' [%d] section: %d\n",
> @@ -7624,7 +7634,8 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
>  
>  	local_func_proto_id = ext->ksym.type_id;
>  
> -	kfunc_id = find_ksym_btf_id(obj, ext->name, BTF_KIND_FUNC, &kern_btf, &mod_btf);
> +	kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
> +				    &mod_btf);
>  	if (kfunc_id < 0) {
>  		if (kfunc_id == -ESRCH && ext->is_weak)
>  			return 0;
> @@ -7642,6 +7653,9 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
>  		pr_warn("extern (func ksym) '%s': func_proto [%d] incompatible with %s [%d]\n",
>  			ext->name, local_func_proto_id,

Should we do ext->essent_name ?: ext->name here or in the below pr's as
well? Hmm, maybe it would be more clear to keep the full name.

>  			mod_btf ? mod_btf->name : "vmlinux", kfunc_proto_id);
> +
> +		if (ext->is_weak)
> +			return 0;

Could you clarify why we want this check? Don't we want to fail if the
prototype of the actual (essent) symbol we resolve to doesn't match
what's in the BPF prog? If we do want to keep this, should we do the
check above the pr_warn()?

>  		return -EINVAL;
>  	}
>  
> @@ -8370,6 +8384,10 @@ void bpf_object__close(struct bpf_object *obj)
>  
>  	zfree(&obj->btf_custom_path);
>  	zfree(&obj->kconfig);
> +
> +	for (i = 0; i < obj->nr_extern; i++)
> +		zfree(&obj->externs[i].essent_name);
> +
>  	zfree(&obj->externs);
>  	obj->nr_extern = 0;
>  
> -- 
> 2.34.1
> 
> 

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add CO-RE relocs kfunc flavors tests

[David Vernet]

On Wed, Aug 16, 2023 at 09:58:13AM -0700, Dave Marchevsky wrote:
> This patch adds selftests that exercise kfunc flavor relocation
> functionality added in the previous patch. The actual kfunc defined in
> kernel/bpf/helpers.c is
> 
>   struct task_struct *bpf_task_acquire(struct task_struct *p)
> 
> The following relocation behaviors are checked:
> 
>   struct task_struct *bpf_task_acquire___one(struct task_struct *name)
>     * Should succeed despite differing param name
> 
>   struct task_struct *bpf_task_acquire___two(struct task_struct *p, void *ctx)
>     * Should fail because there is no two-param bpf_task_acquire
> 
>   struct task_struct *bpf_task_acquire___three(void *ctx)
>     * Should fail because, despite vmlinux's bpf_task_acquire having one param,
>       the types don't match
> 
> Changelog:
> v1 -> v2: https://lore.kernel.org/bpf/20230811201346.3240403-2-davemarchevsky@fb.com/
>   * Change comment on bpf_task_acquire___two to more accurately reflect
>     that it fails in same codepath as bpf_task_acquire___three, and to
>     not mention dead code elimination as thats an implementation detail
>     (Yonghong)
> 
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
>  .../selftests/bpf/prog_tests/task_kfunc.c     |  1 +
>  .../selftests/bpf/progs/task_kfunc_success.c  | 37 +++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> index 740d5f644b40..99abb0350154 100644
> --- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> +++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> @@ -79,6 +79,7 @@ static const char * const success_tests[] = {
>  	"test_task_from_pid_current",
>  	"test_task_from_pid_invalid",
>  	"task_kfunc_acquire_trusted_walked",
> +	"test_task_kfunc_flavor_relo",
>  };
>  
>  void test_task_kfunc(void)
> diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_success.c b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
> index b09371bba204..ffbe3ff72639 100644
> --- a/tools/testing/selftests/bpf/progs/task_kfunc_success.c
> +++ b/tools/testing/selftests/bpf/progs/task_kfunc_success.c

Do you think it's worth it to also add a failure case for if there's no
correct bpf_taks_acquire___one(), to verify e.g. that we can't resolve
bpf_task_acquire___three(void *ctx) __ksym __weak?

> @@ -18,6 +18,13 @@ int err, pid;
>   */
>  
>  struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak;
> +
> +struct task_struct *bpf_task_acquire___one(struct task_struct *task) __ksym __weak;
> +/* The two-param bpf_task_acquire doesn't exist */
> +struct task_struct *bpf_task_acquire___two(struct task_struct *p, void *ctx) __ksym __weak;
> +/* Incorrect type for first param */
> +struct task_struct *bpf_task_acquire___three(void *ctx) __ksym __weak;
> +
>  void invalid_kfunc(void) __ksym __weak;
>  void bpf_testmod_test_mod_kfunc(int i) __ksym __weak;
>  
> @@ -55,6 +62,36 @@ static int test_acquire_release(struct task_struct *task)
>  	return 0;
>  }
>  
> +SEC("tp_btf/task_newtask")
> +int BPF_PROG(test_task_kfunc_flavor_relo, struct task_struct *task, u64 clone_flags)
> +{
> +	struct task_struct *acquired = NULL;
> +	int fake_ctx = 42;
> +
> +	if (bpf_ksym_exists(bpf_task_acquire___one)) {
> +		acquired = bpf_task_acquire___one(task);
> +	} else if (bpf_ksym_exists(bpf_task_acquire___two)) {
> +		/* Here, bpf_object__resolve_ksym_func_btf_id's find_ksym_btf_id
> +		 * call will find vmlinux's bpf_task_acquire, but subsequent
> +		 * bpf_core_types_are_compat will fail
> +		 */
> +		acquired = bpf_task_acquire___two(task, &fake_ctx);
> +		err = 3;
> +		return 0;
> +	} else if (bpf_ksym_exists(bpf_task_acquire___three)) {
> +		/* bpf_core_types_are_compat will fail similarly to above case */
> +		acquired = bpf_task_acquire___three(&fake_ctx);
> +		err = 4;
> +		return 0;
> +	}
> +
> +	if (acquired)
> +		bpf_task_release(acquired);

Might be slightly simpler to do the release + return immediately in the
bpf_task_acquire___one branch, and then to just do the following here
without the if / else:

err = 5;
return 0;

What do you think?

> +	else
> +		err = 5;
> +	return 0;
> +}
> +
>  SEC("tp_btf/task_newtask")
>  int BPF_PROG(test_task_acquire_release_argument, struct task_struct *task, u64 clone_flags)
>  {
> -- 
> 2.34.1
> 
> 

Re: [PATCH v2 bpf-next 1/2] libbpf: Support triple-underscore flavors for kfunc relocation

[David Marchevsky]

On 8/16/23 1:37 PM, David Vernet wrote:
> On Wed, Aug 16, 2023 at 09:58:12AM -0700, Dave Marchevsky wrote:
>> The function signature of kfuncs can change at any time due to their
>> intentional lack of stability guarantees. As kfuncs become more widely
>> used, BPF program writers will need facilities to support calling
>> different versions of a kfunc from a single BPF object. Consider this
>> simplified example based on a real scenario we ran into at Meta:
>>
>>   /* initial kfunc signature */
>>   int some_kfunc(void *ptr)
>>
>>   /* Oops, we need to add some flag to modify behavior. No problem,
>>     change the kfunc. flags = 0 retains original behavior */
>>   int some_kfunc(void *ptr, long flags)
>>
>> If the initial version of the kfunc is deployed on some portion of the
>> fleet and the new version on the rest, a fleetwide service that uses
>> some_kfunc will currently need to load different BPF programs depending
>> on which some_kfunc is available.
>>
>> Luckily CO-RE provides a facility to solve a very similar problem,
>> struct definition changes, by allowing program writers to declare
>> my_struct___old and my_struct___new, with ___suffix being considered a
>> 'flavor' of the non-suffixed name and being ignored by
>> bpf_core_type_exists and similar calls.
>>
>> This patch extends the 'flavor' facility to the kfunc extern
>> relocation process. BPF program writers can now declare
>>
>>   extern int some_kfunc___old(void *ptr)
>>   extern int some_kfunc___new(void *ptr, int flags)
>>
>> then test which version of the kfunc exists with bpf_ksym_exists.
>> Relocation and verifier's dead code elimination will work in concert as
>> expected, allowing this pattern:
>>
>>   if (bpf_ksym_exists(some_kfunc___old))
>>     some_kfunc___old(ptr);
>>   else
>>     some_kfunc___new(ptr, 0);
>>
>> Changelog:
>>
>> v1 -> v2: https://lore.kernel.org/bpf/20230811201346.3240403-1-davemarchevsky@fb.com/
>>   * No need to check obj->externs[i].essent_name before zfree (Jiri)
>>   * Use strndup instead of replicating same functionality (Jiri)
>>   * Properly handle memory allocation falure (Stanislav)
>>
>> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
>> ---
>>  tools/lib/bpf/libbpf.c | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index b14a4376a86e..8899abc04b8c 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -550,6 +550,7 @@ struct extern_desc {
>>  	int btf_id;
>>  	int sec_btf_id;
>>  	const char *name;
>> +	char *essent_name;
>>  	bool is_set;
>>  	bool is_weak;
>>  	union {
>> @@ -3770,6 +3771,7 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
>>  	struct extern_desc *ext;
>>  	int i, n, off, dummy_var_btf_id;
>>  	const char *ext_name, *sec_name;
>> +	size_t ext_essent_len;
>>  	Elf_Scn *scn;
>>  	Elf64_Shdr *sh;
>>  
>> @@ -3819,6 +3821,14 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
>>  		ext->sym_idx = i;
>>  		ext->is_weak = ELF64_ST_BIND(sym->st_info) == STB_WEAK;
>>  
>> +		ext_essent_len = bpf_core_essential_name_len(ext->name);
>> +		ext->essent_name = NULL;
>> +		if (ext_essent_len != strlen(ext->name)) {
>> +			ext->essent_name = strndup(ext->name, ext_essent_len);
>> +			if (!ext->essent_name)
>> +				return -ENOMEM;
>> +		}
>> +
>>  		ext->sec_btf_id = find_extern_sec_btf_id(obj->btf, ext->btf_id);
>>  		if (ext->sec_btf_id <= 0) {
>>  			pr_warn("failed to find BTF for extern '%s' [%d] section: %d\n",
>> @@ -7624,7 +7634,8 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
>>  
>>  	local_func_proto_id = ext->ksym.type_id;
>>  
>> -	kfunc_id = find_ksym_btf_id(obj, ext->name, BTF_KIND_FUNC, &kern_btf, &mod_btf);
>> +	kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
>> +				    &mod_btf);
>>  	if (kfunc_id < 0) {
>>  		if (kfunc_id == -ESRCH && ext->is_weak)
>>  			return 0;
>> @@ -7642,6 +7653,9 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
>>  		pr_warn("extern (func ksym) '%s': func_proto [%d] incompatible with %s [%d]\n",
>>  			ext->name, local_func_proto_id,
> 
> Should we do ext->essent_name ?: ext->name here or in the below pr's as
> well? Hmm, maybe it would be more clear to keep the full name.
> 

Yeah, I agree that the full name should be used in this warning for clarity.
So won't change.

>>  			mod_btf ? mod_btf->name : "vmlinux", kfunc_proto_id);
>> +
>> +		if (ext->is_weak)
>> +			return 0;
> 
> Could you clarify why we want this check? Don't we want to fail if the
> prototype of the actual (essent) symbol we resolve to doesn't match
> what's in the BPF prog? If we do want to keep this, should we do the
> check above the pr_warn()?
> 

Actually this if-and-return was initially above the pr_warn while I was
developing the patch. I moved it down here to confirm via './test_progs -vv'
that the pseudo-failure cases in the selftests were going down the codepaths
I expected, and left it b/c better to err on the side of too much logging
when doing this ___flavor trickery.

In re: "clarify why we want this check?" and subsequent question, IIUC, with an
extern decl like

  struct task_struct *bpf_task_acquire___one(struct task_struct *task) __ksym __weak;

if we removed __weak from the declaration, symbol resolution would happen during
compilation + linking, at which point there would be no opportunity to do
our ___flavor trickery. But __weak is already used to express "if this kfunc
doesn't exist at all, it's not a problem, don't fail loading the program". So
as of this version of the code, it's not possible to express "one of
bpf_task_acquire___{one,two,three} must resolve, otherwise fail to
load" - that check would have to be done at runtime like

  if (!(bpf_ksym_exists(bpf_task_acquire___one) ||
        bpf_ksym_exists(bpf_task_acquire___two) ||
        bpf_ksym_exists(bpf_task_acquire___three)) {
    /* communicate failure to userspace runner via global var or something */
    return 0;
  }

Maybe something like BTF tags could be used to group a set of __weak
kfunc declarations together such that one (probably _only_ one) of them
must resolve at load time. This would obviate the need for such a runtime
check without causing compile+link step to fail. But also seems overly
complex for now.

Feels useful to have "incompatible resolution" log message even if it doesn't
stop loading process. But because __weak ties ___flavor trickery to "not a 
problem if kfunc doesn't exist at all", probably more accurate to make the
pr_warn a pr_debug if ___flavor AND ext->is_weak. Adding the logic to do that
felt like it would raise more questions than answers to a future reader of the
code, so I didn't add it. Now that I'm writing this out, I think it's better
to add it along with a comment.


>>  		return -EINVAL;
>>  	}
>>  
>> @@ -8370,6 +8384,10 @@ void bpf_object__close(struct bpf_object *obj)
>>  
>>  	zfree(&obj->btf_custom_path);
>>  	zfree(&obj->kconfig);
>> +
>> +	for (i = 0; i < obj->nr_extern; i++)
>> +		zfree(&obj->externs[i].essent_name);
>> +
>>  	zfree(&obj->externs);
>>  	obj->nr_extern = 0;
>>  
>> -- 
>> 2.34.1
>>
>>

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add CO-RE relocs kfunc flavors tests

[David Marchevsky]

On 8/16/23 1:44 PM, David Vernet wrote:
> On Wed, Aug 16, 2023 at 09:58:13AM -0700, Dave Marchevsky wrote:
>> This patch adds selftests that exercise kfunc flavor relocation
>> functionality added in the previous patch. The actual kfunc defined in
>> kernel/bpf/helpers.c is
>>
>>   struct task_struct *bpf_task_acquire(struct task_struct *p)
>>
>> The following relocation behaviors are checked:
>>
>>   struct task_struct *bpf_task_acquire___one(struct task_struct *name)
>>     * Should succeed despite differing param name
>>
>>   struct task_struct *bpf_task_acquire___two(struct task_struct *p, void *ctx)
>>     * Should fail because there is no two-param bpf_task_acquire
>>
>>   struct task_struct *bpf_task_acquire___three(void *ctx)
>>     * Should fail because, despite vmlinux's bpf_task_acquire having one param,
>>       the types don't match
>>
>> Changelog:
>> v1 -> v2: https://lore.kernel.org/bpf/20230811201346.3240403-2-davemarchevsky@fb.com/
>>   * Change comment on bpf_task_acquire___two to more accurately reflect
>>     that it fails in same codepath as bpf_task_acquire___three, and to
>>     not mention dead code elimination as thats an implementation detail
>>     (Yonghong)
>>
>> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
>> ---
>>  .../selftests/bpf/prog_tests/task_kfunc.c     |  1 +
>>  .../selftests/bpf/progs/task_kfunc_success.c  | 37 +++++++++++++++++++
>>  2 files changed, 38 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
>> index 740d5f644b40..99abb0350154 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
>> @@ -79,6 +79,7 @@ static const char * const success_tests[] = {
>>  	"test_task_from_pid_current",
>>  	"test_task_from_pid_invalid",
>>  	"task_kfunc_acquire_trusted_walked",
>> +	"test_task_kfunc_flavor_relo",
>>  };
>>  
>>  void test_task_kfunc(void)
>> diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_success.c b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
>> index b09371bba204..ffbe3ff72639 100644
>> --- a/tools/testing/selftests/bpf/progs/task_kfunc_success.c
>> +++ b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
> 
> Do you think it's worth it to also add a failure case for if there's no
> correct bpf_taks_acquire___one(), to verify e.g. that we can't resolve
> bpf_task_acquire___three(void *ctx) __ksym __weak?
> 

IIUC you're asking about whether it's possible to fail loading the program
entirely if _none_ of the three variants resolve successfully? If so, I
sent out a response to another email in this round of your comments that should
address it.

>> @@ -18,6 +18,13 @@ int err, pid;
>>   */
>>  
>>  struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak;
>> +
>> +struct task_struct *bpf_task_acquire___one(struct task_struct *task) __ksym __weak;
>> +/* The two-param bpf_task_acquire doesn't exist */
>> +struct task_struct *bpf_task_acquire___two(struct task_struct *p, void *ctx) __ksym __weak;
>> +/* Incorrect type for first param */
>> +struct task_struct *bpf_task_acquire___three(void *ctx) __ksym __weak;
>> +
>>  void invalid_kfunc(void) __ksym __weak;
>>  void bpf_testmod_test_mod_kfunc(int i) __ksym __weak;
>>  
>> @@ -55,6 +62,36 @@ static int test_acquire_release(struct task_struct *task)
>>  	return 0;
>>  }
>>  
>> +SEC("tp_btf/task_newtask")
>> +int BPF_PROG(test_task_kfunc_flavor_relo, struct task_struct *task, u64 clone_flags)
>> +{
>> +	struct task_struct *acquired = NULL;
>> +	int fake_ctx = 42;
>> +
>> +	if (bpf_ksym_exists(bpf_task_acquire___one)) {
>> +		acquired = bpf_task_acquire___one(task);
>> +	} else if (bpf_ksym_exists(bpf_task_acquire___two)) {
>> +		/* Here, bpf_object__resolve_ksym_func_btf_id's find_ksym_btf_id
>> +		 * call will find vmlinux's bpf_task_acquire, but subsequent
>> +		 * bpf_core_types_are_compat will fail
>> +		 */
>> +		acquired = bpf_task_acquire___two(task, &fake_ctx);
>> +		err = 3;
>> +		return 0;
>> +	} else if (bpf_ksym_exists(bpf_task_acquire___three)) {
>> +		/* bpf_core_types_are_compat will fail similarly to above case */
>> +		acquired = bpf_task_acquire___three(&fake_ctx);
>> +		err = 4;
>> +		return 0;
>> +	}
>> +
>> +	if (acquired)
>> +		bpf_task_release(acquired);
> 
> Might be slightly simpler to do the release + return immediately in the
> bpf_task_acquire___one branch, and then to just do the following here
> without the if / else:
> 
> err = 5;
> return 0;
> 
> What do you think?
> 

Eh, I like this form more because it's easier to visually distinguish that the
bpf_task_acquire___one case above is not a 'failure' case and should
successfully resolve, whereas the other two bail out early.

>> +	else
>> +		err = 5;
>> +	return 0;
>> +}
>> +
>>  SEC("tp_btf/task_newtask")
>>  int BPF_PROG(test_task_acquire_release_argument, struct task_struct *task, u64 clone_flags)
>>  {
>> -- 
>> 2.34.1
>>
>>

Re: [PATCH v2 bpf-next 1/2] libbpf: Support triple-underscore flavors for kfunc relocation

[David Vernet]

On Wed, Aug 16, 2023 at 03:01:10PM -0400, David Marchevsky wrote:
> On 8/16/23 1:37 PM, David Vernet wrote:
> > On Wed, Aug 16, 2023 at 09:58:12AM -0700, Dave Marchevsky wrote:
> >> The function signature of kfuncs can change at any time due to their
> >> intentional lack of stability guarantees. As kfuncs become more widely
> >> used, BPF program writers will need facilities to support calling
> >> different versions of a kfunc from a single BPF object. Consider this
> >> simplified example based on a real scenario we ran into at Meta:
> >>
> >>   /* initial kfunc signature */
> >>   int some_kfunc(void *ptr)
> >>
> >>   /* Oops, we need to add some flag to modify behavior. No problem,
> >>     change the kfunc. flags = 0 retains original behavior */
> >>   int some_kfunc(void *ptr, long flags)
> >>
> >> If the initial version of the kfunc is deployed on some portion of the
> >> fleet and the new version on the rest, a fleetwide service that uses
> >> some_kfunc will currently need to load different BPF programs depending
> >> on which some_kfunc is available.
> >>
> >> Luckily CO-RE provides a facility to solve a very similar problem,
> >> struct definition changes, by allowing program writers to declare
> >> my_struct___old and my_struct___new, with ___suffix being considered a
> >> 'flavor' of the non-suffixed name and being ignored by
> >> bpf_core_type_exists and similar calls.
> >>
> >> This patch extends the 'flavor' facility to the kfunc extern
> >> relocation process. BPF program writers can now declare
> >>
> >>   extern int some_kfunc___old(void *ptr)
> >>   extern int some_kfunc___new(void *ptr, int flags)
> >>
> >> then test which version of the kfunc exists with bpf_ksym_exists.
> >> Relocation and verifier's dead code elimination will work in concert as
> >> expected, allowing this pattern:
> >>
> >>   if (bpf_ksym_exists(some_kfunc___old))
> >>     some_kfunc___old(ptr);
> >>   else
> >>     some_kfunc___new(ptr, 0);
> >>
> >> Changelog:
> >>
> >> v1 -> v2: https://lore.kernel.org/bpf/20230811201346.3240403-1-davemarchevsky@fb.com/
> >>   * No need to check obj->externs[i].essent_name before zfree (Jiri)
> >>   * Use strndup instead of replicating same functionality (Jiri)
> >>   * Properly handle memory allocation falure (Stanislav)
> >>
> >> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> >> ---
> >>  tools/lib/bpf/libbpf.c | 20 +++++++++++++++++++-
> >>  1 file changed, 19 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index b14a4376a86e..8899abc04b8c 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -550,6 +550,7 @@ struct extern_desc {
> >>  	int btf_id;
> >>  	int sec_btf_id;
> >>  	const char *name;
> >> +	char *essent_name;
> >>  	bool is_set;
> >>  	bool is_weak;
> >>  	union {
> >> @@ -3770,6 +3771,7 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
> >>  	struct extern_desc *ext;
> >>  	int i, n, off, dummy_var_btf_id;
> >>  	const char *ext_name, *sec_name;
> >> +	size_t ext_essent_len;
> >>  	Elf_Scn *scn;
> >>  	Elf64_Shdr *sh;
> >>  
> >> @@ -3819,6 +3821,14 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
> >>  		ext->sym_idx = i;
> >>  		ext->is_weak = ELF64_ST_BIND(sym->st_info) == STB_WEAK;
> >>  
> >> +		ext_essent_len = bpf_core_essential_name_len(ext->name);
> >> +		ext->essent_name = NULL;
> >> +		if (ext_essent_len != strlen(ext->name)) {
> >> +			ext->essent_name = strndup(ext->name, ext_essent_len);
> >> +			if (!ext->essent_name)
> >> +				return -ENOMEM;
> >> +		}
> >> +
> >>  		ext->sec_btf_id = find_extern_sec_btf_id(obj->btf, ext->btf_id);
> >>  		if (ext->sec_btf_id <= 0) {
> >>  			pr_warn("failed to find BTF for extern '%s' [%d] section: %d\n",
> >> @@ -7624,7 +7634,8 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
> >>  
> >>  	local_func_proto_id = ext->ksym.type_id;
> >>  
> >> -	kfunc_id = find_ksym_btf_id(obj, ext->name, BTF_KIND_FUNC, &kern_btf, &mod_btf);
> >> +	kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
> >> +				    &mod_btf);
> >>  	if (kfunc_id < 0) {
> >>  		if (kfunc_id == -ESRCH && ext->is_weak)
> >>  			return 0;
> >> @@ -7642,6 +7653,9 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
> >>  		pr_warn("extern (func ksym) '%s': func_proto [%d] incompatible with %s [%d]\n",
> >>  			ext->name, local_func_proto_id,
> > 
> > Should we do ext->essent_name ?: ext->name here or in the below pr's as
> > well? Hmm, maybe it would be more clear to keep the full name.
> > 
> 
> Yeah, I agree that the full name should be used in this warning for clarity.
> So won't change.
> 
> >>  			mod_btf ? mod_btf->name : "vmlinux", kfunc_proto_id);
> >> +
> >> +		if (ext->is_weak)
> >> +			return 0;
> > 
> > Could you clarify why we want this check? Don't we want to fail if the
> > prototype of the actual (essent) symbol we resolve to doesn't match
> > what's in the BPF prog? If we do want to keep this, should we do the
> > check above the pr_warn()?
> > 
> 
> Actually this if-and-return was initially above the pr_warn while I was
> developing the patch. I moved it down here to confirm via './test_progs -vv'
> that the pseudo-failure cases in the selftests were going down the codepaths
> I expected, and left it b/c better to err on the side of too much logging
> when doing this ___flavor trickery.

Normally I'd agree, but this is also a pr_warn(), so it goes a bit
beyond logging IMO. FWIW, I'd vote for erring on the side of matching
the existing behavior of other __weak special symbol resolution.

Edit: Saw your other comment below, which I've responded to more
substantively below as well.

> 
> In re: "clarify why we want this check?" and subsequent question, IIUC, with an
> extern decl like
> 
>   struct task_struct *bpf_task_acquire___one(struct task_struct *task) __ksym __weak;
> 
> if we removed __weak from the declaration, symbol resolution would happen during
> compilation + linking, at which point there would be no opportunity to do
> our ___flavor trickery. But __weak is already used to express "if this kfunc
> doesn't exist at all, it's not a problem, don't fail loading the program". So

To clarify -- I wasn't asking why we need to specify __weak, I was
asking why you added an additional check for __weak on this branch. The
original check on the find_ksym_btf_id() path made sense to me:

kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
			    &mod_btf);
if (kfunc_id < 0) {
	if (kfunc_id == -ESRCH && ext->is_weak)
		return 0;

If the symbol isn't found, it's weak, so it's OK. This next check is
saying if the symbol is found BUT it doesn't match the BTF type that the
kernel expects, that it's OK if it's weak. I wasn't understanding why
__weak would apply here (usually __weak is intended to mean "it's OK to
override this symbol with another symbol with the exact same
declaration", though in practice I don't think symbol resolution works
that way in every dynamic linker). After thinking about it some more, I
guess it's necessary to accommodate the case of e.g.
bpf_task_acquire___three() not matching the BTF signature in the kernel,
and allowing another symbol like bpf_task_acquire___one() to be resolved
on another pass?

> as of this version of the code, it's not possible to express "one of
> bpf_task_acquire___{one,two,three} must resolve, otherwise fail to
> load" - that check would have to be done at runtime like
> 
>   if (!(bpf_ksym_exists(bpf_task_acquire___one) ||
>         bpf_ksym_exists(bpf_task_acquire___two) ||
>         bpf_ksym_exists(bpf_task_acquire___three)) {
>     /* communicate failure to userspace runner via global var or something */
>     return 0;
>   }
> 
> Maybe something like BTF tags could be used to group a set of __weak
> kfunc declarations together such that one (probably _only_ one) of them
> must resolve at load time. This would obviate the need for such a runtime
> check without causing compile+link step to fail. But also seems overly
> complex for now.

This does sound indeed useful. As explained above, I wasn't intending to
imply that we didn't need __weak, but regardless this sounds like a nice
to have at some point down the line.

> Feels useful to have "incompatible resolution" log message even if it doesn't
> stop loading process. But because __weak ties ___flavor trickery to "not a 
> problem if kfunc doesn't exist at all", probably more accurate to make the
> pr_warn a pr_debug if ___flavor AND ext->is_weak. Adding the logic to do that
> felt like it would raise more questions than answers to a future reader of the
> code, so I didn't add it. Now that I'm writing this out, I think it's better
> to add it along with a comment.

Yeah, I agree with you -- in my experience, it's common for automation
to be setup that does nothing other than search for logs with level >=
warn and raise some alert if any is encountered. I think it's best to
keep the warn namespace relegated to logging actual error states for
that reason. So I agree with you that I think this is the proper way
forward, though IMHO I wouldn't even bother with the pr_debug() here,
just like we don't with the find_ksym_btf_id() case above. It's fine if
you want to add it though, I don't feel strongly either way.

Thanks,
David

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add CO-RE relocs kfunc flavors tests

[David Vernet]

On Wed, Aug 16, 2023 at 03:10:23PM -0400, David Marchevsky wrote:
> On 8/16/23 1:44 PM, David Vernet wrote:
> > On Wed, Aug 16, 2023 at 09:58:13AM -0700, Dave Marchevsky wrote:
> >> This patch adds selftests that exercise kfunc flavor relocation
> >> functionality added in the previous patch. The actual kfunc defined in
> >> kernel/bpf/helpers.c is
> >>
> >>   struct task_struct *bpf_task_acquire(struct task_struct *p)
> >>
> >> The following relocation behaviors are checked:
> >>
> >>   struct task_struct *bpf_task_acquire___one(struct task_struct *name)
> >>     * Should succeed despite differing param name
> >>
> >>   struct task_struct *bpf_task_acquire___two(struct task_struct *p, void *ctx)
> >>     * Should fail because there is no two-param bpf_task_acquire
> >>
> >>   struct task_struct *bpf_task_acquire___three(void *ctx)
> >>     * Should fail because, despite vmlinux's bpf_task_acquire having one param,
> >>       the types don't match
> >>
> >> Changelog:
> >> v1 -> v2: https://lore.kernel.org/bpf/20230811201346.3240403-2-davemarchevsky@fb.com/
> >>   * Change comment on bpf_task_acquire___two to more accurately reflect
> >>     that it fails in same codepath as bpf_task_acquire___three, and to
> >>     not mention dead code elimination as thats an implementation detail
> >>     (Yonghong)
> >>
> >> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> >> ---
> >>  .../selftests/bpf/prog_tests/task_kfunc.c     |  1 +
> >>  .../selftests/bpf/progs/task_kfunc_success.c  | 37 +++++++++++++++++++
> >>  2 files changed, 38 insertions(+)
> >>
> >> diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> >> index 740d5f644b40..99abb0350154 100644
> >> --- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> >> +++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> >> @@ -79,6 +79,7 @@ static const char * const success_tests[] = {
> >>  	"test_task_from_pid_current",
> >>  	"test_task_from_pid_invalid",
> >>  	"task_kfunc_acquire_trusted_walked",
> >> +	"test_task_kfunc_flavor_relo",
> >>  };
> >>  
> >>  void test_task_kfunc(void)
> >> diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_success.c b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
> >> index b09371bba204..ffbe3ff72639 100644
> >> --- a/tools/testing/selftests/bpf/progs/task_kfunc_success.c
> >> +++ b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
> > 
> > Do you think it's worth it to also add a failure case for if there's no
> > correct bpf_taks_acquire___one(), to verify e.g. that we can't resolve
> > bpf_task_acquire___three(void *ctx) __ksym __weak?
> > 
> 
> IIUC you're asking about whether it's possible to fail loading the program
> entirely if _none_ of the three variants resolve successfully? If so, I
> sent out a response to another email in this round of your comments that should
> address it.

Sorry, that was unclear in the way I worded it. I understand that the
program will still load if none of the variants resolve succesfully. I
was asking whether we should add a test that verifies that the wrong
variant won't be resolved if a correct one isn't present. Maybe that's
overkill? Seems prudent to add just in case, though. Something like
this:

SEC("tp_btf/task_newtask")
int BPF_PROG(test_task_kfunc_flavor_relo_not_found,
	     struct task_struct *task, u64 clone_flags)
{
	/* Neither symbol should resolve successfully. */
        if (bpf_ksym_exists(bpf_task_acquire___two))
                err = 1;
        else if (bpf_ksym_exists(bpf_task_acquire___three))
                err = 2;
	
	return 0;
}

> 
> >> @@ -18,6 +18,13 @@ int err, pid;
> >>   */
> >>  
> >>  struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak;
> >> +
> >> +struct task_struct *bpf_task_acquire___one(struct task_struct *task) __ksym __weak;
> >> +/* The two-param bpf_task_acquire doesn't exist */
> >> +struct task_struct *bpf_task_acquire___two(struct task_struct *p, void *ctx) __ksym __weak;
> >> +/* Incorrect type for first param */
> >> +struct task_struct *bpf_task_acquire___three(void *ctx) __ksym __weak;
> >> +
> >>  void invalid_kfunc(void) __ksym __weak;
> >>  void bpf_testmod_test_mod_kfunc(int i) __ksym __weak;
> >>  
> >> @@ -55,6 +62,36 @@ static int test_acquire_release(struct task_struct *task)
> >>  	return 0;
> >>  }
> >>  
> >> +SEC("tp_btf/task_newtask")
> >> +int BPF_PROG(test_task_kfunc_flavor_relo, struct task_struct *task, u64 clone_flags)
> >> +{
> >> +	struct task_struct *acquired = NULL;
> >> +	int fake_ctx = 42;
> >> +
> >> +	if (bpf_ksym_exists(bpf_task_acquire___one)) {
> >> +		acquired = bpf_task_acquire___one(task);
> >> +	} else if (bpf_ksym_exists(bpf_task_acquire___two)) {
> >> +		/* Here, bpf_object__resolve_ksym_func_btf_id's find_ksym_btf_id
> >> +		 * call will find vmlinux's bpf_task_acquire, but subsequent
> >> +		 * bpf_core_types_are_compat will fail
> >> +		 */
> >> +		acquired = bpf_task_acquire___two(task, &fake_ctx);
> >> +		err = 3;
> >> +		return 0;
> >> +	} else if (bpf_ksym_exists(bpf_task_acquire___three)) {
> >> +		/* bpf_core_types_are_compat will fail similarly to above case */
> >> +		acquired = bpf_task_acquire___three(&fake_ctx);
> >> +		err = 4;
> >> +		return 0;
> >> +	}
> >> +
> >> +	if (acquired)
> >> +		bpf_task_release(acquired);
> > 
> > Might be slightly simpler to do the release + return immediately in the
> > bpf_task_acquire___one branch, and then to just do the following here
> > without the if / else:
> > 
> > err = 5;
> > return 0;
> > 
> > What do you think?
> > 
> 
> Eh, I like this form more because it's easier to visually distinguish that the
> bpf_task_acquire___one case above is not a 'failure' case and should
> successfully resolve, whereas the other two bail out early.
> 
> >> +	else
> >> +		err = 5;
> >> +	return 0;
> >> +}
> >> +
> >>  SEC("tp_btf/task_newtask")
> >>  int BPF_PROG(test_task_acquire_release_argument, struct task_struct *task, u64 clone_flags)
> >>  {
> >> -- 
> >> 2.34.1
> >>
> >>

Re: [PATCH v2 bpf-next 1/2] libbpf: Support triple-underscore flavors for kfunc relocation

[David Marchevsky]

On 8/16/23 3:28 PM, David Vernet wrote:
> On Wed, Aug 16, 2023 at 03:01:10PM -0400, David Marchevsky wrote:
>> On 8/16/23 1:37 PM, David Vernet wrote:
>>> On Wed, Aug 16, 2023 at 09:58:12AM -0700, Dave Marchevsky wrote:
>>>> The function signature of kfuncs can change at any time due to their
>>>> intentional lack of stability guarantees. As kfuncs become more widely
>>>> used, BPF program writers will need facilities to support calling
>>>> different versions of a kfunc from a single BPF object. Consider this
>>>> simplified example based on a real scenario we ran into at Meta:
>>>>
>>>>   /* initial kfunc signature */
>>>>   int some_kfunc(void *ptr)
>>>>
>>>>   /* Oops, we need to add some flag to modify behavior. No problem,
>>>>     change the kfunc. flags = 0 retains original behavior */
>>>>   int some_kfunc(void *ptr, long flags)
>>>>
>>>> If the initial version of the kfunc is deployed on some portion of the
>>>> fleet and the new version on the rest, a fleetwide service that uses
>>>> some_kfunc will currently need to load different BPF programs depending
>>>> on which some_kfunc is available.
>>>>
>>>> Luckily CO-RE provides a facility to solve a very similar problem,
>>>> struct definition changes, by allowing program writers to declare
>>>> my_struct___old and my_struct___new, with ___suffix being considered a
>>>> 'flavor' of the non-suffixed name and being ignored by
>>>> bpf_core_type_exists and similar calls.
>>>>
>>>> This patch extends the 'flavor' facility to the kfunc extern
>>>> relocation process. BPF program writers can now declare
>>>>
>>>>   extern int some_kfunc___old(void *ptr)
>>>>   extern int some_kfunc___new(void *ptr, int flags)
>>>>
>>>> then test which version of the kfunc exists with bpf_ksym_exists.
>>>> Relocation and verifier's dead code elimination will work in concert as
>>>> expected, allowing this pattern:
>>>>
>>>>   if (bpf_ksym_exists(some_kfunc___old))
>>>>     some_kfunc___old(ptr);
>>>>   else
>>>>     some_kfunc___new(ptr, 0);
>>>>
>>>> Changelog:
>>>>
>>>> v1 -> v2: https://lore.kernel.org/bpf/20230811201346.3240403-1-davemarchevsky@fb.com/
>>>>   * No need to check obj->externs[i].essent_name before zfree (Jiri)
>>>>   * Use strndup instead of replicating same functionality (Jiri)
>>>>   * Properly handle memory allocation falure (Stanislav)
>>>>
>>>> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
>>>> ---
>>>>  tools/lib/bpf/libbpf.c | 20 +++++++++++++++++++-
>>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>>> index b14a4376a86e..8899abc04b8c 100644
>>>> --- a/tools/lib/bpf/libbpf.c
>>>> +++ b/tools/lib/bpf/libbpf.c
>>>> @@ -550,6 +550,7 @@ struct extern_desc {
>>>>  	int btf_id;
>>>>  	int sec_btf_id;
>>>>  	const char *name;
>>>> +	char *essent_name;
>>>>  	bool is_set;
>>>>  	bool is_weak;
>>>>  	union {
>>>> @@ -3770,6 +3771,7 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
>>>>  	struct extern_desc *ext;
>>>>  	int i, n, off, dummy_var_btf_id;
>>>>  	const char *ext_name, *sec_name;
>>>> +	size_t ext_essent_len;
>>>>  	Elf_Scn *scn;
>>>>  	Elf64_Shdr *sh;
>>>>  
>>>> @@ -3819,6 +3821,14 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
>>>>  		ext->sym_idx = i;
>>>>  		ext->is_weak = ELF64_ST_BIND(sym->st_info) == STB_WEAK;
>>>>  
>>>> +		ext_essent_len = bpf_core_essential_name_len(ext->name);
>>>> +		ext->essent_name = NULL;
>>>> +		if (ext_essent_len != strlen(ext->name)) {
>>>> +			ext->essent_name = strndup(ext->name, ext_essent_len);
>>>> +			if (!ext->essent_name)
>>>> +				return -ENOMEM;
>>>> +		}
>>>> +
>>>>  		ext->sec_btf_id = find_extern_sec_btf_id(obj->btf, ext->btf_id);
>>>>  		if (ext->sec_btf_id <= 0) {
>>>>  			pr_warn("failed to find BTF for extern '%s' [%d] section: %d\n",
>>>> @@ -7624,7 +7634,8 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
>>>>  
>>>>  	local_func_proto_id = ext->ksym.type_id;
>>>>  
>>>> -	kfunc_id = find_ksym_btf_id(obj, ext->name, BTF_KIND_FUNC, &kern_btf, &mod_btf);
>>>> +	kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
>>>> +				    &mod_btf);
>>>>  	if (kfunc_id < 0) {
>>>>  		if (kfunc_id == -ESRCH && ext->is_weak)
>>>>  			return 0;
>>>> @@ -7642,6 +7653,9 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
>>>>  		pr_warn("extern (func ksym) '%s': func_proto [%d] incompatible with %s [%d]\n",
>>>>  			ext->name, local_func_proto_id,
>>>
>>> Should we do ext->essent_name ?: ext->name here or in the below pr's as
>>> well? Hmm, maybe it would be more clear to keep the full name.
>>>
>>
>> Yeah, I agree that the full name should be used in this warning for clarity.
>> So won't change.
>>
>>>>  			mod_btf ? mod_btf->name : "vmlinux", kfunc_proto_id);
>>>> +
>>>> +		if (ext->is_weak)
>>>> +			return 0;
>>>
>>> Could you clarify why we want this check? Don't we want to fail if the
>>> prototype of the actual (essent) symbol we resolve to doesn't match
>>> what's in the BPF prog? If we do want to keep this, should we do the
>>> check above the pr_warn()?
>>>
>>
>> Actually this if-and-return was initially above the pr_warn while I was
>> developing the patch. I moved it down here to confirm via './test_progs -vv'
>> that the pseudo-failure cases in the selftests were going down the codepaths
>> I expected, and left it b/c better to err on the side of too much logging
>> when doing this ___flavor trickery.
> 
> Normally I'd agree, but this is also a pr_warn(), so it goes a bit
> beyond logging IMO. FWIW, I'd vote for erring on the side of matching
> the existing behavior of other __weak special symbol resolution.
> 
> Edit: Saw your other comment below, which I've responded to more
> substantively below as well.
> 

I will respond down there too.

>>
>> In re: "clarify why we want this check?" and subsequent question, IIUC, with an
>> extern decl like
>>
>>   struct task_struct *bpf_task_acquire___one(struct task_struct *task) __ksym __weak;
>>
>> if we removed __weak from the declaration, symbol resolution would happen during
>> compilation + linking, at which point there would be no opportunity to do
>> our ___flavor trickery. But __weak is already used to express "if this kfunc
>> doesn't exist at all, it's not a problem, don't fail loading the program". So
> 
> To clarify -- I wasn't asking why we need to specify __weak, I was
> asking why you added an additional check for __weak on this branch. The
> original check on the find_ksym_btf_id() path made sense to me:
> 
> kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
> 			    &mod_btf);
> if (kfunc_id < 0) {
> 	if (kfunc_id == -ESRCH && ext->is_weak)
> 		return 0;
> 

Ah nice catch, I didn't notice that behavior right up there.
Will match existing behavior.

> If the symbol isn't found, it's weak, so it's OK. This next check is
> saying if the symbol is found BUT it doesn't match the BTF type that the
> kernel expects, that it's OK if it's weak. I wasn't understanding why
> __weak would apply here (usually __weak is intended to mean "it's OK to
> override this symbol with another symbol with the exact same
> declaration", though in practice I don't think symbol resolution works
> that way in every dynamic linker). After thinking about it some more, I
> guess it's necessary to accommodate the case of e.g.
> bpf_task_acquire___three() not matching the BTF signature in the kernel,
> and allowing another symbol like bpf_task_acquire___one() to be resolved
> on another pass?
> 

Yeah precisely, before I added the return 0, any failing
bpf_core_types_are_compat match would cause the file to fail
compilation+link step. If the patch was applied without that
test-and-return, the ___flavor behavior could have only be
used to call the 'correct' func signature matching what's in running kernel
by multiple names.

Re: "usually __weak is intended to mean 'it's OK to override this symbol with
another symbol with the same exact declaration'" - with the caveat that I
didn't really use __weak in "normal" C, most docs that I came across when
googling __weak symbol usage show this pattern:

  if (&some_unresolved_weak_symbol)
    some_unresolved_weak_symbol(arg);

(note that bpf_ksym_exists is essentially !this test + static assert)

So presumably the "optionally use it if defined" scenario is roughly as common
as the "override" scenario. The special ___flavor name logic is the only
thing libbpf C is doing here that's new.

>> as of this version of the code, it's not possible to express "one of
>> bpf_task_acquire___{one,two,three} must resolve, otherwise fail to
>> load" - that check would have to be done at runtime like
>>
>>   if (!(bpf_ksym_exists(bpf_task_acquire___one) ||
>>         bpf_ksym_exists(bpf_task_acquire___two) ||
>>         bpf_ksym_exists(bpf_task_acquire___three)) {
>>     /* communicate failure to userspace runner via global var or something */
>>     return 0;
>>   }
>>
>> Maybe something like BTF tags could be used to group a set of __weak
>> kfunc declarations together such that one (probably _only_ one) of them
>> must resolve at load time. This would obviate the need for such a runtime
>> check without causing compile+link step to fail. But also seems overly
>> complex for now.
> 
> This does sound indeed useful. As explained above, I wasn't intending to
> imply that we didn't need __weak, but regardless this sounds like a nice
> to have at some point down the line.
> 
>> Feels useful to have "incompatible resolution" log message even if it doesn't
>> stop loading process. But because __weak ties ___flavor trickery to "not a 
>> problem if kfunc doesn't exist at all", probably more accurate to make the
>> pr_warn a pr_debug if ___flavor AND ext->is_weak. Adding the logic to do that
>> felt like it would raise more questions than answers to a future reader of the
>> code, so I didn't add it. Now that I'm writing this out, I think it's better
>> to add it along with a comment.
> 
> Yeah, I agree with you -- in my experience, it's common for automation
> to be setup that does nothing other than search for logs with level >=
> warn and raise some alert if any is encountered. I think it's best to
> keep the warn namespace relegated to logging actual error states for
> that reason. So I agree with you that I think this is the proper way
> forward, though IMHO I wouldn't even bother with the pr_debug() here,
> just like we don't with the find_ksym_btf_id() case above. It's fine if
> you want to add it though, I don't feel strongly either way.
> 
> Thanks,
> David

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add CO-RE relocs kfunc flavors tests

[David Marchevsky]

On 8/16/23 3:39 PM, David Vernet wrote:
> On Wed, Aug 16, 2023 at 03:10:23PM -0400, David Marchevsky wrote:
>> On 8/16/23 1:44 PM, David Vernet wrote:
>>> On Wed, Aug 16, 2023 at 09:58:13AM -0700, Dave Marchevsky wrote:
>>>> This patch adds selftests that exercise kfunc flavor relocation
>>>> functionality added in the previous patch. The actual kfunc defined in
>>>> kernel/bpf/helpers.c is
>>>>
>>>>   struct task_struct *bpf_task_acquire(struct task_struct *p)
>>>>
>>>> The following relocation behaviors are checked:
>>>>
>>>>   struct task_struct *bpf_task_acquire___one(struct task_struct *name)
>>>>     * Should succeed despite differing param name
>>>>
>>>>   struct task_struct *bpf_task_acquire___two(struct task_struct *p, void *ctx)
>>>>     * Should fail because there is no two-param bpf_task_acquire
>>>>
>>>>   struct task_struct *bpf_task_acquire___three(void *ctx)
>>>>     * Should fail because, despite vmlinux's bpf_task_acquire having one param,
>>>>       the types don't match
>>>>
>>>> Changelog:
>>>> v1 -> v2: https://lore.kernel.org/bpf/20230811201346.3240403-2-davemarchevsky@fb.com/
>>>>   * Change comment on bpf_task_acquire___two to more accurately reflect
>>>>     that it fails in same codepath as bpf_task_acquire___three, and to
>>>>     not mention dead code elimination as thats an implementation detail
>>>>     (Yonghong)
>>>>
>>>> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
>>>> ---
>>>>  .../selftests/bpf/prog_tests/task_kfunc.c     |  1 +
>>>>  .../selftests/bpf/progs/task_kfunc_success.c  | 37 +++++++++++++++++++
>>>>  2 files changed, 38 insertions(+)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
>>>> index 740d5f644b40..99abb0350154 100644
>>>> --- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
>>>> +++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
>>>> @@ -79,6 +79,7 @@ static const char * const success_tests[] = {
>>>>  	"test_task_from_pid_current",
>>>>  	"test_task_from_pid_invalid",
>>>>  	"task_kfunc_acquire_trusted_walked",
>>>> +	"test_task_kfunc_flavor_relo",
>>>>  };
>>>>  
>>>>  void test_task_kfunc(void)
>>>> diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_success.c b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
>>>> index b09371bba204..ffbe3ff72639 100644
>>>> --- a/tools/testing/selftests/bpf/progs/task_kfunc_success.c
>>>> +++ b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
>>>
>>> Do you think it's worth it to also add a failure case for if there's no
>>> correct bpf_taks_acquire___one(), to verify e.g. that we can't resolve
>>> bpf_task_acquire___three(void *ctx) __ksym __weak?
>>>
>>
>> IIUC you're asking about whether it's possible to fail loading the program
>> entirely if _none_ of the three variants resolve successfully? If so, I
>> sent out a response to another email in this round of your comments that should
>> address it.
> 
> Sorry, that was unclear in the way I worded it. I understand that the
> program will still load if none of the variants resolve succesfully. I
> was asking whether we should add a test that verifies that the wrong
> variant won't be resolved if a correct one isn't present. Maybe that's
> overkill? Seems prudent to add just in case, though. Something like
> this:
> 
> SEC("tp_btf/task_newtask")
> int BPF_PROG(test_task_kfunc_flavor_relo_not_found,
> 	     struct task_struct *task, u64 clone_flags)
> {
> 	/* Neither symbol should resolve successfully. */
>         if (bpf_ksym_exists(bpf_task_acquire___two))
>                 err = 1;
>         else if (bpf_ksym_exists(bpf_task_acquire___three))
>                 err = 2;
> 	
> 	return 0;
> }
> 


I was leaning towards pushing back here, but agree with you after digging and
seeing:

  * weak symbols aren't discussed in the C99 standard at all and are an ELF
    specification concept
  * my previous bullet point isn't really relevant to what you're saying here
    as you're talking about the linkage process more generally
  * Then I started digging in the C99 standard and realized that even if there
    was something in there that would allow me to say "well by definition I 
    don't need to test for this", would be too obscure and I should just add the
    test

>>
>>>> @@ -18,6 +18,13 @@ int err, pid;
>>>>   */
>>>>  
>>>>  struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak;
>>>> +
>>>> +struct task_struct *bpf_task_acquire___one(struct task_struct *task) __ksym __weak;
>>>> +/* The two-param bpf_task_acquire doesn't exist */
>>>> +struct task_struct *bpf_task_acquire___two(struct task_struct *p, void *ctx) __ksym __weak;
>>>> +/* Incorrect type for first param */
>>>> +struct task_struct *bpf_task_acquire___three(void *ctx) __ksym __weak;
>>>> +
>>>>  void invalid_kfunc(void) __ksym __weak;
>>>>  void bpf_testmod_test_mod_kfunc(int i) __ksym __weak;
>>>>  
>>>> @@ -55,6 +62,36 @@ static int test_acquire_release(struct task_struct *task)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +SEC("tp_btf/task_newtask")
>>>> +int BPF_PROG(test_task_kfunc_flavor_relo, struct task_struct *task, u64 clone_flags)
>>>> +{
>>>> +	struct task_struct *acquired = NULL;
>>>> +	int fake_ctx = 42;
>>>> +
>>>> +	if (bpf_ksym_exists(bpf_task_acquire___one)) {
>>>> +		acquired = bpf_task_acquire___one(task);
>>>> +	} else if (bpf_ksym_exists(bpf_task_acquire___two)) {
>>>> +		/* Here, bpf_object__resolve_ksym_func_btf_id's find_ksym_btf_id
>>>> +		 * call will find vmlinux's bpf_task_acquire, but subsequent
>>>> +		 * bpf_core_types_are_compat will fail
>>>> +		 */
>>>> +		acquired = bpf_task_acquire___two(task, &fake_ctx);
>>>> +		err = 3;
>>>> +		return 0;
>>>> +	} else if (bpf_ksym_exists(bpf_task_acquire___three)) {
>>>> +		/* bpf_core_types_are_compat will fail similarly to above case */
>>>> +		acquired = bpf_task_acquire___three(&fake_ctx);
>>>> +		err = 4;
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	if (acquired)
>>>> +		bpf_task_release(acquired);
>>>
>>> Might be slightly simpler to do the release + return immediately in the
>>> bpf_task_acquire___one branch, and then to just do the following here
>>> without the if / else:
>>>
>>> err = 5;
>>> return 0;
>>>
>>> What do you think?
>>>
>>
>> Eh, I like this form more because it's easier to visually distinguish that the
>> bpf_task_acquire___one case above is not a 'failure' case and should
>> successfully resolve, whereas the other two bail out early.
>>
>>>> +	else
>>>> +		err = 5;
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  SEC("tp_btf/task_newtask")
>>>>  int BPF_PROG(test_task_acquire_release_argument, struct task_struct *task, u64 clone_flags)
>>>>  {
>>>> -- 
>>>> 2.34.1
>>>>
>>>>