[PATCH bpf 1/2] bpf: Fix calling global functions from BPF_PROG_TYPE_EXT programs

[PATCH bpf 1/2] bpf: Fix calling global functions from BPF_PROG_TYPE_EXT programs

[Toke Høiland-Jørgensen]

The verifier allows programs to call global functions as long as their
argument types match, using BTF to check the function arguments. One of the
allowed argument types to such global functions is PTR_TO_CTX; however the
check for this fails on BPF_PROG_TYPE_EXT functions because the verifier
uses the wrong type to fetch the vmlinux BTF ID for the program context
type. This failure is seen when an XDP program is loaded using
libxdp (which loads it as BPF_PROG_TYPE_EXT and attaches it to a global XDP
type program).

Fix the issue by passing in the target program type instead of the
BPF_PROG_TYPE_EXT type to bpf_prog_get_ctx() when checking function
argument compatibility.

The first Fixes tag refers to the latest commit that touched the code in
question, while the second one points to the code that first introduced
the global function call verification.

Fixes: 3363bd0cfbb8 ("bpf: Extend kfunc with PTR_TO_CTX, PTR_TO_MEM argument support")
Fixes: 51c39bb1d5d1 ("bpf: Introduce function-by-function verification")
Reported-by: Simon Sundberg <simon.sundberg@kau.se>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 kernel/bpf/btf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 7bccaa4646e5..361de7304c4d 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6054,6 +6054,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				    struct bpf_reg_state *regs,
 				    bool ptr_to_mem_ok)
 {
+	enum bpf_prog_type prog_type = env->prog->type;
 	struct bpf_verifier_log *log = &env->log;
 	u32 i, nargs, ref_id, ref_obj_id = 0;
 	bool is_kfunc = btf_is_kernel(btf);
@@ -6095,6 +6096,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 						     BTF_KFUNC_TYPE_KPTR_ACQUIRE, func_id);
 	}
 
+	if (prog_type == BPF_PROG_TYPE_EXT && env->prog->aux->dst_prog)
+		prog_type = env->prog->aux->dst_prog->type;
+
 	/* check that BTF function arguments match actual types that the
 	 * verifier sees.
 	 */
@@ -6171,7 +6175,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				return -EINVAL;
 			}
 			/* rest of the arguments can be anything, like normal kfunc */
-		} else if (btf_get_prog_ctx_type(log, btf, t, env->prog->type, i)) {
+		} else if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
 			/* If function expects ctx type in BTF check that caller
 			 * is passing PTR_TO_CTX.
 			 */
-- 
2.36.1

[PATCH bpf 2/2] selftests/bpf: Add selftest for calling global functions from freplace

[Toke Høiland-Jørgensen]

Add a selftest that calls a global function with a context object parameter
from an freplace function to check that the program context type is
correctly converted to the freplace target when fetching the context type
from the kernel BTF.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  | 13 ++++++++++
 .../bpf/progs/freplace_global_func.c          | 24 +++++++++++++++++++
 2 files changed, 37 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_global_func.c

diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index d9aad15e0d24..6e86a1d92e97 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -395,6 +395,17 @@ static void test_func_map_prog_compatibility(void)
 				     "./test_attach_probe.o");
 }
 
+static void test_func_replace_global_func(void)
+{
+	const char *prog_name[] = {
+		"freplace/test_pkt_access",
+	};
+	test_fexit_bpf2bpf_common("./freplace_global_func.o",
+				  "./test_pkt_access.o",
+				  ARRAY_SIZE(prog_name),
+				  prog_name, false, NULL);
+}
+
 /* NOTE: affect other tests, must run in serial mode */
 void serial_test_fexit_bpf2bpf(void)
 {
@@ -416,4 +427,6 @@ void serial_test_fexit_bpf2bpf(void)
 		test_func_replace_multi();
 	if (test__start_subtest("fmod_ret_freplace"))
 		test_fmod_ret_freplace();
+	if (test__start_subtest("func_replace_global_func"))
+		test_func_replace_global_func();
 }
diff --git a/tools/testing/selftests/bpf/progs/freplace_global_func.c b/tools/testing/selftests/bpf/progs/freplace_global_func.c
new file mode 100644
index 000000000000..d9f8276229cc
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/freplace_global_func.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2019 Facebook */
+#include <linux/stddef.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+#include <bpf/bpf_tracing.h>
+
+__attribute__ ((noinline))
+int test_ctx_global_func(struct __sk_buff *skb)
+{
+	volatile int retval = 1;
+	return retval;
+}
+
+__u64 test_pkt_access_global_func = 0;
+SEC("freplace/test_pkt_access")
+int new_test_pkt_access(struct __sk_buff *skb)
+{
+	test_pkt_access_global_func = test_ctx_global_func(skb);
+	return -1;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.36.1

Re: [PATCH bpf 1/2] bpf: Fix calling global functions from BPF_PROG_TYPE_EXT programs

[Kumar Kartikeya Dwivedi]

On Fri, Jun 03, 2022 at 09:10:26PM IST, Toke Høiland-Jørgensen wrote:
> The verifier allows programs to call global functions as long as their
> argument types match, using BTF to check the function arguments. One of the
> allowed argument types to such global functions is PTR_TO_CTX; however the
> check for this fails on BPF_PROG_TYPE_EXT functions because the verifier
> uses the wrong type to fetch the vmlinux BTF ID for the program context
> type. This failure is seen when an XDP program is loaded using
> libxdp (which loads it as BPF_PROG_TYPE_EXT and attaches it to a global XDP
> type program).
>
> Fix the issue by passing in the target program type instead of the
> BPF_PROG_TYPE_EXT type to bpf_prog_get_ctx() when checking function
> argument compatibility.
>
> The first Fixes tag refers to the latest commit that touched the code in
> question, while the second one points to the code that first introduced
> the global function call verification.
>
> Fixes: 3363bd0cfbb8 ("bpf: Extend kfunc with PTR_TO_CTX, PTR_TO_MEM argument support")
> Fixes: 51c39bb1d5d1 ("bpf: Introduce function-by-function verification")
> Reported-by: Simon Sundberg <simon.sundberg@kau.se>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  kernel/bpf/btf.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 7bccaa4646e5..361de7304c4d 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6054,6 +6054,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  				    struct bpf_reg_state *regs,
>  				    bool ptr_to_mem_ok)
>  {
> +	enum bpf_prog_type prog_type = env->prog->type;
>  	struct bpf_verifier_log *log = &env->log;
>  	u32 i, nargs, ref_id, ref_obj_id = 0;
>  	bool is_kfunc = btf_is_kernel(btf);
> @@ -6095,6 +6096,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  						     BTF_KFUNC_TYPE_KPTR_ACQUIRE, func_id);
>  	}
>
> +	if (prog_type == BPF_PROG_TYPE_EXT && env->prog->aux->dst_prog)
> +		prog_type = env->prog->aux->dst_prog->type;
> +

nit: it might be better to reuse resolve_prog_type here.

>  	/* check that BTF function arguments match actual types that the
>  	 * verifier sees.
>  	 */
> @@ -6171,7 +6175,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  				return -EINVAL;
>  			}
>  			/* rest of the arguments can be anything, like normal kfunc */
> -		} else if (btf_get_prog_ctx_type(log, btf, t, env->prog->type, i)) {
> +		} else if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
>  			/* If function expects ctx type in BTF check that caller
>  			 * is passing PTR_TO_CTX.
>  			 */
> --
> 2.36.1
>

--
Kartikeya

Re: [PATCH bpf 2/2] selftests/bpf: Add selftest for calling global functions from freplace

[Joanne Koong]

On Fri, Jun 3, 2022 at 11:01 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Add a selftest that calls a global function with a context object parameter
> from an freplace function to check that the program context type is
> correctly converted to the freplace target when fetching the context type
> from the kernel BTF.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  | 13 ++++++++++
>  .../bpf/progs/freplace_global_func.c          | 24 +++++++++++++++++++
>  2 files changed, 37 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/freplace_global_func.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> index d9aad15e0d24..6e86a1d92e97 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> @@ -395,6 +395,17 @@ static void test_func_map_prog_compatibility(void)
>                                      "./test_attach_probe.o");
>  }
>
> +static void test_func_replace_global_func(void)
> +{
> +       const char *prog_name[] = {
> +               "freplace/test_pkt_access",
> +       };
> +       test_fexit_bpf2bpf_common("./freplace_global_func.o",
> +                                 "./test_pkt_access.o",
> +                                 ARRAY_SIZE(prog_name),
> +                                 prog_name, false, NULL);
> +}
> +
>  /* NOTE: affect other tests, must run in serial mode */
>  void serial_test_fexit_bpf2bpf(void)
>  {
> @@ -416,4 +427,6 @@ void serial_test_fexit_bpf2bpf(void)
>                 test_func_replace_multi();
>         if (test__start_subtest("fmod_ret_freplace"))
>                 test_fmod_ret_freplace();
> +       if (test__start_subtest("func_replace_global_func"))
> +               test_func_replace_global_func();
>  }
> diff --git a/tools/testing/selftests/bpf/progs/freplace_global_func.c b/tools/testing/selftests/bpf/progs/freplace_global_func.c
> new file mode 100644
> index 000000000000..d9f8276229cc
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/freplace_global_func.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Facebook */
nit: I think you meant 2022, not 2019? :)
> +#include <linux/stddef.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_endian.h>
> +#include <bpf/bpf_tracing.h>
nit: I think the only headers you need here are the linux/bpf.h and
bpf/bpf_helpers.h ones.
> +
> +__attribute__ ((noinline))
> +int test_ctx_global_func(struct __sk_buff *skb)
> +{
> +       volatile int retval = 1;
> +       return retval;
> +}
> +
> +__u64 test_pkt_access_global_func = 0;
> +SEC("freplace/test_pkt_access")
> +int new_test_pkt_access(struct __sk_buff *skb)
> +{
> +       test_pkt_access_global_func = test_ctx_global_func(skb);
> +       return -1;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.36.1
>

Re: [PATCH bpf 2/2] selftests/bpf: Add selftest for calling global functions from freplace

[Andrii Nakryiko]

On Fri, Jun 3, 2022 at 8:42 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Add a selftest that calls a global function with a context object parameter
> from an freplace function to check that the program context type is
> correctly converted to the freplace target when fetching the context type
> from the kernel BTF.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  | 13 ++++++++++
>  .../bpf/progs/freplace_global_func.c          | 24 +++++++++++++++++++
>  2 files changed, 37 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/freplace_global_func.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> index d9aad15e0d24..6e86a1d92e97 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> @@ -395,6 +395,17 @@ static void test_func_map_prog_compatibility(void)
>                                      "./test_attach_probe.o");
>  }
>
> +static void test_func_replace_global_func(void)
> +{
> +       const char *prog_name[] = {
> +               "freplace/test_pkt_access",
> +       };

empty line between variables and statements

> +       test_fexit_bpf2bpf_common("./freplace_global_func.o",
> +                                 "./test_pkt_access.o",
> +                                 ARRAY_SIZE(prog_name),
> +                                 prog_name, false, NULL);
> +}
> +
>  /* NOTE: affect other tests, must run in serial mode */
>  void serial_test_fexit_bpf2bpf(void)
>  {
> @@ -416,4 +427,6 @@ void serial_test_fexit_bpf2bpf(void)
>                 test_func_replace_multi();
>         if (test__start_subtest("fmod_ret_freplace"))
>                 test_fmod_ret_freplace();
> +       if (test__start_subtest("func_replace_global_func"))
> +               test_func_replace_global_func();
>  }
> diff --git a/tools/testing/selftests/bpf/progs/freplace_global_func.c b/tools/testing/selftests/bpf/progs/freplace_global_func.c
> new file mode 100644
> index 000000000000..d9f8276229cc
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/freplace_global_func.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Facebook */
> +#include <linux/stddef.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_endian.h>
> +#include <bpf/bpf_tracing.h>
> +
> +__attribute__ ((noinline))

__noinline

> +int test_ctx_global_func(struct __sk_buff *skb)
> +{
> +       volatile int retval = 1;
> +       return retval;

just curious, why volatile instead of direct `return 1;`? Also, empty
line between variable and return.

> +}
> +
> +__u64 test_pkt_access_global_func = 0;

nit: it's a variable, please put it at the top before functions, or at
the very least add empty line between it and SEC()

> +SEC("freplace/test_pkt_access")
> +int new_test_pkt_access(struct __sk_buff *skb)
> +{
> +       test_pkt_access_global_func = test_ctx_global_func(skb);
> +       return -1;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.36.1
>

Re: [PATCH bpf 2/2] selftests/bpf: Add selftest for calling global functions from freplace

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Fri, Jun 3, 2022 at 8:42 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Add a selftest that calls a global function with a context object parameter
>> from an freplace function to check that the program context type is
>> correctly converted to the freplace target when fetching the context type
>> from the kernel BTF.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>>  .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  | 13 ++++++++++
>>  .../bpf/progs/freplace_global_func.c          | 24 +++++++++++++++++++
>>  2 files changed, 37 insertions(+)
>>  create mode 100644 tools/testing/selftests/bpf/progs/freplace_global_func.c
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
>> index d9aad15e0d24..6e86a1d92e97 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
>> @@ -395,6 +395,17 @@ static void test_func_map_prog_compatibility(void)
>>                                      "./test_attach_probe.o");
>>  }
>>
>> +static void test_func_replace_global_func(void)
>> +{
>> +       const char *prog_name[] = {
>> +               "freplace/test_pkt_access",
>> +       };
>
> empty line between variables and statements
>
>> +       test_fexit_bpf2bpf_common("./freplace_global_func.o",
>> +                                 "./test_pkt_access.o",
>> +                                 ARRAY_SIZE(prog_name),
>> +                                 prog_name, false, NULL);
>> +}
>> +
>>  /* NOTE: affect other tests, must run in serial mode */
>>  void serial_test_fexit_bpf2bpf(void)
>>  {
>> @@ -416,4 +427,6 @@ void serial_test_fexit_bpf2bpf(void)
>>                 test_func_replace_multi();
>>         if (test__start_subtest("fmod_ret_freplace"))
>>                 test_fmod_ret_freplace();
>> +       if (test__start_subtest("func_replace_global_func"))
>> +               test_func_replace_global_func();
>>  }
>> diff --git a/tools/testing/selftests/bpf/progs/freplace_global_func.c b/tools/testing/selftests/bpf/progs/freplace_global_func.c
>> new file mode 100644
>> index 000000000000..d9f8276229cc
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/freplace_global_func.c
>> @@ -0,0 +1,24 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2019 Facebook */
>> +#include <linux/stddef.h>
>> +#include <linux/bpf.h>
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_endian.h>
>> +#include <bpf/bpf_tracing.h>
>> +
>> +__attribute__ ((noinline))
>
> __noinline
>
>> +int test_ctx_global_func(struct __sk_buff *skb)
>> +{
>> +       volatile int retval = 1;
>> +       return retval;
>
> just curious, why volatile instead of direct `return 1;`? Also, empty
> line between variable and return.

To prevent the compiler from optimising away the whole thing. I was
actually under the impression that it shouldn't do that for global
functions, but, well, it does and the 'volatile' keeps it from doing
so...

Will fix the other nits and resubmit.

-Toke