[PATCH bpf-next v2 0/2] bpf: Fix updating attached freplace prog to prog_array map

[PATCH bpf-next v2 0/2] bpf: Fix updating attached freplace prog to prog_array map

[Leon Hwang]

The commit f7866c3587337731 ("bpf: Fix null pointer dereference in
resolve_prog_type() for BPF_PROG_TYPE_EXT") fixed a NULL pointer
dereference panic, but didn't fix the issue that fails to update attached
freplace prog to prog_array map.

This patch fixes the issue.

v1 -> v2:
 * Address comments from Yonghong:
   * Check then return prog->aux->saved_dst_prog_type.
   * Remove ASSERT_GE() that checks prog_fd and map_fd.
   * Remove #include "bpf_legacy.h".
   * Fix some code style issues.

RFC PATCH -> v1:
 * Respin the PATCH with updated message.

Links:
[0] https://lore.kernel.org/bpf/20240602122421.50892-1-hffilwlqm@gmail.com/

Leon Hwang (2):
  bpf: Fix updating attached freplace prog to prog_array map
  selftests/bpf: Add testcase for updating attached freplace prog to
    prog_array map

 include/linux/bpf_verifier.h                  |  4 +-
 .../selftests/bpf/prog_tests/tailcalls.c      | 65 ++++++++++++++++++-
 .../selftests/bpf/progs/tailcall_freplace.c   | 25 +++++++
 .../testing/selftests/bpf/progs/tc_bpf2bpf.c  | 21 ++++++
 4 files changed, 112 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/tailcall_freplace.c
 create mode 100644 tools/testing/selftests/bpf/progs/tc_bpf2bpf.c

-- 
2.44.0

[PATCH bpf-next v2 1/2] bpf: Fix updating attached freplace prog to prog_array map

[Leon Hwang]

The commit f7866c3587337731 ("bpf: Fix null pointer dereference in
resolve_prog_type() for BPF_PROG_TYPE_EXT") fixed a NULL pointer
dereference panic, but didn't fix the issue that fails to update attached
freplace prog to prog_array map.

Since commit 1c123c567fb138eb ("bpf: Resolve fext program type when
checking map compatibility"), freplace prog and its target prog are able
to tail call each other.

And the commit 3aac1ead5eb6b76f ("bpf: Move prog->aux->linked_prog and
trampoline into bpf_link on attach") sets prog->aux->dst_prog as NULL
after attaching freplace prog to its target prog.

Then, as for following example:

tailcall_freplace.c:

// SPDX-License-Identifier: GPL-2.0

\#include <linux/bpf.h>
\#include <bpf/bpf_helpers.h>

struct {
	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
	__uint(max_entries, 1);
	__uint(key_size, sizeof(__u32));
	__uint(value_size, sizeof(__u32));
} jmp_table SEC(".maps");

int count = 0;

SEC("freplace")
int entry_freplace(struct __sk_buff *skb)
{
	count++;

	bpf_tail_call_static(skb, &jmp_table, 0);

	return count;
}

char __license[] SEC("license") = "GPL";

tc_bpf2bpf.c:

// SPDX-License-Identifier: GPL-2.0

\#include <linux/bpf.h>
\#include <bpf/bpf_helpers.h>

__noinline
int subprog(struct __sk_buff *skb)
{
	volatile int ret = 1;

	asm volatile (""::"r+"(ret));
	return ret;
}

SEC("tc")
int entry_tc(struct __sk_buff *skb)
{
	return subprog(skb);
}

char __license[] SEC("license") = "GPL";

And entry_freplace's target is the entry_tc's subprog.

After loading entry_freplace, the jmp_table's owner type is
BPF_PROG_TYPE_SCHED_CLS.

Next, after attaching entry_freplace to entry_tc's subprog, its prog->aux->
dst_prog is NULL.

Next, while updating entry_freplace to jmp_table, bpf_prog_map_compatible()
returns false because resolve_prog_type() returns BPF_PROG_TYPE_EXT instead
of BPF_PROG_TYPE_SCHED_CLS.

With this patch, resolve_prog_type() returns BPF_PROG_TYPE_SCHED_CLS to
support updating the attached entry_freplace to jmp_table.

Fixes: f7866c358733 ("bpf: Fix null pointer dereference in resolve_prog_type() for BPF_PROG_TYPE_EXT")
Cc: Toke Høiland-Jørgensen <toke@redhat.com>
Cc: Martin KaFai Lau <martin.lau@kernel.org>
Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 include/linux/bpf_verifier.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 5cea15c81b8a8..bfd093ac333f2 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -874,8 +874,8 @@ static inline u32 type_flag(u32 type)
 /* only use after check_attach_btf_id() */
 static inline enum bpf_prog_type resolve_prog_type(const struct bpf_prog *prog)
 {
-	return (prog->type == BPF_PROG_TYPE_EXT && prog->aux->dst_prog) ?
-		prog->aux->dst_prog->type : prog->type;
+	return (prog->type == BPF_PROG_TYPE_EXT && prog->aux->saved_dst_prog_type) ?
+		prog->aux->saved_dst_prog_type : prog->type;
 }
 
 static inline bool bpf_prog_check_recur(const struct bpf_prog *prog)
-- 
2.44.0

Re: [PATCH bpf-next v2 1/2] bpf: Fix updating attached freplace prog to prog_array map

[Yonghong Song]

On 7/26/24 8:39 AM, Leon Hwang wrote:
> The commit f7866c3587337731 ("bpf: Fix null pointer dereference in
> resolve_prog_type() for BPF_PROG_TYPE_EXT") fixed a NULL pointer
> dereference panic, but didn't fix the issue that fails to update attached
> freplace prog to prog_array map.
>
> Since commit 1c123c567fb138eb ("bpf: Resolve fext program type when
> checking map compatibility"), freplace prog and its target prog are able
> to tail call each other.
>
> And the commit 3aac1ead5eb6b76f ("bpf: Move prog->aux->linked_prog and
> trampoline into bpf_link on attach") sets prog->aux->dst_prog as NULL
> after attaching freplace prog to its target prog.

Similar to my previous comment in the cover letter, please use 12 alphanum
for the commit and the commit subject should be in the same line.

>
> Then, as for following example:
>
> tailcall_freplace.c:
>
> // SPDX-License-Identifier: GPL-2.0
>
> \#include <linux/bpf.h>
> \#include <bpf/bpf_helpers.h>
>
> struct {
> 	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> 	__uint(max_entries, 1);
> 	__uint(key_size, sizeof(__u32));
> 	__uint(value_size, sizeof(__u32));
> } jmp_table SEC(".maps");
>
> int count = 0;
>
> SEC("freplace")
> int entry_freplace(struct __sk_buff *skb)
> {
> 	count++;

Empty line is not necessary.

> 	bpf_tail_call_static(skb, &jmp_table, 0);

Empty line is not necessary.

> 	return count;
> }
>
> char __license[] SEC("license") = "GPL";
>
> tc_bpf2bpf.c:
>
> // SPDX-License-Identifier: GPL-2.0
>
> \#include <linux/bpf.h>
> \#include <bpf/bpf_helpers.h>
>
> __noinline
> int subprog(struct __sk_buff *skb)
> {
> 	volatile int ret = 1;
>
> 	asm volatile (""::"r+"(ret));

Let us replace the above asm volatile to __sink(ret). Also, I think 'volatile' is not needed.

Also remove the empty line.

> 	return ret;
> }
>
> SEC("tc")
> int entry_tc(struct __sk_buff *skb)
> {
> 	return subprog(skb);
> }
>
> char __license[] SEC("license") = "GPL";
>
> And entry_freplace's target is the entry_tc's subprog.
>
> After loading entry_freplace, the jmp_table's owner type is
> BPF_PROG_TYPE_SCHED_CLS.
>
> Next, after attaching entry_freplace to entry_tc's subprog, its prog->aux->
> dst_prog is NULL.
>
> Next, while updating entry_freplace to jmp_table, bpf_prog_map_compatible()
> returns false because resolve_prog_type() returns BPF_PROG_TYPE_EXT instead
> of BPF_PROG_TYPE_SCHED_CLS.
>
> With this patch, resolve_prog_type() returns BPF_PROG_TYPE_SCHED_CLS to
> support updating the attached entry_freplace to jmp_table.
>
> Fixes: f7866c358733 ("bpf: Fix null pointer dereference in resolve_prog_type() for BPF_PROG_TYPE_EXT")
> Cc: Toke Høiland-Jørgensen <toke@redhat.com>
> Cc: Martin KaFai Lau <martin.lau@kernel.org>
> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>

Other than the above, LGTM.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

[PATCH bpf-next v2 2/2] selftests/bpf: Add testcase for updating attached freplace prog to prog_array map

[Leon Hwang]

Add a selftest to confirm the issue, which gets -EINVAL when update
attached freplace prog to prog_array map, has been fixed.

cd tools/testing/selftests/bpf; ./test_progs -t tailcalls
327/25  tailcalls/tailcall_freplace:OK
327     tailcalls:OK
Summary: 1/25 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 .../selftests/bpf/prog_tests/tailcalls.c      | 65 ++++++++++++++++++-
 .../selftests/bpf/progs/tailcall_freplace.c   | 25 +++++++
 .../testing/selftests/bpf/progs/tc_bpf2bpf.c  | 21 ++++++
 3 files changed, 110 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/progs/tailcall_freplace.c
 create mode 100644 tools/testing/selftests/bpf/progs/tc_bpf2bpf.c

diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
index e01fabb8cc415..21c5a37846ade 100644
--- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
+++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
@@ -5,7 +5,8 @@
 #include "tailcall_poke.skel.h"
 #include "tailcall_bpf2bpf_hierarchy2.skel.h"
 #include "tailcall_bpf2bpf_hierarchy3.skel.h"
-
+#include "tailcall_freplace.skel.h"
+#include "tc_bpf2bpf.skel.h"
 
 /* test_tailcall_1 checks basic functionality by patching multiple locations
  * in a single program for a single tail call slot with nop->jmp, jmp->nop
@@ -1495,6 +1496,66 @@ static void test_tailcall_bpf2bpf_hierarchy_3(void)
 	RUN_TESTS(tailcall_bpf2bpf_hierarchy3);
 }
 
+/* test_tailcall_freplace checks that the attached freplace prog is OK to
+ * update the prog_array map.
+ */
+static void test_tailcall_freplace(void)
+{
+	struct tailcall_freplace *freplace_skel = NULL;
+	struct bpf_link *freplace_link = NULL;
+	struct bpf_program *freplace_prog;
+	struct tc_bpf2bpf *tc_skel = NULL;
+	int prog_fd, map_fd;
+	char buff[128] = {};
+	int err, key;
+
+	LIBBPF_OPTS(bpf_test_run_opts, topts,
+		    .data_in = buff,
+		    .data_size_in = sizeof(buff),
+		    .repeat = 1,
+	);
+
+	freplace_skel = tailcall_freplace__open();
+	if (!ASSERT_OK_PTR(freplace_skel, "tailcall_freplace__open"))
+		return;
+
+	tc_skel = tc_bpf2bpf__open_and_load();
+	if (!ASSERT_OK_PTR(tc_skel, "tc_bpf2bpf__open_and_load"))
+		goto out;
+
+	prog_fd = bpf_program__fd(tc_skel->progs.entry_tc);
+	freplace_prog = freplace_skel->progs.entry_freplace;
+	err = bpf_program__set_attach_target(freplace_prog, prog_fd, "subprog");
+	if (!ASSERT_OK(err, "set_attach_target"))
+		goto out;
+
+	err = tailcall_freplace__load(freplace_skel);
+	if (!ASSERT_OK(err, "tailcall_freplace__load"))
+		goto out;
+
+	freplace_link = bpf_program__attach_freplace(freplace_prog, prog_fd,
+						     "subprog");
+	if (!ASSERT_OK_PTR(freplace_link, "attach_freplace"))
+		goto out;
+
+	map_fd = bpf_map__fd(freplace_skel->maps.jmp_table);
+	prog_fd = bpf_program__fd(freplace_prog);
+	key = 0;
+	err = bpf_map_update_elem(map_fd, &key, &prog_fd, BPF_ANY);
+	if (!ASSERT_OK(err, "update jmp_table"))
+		goto out;
+
+	prog_fd = bpf_program__fd(tc_skel->progs.entry_tc);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	ASSERT_OK(err, "test_run");
+	ASSERT_EQ(topts.retval, 34, "test_run retval");
+
+out:
+	bpf_link__destroy(freplace_link);
+	tc_bpf2bpf__destroy(tc_skel);
+	tailcall_freplace__destroy(freplace_skel);
+}
+
 void test_tailcalls(void)
 {
 	if (test__start_subtest("tailcall_1"))
@@ -1543,4 +1604,6 @@ void test_tailcalls(void)
 		test_tailcall_bpf2bpf_hierarchy_fentry_entry();
 	test_tailcall_bpf2bpf_hierarchy_2();
 	test_tailcall_bpf2bpf_hierarchy_3();
+	if (test__start_subtest("tailcall_freplace"))
+		test_tailcall_freplace();
 }
diff --git a/tools/testing/selftests/bpf/progs/tailcall_freplace.c b/tools/testing/selftests/bpf/progs/tailcall_freplace.c
new file mode 100644
index 0000000000000..2966efc06ae8f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tailcall_freplace.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+	__uint(max_entries, 1);
+	__uint(key_size, sizeof(__u32));
+	__uint(value_size, sizeof(__u32));
+} jmp_table SEC(".maps");
+
+int count = 0;
+
+SEC("freplace")
+int entry_freplace(struct __sk_buff *skb)
+{
+	count++;
+
+	bpf_tail_call_static(skb, &jmp_table, 0);
+
+	return count;
+}
+
+char __license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
new file mode 100644
index 0000000000000..980bb810b481c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+__noinline
+int subprog(struct __sk_buff *skb)
+{
+	volatile int ret = 1;
+
+	asm volatile (""::"r+"(ret));
+	return ret;
+}
+
+SEC("tc")
+int entry_tc(struct __sk_buff *skb)
+{
+	return subprog(skb);
+}
+
+char __license[] SEC("license") = "GPL";
-- 
2.44.0

Re: [PATCH bpf-next v2 2/2] selftests/bpf: Add testcase for updating attached freplace prog to prog_array map

[Yonghong Song]

On 7/26/24 8:39 AM, Leon Hwang wrote:
> Add a selftest to confirm the issue, which gets -EINVAL when update
> attached freplace prog to prog_array map, has been fixed.
>
> cd tools/testing/selftests/bpf; ./test_progs -t tailcalls
> 327/25  tailcalls/tailcall_freplace:OK
> 327     tailcalls:OK
> Summary: 1/25 PASSED, 0 SKIPPED, 0 FAILED
>
> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>

LGTM with some comments below.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   .../selftests/bpf/prog_tests/tailcalls.c      | 65 ++++++++++++++++++-
>   .../selftests/bpf/progs/tailcall_freplace.c   | 25 +++++++
>   .../testing/selftests/bpf/progs/tc_bpf2bpf.c  | 21 ++++++
>   3 files changed, 110 insertions(+), 1 deletion(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/tailcall_freplace.c
>   create mode 100644 tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> index e01fabb8cc415..21c5a37846ade 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> @@ -5,7 +5,8 @@
>   #include "tailcall_poke.skel.h"
>   #include "tailcall_bpf2bpf_hierarchy2.skel.h"
>   #include "tailcall_bpf2bpf_hierarchy3.skel.h"
> -
> +#include "tailcall_freplace.skel.h"
> +#include "tc_bpf2bpf.skel.h"
>   
>   /* test_tailcall_1 checks basic functionality by patching multiple locations
>    * in a single program for a single tail call slot with nop->jmp, jmp->nop
> @@ -1495,6 +1496,66 @@ static void test_tailcall_bpf2bpf_hierarchy_3(void)
>   	RUN_TESTS(tailcall_bpf2bpf_hierarchy3);
>   }
>   
> +/* test_tailcall_freplace checks that the attached freplace prog is OK to
> + * update the prog_array map.
> + */
> +static void test_tailcall_freplace(void)
> +{
> +	struct tailcall_freplace *freplace_skel = NULL;
> +	struct bpf_link *freplace_link = NULL;
> +	struct bpf_program *freplace_prog;
> +	struct tc_bpf2bpf *tc_skel = NULL;
> +	int prog_fd, map_fd;
> +	char buff[128] = {};
> +	int err, key;
> +
> +	LIBBPF_OPTS(bpf_test_run_opts, topts,
> +		    .data_in = buff,
> +		    .data_size_in = sizeof(buff),
> +		    .repeat = 1,
> +	);
> +
> +	freplace_skel = tailcall_freplace__open();
> +	if (!ASSERT_OK_PTR(freplace_skel, "tailcall_freplace__open"))
> +		return;
> +
> +	tc_skel = tc_bpf2bpf__open_and_load();
> +	if (!ASSERT_OK_PTR(tc_skel, "tc_bpf2bpf__open_and_load"))
> +		goto out;
> +
> +	prog_fd = bpf_program__fd(tc_skel->progs.entry_tc);
> +	freplace_prog = freplace_skel->progs.entry_freplace;
> +	err = bpf_program__set_attach_target(freplace_prog, prog_fd, "subprog");
> +	if (!ASSERT_OK(err, "set_attach_target"))
> +		goto out;
> +
> +	err = tailcall_freplace__load(freplace_skel);
> +	if (!ASSERT_OK(err, "tailcall_freplace__load"))
> +		goto out;
> +
> +	freplace_link = bpf_program__attach_freplace(freplace_prog, prog_fd,
> +						     "subprog");
> +	if (!ASSERT_OK_PTR(freplace_link, "attach_freplace"))
> +		goto out;
> +
> +	map_fd = bpf_map__fd(freplace_skel->maps.jmp_table);
> +	prog_fd = bpf_program__fd(freplace_prog);
> +	key = 0;
> +	err = bpf_map_update_elem(map_fd, &key, &prog_fd, BPF_ANY);
> +	if (!ASSERT_OK(err, "update jmp_table"))
> +		goto out;
> +
> +	prog_fd = bpf_program__fd(tc_skel->progs.entry_tc);
> +	err = bpf_prog_test_run_opts(prog_fd, &topts);
> +	ASSERT_OK(err, "test_run");
> +	ASSERT_EQ(topts.retval, 34, "test_run retval");
> +
> +out:
> +	bpf_link__destroy(freplace_link);
> +	tc_bpf2bpf__destroy(tc_skel);
> +	tailcall_freplace__destroy(freplace_skel);
> +}
> +
>   void test_tailcalls(void)
>   {
>   	if (test__start_subtest("tailcall_1"))
> @@ -1543,4 +1604,6 @@ void test_tailcalls(void)
>   		test_tailcall_bpf2bpf_hierarchy_fentry_entry();
>   	test_tailcall_bpf2bpf_hierarchy_2();
>   	test_tailcall_bpf2bpf_hierarchy_3();
> +	if (test__start_subtest("tailcall_freplace"))
> +		test_tailcall_freplace();
>   }
> diff --git a/tools/testing/selftests/bpf/progs/tailcall_freplace.c b/tools/testing/selftests/bpf/progs/tailcall_freplace.c
> new file mode 100644
> index 0000000000000..2966efc06ae8f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/tailcall_freplace.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> +	__uint(max_entries, 1);
> +	__uint(key_size, sizeof(__u32));
> +	__uint(value_size, sizeof(__u32));
> +} jmp_table SEC(".maps");
> +
> +int count = 0;
> +
> +SEC("freplace")
> +int entry_freplace(struct __sk_buff *skb)
> +{
> +	count++;
> +
remove empty line here.
> +	bpf_tail_call_static(skb, &jmp_table, 0);
> +
remove empty line here.
> +	return count;
> +}
> +
> +char __license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
> new file mode 100644
> index 0000000000000..980bb810b481c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +__noinline
> +int subprog(struct __sk_buff *skb)
> +{
> +	volatile int ret = 1;
> +
remove empty line here.
> +	asm volatile (""::"r+"(ret));
remove above 'volatile' key word and replace asm volatile with __sink(ret).
> +	return ret;
> +}
> +
> +SEC("tc")
> +int entry_tc(struct __sk_buff *skb)
> +{
> +	return subprog(skb);
> +}
> +
> +char __license[] SEC("license") = "GPL";

Re: [PATCH bpf-next v2 2/2] selftests/bpf: Add testcase for updating attached freplace prog to prog_array map

[Leon Hwang]

On 2024/7/27 03:38, Yonghong Song wrote:
> 
> On 7/26/24 8:39 AM, Leon Hwang wrote:
>> Add a selftest to confirm the issue, which gets -EINVAL when update
>> attached freplace prog to prog_array map, has been fixed.
>>
>> cd tools/testing/selftests/bpf; ./test_progs -t tailcalls
>> 327/25  tailcalls/tailcall_freplace:OK
>> 327     tailcalls:OK
>> Summary: 1/25 PASSED, 0 SKIPPED, 0 FAILED
>>
>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
> 
> LGTM with some comments below.
> 
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
> 
>> ---
>>   .../selftests/bpf/prog_tests/tailcalls.c      | 65 ++++++++++++++++++-
>>   .../selftests/bpf/progs/tailcall_freplace.c   | 25 +++++++
>>   .../testing/selftests/bpf/progs/tc_bpf2bpf.c  | 21 ++++++
>>   3 files changed, 110 insertions(+), 1 deletion(-)
>>   create mode 100644
>> tools/testing/selftests/bpf/progs/tailcall_freplace.c
>>   create mode 100644 tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>>

[...]

>> diff --git a/tools/testing/selftests/bpf/progs/tailcall_freplace.c
>> b/tools/testing/selftests/bpf/progs/tailcall_freplace.c
>> new file mode 100644
>> index 0000000000000..2966efc06ae8f
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/tailcall_freplace.c
>> @@ -0,0 +1,25 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/bpf.h>
>> +#include <bpf/bpf_helpers.h>
>> +
>> +struct {
>> +    __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
>> +    __uint(max_entries, 1);
>> +    __uint(key_size, sizeof(__u32));
>> +    __uint(value_size, sizeof(__u32));
>> +} jmp_table SEC(".maps");
>> +
>> +int count = 0;
>> +
>> +SEC("freplace")
>> +int entry_freplace(struct __sk_buff *skb)
>> +{
>> +    count++;
>> +
> remove empty line here.
>> +    bpf_tail_call_static(skb, &jmp_table, 0);
>> +
> remove empty line here.
>> +    return count;
>> +}
>> +
>> +char __license[] SEC("license") = "GPL";
>> diff --git a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>> b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>> new file mode 100644
>> index 0000000000000..980bb810b481c
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>> @@ -0,0 +1,21 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/bpf.h>
>> +#include <bpf/bpf_helpers.h>
>> +
>> +__noinline
>> +int subprog(struct __sk_buff *skb)
>> +{
>> +    volatile int ret = 1;
>> +
> remove empty line here.

Should we remove this empty line?

./scripts/checkpatch.pl:

WARNING: Missing a blank line after declarations
#158: FILE: tools/testing/selftests/bpf/progs/tc_bpf2bpf.c:11:
+	int ret = 1;
+	__sink(ret);

>> +    asm volatile (""::"r+"(ret));
> remove above 'volatile' key word and replace asm volatile with __sink(ret).
>> +    return ret;
>> +}
>> +
>> +SEC("tc")
>> +int entry_tc(struct __sk_buff *skb)
>> +{
>> +    return subprog(skb);
>> +}
>> +
>> +char __license[] SEC("license") = "GPL";

Re: [PATCH bpf-next v2 2/2] selftests/bpf: Add testcase for updating attached freplace prog to prog_array map

[Yonghong Song]

On 7/26/24 8:28 PM, Leon Hwang wrote:
>
> On 2024/7/27 03:38, Yonghong Song wrote:
>> On 7/26/24 8:39 AM, Leon Hwang wrote:
>>> Add a selftest to confirm the issue, which gets -EINVAL when update
>>> attached freplace prog to prog_array map, has been fixed.
>>>
>>> cd tools/testing/selftests/bpf; ./test_progs -t tailcalls
>>> 327/25  tailcalls/tailcall_freplace:OK
>>> 327     tailcalls:OK
>>> Summary: 1/25 PASSED, 0 SKIPPED, 0 FAILED
>>>
>>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
>> LGTM with some comments below.
>>
>> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>>
>>> ---
>>>    .../selftests/bpf/prog_tests/tailcalls.c      | 65 ++++++++++++++++++-
>>>    .../selftests/bpf/progs/tailcall_freplace.c   | 25 +++++++
>>>    .../testing/selftests/bpf/progs/tc_bpf2bpf.c  | 21 ++++++
>>>    3 files changed, 110 insertions(+), 1 deletion(-)
>>>    create mode 100644
>>> tools/testing/selftests/bpf/progs/tailcall_freplace.c
>>>    create mode 100644 tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>>>
> [...]
>
>>> diff --git a/tools/testing/selftests/bpf/progs/tailcall_freplace.c
>>> b/tools/testing/selftests/bpf/progs/tailcall_freplace.c
>>> new file mode 100644
>>> index 0000000000000..2966efc06ae8f
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/bpf/progs/tailcall_freplace.c
>>> @@ -0,0 +1,25 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +#include <linux/bpf.h>
>>> +#include <bpf/bpf_helpers.h>
>>> +
>>> +struct {
>>> +    __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
>>> +    __uint(max_entries, 1);
>>> +    __uint(key_size, sizeof(__u32));
>>> +    __uint(value_size, sizeof(__u32));
>>> +} jmp_table SEC(".maps");
>>> +
>>> +int count = 0;
>>> +
>>> +SEC("freplace")
>>> +int entry_freplace(struct __sk_buff *skb)
>>> +{
>>> +    count++;
>>> +
>> remove empty line here.
>>> +    bpf_tail_call_static(skb, &jmp_table, 0);
>>> +
>> remove empty line here.
>>> +    return count;
>>> +}
>>> +
>>> +char __license[] SEC("license") = "GPL";
>>> diff --git a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>>> b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>>> new file mode 100644
>>> index 0000000000000..980bb810b481c
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>>> @@ -0,0 +1,21 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +#include <linux/bpf.h>
>>> +#include <bpf/bpf_helpers.h>
>>> +
>>> +__noinline
>>> +int subprog(struct __sk_buff *skb)
>>> +{
>>> +    volatile int ret = 1;
>>> +
>> remove empty line here.
> Should we remove this empty line?
>
> ./scripts/checkpatch.pl:
>
> WARNING: Missing a blank line after declarations
> #158: FILE: tools/testing/selftests/bpf/progs/tc_bpf2bpf.c:11:
> +	int ret = 1;
> +	__sink(ret);
sorry, we should keep blank line between 'int ret = 1' and '__sink(ret)'.
>
>>> +    asm volatile (""::"r+"(ret));
>> remove above 'volatile' key word and replace asm volatile with __sink(ret).
>>> +    return ret;
>>> +}
>>> +
>>> +SEC("tc")
>>> +int entry_tc(struct __sk_buff *skb)
>>> +{
>>> +    return subprog(skb);
>>> +}
>>> +
>>> +char __license[] SEC("license") = "GPL";

Re: [PATCH bpf-next v2 0/2] bpf: Fix updating attached freplace prog to prog_array map

[Yonghong Song]

On 7/26/24 8:39 AM, Leon Hwang wrote:
> The commit f7866c3587337731 ("bpf: Fix null pointer dereference in
> resolve_prog_type() for BPF_PROG_TYPE_EXT") fixed a NULL pointer

Typically 12 alphanums are used for a commit. So please use
   f7866c358733 ("bpf: Fix null pointer dereference in resolve_prog_type() for BPF_PROG_TYPE_EXT")
Please have the commit subject ("bpf: Fix ...") in the same line for easy search purpose.

> dereference panic, but didn't fix the issue that fails to update attached
> freplace prog to prog_array map.
>
> This patch fixes the issue.
>
> v1 -> v2:
>   * Address comments from Yonghong:
>     * Check then return prog->aux->saved_dst_prog_type.
>     * Remove ASSERT_GE() that checks prog_fd and map_fd.
>     * Remove #include "bpf_legacy.h".
>     * Fix some code style issues.
>
> RFC PATCH -> v1:
>   * Respin the PATCH with updated message.
>
> Links:
> [0] https://lore.kernel.org/bpf/20240602122421.50892-1-hffilwlqm@gmail.com/
>
> Leon Hwang (2):
>    bpf: Fix updating attached freplace prog to prog_array map
>    selftests/bpf: Add testcase for updating attached freplace prog to
>      prog_array map
>
>   include/linux/bpf_verifier.h                  |  4 +-
>   .../selftests/bpf/prog_tests/tailcalls.c      | 65 ++++++++++++++++++-
>   .../selftests/bpf/progs/tailcall_freplace.c   | 25 +++++++
>   .../testing/selftests/bpf/progs/tc_bpf2bpf.c  | 21 ++++++
>   4 files changed, 112 insertions(+), 3 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/tailcall_freplace.c
>   create mode 100644 tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
>