[PATCHv3 bpf 0/2] bpf: Fix map poke update

[PATCHv3 bpf 0/2] bpf: Fix map poke update

[Jiri Olsa]

hi,
this patchset fixes the issue reported in [0].

v3 changes:
  - moving the update logic to arch function
  - added test [Ilya]

thanks,
jirka


[0] https://syzkaller.appspot.com/bug?extid=97a4fe20470e9bc30810
---
Jiri Olsa (2):
      bpf: Fix prog_array_map_poke_run map poke update
      selftests/bpf: Add test for early update in prog_array_map_poke_run

 arch/x86/net/bpf_jit_comp.c                            | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 kernel/bpf/arraymap.c                                  | 58 ++++++++++------------------------------------------------
 tools/testing/selftests/bpf/prog_tests/tailcall_poke.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/tailcall_poke.c      | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 162 insertions(+), 48 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
 create mode 100644 tools/testing/selftests/bpf/progs/tailcall_poke.c

[PATCHv3 bpf 1/2] bpf: Fix prog_array_map_poke_run map poke update

[Jiri Olsa]

Lee pointed out issue found by syscaller [0] hitting BUG in prog array
map poke update in prog_array_map_poke_run function due to error value
returned from bpf_arch_text_poke function.

There's race window where bpf_arch_text_poke can fail due to missing
bpf program kallsym symbols, which is accounted for with check for
-EINVAL in that BUG_ON call.

The problem is that in such case we won't update the tail call jump
and cause imbalance for the next tail call update check which will
fail with -EBUSY in bpf_arch_text_poke.

I'm hitting following race during the program load:

  CPU 0                             CPU 1

  bpf_prog_load
    bpf_check
      do_misc_fixups
        prog_array_map_poke_track

                                    map_update_elem
                                      bpf_fd_array_map_update_elem
                                        prog_array_map_poke_run

                                          bpf_arch_text_poke returns -EINVAL

    bpf_prog_kallsyms_add

After bpf_arch_text_poke (CPU 1) fails to update the tail call jump, the next
poke update fails on expected jump instruction check in bpf_arch_text_poke
with -EBUSY and triggers the BUG_ON in prog_array_map_poke_run.

Similar race exists on the program unload.

Fixing this by moving the update to bpf_arch_poke_desc_update function which
makes sure we call __bpf_arch_text_poke that skips the bpf address check.

Each architecture has slightly different approach wrt looking up bpf address
in bpf_arch_text_poke, so instead of splitting the function or adding new
'checkip' argument in previous version, it seems best to move the whole
map_poke_run update as arch specific code.

[0] https://syzkaller.appspot.com/bug?extid=97a4fe20470e9bc30810

Cc: Lee Jones <lee@kernel.org>
Cc: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Fixes: ebf7d1f508a7 ("bpf, x64: rework pro/epilogue and tailcall handling in JIT")
Reported-by: syzbot+97a4fe20470e9bc30810@syzkaller.appspotmail.com
Tested-by: Lee Jones <lee@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/net/bpf_jit_comp.c | 46 +++++++++++++++++++++++++++++
 kernel/bpf/arraymap.c       | 58 +++++++------------------------------
 2 files changed, 56 insertions(+), 48 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 8c10d9abc239..e89e415aa743 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -3025,3 +3025,49 @@ void arch_bpf_stack_walk(bool (*consume_fn)(void *cookie, u64 ip, u64 sp, u64 bp
 #endif
 	WARN(1, "verification of programs using bpf_throw should have failed\n");
 }
+
+void bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,
+			       struct bpf_prog *new, struct bpf_prog *old)
+{
+	u8 *old_addr, *new_addr, *old_bypass_addr;
+	int ret;
+
+	old_bypass_addr = old ? NULL : poke->bypass_addr;
+	old_addr = old ? (u8 *)old->bpf_func + poke->adj_off : NULL;
+	new_addr = new ? (u8 *)new->bpf_func + poke->adj_off : NULL;
+
+	/*
+	 * On program loading or teardown, the program's kallsym entry
+	 * might not be in place, so we use __bpf_arch_text_poke to skip
+	 * the kallsyms check.
+	 */
+	if (new) {
+		ret = __bpf_arch_text_poke(poke->tailcall_target,
+					   BPF_MOD_JUMP,
+					   old_addr, new_addr);
+		BUG_ON(ret < 0);
+		if (!old) {
+			ret = __bpf_arch_text_poke(poke->tailcall_bypass,
+						   BPF_MOD_JUMP,
+						   poke->bypass_addr,
+						   NULL);
+			BUG_ON(ret < 0);
+		}
+	} else {
+		ret = __bpf_arch_text_poke(poke->tailcall_bypass,
+					   BPF_MOD_JUMP,
+					   old_bypass_addr,
+					   poke->bypass_addr);
+		BUG_ON(ret < 0);
+		/* let other CPUs finish the execution of program
+		 * so that it will not possible to expose them
+		 * to invalid nop, stack unwind, nop state
+		 */
+		if (!ret)
+			synchronize_rcu();
+		ret = __bpf_arch_text_poke(poke->tailcall_target,
+					   BPF_MOD_JUMP,
+					   old_addr, NULL);
+		BUG_ON(ret < 0);
+	}
+}
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 2058e89b5ddd..c85ff9162a5c 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -1012,11 +1012,16 @@ static void prog_array_map_poke_untrack(struct bpf_map *map,
 	mutex_unlock(&aux->poke_mutex);
 }
 
+void __weak bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,
+				      struct bpf_prog *new, struct bpf_prog *old)
+{
+	WARN_ON_ONCE(1);
+}
+
 static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 				    struct bpf_prog *old,
 				    struct bpf_prog *new)
 {
-	u8 *old_addr, *new_addr, *old_bypass_addr;
 	struct prog_poke_elem *elem;
 	struct bpf_array_aux *aux;
 
@@ -1025,7 +1030,7 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 
 	list_for_each_entry(elem, &aux->poke_progs, list) {
 		struct bpf_jit_poke_descriptor *poke;
-		int i, ret;
+		int i;
 
 		for (i = 0; i < elem->aux->size_poke_tab; i++) {
 			poke = &elem->aux->poke_tab[i];
@@ -1044,21 +1049,10 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 			 *    activated, so tail call updates can arrive from here
 			 *    while JIT is still finishing its final fixup for
 			 *    non-activated poke entries.
-			 * 3) On program teardown, the program's kallsym entry gets
-			 *    removed out of RCU callback, but we can only untrack
-			 *    from sleepable context, therefore bpf_arch_text_poke()
-			 *    might not see that this is in BPF text section and
-			 *    bails out with -EINVAL. As these are unreachable since
-			 *    RCU grace period already passed, we simply skip them.
-			 * 4) Also programs reaching refcount of zero while patching
+			 * 3) Also programs reaching refcount of zero while patching
 			 *    is in progress is okay since we're protected under
 			 *    poke_mutex and untrack the programs before the JIT
-			 *    buffer is freed. When we're still in the middle of
-			 *    patching and suddenly kallsyms entry of the program
-			 *    gets evicted, we just skip the rest which is fine due
-			 *    to point 3).
-			 * 5) Any other error happening below from bpf_arch_text_poke()
-			 *    is a unexpected bug.
+			 *    buffer is freed.
 			 */
 			if (!READ_ONCE(poke->tailcall_target_stable))
 				continue;
@@ -1068,39 +1062,7 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 			    poke->tail_call.key != key)
 				continue;
 
-			old_bypass_addr = old ? NULL : poke->bypass_addr;
-			old_addr = old ? (u8 *)old->bpf_func + poke->adj_off : NULL;
-			new_addr = new ? (u8 *)new->bpf_func + poke->adj_off : NULL;
-
-			if (new) {
-				ret = bpf_arch_text_poke(poke->tailcall_target,
-							 BPF_MOD_JUMP,
-							 old_addr, new_addr);
-				BUG_ON(ret < 0 && ret != -EINVAL);
-				if (!old) {
-					ret = bpf_arch_text_poke(poke->tailcall_bypass,
-								 BPF_MOD_JUMP,
-								 poke->bypass_addr,
-								 NULL);
-					BUG_ON(ret < 0 && ret != -EINVAL);
-				}
-			} else {
-				ret = bpf_arch_text_poke(poke->tailcall_bypass,
-							 BPF_MOD_JUMP,
-							 old_bypass_addr,
-							 poke->bypass_addr);
-				BUG_ON(ret < 0 && ret != -EINVAL);
-				/* let other CPUs finish the execution of program
-				 * so that it will not possible to expose them
-				 * to invalid nop, stack unwind, nop state
-				 */
-				if (!ret)
-					synchronize_rcu();
-				ret = bpf_arch_text_poke(poke->tailcall_target,
-							 BPF_MOD_JUMP,
-							 old_addr, NULL);
-				BUG_ON(ret < 0 && ret != -EINVAL);
-			}
+			bpf_arch_poke_desc_update(poke, new, old);
 		}
 	}
 }
-- 
2.43.0

[PATCHv3 bpf 2/2] selftests/bpf: Add test for early update in prog_array_map_poke_run

[Jiri Olsa]

Adding test that tries to trigger the BUG_ON during early map update
in prog_array_map_poke_run function.

The idea is to share prog array map between thread that constantly
updates it and another one loading a program that uses that prog
array.

Eventually we will hit a place where the program is ok to be updated
(poke->tailcall_target_stable check) but the address is still not
registered in kallsyms, so the bpf_arch_text_poke returns -EINVAL
and cause imbalance for the next tail call update check, which will
fail with -EBUSY in bpf_arch_text_poke as described in previous fix.

Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../selftests/bpf/prog_tests/tailcall_poke.c  | 74 +++++++++++++++++++
 .../selftests/bpf/progs/tailcall_poke.c       | 32 ++++++++
 2 files changed, 106 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
 create mode 100644 tools/testing/selftests/bpf/progs/tailcall_poke.c

diff --git a/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
new file mode 100644
index 000000000000..f7e2c09fd772
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <unistd.h>
+#include <test_progs.h>
+#include "tailcall_poke.skel.h"
+
+#define JMP_TABLE "/sys/fs/bpf/jmp_table"
+
+static int thread_exit;
+
+static void *update(void *arg)
+{
+	__u32 zero = 0, prog1_fd, prog2_fd, map_fd;
+	struct tailcall_poke *call = arg;
+
+	map_fd = bpf_map__fd(call->maps.jmp_table);
+	prog1_fd = bpf_program__fd(call->progs.call1);
+	prog2_fd = bpf_program__fd(call->progs.call2);
+
+	while (!thread_exit) {
+		bpf_map_update_elem(map_fd, &zero, &prog1_fd, BPF_ANY);
+		bpf_map_update_elem(map_fd, &zero, &prog2_fd, BPF_ANY);
+	}
+
+	return NULL;
+}
+
+void test_tailcall_poke(void)
+{
+	struct tailcall_poke *call, *test;
+	int err, cnt = 10;
+	pthread_t thread;
+
+	unlink(JMP_TABLE);
+
+	call = tailcall_poke__open_and_load();
+	if (!ASSERT_OK_PTR(call, "tailcall_poke__open"))
+		return;
+
+	err = bpf_map__pin(call->maps.jmp_table, JMP_TABLE);
+	if (!ASSERT_OK(err, "bpf_map__pin"))
+		goto out;
+
+	err = pthread_create(&thread, NULL, update, call);
+	if (!ASSERT_OK(err, "new toggler"))
+		goto out;
+
+	while (cnt--) {
+		test = tailcall_poke__open();
+		if (!ASSERT_OK_PTR(test, "tailcall_poke__open"))
+			break;
+
+		err = bpf_map__set_pin_path(test->maps.jmp_table, JMP_TABLE);
+		if (!ASSERT_OK(err, "bpf_map__pin")) {
+			tailcall_poke__destroy(test);
+			break;
+		}
+
+		bpf_program__set_autoload(test->progs.test, true);
+		bpf_program__set_autoload(test->progs.call1, false);
+		bpf_program__set_autoload(test->progs.call2, false);
+
+		err = tailcall_poke__load(test);
+		tailcall_poke__destroy(test);
+		if (!ASSERT_OK(err, "tailcall_poke__load"))
+			break;
+	}
+
+	thread_exit = 1;
+	ASSERT_OK(pthread_join(thread, NULL), "pthread_join");
+
+out:
+	bpf_map__unpin(call->maps.jmp_table, JMP_TABLE);
+	tailcall_poke__destroy(call);
+}
diff --git a/tools/testing/selftests/bpf/progs/tailcall_poke.c b/tools/testing/selftests/bpf/progs/tailcall_poke.c
new file mode 100644
index 000000000000..c78b94b75e83
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tailcall_poke.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+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");
+
+SEC("?fentry/bpf_fentry_test1")
+int BPF_PROG(test, int a)
+{
+	bpf_tail_call_static(ctx, &jmp_table, 0);
+	return 0;
+}
+
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(call1, int a)
+{
+	return 0;
+}
+
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(call2, int a)
+{
+	return 0;
+}
-- 
2.43.0

Re: [PATCHv3 bpf 1/2] bpf: Fix prog_array_map_poke_run map poke update

[Yonghong Song]

On 12/3/23 3:48 PM, Jiri Olsa wrote:
> Lee pointed out issue found by syscaller [0] hitting BUG in prog array
> map poke update in prog_array_map_poke_run function due to error value
> returned from bpf_arch_text_poke function.
>
> There's race window where bpf_arch_text_poke can fail due to missing
> bpf program kallsym symbols, which is accounted for with check for
> -EINVAL in that BUG_ON call.
>
> The problem is that in such case we won't update the tail call jump
> and cause imbalance for the next tail call update check which will
> fail with -EBUSY in bpf_arch_text_poke.
>
> I'm hitting following race during the program load:
>
>    CPU 0                             CPU 1
>
>    bpf_prog_load
>      bpf_check
>        do_misc_fixups
>          prog_array_map_poke_track
>
>                                      map_update_elem
>                                        bpf_fd_array_map_update_elem
>                                          prog_array_map_poke_run
>
>                                            bpf_arch_text_poke returns -EINVAL
>
>      bpf_prog_kallsyms_add
>
> After bpf_arch_text_poke (CPU 1) fails to update the tail call jump, the next
> poke update fails on expected jump instruction check in bpf_arch_text_poke
> with -EBUSY and triggers the BUG_ON in prog_array_map_poke_run.
>
> Similar race exists on the program unload.
>
> Fixing this by moving the update to bpf_arch_poke_desc_update function which
> makes sure we call __bpf_arch_text_poke that skips the bpf address check.
>
> Each architecture has slightly different approach wrt looking up bpf address
> in bpf_arch_text_poke, so instead of splitting the function or adding new
> 'checkip' argument in previous version, it seems best to move the whole
> map_poke_run update as arch specific code.
>
> [0] https://syzkaller.appspot.com/bug?extid=97a4fe20470e9bc30810
>
> Cc: Lee Jones <lee@kernel.org>
> Cc: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Fixes: ebf7d1f508a7 ("bpf, x64: rework pro/epilogue and tailcall handling in JIT")
> Reported-by: syzbot+97a4fe20470e9bc30810@syzkaller.appspotmail.com
> Tested-by: Lee Jones <lee@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

In bpf_arch_text_poke(), we also have
         if (is_endbr(*(u32 *)ip))
                 ip += ENDBR_INSN_SIZE;
Since the indirect call be converted to direct call,
so I think we do not need endbr insn at the beginning of
the function even if jit might have added endbr or
some other stuff in the beginning of the function.
See
   https://lore.kernel.org/bpf/20231130133630.192490507@infradead.org/

The patch looks good to me.

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

Re: [PATCHv3 bpf 2/2] selftests/bpf: Add test for early update in prog_array_map_poke_run

[Yonghong Song]

On 12/3/23 3:48 PM, Jiri Olsa wrote:
> Adding test that tries to trigger the BUG_ON during early map update
> in prog_array_map_poke_run function.
>
> The idea is to share prog array map between thread that constantly
> updates it and another one loading a program that uses that prog
> array.
>
> Eventually we will hit a place where the program is ok to be updated
> (poke->tailcall_target_stable check) but the address is still not
> registered in kallsyms, so the bpf_arch_text_poke returns -EINVAL
> and cause imbalance for the next tail call update check, which will
> fail with -EBUSY in bpf_arch_text_poke as described in previous fix.
>
> Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>   .../selftests/bpf/prog_tests/tailcall_poke.c  | 74 +++++++++++++++++++
>   .../selftests/bpf/progs/tailcall_poke.c       | 32 ++++++++
>   2 files changed, 106 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
>   create mode 100644 tools/testing/selftests/bpf/progs/tailcall_poke.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
> new file mode 100644
> index 000000000000..f7e2c09fd772
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <unistd.h>
> +#include <test_progs.h>
> +#include "tailcall_poke.skel.h"
> +
> +#define JMP_TABLE "/sys/fs/bpf/jmp_table"
> +
> +static int thread_exit;
> +
> +static void *update(void *arg)
> +{
> +	__u32 zero = 0, prog1_fd, prog2_fd, map_fd;
> +	struct tailcall_poke *call = arg;
> +
> +	map_fd = bpf_map__fd(call->maps.jmp_table);
> +	prog1_fd = bpf_program__fd(call->progs.call1);
> +	prog2_fd = bpf_program__fd(call->progs.call2);
> +
> +	while (!thread_exit) {
> +		bpf_map_update_elem(map_fd, &zero, &prog1_fd, BPF_ANY);
> +		bpf_map_update_elem(map_fd, &zero, &prog2_fd, BPF_ANY);
> +	}
> +
> +	return NULL;
> +}
> +
> +void test_tailcall_poke(void)
> +{
> +	struct tailcall_poke *call, *test;
> +	int err, cnt = 10;
> +	pthread_t thread;
> +
> +	unlink(JMP_TABLE);
> +
> +	call = tailcall_poke__open_and_load();
> +	if (!ASSERT_OK_PTR(call, "tailcall_poke__open"))
> +		return;
> +
> +	err = bpf_map__pin(call->maps.jmp_table, JMP_TABLE);
> +	if (!ASSERT_OK(err, "bpf_map__pin"))
> +		goto out;

Just curious. What is the reason having bpf_map__pin() here
and below? I tried and it looks like removing bpf_map__pin()
and below bpf_map__set_pin_path() will make reproducing
the failure hard/impossible.

> +
> +	err = pthread_create(&thread, NULL, update, call);
> +	if (!ASSERT_OK(err, "new toggler"))
> +		goto out;
> +
> +	while (cnt--) {
> +		test = tailcall_poke__open();
> +		if (!ASSERT_OK_PTR(test, "tailcall_poke__open"))
> +			break;
> +
> +		err = bpf_map__set_pin_path(test->maps.jmp_table, JMP_TABLE);
> +		if (!ASSERT_OK(err, "bpf_map__pin")) {
> +			tailcall_poke__destroy(test);
> +			break;
> +		}
> +
> +		bpf_program__set_autoload(test->progs.test, true);
> +		bpf_program__set_autoload(test->progs.call1, false);
> +		bpf_program__set_autoload(test->progs.call2, false);
> +
> +		err = tailcall_poke__load(test);
> +		tailcall_poke__destroy(test);
> +		if (!ASSERT_OK(err, "tailcall_poke__load"))
> +			break;
> +	}
> +
> +	thread_exit = 1;
> +	ASSERT_OK(pthread_join(thread, NULL), "pthread_join");
> +
> +out:
> +	bpf_map__unpin(call->maps.jmp_table, JMP_TABLE);
> +	tailcall_poke__destroy(call);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/tailcall_poke.c b/tools/testing/selftests/bpf/progs/tailcall_poke.c
> new file mode 100644
> index 000000000000..c78b94b75e83
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/tailcall_poke.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +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");
> +
> +SEC("?fentry/bpf_fentry_test1")
> +int BPF_PROG(test, int a)
> +{
> +	bpf_tail_call_static(ctx, &jmp_table, 0);
> +	return 0;
> +}
> +
> +SEC("fentry/bpf_fentry_test1")
> +int BPF_PROG(call1, int a)
> +{
> +	return 0;
> +}
> +
> +SEC("fentry/bpf_fentry_test1")
> +int BPF_PROG(call2, int a)
> +{
> +	return 0;
> +}

Re: [PATCHv3 bpf 1/2] bpf: Fix prog_array_map_poke_run map poke update

[kernel test robot]

Hi Jiri,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Jiri-Olsa/bpf-Fix-prog_array_map_poke_run-map-poke-update/20231204-045204
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
patch link:    https://lore.kernel.org/r/20231203204851.388654-2-jolsa%40kernel.org
patch subject: [PATCHv3 bpf 1/2] bpf: Fix prog_array_map_poke_run map poke update
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231205/202312051420.clYpN744-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/202312051420.clYpN744-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312051420.clYpN744-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> kernel/bpf/arraymap.c:1015:13: warning: no previous prototype for 'bpf_arch_poke_desc_update' [-Wmissing-prototypes]
    1015 | void __weak bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~


vim +/bpf_arch_poke_desc_update +1015 kernel/bpf/arraymap.c

  1014	
> 1015	void __weak bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,
  1016					      struct bpf_prog *new, struct bpf_prog *old)
  1017	{
  1018		WARN_ON_ONCE(1);
  1019	}
  1020	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCHv3 bpf 1/2] bpf: Fix prog_array_map_poke_run map poke update

[kernel test robot]

Hi Jiri,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Jiri-Olsa/bpf-Fix-prog_array_map_poke_run-map-poke-update/20231204-045204
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
patch link:    https://lore.kernel.org/r/20231203204851.388654-2-jolsa%40kernel.org
patch subject: [PATCHv3 bpf 1/2] bpf: Fix prog_array_map_poke_run map poke update
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20231205/202312051557.5OKOsVGa-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/202312051557.5OKOsVGa-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312051557.5OKOsVGa-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> arch/x86/net/bpf_jit_comp.c:3029:6: warning: no previous prototype for function 'bpf_arch_poke_desc_update' [-Wmissing-prototypes]
   void bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,
        ^
   arch/x86/net/bpf_jit_comp.c:3029:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,
   ^
   static 
   1 warning generated.
--
>> kernel/bpf/arraymap.c:1015:13: warning: no previous prototype for function 'bpf_arch_poke_desc_update' [-Wmissing-prototypes]
   void __weak bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,
               ^
   kernel/bpf/arraymap.c:1015:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void __weak bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,
   ^
   static 
   1 warning generated.


vim +/bpf_arch_poke_desc_update +3029 arch/x86/net/bpf_jit_comp.c

  3028	
> 3029	void bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCHv3 bpf 2/2] selftests/bpf: Add test for early update in prog_array_map_poke_run

[Jiri Olsa]

On Mon, Dec 04, 2023 at 09:16:52PM -0800, Yonghong Song wrote:
> 
> On 12/3/23 3:48 PM, Jiri Olsa wrote:
> > Adding test that tries to trigger the BUG_ON during early map update
> > in prog_array_map_poke_run function.
> > 
> > The idea is to share prog array map between thread that constantly
> > updates it and another one loading a program that uses that prog
> > array.
> > 
> > Eventually we will hit a place where the program is ok to be updated
> > (poke->tailcall_target_stable check) but the address is still not
> > registered in kallsyms, so the bpf_arch_text_poke returns -EINVAL
> > and cause imbalance for the next tail call update check, which will
> > fail with -EBUSY in bpf_arch_text_poke as described in previous fix.
> > 
> > Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >   .../selftests/bpf/prog_tests/tailcall_poke.c  | 74 +++++++++++++++++++
> >   .../selftests/bpf/progs/tailcall_poke.c       | 32 ++++++++
> >   2 files changed, 106 insertions(+)
> >   create mode 100644 tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
> >   create mode 100644 tools/testing/selftests/bpf/progs/tailcall_poke.c
> > 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
> > new file mode 100644
> > index 000000000000..f7e2c09fd772
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
> > @@ -0,0 +1,74 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <unistd.h>
> > +#include <test_progs.h>
> > +#include "tailcall_poke.skel.h"
> > +
> > +#define JMP_TABLE "/sys/fs/bpf/jmp_table"
> > +
> > +static int thread_exit;
> > +
> > +static void *update(void *arg)
> > +{
> > +	__u32 zero = 0, prog1_fd, prog2_fd, map_fd;
> > +	struct tailcall_poke *call = arg;
> > +
> > +	map_fd = bpf_map__fd(call->maps.jmp_table);
> > +	prog1_fd = bpf_program__fd(call->progs.call1);
> > +	prog2_fd = bpf_program__fd(call->progs.call2);
> > +
> > +	while (!thread_exit) {
> > +		bpf_map_update_elem(map_fd, &zero, &prog1_fd, BPF_ANY);
> > +		bpf_map_update_elem(map_fd, &zero, &prog2_fd, BPF_ANY);
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +void test_tailcall_poke(void)
> > +{
> > +	struct tailcall_poke *call, *test;
> > +	int err, cnt = 10;
> > +	pthread_t thread;
> > +
> > +	unlink(JMP_TABLE);
> > +
> > +	call = tailcall_poke__open_and_load();
> > +	if (!ASSERT_OK_PTR(call, "tailcall_poke__open"))
> > +		return;
> > +
> > +	err = bpf_map__pin(call->maps.jmp_table, JMP_TABLE);
> > +	if (!ASSERT_OK(err, "bpf_map__pin"))
> > +		goto out;
> 
> Just curious. What is the reason having bpf_map__pin() here
> and below? I tried and it looks like removing bpf_map__pin()
> and below bpf_map__set_pin_path() will make reproducing
> the failure hard/impossible.

yes, it's there to share the jmp_table map between the two
skeleton instances, so the update thread changes the same
jmp_table map that's used in the skeleton we load in the
while loop below

I'll add some comments to the test

jirka

> 
> > +
> > +	err = pthread_create(&thread, NULL, update, call);
> > +	if (!ASSERT_OK(err, "new toggler"))
> > +		goto out;
> > +
> > +	while (cnt--) {
> > +		test = tailcall_poke__open();
> > +		if (!ASSERT_OK_PTR(test, "tailcall_poke__open"))
> > +			break;
> > +
> > +		err = bpf_map__set_pin_path(test->maps.jmp_table, JMP_TABLE);
> > +		if (!ASSERT_OK(err, "bpf_map__pin")) {
> > +			tailcall_poke__destroy(test);
> > +			break;
> > +		}
> > +
> > +		bpf_program__set_autoload(test->progs.test, true);
> > +		bpf_program__set_autoload(test->progs.call1, false);
> > +		bpf_program__set_autoload(test->progs.call2, false);
> > +
> > +		err = tailcall_poke__load(test);
> > +		tailcall_poke__destroy(test);
> > +		if (!ASSERT_OK(err, "tailcall_poke__load"))
> > +			break;
> > +	}
> > +
> > +	thread_exit = 1;
> > +	ASSERT_OK(pthread_join(thread, NULL), "pthread_join");
> > +
> > +out:
> > +	bpf_map__unpin(call->maps.jmp_table, JMP_TABLE);
> > +	tailcall_poke__destroy(call);
> > +}

SNIP

Re: [PATCHv3 bpf 2/2] selftests/bpf: Add test for early update in prog_array_map_poke_run

[Yonghong Song]

On 12/5/23 3:43 AM, Jiri Olsa wrote:
> On Mon, Dec 04, 2023 at 09:16:52PM -0800, Yonghong Song wrote:
>> On 12/3/23 3:48 PM, Jiri Olsa wrote:
>>> Adding test that tries to trigger the BUG_ON during early map update
>>> in prog_array_map_poke_run function.
>>>
>>> The idea is to share prog array map between thread that constantly
>>> updates it and another one loading a program that uses that prog
>>> array.
>>>
>>> Eventually we will hit a place where the program is ok to be updated
>>> (poke->tailcall_target_stable check) but the address is still not
>>> registered in kallsyms, so the bpf_arch_text_poke returns -EINVAL
>>> and cause imbalance for the next tail call update check, which will
>>> fail with -EBUSY in bpf_arch_text_poke as described in previous fix.
>>>
>>> Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>>> ---
>>>    .../selftests/bpf/prog_tests/tailcall_poke.c  | 74 +++++++++++++++++++
>>>    .../selftests/bpf/progs/tailcall_poke.c       | 32 ++++++++
>>>    2 files changed, 106 insertions(+)
>>>    create mode 100644 tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
>>>    create mode 100644 tools/testing/selftests/bpf/progs/tailcall_poke.c
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
>>> new file mode 100644
>>> index 000000000000..f7e2c09fd772
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
>>> @@ -0,0 +1,74 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +#include <unistd.h>
>>> +#include <test_progs.h>
>>> +#include "tailcall_poke.skel.h"
>>> +
>>> +#define JMP_TABLE "/sys/fs/bpf/jmp_table"
>>> +
>>> +static int thread_exit;
>>> +
>>> +static void *update(void *arg)
>>> +{
>>> +	__u32 zero = 0, prog1_fd, prog2_fd, map_fd;
>>> +	struct tailcall_poke *call = arg;
>>> +
>>> +	map_fd = bpf_map__fd(call->maps.jmp_table);
>>> +	prog1_fd = bpf_program__fd(call->progs.call1);
>>> +	prog2_fd = bpf_program__fd(call->progs.call2);
>>> +
>>> +	while (!thread_exit) {
>>> +		bpf_map_update_elem(map_fd, &zero, &prog1_fd, BPF_ANY);
>>> +		bpf_map_update_elem(map_fd, &zero, &prog2_fd, BPF_ANY);
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +void test_tailcall_poke(void)
>>> +{
>>> +	struct tailcall_poke *call, *test;
>>> +	int err, cnt = 10;
>>> +	pthread_t thread;
>>> +
>>> +	unlink(JMP_TABLE);
>>> +
>>> +	call = tailcall_poke__open_and_load();
>>> +	if (!ASSERT_OK_PTR(call, "tailcall_poke__open"))
>>> +		return;
>>> +
>>> +	err = bpf_map__pin(call->maps.jmp_table, JMP_TABLE);
>>> +	if (!ASSERT_OK(err, "bpf_map__pin"))
>>> +		goto out;
>> Just curious. What is the reason having bpf_map__pin() here
>> and below? I tried and it looks like removing bpf_map__pin()
>> and below bpf_map__set_pin_path() will make reproducing
>> the failure hard/impossible.
> yes, it's there to share the jmp_table map between the two
> skeleton instances, so the update thread changes the same
> jmp_table map that's used in the skeleton we load in the
> while loop below

This does make sense.

>
> I'll add some comments to the test

Thanks for explanation. Some comments are definitely helpful!

>
> jirka
>
>>> +
>>> +	err = pthread_create(&thread, NULL, update, call);
>>> +	if (!ASSERT_OK(err, "new toggler"))
>>> +		goto out;
>>> +
>>> +	while (cnt--) {
>>> +		test = tailcall_poke__open();
>>> +		if (!ASSERT_OK_PTR(test, "tailcall_poke__open"))
>>> +			break;
>>> +
>>> +		err = bpf_map__set_pin_path(test->maps.jmp_table, JMP_TABLE);
>>> +		if (!ASSERT_OK(err, "bpf_map__pin")) {
>>> +			tailcall_poke__destroy(test);
>>> +			break;
>>> +		}
>>> +
>>> +		bpf_program__set_autoload(test->progs.test, true);
>>> +		bpf_program__set_autoload(test->progs.call1, false);
>>> +		bpf_program__set_autoload(test->progs.call2, false);
>>> +
>>> +		err = tailcall_poke__load(test);
>>> +		tailcall_poke__destroy(test);
>>> +		if (!ASSERT_OK(err, "tailcall_poke__load"))
>>> +			break;
>>> +	}
>>> +
>>> +	thread_exit = 1;
>>> +	ASSERT_OK(pthread_join(thread, NULL), "pthread_join");
>>> +
>>> +out:
>>> +	bpf_map__unpin(call->maps.jmp_table, JMP_TABLE);
>>> +	tailcall_poke__destroy(call);
>>> +}
> SNIP

Re: [PATCHv3 bpf 2/2] selftests/bpf: Add test for early update in prog_array_map_poke_run

[Jiri Olsa]

On Tue, Dec 05, 2023 at 08:00:48AM -0800, Yonghong Song wrote:

SNIP

> > > > +void test_tailcall_poke(void)
> > > > +{
> > > > +	struct tailcall_poke *call, *test;
> > > > +	int err, cnt = 10;
> > > > +	pthread_t thread;
> > > > +
> > > > +	unlink(JMP_TABLE);
> > > > +
> > > > +	call = tailcall_poke__open_and_load();
> > > > +	if (!ASSERT_OK_PTR(call, "tailcall_poke__open"))
> > > > +		return;
> > > > +
> > > > +	err = bpf_map__pin(call->maps.jmp_table, JMP_TABLE);
> > > > +	if (!ASSERT_OK(err, "bpf_map__pin"))
> > > > +		goto out;
> > > Just curious. What is the reason having bpf_map__pin() here
> > > and below? I tried and it looks like removing bpf_map__pin()
> > > and below bpf_map__set_pin_path() will make reproducing
> > > the failure hard/impossible.
> > yes, it's there to share the jmp_table map between the two
> > skeleton instances, so the update thread changes the same
> > jmp_table map that's used in the skeleton we load in the
> > while loop below
> 
> This does make sense.
> 
> > 
> > I'll add some comments to the test
> 
> Thanks for explanation. Some comments are definitely helpful!

np, also looks like I should move this to prog_tests/tailcalls.c,
will send new version with that

thanks,
jirka


> 
> > 
> > jirka
> > 
> > > > +
> > > > +	err = pthread_create(&thread, NULL, update, call);
> > > > +	if (!ASSERT_OK(err, "new toggler"))
> > > > +		goto out;
> > > > +
> > > > +	while (cnt--) {
> > > > +		test = tailcall_poke__open();
> > > > +		if (!ASSERT_OK_PTR(test, "tailcall_poke__open"))
> > > > +			break;
> > > > +
> > > > +		err = bpf_map__set_pin_path(test->maps.jmp_table, JMP_TABLE);
> > > > +		if (!ASSERT_OK(err, "bpf_map__pin")) {
> > > > +			tailcall_poke__destroy(test);
> > > > +			break;
> > > > +		}
> > > > +
> > > > +		bpf_program__set_autoload(test->progs.test, true);
> > > > +		bpf_program__set_autoload(test->progs.call1, false);
> > > > +		bpf_program__set_autoload(test->progs.call2, false);
> > > > +
> > > > +		err = tailcall_poke__load(test);
> > > > +		tailcall_poke__destroy(test);
> > > > +		if (!ASSERT_OK(err, "tailcall_poke__load"))
> > > > +			break;
> > > > +	}
> > > > +
> > > > +	thread_exit = 1;
> > > > +	ASSERT_OK(pthread_join(thread, NULL), "pthread_join");
> > > > +
> > > > +out:
> > > > +	bpf_map__unpin(call->maps.jmp_table, JMP_TABLE);
> > > > +	tailcall_poke__destroy(call);
> > > > +}
> > SNIP