[PATCH bpf-next 0/4] bpf: Follow up on the patch list

[PATCH bpf-next 0/4] bpf: Follow up on the patch list

[Qiliang Yuan]

Alexei listed what the patch list series [1] still needed when he
applied it [2]. This series covers items 1, 2, 3 and 5 of that list:

  item  problem                                           patch
  1     the tests break with bpf_jit_harden at 2          2
  2     the out-of-range test ignores the insn number     3
  3     the comments on the aux data rules are gone       1
  5     no test has a second patch with a jump            4

Item 4 is about a commit log that is already in the tree. Items 10-12,
bugs that were there before the series, come as separate fixes.

With net.core.bpf_jit_harden at 0 and at 2 before the run,
verifier_patch_list and patch_list pass and leave the sysctl as it
was. Without patch 2 and with the sysctl at 2, 8 verifier_patch_list
tests and patch_list/line_and_func_info fail. test_verifier and
test_progs show no new failures.

[1] https://lore.kernel.org/r/20261102012940.1756142-1-alexei.starovoitov@gmail.com
[2] https://lore.kernel.org/bpf/DMKHZS5B5CNO.186F6YH1T6YU4@gmail.com/

Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
Qiliang Yuan (4):
      bpf: Document the aux data rules of patched instructions
      selftests/bpf: Run the patch list tests with bpf_jit_harden off
      selftests/bpf: Match the insn number of the out-of-range message
      selftests/bpf: Cover a second patch with a jump behind a barrier

 kernel/bpf/fixups.c                                | 18 +++++++++-
 .../testing/selftests/bpf/prog_tests/patch_list.c  | 10 +++++-
 tools/testing/selftests/bpf/prog_tests/verifier.c  | 17 +++++++--
 .../selftests/bpf/progs/verifier_patch_list.c      | 41 +++++++++++++++++++++-
 4 files changed, 81 insertions(+), 5 deletions(-)
---
base-commit: cd5bc55288d4fd999595c8f2af6a5408cbe92a52
change-id: 20261102-bpf-patch-list-followups-e6638030c913

Best regards,
-- 
Qiliang Yuan <odys.yuan@gmail.com>

[PATCH bpf-next 1/4] bpf: Document the aux data rules of patched instructions

[Qiliang Yuan]

The aux data of a patch derives from the aux data of the insn that it
replaces: zext_dst is always recomputed, the new insns inherit the seen
count, and a memory accessing insn that is not a copy of the patched
one is marked for non-stack access.

patch_list_adj_aux() and the in-place path of bpf_patch_list_add()
implement these rules without a word about them. The comments that
explained them went away with adjust_insn_aux_data().

Add the comments back next to the code that implements the rules.

Reported-by: Alexei Starovoitov <ast@kernel.org>
Closes: https://lore.kernel.org/bpf/DMKHZS5B5CNO.186F6YH1T6YU4@gmail.com/
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
 kernel/bpf/fixups.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index ead78713e04f4..6d186a096634c 100644
--- a/kernel/bpf/fixups.c
+++ b/kernel/bpf/fixups.c
@@ -203,7 +203,11 @@ struct bpf_insn *bpf_patch_list_add(struct bpf_verifier_env *env, u32 off,
 
 		*insn = *patch;
 		env->insn_aux_data[off].zext_dst = bpf_insn_def32(env->prog, insn) >= 0;
-		/* a new insn may access memory, systematically mark it non-stack */
+		/*
+		 * A non-memory accessing insn could have been replaced by a
+		 * memory accessing insn, systematically mark it for non-stack
+		 * access.
+		 */
 		env->insn_aux_data[off].non_stack_access = bpf_is_mem_insn(insn);
 		return insn;
 	}
@@ -270,14 +274,21 @@ static void patch_list_adj_aux(struct bpf_prog *prog, struct bpf_insn_patch *p,
 	u32 i;
 
 	*last = *old;
+	/* Recompute zext_dst: the last insn of the patch need not be the patched one. */
 	last->zext_dst = bpf_insn_def32(prog, &patch[p->len - 1]) >= 0;
 	memset(slot, 0, sizeof(*slot) * (p->len - 1));
 
 	for (i = 0; i < p->len - 1; i++) {
+		/* Expand the seen count of the patched insn to the patch. */
 		slot[i].seen = last->seen;
 		/* messages name the insn of the original program */
 		slot[i].orig_idx = last->orig_idx;
 		slot[i].zext_dst = bpf_insn_def32(prog, &patch[i]) >= 0;
+		/*
+		 * A copy of the patched insn keeps its marking. Any other
+		 * memory accessing insn is new, systematically mark it for
+		 * non-stack access.
+		 */
 		if (i >= p->target && !memcmp(&patch[i], orig, sizeof(*orig))) {
 			slot[i].non_stack_access = last->non_stack_access;
 			last->non_stack_access = false;
@@ -286,6 +297,11 @@ static void patch_list_adj_aux(struct bpf_prog *prog, struct bpf_insn_patch *p,
 		}
 	}
 
+	/*
+	 * The last insn could be a newly generated BPF_ST/BPF_LDX/BPF_STX,
+	 * systematically mark it for non-stack access if it is not the patched
+	 * insn, otherwise keep the original marking.
+	 */
 	if (bpf_is_mem_insn(&patch[p->len - 1]) &&
 	    memcmp(&patch[p->len - 1], orig, sizeof(*orig)))
 		last->non_stack_access = true;

-- 
2.43.0

[PATCH bpf-next 2/4] selftests/bpf: Run the patch list tests with bpf_jit_harden off

[Qiliang Yuan]

The patch list tests compare the xlated insns of their progs, and
patch_list/line_and_func_info and patch_list/poke_descriptor look at
the line info and the poke descriptors of a prog. They run serially,
because other tests change net.core.bpf_jit_harden.

They don't set the sysctl themselves, though. When it is 2 at the
start, constant blinding rewrites the progs: 8 of the
verifier_patch_list tests and patch_list/line_and_func_info fail, and
patch_list/poke_descriptor passes with no poke descriptor to check:

  XLATED:
  ...
  5: r12 = -360925233
  6: r12 ^= -360925238
  7: if r1 > r12 goto pc+3
  ...
  #719/1   verifier_patch_list/patch list: forward jumps to and over patched insns:FAIL
  ...
  test_line_and_func_info:FAIL:fn_1001 unexpected fn_1001: got FALSE

Set the sysctl to 0 while verifier_patch_list and the first two
subtests of patch_list run, and restore it afterwards, the way
patch_list/classic_filter does.

Fixes: 67d4d0b73c29 ("selftests/bpf: Test jumps around patched instructions")
Fixes: d86e6695cb2f ("selftests/bpf: Test the tables that follow patched and removed instructions")
Reported-by: Alexei Starovoitov <ast@kernel.org>
Closes: https://lore.kernel.org/bpf/DMKHZS5B5CNO.186F6YH1T6YU4@gmail.com/
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/patch_list.c | 10 +++++++++-
 tools/testing/selftests/bpf/prog_tests/verifier.c   | 17 +++++++++++++++--
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/patch_list.c b/tools/testing/selftests/bpf/prog_tests/patch_list.c
index 127ed58cfc39d..43bd8a4428060 100644
--- a/tools/testing/selftests/bpf/prog_tests/patch_list.c
+++ b/tools/testing/selftests/bpf/prog_tests/patch_list.c
@@ -243,14 +243,22 @@ static void test_classic_filter(void)
 
 /*
  * Serial: the xlated code is another one and there are no poke descriptors
- * while another test sets bpf_jit_harden, and classic_filter sets it.
+ * while another test sets bpf_jit_harden, and classic_filter sets it. Keep
+ * the sysctl at 0 for the subtests that need the code unblinded.
  */
 void serial_test_patch_list(void)
 {
+	const char *harden = "/proc/sys/net/core/bpf_jit_harden";
+	char old[16] = {};
+
+	if (sysctl_set_or_fail(harden, old, "0"))
+		return;
 	if (test__start_subtest("line_and_func_info"))
 		test_line_and_func_info();
 	if (test__start_subtest("poke_descriptor"))
 		test_poke_descriptor();
+	sysctl_set_or_fail(harden, NULL, old);
+
 	if (test__start_subtest("classic_filter"))
 		test_classic_filter();
 }
diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index 99f0551c8b592..1108fd5e4d162 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -6,6 +6,7 @@
 #include "arena_noarena.skel.h"
 #include "arena_kfunc_jit.skel.h"
 #include "cap_helpers.h"
+#include "sysctl_helpers.h"
 #include "cb_refs.skel.h"
 #include "verifier_aggregate_arg.skel.h"
 #include "verifier_aggregate_ret.skel.h"
@@ -288,8 +289,20 @@ void test_verifier_netfilter_ctx(void)        { RUN(verifier_netfilter_ctx); }
 void test_verifier_netfilter_retcode(void)    { RUN(verifier_netfilter_retcode); }
 void test_verifier_bpf_fastcall(void)         { RUN(verifier_bpf_fastcall); }
 void test_verifier_or_jmp32_k(void)           { RUN(verifier_or_jmp32_k); }
-/* Serial: the xlated code is another one while another test sets bpf_jit_harden */
-void serial_test_verifier_patch_list(void)    { RUN(verifier_patch_list); }
+/*
+ * Run serially with bpf_jit_harden at 0: blinding changes the xlated insns that
+ * the tests compare, and other tests set the sysctl.
+ */
+void serial_test_verifier_patch_list(void)
+{
+	const char *harden = "/proc/sys/net/core/bpf_jit_harden";
+	char old[16] = {};
+
+	if (sysctl_set_or_fail(harden, old, "0"))
+		return;
+	RUN(verifier_patch_list);
+	sysctl_set_or_fail(harden, NULL, old);
+}
 void test_verifier_percpu_addr(void)          { RUN(verifier_percpu_addr); }
 void test_verifier_precision(void)            { RUN(verifier_precision); }
 void test_verifier_prevent_map_lookup(void)   { RUN(verifier_prevent_map_lookup); }

-- 
2.43.0

[PATCH bpf-next 3/4] selftests/bpf: Match the insn number of the out-of-range message

[Qiliang Yuan]

"patch list: jump pushed out of range by patches" expects the load to
fail with "insn %d cannot be patched due to 16-bit range".

It matches only the fixed part of the message. Which insn the message
names is what the patch list decides: the jump that got out of range,
not the insn that a patch replaced.

Match the whole message, with insn 2, the jump over the divisions.

Fixes: 67d4d0b73c29 ("selftests/bpf: Test jumps around patched instructions")
Reported-by: Alexei Starovoitov <ast@kernel.org>
Closes: https://lore.kernel.org/bpf/DMKHZS5B5CNO.186F6YH1T6YU4@gmail.com/
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
 tools/testing/selftests/bpf/progs/verifier_patch_list.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/progs/verifier_patch_list.c b/tools/testing/selftests/bpf/progs/verifier_patch_list.c
index 3ba99cf16e96b..045081936d73d 100644
--- a/tools/testing/selftests/bpf/progs/verifier_patch_list.c
+++ b/tools/testing/selftests/bpf/progs/verifier_patch_list.c
@@ -128,7 +128,7 @@ __naked void patch_list_may_goto(void)
  */
 SEC("raw_tp")
 __description("patch list: jump pushed out of range by patches")
-__failure __msg("cannot be patched due to 16-bit range")
+__failure __msg("insn 2 cannot be patched due to 16-bit range")
 __naked void patch_list_jump_out_of_range(void)
 {
 	asm volatile ("					\

-- 
2.43.0

[PATCH bpf-next 4/4] selftests/bpf: Cover a second patch with a jump behind a barrier

[Qiliang Yuan]

bpf_convert_ctx_accesses() can patch an insn twice: a barrier goes in
front of it, and then its ctx access replaces the copy of the insn
behind the barrier. bpf_patch_list_add() merges the two into one patch.

No test has a second patch with a jump in it, so nothing checks that
the jumps of the second patch survive the merge. The barrier needs a
prog without CAP_PERFMON, and "nospec, store, nospec" in
verifier_unpriv.c has no jump.

Load skb->ifindex, which becomes a load, a jump and another load, at an
insn that a mispredicted path reaches with a scalar instead of the ctx.
Without CAP_PERFMON the verifier puts a barrier in front of it. Check
that the jump still skips one insn.

Reported-by: Alexei Starovoitov <ast@kernel.org>
Closes: https://lore.kernel.org/bpf/DMKHZS5B5CNO.186F6YH1T6YU4@gmail.com/
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
 .../selftests/bpf/progs/verifier_patch_list.c      | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_patch_list.c b/tools/testing/selftests/bpf/progs/verifier_patch_list.c
index 045081936d73d..287b33e17b730 100644
--- a/tools/testing/selftests/bpf/progs/verifier_patch_list.c
+++ b/tools/testing/selftests/bpf/progs/verifier_patch_list.c
@@ -423,4 +423,43 @@ __naked void patch_list_goto_over_nop_and_insn(void)
 	::: __clobber_all);
 }
 
+/*
+ * Load skb->ifindex where a mispredicted path through l1 has a scalar instead
+ * of the ctx, so that the verifier puts a barrier in front of the load. The
+ * load becomes a second patch of 3 insns behind the barrier, and its jump must
+ * still skip one insn. Only a prog without CAP_PERFMON gets the barrier.
+ */
+SEC("socket")
+__description("patch list: second patch with a jump behind a barrier")
+__success __success_unpriv
+__caps_unpriv(CAP_BPF)
+#ifdef SPEC_V1
+__xlated_unpriv("nospec")
+__xlated_unpriv("r0 = *(u64 *)(r2 +{{[0-9]+}})")
+__xlated_unpriv("if r0 == 0x0 goto pc+1")
+__xlated_unpriv("r0 = *(u32 *)(r0 +{{[0-9]+}})")
+__xlated_unpriv("exit")
+#endif
+__naked void patch_list_second_patch_jump(void)
+{
+	asm volatile ("					\
+	r8 = 0;						\
+	r8 ^= r8;					\
+	r9 = 0;						\
+	r9 ^= r9;					\
+	r0 = 0;						\
+	r2 = r1;					\
+	if r8 != 0 goto l1_%=;				\
+	goto l2_%=;					\
+l1_%=:	if r9 == 0 goto l3_%=;				\
+	r2 = r0;					\
+l2_%=:	r0 = *(u32 *)(r2 + %[ifindex]);			\
+	goto l4_%=;					\
+l3_%=:	r0 = 0;						\
+l4_%=:	exit;						\
+"	:
+	: __imm_const(ifindex, offsetof(struct __sk_buff, ifindex))
+	: __clobber_all);
+}
+
 char _license[] SEC("license") = "GPL";

-- 
2.43.0