[PATCH v4 bpf-next 0/9] bpf: Add gen_epilogue to bpf_verifier_ops

[PATCH v4 bpf-next 0/9] bpf: Add gen_epilogue to bpf_verifier_ops

[Martin KaFai Lau]

From: Martin KaFai Lau <martin.lau@kernel.org>

This set allows the subsystem to patch codes before BPF_EXIT.
The verifier ops, .gen_epilogue, is added for this purpose.
One of the use case will be in the bpf qdisc, the bpf qdisc
subsystem can ensure the skb->dev is in the correct value.
The bpf qdisc subsystem can either inline fixing it in the
epilogue or call another kernel function to handle it (e.g. drop)
in the epilogue. Another use case could be in bpf_tcp_ca.c to
enforce snd_cwnd has valid value (e.g. positive value).

v4:
 * Fixed a bug in the memcpy in patch 3
   The size in the memcpy should be
   epilogue_cnt * sizeof(*epilogue_buf)
v3:
 * Moved epilogue_buf[16] to env.
   Patch 1 is added to move the existing insn_buf[16] to env.
 * Fixed a case that the bpf prog has a BPF_JMP that goes back
   to the first instruction of the main prog.
   The jump back to 1st insn case also applies to the prologue.
   Patch 2 is added to handle it.
 * If the bpf main prog has multiple BPF_EXIT, use a BPF_JA
   to goto the earlier patched epilogue.
   Note that there are (BPF_JMP32 | BPF_JA) vs (BPF_JMP | BPF_JA)
   details in the patch 3 commit message.
 * There are subtle changes in patch 3, so I reset the Reviewed-by.
 * Added patch 8 and patch 9 to cover the changes in patch 2 and patch 3.
 * Dropped the kfunc call from pro/epilogue and its selftests.

v2:
 * Remove the RFC tag. Keep the ordering at where .gen_epilogue is
   called in the verifier relative to the check_max_stack_depth().
   This will be consistent with the other extra stack_depth
   usage like optimize_bpf_loop().
 * Use __xlated check provided by the test_loader to
   check the patched instructions after gen_pro/epilogue (Eduard).
 * Added Patch 3 by Eduard (Thanks!).

Eduard Zingerman (1):
  selftests/bpf: attach struct_ops maps before test prog runs

Martin KaFai Lau (8):
  bpf: Move insn_buf[16] to bpf_verifier_env
  bpf: Adjust BPF_JMP that jumps to the 1st insn of the prologue
  bpf: Add gen_epilogue to bpf_verifier_ops
  bpf: Export bpf_base_func_proto
  selftests/bpf: Test gen_prologue and gen_epilogue
  selftests/bpf: Add tailcall epilogue test
  selftests/bpf: A pro/epilogue test when the main prog jumps back to
    the 1st insn
  selftests/bpf: Test epilogue patching when the main prog has multiple
    BPF_EXIT

 include/linux/bpf.h                           |   2 +
 include/linux/bpf_verifier.h                  |   4 +
 kernel/bpf/helpers.c                          |   1 +
 kernel/bpf/verifier.c                         |  73 +++++--
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 190 ++++++++++++++++++
 .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  11 +
 .../bpf/bpf_testmod/bpf_testmod_kfunc.h       |   6 +
 .../selftests/bpf/prog_tests/pro_epilogue.c   |  54 +++++
 .../selftests/bpf/progs/epilogue_exit.c       |  78 +++++++
 .../selftests/bpf/progs/epilogue_tailcall.c   |  58 ++++++
 .../bpf/progs/pro_epilogue_goto_start.c       | 149 ++++++++++++++
 .../selftests/bpf/progs/pro_epilogue_kfunc.c  | 156 ++++++++++++++
 .../bpf/progs/pro_epilogue_subprog.c          | 125 ++++++++++++
 tools/testing/selftests/bpf/test_loader.c     |  27 +++
 14 files changed, 922 insertions(+), 12 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
 create mode 100644 tools/testing/selftests/bpf/progs/epilogue_exit.c
 create mode 100644 tools/testing/selftests/bpf/progs/epilogue_tailcall.c
 create mode 100644 tools/testing/selftests/bpf/progs/pro_epilogue_goto_start.c
 create mode 100644 tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c
 create mode 100644 tools/testing/selftests/bpf/progs/pro_epilogue_subprog.c

-- 
2.43.5

[PATCH v4 bpf-next 1/9] bpf: Move insn_buf[16] to bpf_verifier_env

[Martin KaFai Lau]

From: Martin KaFai Lau <martin.lau@kernel.org>

This patch moves the 'struct bpf_insn insn_buf[16]' stack usage
to the bpf_verifier_env. A '#define INSN_BUF_SIZE 16' is also added
to replace the ARRAY_SIZE(insn_buf) usages.

Both convert_ctx_accesses() and do_misc_fixup() are changed
to use the env->insn_buf.

It is a prep work for adding the epilogue_buf[16] in a later patch.

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 include/linux/bpf_verifier.h |  3 +++
 kernel/bpf/verifier.c        | 15 ++++++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 279b4a640644..0ad2d189c546 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -23,6 +23,8 @@
  * (in the "-8,-16,...,-512" form)
  */
 #define TMP_STR_BUF_LEN 320
+/* Patch buffer size */
+#define INSN_BUF_SIZE 16
 
 /* Liveness marks, used for registers and spilled-regs (in stack slots).
  * Read marks propagate upwards until they find a write mark; they record that
@@ -780,6 +782,7 @@ struct bpf_verifier_env {
 	 * e.g., in reg_type_str() to generate reg_type string
 	 */
 	char tmp_str_buf[TMP_STR_BUF_LEN];
+	struct bpf_insn insn_buf[INSN_BUF_SIZE];
 };
 
 static inline struct bpf_func_info_aux *subprog_aux(struct bpf_verifier_env *env, int subprog)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 33270b363689..b408692a12d7 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19678,7 +19678,8 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 	const struct bpf_verifier_ops *ops = env->ops;
 	int i, cnt, size, ctx_field_size, delta = 0;
 	const int insn_cnt = env->prog->len;
-	struct bpf_insn insn_buf[16], *insn;
+	struct bpf_insn *insn_buf = env->insn_buf;
+	struct bpf_insn *insn;
 	u32 target_size, size_default, off;
 	struct bpf_prog *new_prog;
 	enum bpf_access_type type;
@@ -19691,7 +19692,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 		}
 		cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
 					env->prog);
-		if (cnt >= ARRAY_SIZE(insn_buf)) {
+		if (cnt >= INSN_BUF_SIZE) {
 			verbose(env, "bpf verifier is misconfigured\n");
 			return -EINVAL;
 		} else if (cnt) {
@@ -19838,7 +19839,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 		target_size = 0;
 		cnt = convert_ctx_access(type, insn, insn_buf, env->prog,
 					 &target_size);
-		if (cnt == 0 || cnt >= ARRAY_SIZE(insn_buf) ||
+		if (cnt == 0 || cnt >= INSN_BUF_SIZE ||
 		    (ctx_field_size && !target_size)) {
 			verbose(env, "bpf verifier is misconfigured\n");
 			return -EINVAL;
@@ -19847,7 +19848,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 		if (is_narrower_load && size < target_size) {
 			u8 shift = bpf_ctx_narrow_access_offset(
 				off, size, size_default) * 8;
-			if (shift && cnt + 1 >= ARRAY_SIZE(insn_buf)) {
+			if (shift && cnt + 1 >= INSN_BUF_SIZE) {
 				verbose(env, "bpf verifier narrow ctx load misconfigured\n");
 				return -EINVAL;
 			}
@@ -20392,7 +20393,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 	const int insn_cnt = prog->len;
 	const struct bpf_map_ops *ops;
 	struct bpf_insn_aux_data *aux;
-	struct bpf_insn insn_buf[16];
+	struct bpf_insn *insn_buf = env->insn_buf;
 	struct bpf_prog *new_prog;
 	struct bpf_map *map_ptr;
 	int i, ret, cnt, delta = 0, cur_subprog = 0;
@@ -20511,7 +20512,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 		    (BPF_MODE(insn->code) == BPF_ABS ||
 		     BPF_MODE(insn->code) == BPF_IND)) {
 			cnt = env->ops->gen_ld_abs(insn, insn_buf);
-			if (cnt == 0 || cnt >= ARRAY_SIZE(insn_buf)) {
+			if (cnt == 0 || cnt >= INSN_BUF_SIZE) {
 				verbose(env, "bpf verifier is misconfigured\n");
 				return -EINVAL;
 			}
@@ -20804,7 +20805,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 				cnt = ops->map_gen_lookup(map_ptr, insn_buf);
 				if (cnt == -EOPNOTSUPP)
 					goto patch_map_ops_generic;
-				if (cnt <= 0 || cnt >= ARRAY_SIZE(insn_buf)) {
+				if (cnt <= 0 || cnt >= INSN_BUF_SIZE) {
 					verbose(env, "bpf verifier is misconfigured\n");
 					return -EINVAL;
 				}
-- 
2.43.5

Re: [PATCH v4 bpf-next 1/9] bpf: Move insn_buf[16] to bpf_verifier_env

[Eduard Zingerman]

On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> This patch moves the 'struct bpf_insn insn_buf[16]' stack usage
> to the bpf_verifier_env. A '#define INSN_BUF_SIZE 16' is also added
> to replace the ARRAY_SIZE(insn_buf) usages.
> 
> Both convert_ctx_accesses() and do_misc_fixup() are changed
> to use the env->insn_buf.
> 
> It is a prep work for adding the epilogue_buf[16] in a later patch.
> 
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---

Not sure if this refactoring is worth it but code looks correct.
Note that there is also inline_bpf_loop()
(it needs a slightly bigger buffer).

Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

Re: [PATCH v4 bpf-next 1/9] bpf: Move insn_buf[16] to bpf_verifier_env

[Alexei Starovoitov]

On Wed, Aug 28, 2024 at 5:41 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
> > From: Martin KaFai Lau <martin.lau@kernel.org>
> >
> > This patch moves the 'struct bpf_insn insn_buf[16]' stack usage
> > to the bpf_verifier_env. A '#define INSN_BUF_SIZE 16' is also added
> > to replace the ARRAY_SIZE(insn_buf) usages.
> >
> > Both convert_ctx_accesses() and do_misc_fixup() are changed
> > to use the env->insn_buf.
> >
> > It is a prep work for adding the epilogue_buf[16] in a later patch.
> >
> > Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> > ---
>
> Not sure if this refactoring is worth it but code looks correct.
> Note that there is also inline_bpf_loop()
> (it needs a slightly bigger buffer).

Probably worth it in the follow up, since people complain that
this or that function in verifier.c reaches stack size limit
when compiled with sanitizers.
These buffers on stack are the biggest consumers.

Re: [PATCH v4 bpf-next 1/9] bpf: Move insn_buf[16] to bpf_verifier_env

[Martin KaFai Lau]

On 8/28/24 6:46 PM, Alexei Starovoitov wrote:
> On Wed, Aug 28, 2024 at 5:41 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>>
>> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
>>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>>
>>> This patch moves the 'struct bpf_insn insn_buf[16]' stack usage
>>> to the bpf_verifier_env. A '#define INSN_BUF_SIZE 16' is also added
>>> to replace the ARRAY_SIZE(insn_buf) usages.
>>>
>>> Both convert_ctx_accesses() and do_misc_fixup() are changed
>>> to use the env->insn_buf.
>>>
>>> It is a prep work for adding the epilogue_buf[16] in a later patch.
>>>
>>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>>> ---
>>
>> Not sure if this refactoring is worth it but code looks correct.
>> Note that there is also inline_bpf_loop()
>> (it needs a slightly bigger buffer).
> 
> Probably worth it in the follow up, since people complain that
> this or that function in verifier.c reaches stack size limit
> when compiled with sanitizers.
> These buffers on stack are the biggest consumers.

ok. I will drop this patch for now. Redo it again as a followup and will 
consider inline_bpf_loop() together at that time.

Regarding the stack size, I did notice the compilation warning difference on the 
stack size which I should have put in the commit message.

Before:
./kernel/bpf/verifier.c:22133:5: warning: stack frame size (2584) exceeds limit 
(2048) in 'bpf_check' [-Wframe-larger-than]

After:
./kernel/bpf/verifier.c:22184:5: warning: stack frame size (2264) exceeds limit 
(2048) in 'bpf_check' [-Wframe-larger-than]

Re: [PATCH v4 bpf-next 1/9] bpf: Move insn_buf[16] to bpf_verifier_env

[Alexei Starovoitov]

On Thu, Aug 29, 2024 at 8:20 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 8/28/24 6:46 PM, Alexei Starovoitov wrote:
> > On Wed, Aug 28, 2024 at 5:41 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> >>
> >> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
> >>> From: Martin KaFai Lau <martin.lau@kernel.org>
> >>>
> >>> This patch moves the 'struct bpf_insn insn_buf[16]' stack usage
> >>> to the bpf_verifier_env. A '#define INSN_BUF_SIZE 16' is also added
> >>> to replace the ARRAY_SIZE(insn_buf) usages.
> >>>
> >>> Both convert_ctx_accesses() and do_misc_fixup() are changed
> >>> to use the env->insn_buf.
> >>>
> >>> It is a prep work for adding the epilogue_buf[16] in a later patch.
> >>>
> >>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> >>> ---
> >>
> >> Not sure if this refactoring is worth it but code looks correct.
> >> Note that there is also inline_bpf_loop()
> >> (it needs a slightly bigger buffer).
> >
> > Probably worth it in the follow up, since people complain that
> > this or that function in verifier.c reaches stack size limit
> > when compiled with sanitizers.
> > These buffers on stack are the biggest consumers.
>
> ok. I will drop this patch for now. Redo it again as a followup and will
> consider inline_bpf_loop() together at that time.

why? Keep it. It's an improvement already.

> Regarding the stack size, I did notice the compilation warning difference on the
> stack size which I should have put in the commit message.
>
> Before:
> ./kernel/bpf/verifier.c:22133:5: warning: stack frame size (2584) exceeds limit
> (2048) in 'bpf_check' [-Wframe-larger-than]
>
> After:
> ./kernel/bpf/verifier.c:22184:5: warning: stack frame size (2264) exceeds limit
> (2048) in 'bpf_check' [-Wframe-larger-than]

Exactly. It's a step forward.

Re: [PATCH v4 bpf-next 1/9] bpf: Move insn_buf[16] to bpf_verifier_env

[Martin KaFai Lau]

On 8/29/24 8:26 AM, Alexei Starovoitov wrote:
> On Thu, Aug 29, 2024 at 8:20 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 8/28/24 6:46 PM, Alexei Starovoitov wrote:
>>> On Wed, Aug 28, 2024 at 5:41 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>>>>
>>>> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
>>>>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>>>>
>>>>> This patch moves the 'struct bpf_insn insn_buf[16]' stack usage
>>>>> to the bpf_verifier_env. A '#define INSN_BUF_SIZE 16' is also added
>>>>> to replace the ARRAY_SIZE(insn_buf) usages.
>>>>>
>>>>> Both convert_ctx_accesses() and do_misc_fixup() are changed
>>>>> to use the env->insn_buf.
>>>>>
>>>>> It is a prep work for adding the epilogue_buf[16] in a later patch.
>>>>>
>>>>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>>>>> ---
>>>>
>>>> Not sure if this refactoring is worth it but code looks correct.
>>>> Note that there is also inline_bpf_loop()
>>>> (it needs a slightly bigger buffer).
>>>
>>> Probably worth it in the follow up, since people complain that

I read it and Eduard's earlier comment together as the whole patch 1 as a follow 
up. :)

yep. I will keep this one and follow up with the insn_buf in the inline_bpf_loop().

I will update the commit message with the stack usage in the next respin.

>>> this or that function in verifier.c reaches stack size limit
>>> when compiled with sanitizers.
>>> These buffers on stack are the biggest consumers.
>>
>> ok. I will drop this patch for now. Redo it again as a followup and will
>> consider inline_bpf_loop() together at that time.
> 
> why? Keep it. It's an improvement already.

[PATCH v4 bpf-next 3/9] bpf: Add gen_epilogue to bpf_verifier_ops

[Martin KaFai Lau]

From: Martin KaFai Lau <martin.lau@kernel.org>

This patch adds a .gen_epilogue to the bpf_verifier_ops. It is similar
to the existing .gen_prologue. Instead of allowing a subsystem
to run code at the beginning of a bpf prog, it allows the subsystem
to run code just before the bpf prog exit.

One of the use case is to allow the upcoming bpf qdisc to ensure that
the skb->dev is the same as the qdisc->dev_queue->dev. The bpf qdisc
struct_ops implementation could either fix it up or drop the skb.
Another use case could be in bpf_tcp_ca.c to enforce snd_cwnd
has sane value (e.g. non zero).

The epilogue can do the useful thing (like checking skb->dev) if it
can access the bpf prog's ctx. Unlike prologue, r1 may not hold the
ctx pointer. This patch saves the r1 in the stack if the .gen_epilogue
has returned some instructions in the "epilogue_buf".

The existing .gen_prologue is done in convert_ctx_accesses().
The new .gen_epilogue is done in the convert_ctx_accesses() also.
When it sees the (BPF_JMP | BPF_EXIT) instruction, it will be patched
with the earlier generated "epilogue_buf". The epilogue patching is
only done for the main prog.

Only one epilogue will be patched to the main program. When the
bpf prog has multiple BPF_EXIT instructions, a BPF_JA is used
to goto the earlier patched epilogue. Majority of the archs
support (BPF_JMP32 | BPF_JA): x86, arm, s390, risv64, loongarch,
powerpc and arc. This patch keeps it simple and always
use (BPF_JMP32 | BPF_JA).

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 include/linux/bpf.h          |  2 ++
 include/linux/bpf_verifier.h |  1 +
 kernel/bpf/verifier.c        | 47 +++++++++++++++++++++++++++++++++++-
 3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index dc63083f76b7..6f87fb014fba 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -974,6 +974,8 @@ struct bpf_verifier_ops {
 				struct bpf_insn_access_aux *info);
 	int (*gen_prologue)(struct bpf_insn *insn, bool direct_write,
 			    const struct bpf_prog *prog);
+	int (*gen_epilogue)(struct bpf_insn *insn, const struct bpf_prog *prog,
+			    s16 ctx_stack_off);
 	int (*gen_ld_abs)(const struct bpf_insn *orig,
 			  struct bpf_insn *insn_buf);
 	u32 (*convert_ctx_access)(enum bpf_access_type type,
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 0ad2d189c546..2e20207315a9 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -783,6 +783,7 @@ struct bpf_verifier_env {
 	 */
 	char tmp_str_buf[TMP_STR_BUF_LEN];
 	struct bpf_insn insn_buf[INSN_BUF_SIZE];
+	struct bpf_insn epilogue_buf[INSN_BUF_SIZE];
 };
 
 static inline struct bpf_func_info_aux *subprog_aux(struct bpf_verifier_env *env, int subprog)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8714b83c5fb8..abbe43b33f91 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19675,15 +19675,39 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
  */
 static int convert_ctx_accesses(struct bpf_verifier_env *env)
 {
+	struct bpf_subprog_info *subprogs = env->subprog_info;
 	const struct bpf_verifier_ops *ops = env->ops;
-	int i, cnt, size, ctx_field_size, delta = 0;
+	int i, cnt, size, ctx_field_size, delta = 0, epilogue_cnt = 0;
 	const int insn_cnt = env->prog->len;
+	struct bpf_insn *epilogue_buf = env->epilogue_buf;
 	struct bpf_insn *insn_buf = env->insn_buf;
 	struct bpf_insn *insn;
 	u32 target_size, size_default, off;
 	struct bpf_prog *new_prog;
 	enum bpf_access_type type;
 	bool is_narrower_load;
+	int epilogue_idx = 0;
+
+	if (ops->gen_epilogue) {
+		epilogue_cnt = ops->gen_epilogue(epilogue_buf, env->prog,
+						 -(subprogs[0].stack_depth + 8));
+		if (epilogue_cnt >= INSN_BUF_SIZE) {
+			verbose(env, "bpf verifier is misconfigured\n");
+			return -EINVAL;
+		} else if (epilogue_cnt) {
+			/* Save the ARG_PTR_TO_CTX for the epilogue to use */
+			cnt = 0;
+			subprogs[0].stack_depth += 8;
+			insn_buf[cnt++] = BPF_STX_MEM(BPF_DW, BPF_REG_FP, BPF_REG_1,
+						      -subprogs[0].stack_depth);
+			insn_buf[cnt++] = env->prog->insnsi[0];
+			new_prog = bpf_patch_insn_data(env, 0, insn_buf, cnt);
+			if (!new_prog)
+				return -ENOMEM;
+			env->prog = new_prog;
+			delta += cnt - 1;
+		}
+	}
 
 	if (ops->gen_prologue || env->seen_direct_write) {
 		if (!ops->gen_prologue) {
@@ -19740,6 +19764,26 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 			insn->code = BPF_STX | BPF_PROBE_ATOMIC | BPF_SIZE(insn->code);
 			env->prog->aux->num_exentries++;
 			continue;
+		} else if (insn->code == (BPF_JMP | BPF_EXIT) &&
+			   epilogue_cnt &&
+			   i + delta < subprogs[1].start) {
+			/* Generate epilogue for the main prog */
+			if (epilogue_idx) {
+				/* jump back to the earlier generated epilogue */
+				insn_buf[0] = BPF_JMP32_IMM(BPF_JA, 0,
+							    epilogue_idx - i - delta - 1, 0);
+				cnt = 1;
+			} else {
+				memcpy(insn_buf, epilogue_buf,
+				       epilogue_cnt * sizeof(*epilogue_buf));
+				cnt = epilogue_cnt;
+				/* epilogue_idx cannot be 0. It must have at
+				 * least one ctx ptr saving insn before the
+				 * epilogue.
+				 */
+				 epilogue_idx = i + delta;
+			}
+			goto patch_insn_buf;
 		} else {
 			continue;
 		}
@@ -19876,6 +19920,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 						       insn->dst_reg, insn->dst_reg,
 						       size * 8, 0);
 
+patch_insn_buf:
 		new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
 		if (!new_prog)
 			return -ENOMEM;
-- 
2.43.5

Re: [PATCH v4 bpf-next 3/9] bpf: Add gen_epilogue to bpf_verifier_ops

[Eduard Zingerman]

On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> This patch adds a .gen_epilogue to the bpf_verifier_ops. It is similar
> to the existing .gen_prologue. Instead of allowing a subsystem
> to run code at the beginning of a bpf prog, it allows the subsystem
> to run code just before the bpf prog exit.
> 
> One of the use case is to allow the upcoming bpf qdisc to ensure that
> the skb->dev is the same as the qdisc->dev_queue->dev. The bpf qdisc
> struct_ops implementation could either fix it up or drop the skb.
> Another use case could be in bpf_tcp_ca.c to enforce snd_cwnd
> has sane value (e.g. non zero).
> 
> The epilogue can do the useful thing (like checking skb->dev) if it
> can access the bpf prog's ctx. Unlike prologue, r1 may not hold the
> ctx pointer. This patch saves the r1 in the stack if the .gen_epilogue
> has returned some instructions in the "epilogue_buf".
> 
> The existing .gen_prologue is done in convert_ctx_accesses().
> The new .gen_epilogue is done in the convert_ctx_accesses() also.
> When it sees the (BPF_JMP | BPF_EXIT) instruction, it will be patched
> with the earlier generated "epilogue_buf". The epilogue patching is
> only done for the main prog.
> 
> Only one epilogue will be patched to the main program. When the
> bpf prog has multiple BPF_EXIT instructions, a BPF_JA is used
> to goto the earlier patched epilogue. Majority of the archs
> support (BPF_JMP32 | BPF_JA): x86, arm, s390, risv64, loongarch,
> powerpc and arc. This patch keeps it simple and always
> use (BPF_JMP32 | BPF_JA).
> 
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> @@ -19740,6 +19764,26 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>  			insn->code = BPF_STX | BPF_PROBE_ATOMIC | BPF_SIZE(insn->code);
>  			env->prog->aux->num_exentries++;
>  			continue;
> +		} else if (insn->code == (BPF_JMP | BPF_EXIT) &&
> +			   epilogue_cnt &&
> +			   i + delta < subprogs[1].start) {
> +			/* Generate epilogue for the main prog */
> +			if (epilogue_idx) {
> +				/* jump back to the earlier generated epilogue */
> +				insn_buf[0] = BPF_JMP32_IMM(BPF_JA, 0,
> +							    epilogue_idx - i - delta - 1, 0);

Nit: maybe add BPF_GOTOL macro or mention that this is a 'gotol' instruction in the comment?
     (this is how it is called in llvm).

> +				cnt = 1;
> +			} else {
> +				memcpy(insn_buf, epilogue_buf,
> +				       epilogue_cnt * sizeof(*epilogue_buf));
> +				cnt = epilogue_cnt;
> +				/* epilogue_idx cannot be 0. It must have at
> +				 * least one ctx ptr saving insn before the
> +				 * epilogue.
> +				 */
> +				 epilogue_idx = i + delta;
> +			}
> +			goto patch_insn_buf;
>  		} else {
>  			continue;
>  		}

[...]

Re: [PATCH v4 bpf-next 3/9] bpf: Add gen_epilogue to bpf_verifier_ops

[Martin KaFai Lau]

On 8/28/24 7:26 PM, Eduard Zingerman wrote:
> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>
>> This patch adds a .gen_epilogue to the bpf_verifier_ops. It is similar
>> to the existing .gen_prologue. Instead of allowing a subsystem
>> to run code at the beginning of a bpf prog, it allows the subsystem
>> to run code just before the bpf prog exit.
>>
>> One of the use case is to allow the upcoming bpf qdisc to ensure that
>> the skb->dev is the same as the qdisc->dev_queue->dev. The bpf qdisc
>> struct_ops implementation could either fix it up or drop the skb.
>> Another use case could be in bpf_tcp_ca.c to enforce snd_cwnd
>> has sane value (e.g. non zero).
>>
>> The epilogue can do the useful thing (like checking skb->dev) if it
>> can access the bpf prog's ctx. Unlike prologue, r1 may not hold the
>> ctx pointer. This patch saves the r1 in the stack if the .gen_epilogue
>> has returned some instructions in the "epilogue_buf".
>>
>> The existing .gen_prologue is done in convert_ctx_accesses().
>> The new .gen_epilogue is done in the convert_ctx_accesses() also.
>> When it sees the (BPF_JMP | BPF_EXIT) instruction, it will be patched
>> with the earlier generated "epilogue_buf". The epilogue patching is
>> only done for the main prog.
>>
>> Only one epilogue will be patched to the main program. When the
>> bpf prog has multiple BPF_EXIT instructions, a BPF_JA is used
>> to goto the earlier patched epilogue. Majority of the archs
>> support (BPF_JMP32 | BPF_JA): x86, arm, s390, risv64, loongarch,
>> powerpc and arc. This patch keeps it simple and always
>> use (BPF_JMP32 | BPF_JA).
>>
>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>> ---
> 
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> 
> [...]
> 
>> @@ -19740,6 +19764,26 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>>   			insn->code = BPF_STX | BPF_PROBE_ATOMIC | BPF_SIZE(insn->code);
>>   			env->prog->aux->num_exentries++;
>>   			continue;
>> +		} else if (insn->code == (BPF_JMP | BPF_EXIT) &&
>> +			   epilogue_cnt &&
>> +			   i + delta < subprogs[1].start) {
>> +			/* Generate epilogue for the main prog */
>> +			if (epilogue_idx) {
>> +				/* jump back to the earlier generated epilogue */
>> +				insn_buf[0] = BPF_JMP32_IMM(BPF_JA, 0,
>> +							    epilogue_idx - i - delta - 1, 0);
> 
> Nit: maybe add BPF_GOTOL macro or mention that this is a 'gotol' instruction in the comment?
>       (this is how it is called in llvm).

sgtm.

There is a BPF_JMP_A(OFF). BPF_JMP32_A(IMM) probably will be more consistent 
with the other existing macros.

[PATCH v4 bpf-next 4/9] bpf: Export bpf_base_func_proto

[Martin KaFai Lau]

From: Martin KaFai Lau <martin.lau@kernel.org>

The bpf_testmod needs to use the bpf_tail_call helper in
a later selftest patch. This patch is to EXPORT_GPL_SYMBOL
the bpf_base_func_proto.

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 kernel/bpf/helpers.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 7e7761c537f8..3956be5d6440 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2034,6 +2034,7 @@ bpf_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return NULL;
 	}
 }
+EXPORT_SYMBOL_GPL(bpf_base_func_proto);
 
 void bpf_list_head_free(const struct btf_field *field, void *list_head,
 			struct bpf_spin_lock *spin_lock)
-- 
2.43.5

[PATCH v4 bpf-next 5/9] selftests/bpf: attach struct_ops maps before test prog runs

[Martin KaFai Lau]

From: Eduard Zingerman <eddyz87@gmail.com>

In test_loader based tests to bpf_map__attach_struct_ops()
before call to bpf_prog_test_run_opts() in order to trigger
bpf_struct_ops->reg() callbacks on kernel side.
This allows to use __retval macro for struct_ops tests.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 tools/testing/selftests/bpf/test_loader.c | 27 +++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 4223cffc090e..3e9b009580d4 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -890,11 +890,13 @@ void run_subtest(struct test_loader *tester,
 {
 	struct test_subspec *subspec = unpriv ? &spec->unpriv : &spec->priv;
 	struct bpf_program *tprog = NULL, *tprog_iter;
+	struct bpf_link *link, *links[32] = {};
 	struct test_spec *spec_iter;
 	struct cap_state caps = {};
 	struct bpf_object *tobj;
 	struct bpf_map *map;
 	int retval, err, i;
+	int links_cnt = 0;
 	bool should_load;
 
 	if (!test__start_subtest(subspec->name))
@@ -999,6 +1001,26 @@ void run_subtest(struct test_loader *tester,
 		if (restore_capabilities(&caps))
 			goto tobj_cleanup;
 
+		/* Do bpf_map__attach_struct_ops() for each struct_ops map.
+		 * This should trigger bpf_struct_ops->reg callback on kernel side.
+		 */
+		bpf_object__for_each_map(map, tobj) {
+			if (!bpf_map__autocreate(map) ||
+			    bpf_map__type(map) != BPF_MAP_TYPE_STRUCT_OPS)
+				continue;
+			if (links_cnt >= ARRAY_SIZE(links)) {
+				PRINT_FAIL("too many struct_ops maps");
+				goto tobj_cleanup;
+			}
+			link = bpf_map__attach_struct_ops(map);
+			if (!link) {
+				PRINT_FAIL("bpf_map__attach_struct_ops failed for map %s: err=%d\n",
+					   bpf_map__name(map), err);
+				goto tobj_cleanup;
+			}
+			links[links_cnt++] = link;
+		}
+
 		if (tester->pre_execution_cb) {
 			err = tester->pre_execution_cb(tobj);
 			if (err) {
@@ -1013,9 +1035,14 @@ void run_subtest(struct test_loader *tester,
 			PRINT_FAIL("Unexpected retval: %d != %d\n", retval, subspec->retval);
 			goto tobj_cleanup;
 		}
+		/* redo bpf_map__attach_struct_ops for each test */
+		while (links_cnt > 0)
+			bpf_link__destroy(links[--links_cnt]);
 	}
 
 tobj_cleanup:
+	while (links_cnt > 0)
+		bpf_link__destroy(links[--links_cnt]);
 	bpf_object__close(tobj);
 subtest_cleanup:
 	test__end_subtest();
-- 
2.43.5

[PATCH v4 bpf-next 6/9] selftests/bpf: Test gen_prologue and gen_epilogue

[Martin KaFai Lau]

From: Martin KaFai Lau <martin.lau@kernel.org>

This test adds a new struct_ops "bpf_testmod_st_ops" in bpf_testmod.
The ops of the bpf_testmod_st_ops is triggered by new kfunc calls
"bpf_kfunc_st_ops_test_*logue". These new kfunc calls are
primarily used by the SEC("syscall") program. The test triggering
sequence is like:
    SEC("syscall")
    syscall_prologue_subprog(struct st_ops_args *args)
        bpf_kfunc_st_op_test_prologue(args)
	    st_ops->test_prologue(args)

.gen_prologue adds 1000 to args->a
.gen_epilogue adds 10000 to args->a
.gen_epilogue will also set the r0 to 2 * args->a.

The .gen_prologue and .gen_epilogue of the bpf_testmod_st_ops
will test the prog->aux->attach_func_name to decide if
it needs to generate codes.

The main programs of the pro_epilogue_subprog.c will call a subprog()
which does "args->a += 1".

The main programs of the pro_epilogue_kfunc.c will call a
new kfunc bpf_kfunc_st_ops_inc10 which does "args->a += 10".

This patch uses the test_loader infra to check the __xlated
instructions patched after gen_prologue and/or gen_epilogue.
The __xlated check is based on Eduard's example (Thanks!) in v1.

args->a is returned by the struct_ops prog (either the main prog
or the epilogue). Thus, the __retval of the SEC("syscall") prog
is checked. For example, when triggering the ops in the
'SEC("struct_ops/test_epilogue_subprog") int test_epilogue_subprog'
The expected args->a is +1 (subprog call) + 10000 (.gen_epilogue) = 10001.
The expected return value is 2 * 10001 (.gen_epilogue).

Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 190 ++++++++++++++++++
 .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  11 +
 .../bpf/bpf_testmod/bpf_testmod_kfunc.h       |   6 +
 .../selftests/bpf/prog_tests/pro_epilogue.c   |  12 ++
 .../selftests/bpf/progs/pro_epilogue_kfunc.c  | 156 ++++++++++++++
 .../bpf/progs/pro_epilogue_subprog.c          | 125 ++++++++++++
 6 files changed, 500 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
 create mode 100644 tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c
 create mode 100644 tools/testing/selftests/bpf/progs/pro_epilogue_subprog.c

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index c04b7dec2ab9..b8b6989d596a 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -17,6 +17,7 @@
 #include <linux/in.h>
 #include <linux/in6.h>
 #include <linux/un.h>
+#include <linux/filter.h>
 #include <net/sock.h>
 #include <linux/namei.h>
 #include "bpf_testmod.h"
@@ -928,6 +929,51 @@ __bpf_kfunc int bpf_kfunc_call_kernel_getpeername(struct addr_args *args)
 	return err;
 }
 
+static DEFINE_MUTEX(st_ops_mutex);
+static struct bpf_testmod_st_ops *st_ops;
+
+__bpf_kfunc int bpf_kfunc_st_ops_test_prologue(struct st_ops_args *args)
+{
+	int ret = -1;
+
+	mutex_lock(&st_ops_mutex);
+	if (st_ops && st_ops->test_prologue)
+		ret = st_ops->test_prologue(args);
+	mutex_unlock(&st_ops_mutex);
+
+	return ret;
+}
+
+__bpf_kfunc int bpf_kfunc_st_ops_test_epilogue(struct st_ops_args *args)
+{
+	int ret = -1;
+
+	mutex_lock(&st_ops_mutex);
+	if (st_ops && st_ops->test_epilogue)
+		ret = st_ops->test_epilogue(args);
+	mutex_unlock(&st_ops_mutex);
+
+	return ret;
+}
+
+__bpf_kfunc int bpf_kfunc_st_ops_test_pro_epilogue(struct st_ops_args *args)
+{
+	int ret = -1;
+
+	mutex_lock(&st_ops_mutex);
+	if (st_ops && st_ops->test_pro_epilogue)
+		ret = st_ops->test_pro_epilogue(args);
+	mutex_unlock(&st_ops_mutex);
+
+	return ret;
+}
+
+__bpf_kfunc int bpf_kfunc_st_ops_inc10(struct st_ops_args *args)
+{
+	args->a += 10;
+	return args->a;
+}
+
 BTF_KFUNCS_START(bpf_testmod_check_kfunc_ids)
 BTF_ID_FLAGS(func, bpf_testmod_test_mod_kfunc)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
@@ -964,6 +1010,10 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_kernel_sendmsg, KF_SLEEPABLE)
 BTF_ID_FLAGS(func, bpf_kfunc_call_sock_sendmsg, KF_SLEEPABLE)
 BTF_ID_FLAGS(func, bpf_kfunc_call_kernel_getsockname, KF_SLEEPABLE)
 BTF_ID_FLAGS(func, bpf_kfunc_call_kernel_getpeername, KF_SLEEPABLE)
+BTF_ID_FLAGS(func, bpf_kfunc_st_ops_test_prologue, KF_TRUSTED_ARGS | KF_SLEEPABLE)
+BTF_ID_FLAGS(func, bpf_kfunc_st_ops_test_epilogue, KF_TRUSTED_ARGS | KF_SLEEPABLE)
+BTF_ID_FLAGS(func, bpf_kfunc_st_ops_test_pro_epilogue, KF_TRUSTED_ARGS | KF_SLEEPABLE)
+BTF_ID_FLAGS(func, bpf_kfunc_st_ops_inc10, KF_TRUSTED_ARGS)
 BTF_KFUNCS_END(bpf_testmod_check_kfunc_ids)
 
 static int bpf_testmod_ops_init(struct btf *btf)
@@ -1083,6 +1133,144 @@ struct bpf_struct_ops bpf_testmod_ops2 = {
 	.owner = THIS_MODULE,
 };
 
+static int bpf_test_mod_st_ops__test_prologue(struct st_ops_args *args)
+{
+	return 0;
+}
+
+static int bpf_test_mod_st_ops__test_epilogue(struct st_ops_args *args)
+{
+	return 0;
+}
+
+static int bpf_test_mod_st_ops__test_pro_epilogue(struct st_ops_args *args)
+{
+	return 0;
+}
+
+static int st_ops_gen_prologue(struct bpf_insn *insn_buf, bool direct_write,
+			       const struct bpf_prog *prog)
+{
+	struct bpf_insn *insn = insn_buf;
+
+	if (strcmp(prog->aux->attach_func_name, "test_prologue") &&
+	    strcmp(prog->aux->attach_func_name, "test_pro_epilogue"))
+		return 0;
+
+	/* r6 = r1[0]; // r6 will be "struct st_ops *args". r1 is "u64 *ctx".
+	 * r7 = r6->a;
+	 * r7 += 1000;
+	 * r6->a = r7;
+	 */
+	*insn++ = BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, 0);
+	*insn++ = BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_6, offsetof(struct st_ops_args, a));
+	*insn++ = BPF_ALU32_IMM(BPF_ADD, BPF_REG_7, 1000);
+	*insn++ = BPF_STX_MEM(BPF_W, BPF_REG_6, BPF_REG_7, offsetof(struct st_ops_args, a));
+	*insn++ = prog->insnsi[0];
+
+	return insn - insn_buf;
+}
+
+static int st_ops_gen_epilogue(struct bpf_insn *insn_buf, const struct bpf_prog *prog,
+			       s16 ctx_stack_off)
+{
+	struct bpf_insn *insn = insn_buf;
+
+	if (strcmp(prog->aux->attach_func_name, "test_epilogue") &&
+	    strcmp(prog->aux->attach_func_name, "test_pro_epilogue"))
+		return 0;
+
+	/* r1 = stack[ctx_stack_off]; // r1 will be "u64 *ctx"
+	 * r1 = r1[0]; // r1 will be "struct st_ops *args"
+	 * r6 = r1->a;
+	 * r6 += 10000;
+	 * r1->a = r6;
+	 * r0 = r6;
+	 * r0 *= 2;
+	 * BPF_EXIT;
+	 */
+	*insn++ = BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_FP, ctx_stack_off);
+	*insn++ = BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, 0);
+	*insn++ = BPF_LDX_MEM(BPF_W, BPF_REG_6, BPF_REG_1, offsetof(struct st_ops_args, a));
+	*insn++ = BPF_ALU32_IMM(BPF_ADD, BPF_REG_6, 10000);
+	*insn++ = BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_6, offsetof(struct st_ops_args, a));
+	*insn++ = BPF_MOV32_REG(BPF_REG_0, BPF_REG_6);
+	*insn++ = BPF_ALU32_IMM(BPF_MUL, BPF_REG_0, 2);
+	*insn++ = BPF_EXIT_INSN();
+
+	return insn - insn_buf;
+}
+
+static int st_ops_btf_struct_access(struct bpf_verifier_log *log,
+				    const struct bpf_reg_state *reg,
+				    int off, int size)
+{
+	if (off < 0 || off + size > sizeof(struct st_ops_args))
+		return -EACCES;
+	return 0;
+}
+
+static const struct bpf_verifier_ops st_ops_verifier_ops = {
+	.is_valid_access = bpf_testmod_ops_is_valid_access,
+	.btf_struct_access = st_ops_btf_struct_access,
+	.gen_prologue = st_ops_gen_prologue,
+	.gen_epilogue = st_ops_gen_epilogue,
+	.get_func_proto = bpf_base_func_proto,
+};
+
+static struct bpf_testmod_st_ops st_ops_cfi_stubs = {
+	.test_prologue = bpf_test_mod_st_ops__test_prologue,
+	.test_epilogue = bpf_test_mod_st_ops__test_epilogue,
+	.test_pro_epilogue = bpf_test_mod_st_ops__test_pro_epilogue,
+};
+
+static int st_ops_reg(void *kdata, struct bpf_link *link)
+{
+	int err = 0;
+
+	mutex_lock(&st_ops_mutex);
+	if (st_ops) {
+		pr_err("st_ops has already been registered\n");
+		err = -EEXIST;
+		goto unlock;
+	}
+	st_ops = kdata;
+
+unlock:
+	mutex_unlock(&st_ops_mutex);
+	return err;
+}
+
+static void st_ops_unreg(void *kdata, struct bpf_link *link)
+{
+	mutex_lock(&st_ops_mutex);
+	st_ops = NULL;
+	mutex_unlock(&st_ops_mutex);
+}
+
+static int st_ops_init(struct btf *btf)
+{
+	return 0;
+}
+
+static int st_ops_init_member(const struct btf_type *t,
+			      const struct btf_member *member,
+			      void *kdata, const void *udata)
+{
+	return 0;
+}
+
+static struct bpf_struct_ops testmod_st_ops = {
+	.verifier_ops = &st_ops_verifier_ops,
+	.init = st_ops_init,
+	.init_member = st_ops_init_member,
+	.reg = st_ops_reg,
+	.unreg = st_ops_unreg,
+	.cfi_stubs = &st_ops_cfi_stubs,
+	.name = "bpf_testmod_st_ops",
+	.owner = THIS_MODULE,
+};
+
 extern int bpf_fentry_test1(int a);
 
 static int bpf_testmod_init(void)
@@ -1100,8 +1288,10 @@ static int bpf_testmod_init(void)
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_testmod_kfunc_set);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_testmod_kfunc_set);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_testmod_kfunc_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_testmod_kfunc_set);
 	ret = ret ?: register_bpf_struct_ops(&bpf_bpf_testmod_ops, bpf_testmod_ops);
 	ret = ret ?: register_bpf_struct_ops(&bpf_testmod_ops2, bpf_testmod_ops2);
+	ret = ret ?: register_bpf_struct_ops(&testmod_st_ops, bpf_testmod_st_ops);
 	ret = ret ?: register_btf_id_dtor_kfuncs(bpf_testmod_dtors,
 						 ARRAY_SIZE(bpf_testmod_dtors),
 						 THIS_MODULE);
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
index fe0d402b0d65..3241a9d796ed 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
@@ -94,4 +94,15 @@ struct bpf_testmod_ops2 {
 	int (*test_1)(void);
 };
 
+struct st_ops_args {
+	int a;
+};
+
+struct bpf_testmod_st_ops {
+	int (*test_prologue)(struct st_ops_args *args);
+	int (*test_epilogue)(struct st_ops_args *args);
+	int (*test_pro_epilogue)(struct st_ops_args *args);
+	struct module *owner;
+};
+
 #endif /* _BPF_TESTMOD_H */
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
index e587a79f2239..0df429a0edaa 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
@@ -144,4 +144,10 @@ void bpf_kfunc_dynptr_test(struct bpf_dynptr *ptr, struct bpf_dynptr *ptr__nulla
 struct bpf_testmod_ctx *bpf_testmod_ctx_create(int *err) __ksym;
 void bpf_testmod_ctx_release(struct bpf_testmod_ctx *ctx) __ksym;
 
+struct st_ops_args;
+int bpf_kfunc_st_ops_test_prologue(struct st_ops_args *args) __ksym;
+int bpf_kfunc_st_ops_test_epilogue(struct st_ops_args *args) __ksym;
+int bpf_kfunc_st_ops_test_pro_epilogue(struct st_ops_args *args) __ksym;
+int bpf_kfunc_st_ops_inc10(struct st_ops_args *args) __ksym;
+
 #endif /* _BPF_TESTMOD_KFUNC_H */
diff --git a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
new file mode 100644
index 000000000000..69e4a5a1756d
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+
+#include <test_progs.h>
+#include "pro_epilogue_subprog.skel.h"
+#include "pro_epilogue_kfunc.skel.h"
+
+void test_pro_epilogue(void)
+{
+	RUN_TESTS(pro_epilogue_subprog);
+	RUN_TESTS(pro_epilogue_kfunc);
+}
diff --git a/tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c b/tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c
new file mode 100644
index 000000000000..7d1124cf4942
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+#include "../bpf_testmod/bpf_testmod.h"
+#include "../bpf_testmod/bpf_testmod_kfunc.h"
+
+char _license[] SEC("license") = "GPL";
+
+void __kfunc_btf_root(void)
+{
+	struct st_ops_args args = {};
+
+	bpf_kfunc_st_ops_inc10(&args);
+}
+
+static __noinline __used int subprog(struct st_ops_args *args)
+{
+	args->a += 1;
+	return args->a;
+}
+
+__success
+/* prologue */
+__xlated("0: r6 = *(u64 *)(r1 +0)")
+__xlated("1: r7 = *(u32 *)(r6 +0)")
+__xlated("2: w7 += 1000")
+__xlated("3: *(u32 *)(r6 +0) = r7")
+/* main prog */
+__xlated("4: r1 = *(u64 *)(r1 +0)")
+__xlated("5: r6 = r1")
+__xlated("6: call kernel-function")
+__xlated("7: r1 = r6")
+__xlated("8: call pc+1")
+__xlated("9: exit")
+SEC("struct_ops/test_prologue_kfunc")
+__naked int test_prologue_kfunc(void)
+{
+	asm volatile (
+	"r1 = *(u64 *)(r1 +0);"
+	"r6 = r1;"
+	"call %[bpf_kfunc_st_ops_inc10];"
+	"r1 = r6;"
+	"call subprog;"
+	"exit;"
+	:
+	: __imm(bpf_kfunc_st_ops_inc10)
+	: __clobber_all);
+}
+
+__success
+/* save __u64 *ctx to stack */
+__xlated("0: *(u64 *)(r10 -8) = r1")
+/* main prog */
+__xlated("1: r1 = *(u64 *)(r1 +0)")
+__xlated("2: r6 = r1")
+__xlated("3: call kernel-function")
+__xlated("4: r1 = r6")
+__xlated("5: call pc+")
+/* epilogue */
+__xlated("6: r1 = *(u64 *)(r10 -8)")
+__xlated("7: r1 = *(u64 *)(r1 +0)")
+__xlated("8: r6 = *(u32 *)(r1 +0)")
+__xlated("9: w6 += 10000")
+__xlated("10: *(u32 *)(r1 +0) = r6")
+__xlated("11: w0 = w6")
+__xlated("12: w0 *= 2")
+__xlated("13: exit")
+SEC("struct_ops/test_epilogue_kfunc")
+__naked int test_epilogue_kfunc(void)
+{
+	asm volatile (
+	"r1 = *(u64 *)(r1 +0);"
+	"r6 = r1;"
+	"call %[bpf_kfunc_st_ops_inc10];"
+	"r1 = r6;"
+	"call subprog;"
+	"exit;"
+	:
+	: __imm(bpf_kfunc_st_ops_inc10)
+	: __clobber_all);
+}
+
+__success
+/* prologue */
+__xlated("0: r6 = *(u64 *)(r1 +0)")
+__xlated("1: r7 = *(u32 *)(r6 +0)")
+__xlated("2: w7 += 1000")
+__xlated("3: *(u32 *)(r6 +0) = r7")
+/* save __u64 *ctx to stack */
+__xlated("4: *(u64 *)(r10 -8) = r1")
+/* main prog */
+__xlated("5: r1 = *(u64 *)(r1 +0)")
+__xlated("6: r6 = r1")
+__xlated("7: call kernel-function")
+__xlated("8: r1 = r6")
+__xlated("9: call pc+")
+/* epilogue */
+__xlated("10: r1 = *(u64 *)(r10 -8)")
+__xlated("11: r1 = *(u64 *)(r1 +0)")
+__xlated("12: r6 = *(u32 *)(r1 +0)")
+__xlated("13: w6 += 10000")
+__xlated("14: *(u32 *)(r1 +0) = r6")
+__xlated("15: w0 = w6")
+__xlated("16: w0 *= 2")
+__xlated("17: exit")
+SEC("struct_ops/test_pro_epilogue_kfunc")
+__naked int test_pro_epilogue_kfunc(void)
+{
+	asm volatile (
+	"r1 = *(u64 *)(r1 +0);"
+	"r6 = r1;"
+	"call %[bpf_kfunc_st_ops_inc10];"
+	"r1 = r6;"
+	"call subprog;"
+	"exit;"
+	:
+	: __imm(bpf_kfunc_st_ops_inc10)
+	: __clobber_all);
+}
+
+SEC("syscall")
+__retval(1011) /* PROLOGUE_A [1000] + KFUNC_INC10 + SUBPROG_A [1] */
+int syscall_prologue_kfunc(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_prologue(&args);
+}
+
+SEC("syscall")
+__retval(20022) /* (KFUNC_INC10 + SUBPROG_A [1] + EPILOGUE_A [10000]) * 2 */
+int syscall_epilogue_kfunc(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_epilogue(&args);
+}
+
+SEC("syscall")
+__retval(22022) /* (PROLOGUE_A [1000] + KFUNC_INC10 + SUBPROG_A [1] + EPILOGUE_A [10000]) * 2 */
+int syscall_pro_epilogue_kfunc(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_pro_epilogue(&args);
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops pro_epilogue_kfunc = {
+	.test_prologue = (void *)test_prologue_kfunc,
+	.test_epilogue = (void *)test_epilogue_kfunc,
+	.test_pro_epilogue = (void *)test_pro_epilogue_kfunc,
+};
diff --git a/tools/testing/selftests/bpf/progs/pro_epilogue_subprog.c b/tools/testing/selftests/bpf/progs/pro_epilogue_subprog.c
new file mode 100644
index 000000000000..c91b1bf30e37
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/pro_epilogue_subprog.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+#include "../bpf_testmod/bpf_testmod.h"
+#include "../bpf_testmod/bpf_testmod_kfunc.h"
+
+char _license[] SEC("license") = "GPL";
+
+static __noinline __used int subprog(struct st_ops_args *args)
+{
+	args->a += 1;
+	return args->a;
+}
+
+__success
+/* prologue */
+__xlated("0: r6 = *(u64 *)(r1 +0)")
+__xlated("1: r7 = *(u32 *)(r6 +0)")
+__xlated("2: w7 += 1000")
+__xlated("3: *(u32 *)(r6 +0) = r7")
+/* main prog */
+__xlated("4: r1 = *(u64 *)(r1 +0)")
+__xlated("5: call pc+1")
+__xlated("6: exit")
+SEC("struct_ops/test_prologue_subprog")
+__naked int test_prologue_subprog(void)
+{
+	asm volatile (
+	"r1 = *(u64 *)(r1 +0);"
+	"call subprog;"
+	"exit;"
+	::: __clobber_all);
+}
+
+__success
+/* save __u64 *ctx to stack */
+__xlated("0: *(u64 *)(r10 -8) = r1")
+/* main prog */
+__xlated("1: r1 = *(u64 *)(r1 +0)")
+__xlated("2: call pc+")
+/* epilogue */
+__xlated("3: r1 = *(u64 *)(r10 -8)")
+__xlated("4: r1 = *(u64 *)(r1 +0)")
+__xlated("5: r6 = *(u32 *)(r1 +0)")
+__xlated("6: w6 += 10000")
+__xlated("7: *(u32 *)(r1 +0) = r6")
+__xlated("8: w0 = w6")
+__xlated("9: w0 *= 2")
+__xlated("10: exit")
+SEC("struct_ops/test_epilogue_subprog")
+__naked int test_epilogue_subprog(void)
+{
+	asm volatile (
+	"r1 = *(u64 *)(r1 +0);"
+	"call subprog;"
+	"exit;"
+	::: __clobber_all);
+}
+
+__success
+/* prologue */
+__xlated("0: r6 = *(u64 *)(r1 +0)")
+__xlated("1: r7 = *(u32 *)(r6 +0)")
+__xlated("2: w7 += 1000")
+__xlated("3: *(u32 *)(r6 +0) = r7")
+/* save __u64 *ctx to stack */
+__xlated("4: *(u64 *)(r10 -8) = r1")
+/* main prog */
+__xlated("5: r1 = *(u64 *)(r1 +0)")
+__xlated("6: call pc+")
+/* epilogue */
+__xlated("7: r1 = *(u64 *)(r10 -8)")
+__xlated("8: r1 = *(u64 *)(r1 +0)")
+__xlated("9: r6 = *(u32 *)(r1 +0)")
+__xlated("10: w6 += 10000")
+__xlated("11: *(u32 *)(r1 +0) = r6")
+__xlated("12: w0 = w6")
+__xlated("13: w0 *= 2")
+__xlated("14: exit")
+SEC("struct_ops/test_pro_epilogue_subprog")
+__naked int test_pro_epilogue_subprog(void)
+{
+	asm volatile (
+	"r1 = *(u64 *)(r1 +0);"
+	"call subprog;"
+	"exit;"
+	::: __clobber_all);
+}
+
+SEC("syscall")
+__retval(1001) /* PROLOGUE_A [1000] + SUBPROG_A [1] */
+int syscall_prologue_subprog(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_prologue(&args);
+}
+
+SEC("syscall")
+__retval(20002) /* (SUBPROG_A [1] + EPILOGUE_A [10000]) * 2 */
+int syscall_epilogue_subprog(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_epilogue(&args);
+}
+
+SEC("syscall")
+__retval(22002) /* (PROLOGUE_A [1000] + SUBPROG_A [1] + EPILOGUE_A [10000]) * 2 */
+int syscall_pro_epilogue_subprog(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_pro_epilogue(&args);
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops pro_epilogue_subprog = {
+	.test_prologue = (void *)test_prologue_subprog,
+	.test_epilogue = (void *)test_epilogue_subprog,
+	.test_pro_epilogue = (void *)test_pro_epilogue_subprog,
+};
-- 
2.43.5

Re: [PATCH v4 bpf-next 6/9] selftests/bpf: Test gen_prologue and gen_epilogue

[Eduard Zingerman]

On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> This test adds a new struct_ops "bpf_testmod_st_ops" in bpf_testmod.
> The ops of the bpf_testmod_st_ops is triggered by new kfunc calls
> "bpf_kfunc_st_ops_test_*logue". These new kfunc calls are
> primarily used by the SEC("syscall") program. The test triggering
> sequence is like:
>     SEC("syscall")
>     syscall_prologue_subprog(struct st_ops_args *args)
>         bpf_kfunc_st_op_test_prologue(args)
> 	    st_ops->test_prologue(args)
> 
> .gen_prologue adds 1000 to args->a
> .gen_epilogue adds 10000 to args->a
> .gen_epilogue will also set the r0 to 2 * args->a.
> 
> The .gen_prologue and .gen_epilogue of the bpf_testmod_st_ops
> will test the prog->aux->attach_func_name to decide if
> it needs to generate codes.
> 
> The main programs of the pro_epilogue_subprog.c will call a subprog()
> which does "args->a += 1".
> 
> The main programs of the pro_epilogue_kfunc.c will call a
> new kfunc bpf_kfunc_st_ops_inc10 which does "args->a += 10".
> 
> This patch uses the test_loader infra to check the __xlated
> instructions patched after gen_prologue and/or gen_epilogue.
> The __xlated check is based on Eduard's example (Thanks!) in v1.
> 
> args->a is returned by the struct_ops prog (either the main prog
> or the epilogue). Thus, the __retval of the SEC("syscall") prog
> is checked. For example, when triggering the ops in the
> 'SEC("struct_ops/test_epilogue_subprog") int test_epilogue_subprog'
> The expected args->a is +1 (subprog call) + 10000 (.gen_epilogue) = 10001.
> The expected return value is 2 * 10001 (.gen_epilogue).
> 
> Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> diff --git a/tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c b/tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c
> new file mode 100644
> index 000000000000..7d1124cf4942
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_tracing.h>
> +#include "bpf_misc.h"
> +#include "../bpf_testmod/bpf_testmod.h"
> +#include "../bpf_testmod/bpf_testmod_kfunc.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +void __kfunc_btf_root(void)
> +{
> +	struct st_ops_args args = {};
> +
> +	bpf_kfunc_st_ops_inc10(&args);

Nit: 'bpf_kfunc_st_ops_inc10(0);' would also work.

> +}

As a side note, I think that kfunc and subprog sets of tests could be
combined in order to have less code. Probably does not matter.

[...]

Re: [PATCH v4 bpf-next 6/9] selftests/bpf: Test gen_prologue and gen_epilogue

[Martin KaFai Lau]

On 8/29/24 12:27 AM, Eduard Zingerman wrote:
> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>
>> This test adds a new struct_ops "bpf_testmod_st_ops" in bpf_testmod.
>> The ops of the bpf_testmod_st_ops is triggered by new kfunc calls
>> "bpf_kfunc_st_ops_test_*logue". These new kfunc calls are
>> primarily used by the SEC("syscall") program. The test triggering
>> sequence is like:
>>      SEC("syscall")
>>      syscall_prologue_subprog(struct st_ops_args *args)
>>          bpf_kfunc_st_op_test_prologue(args)
>> 	    st_ops->test_prologue(args)
>>
>> .gen_prologue adds 1000 to args->a
>> .gen_epilogue adds 10000 to args->a
>> .gen_epilogue will also set the r0 to 2 * args->a.
>>
>> The .gen_prologue and .gen_epilogue of the bpf_testmod_st_ops
>> will test the prog->aux->attach_func_name to decide if
>> it needs to generate codes.
>>
>> The main programs of the pro_epilogue_subprog.c will call a subprog()
>> which does "args->a += 1".
>>
>> The main programs of the pro_epilogue_kfunc.c will call a
>> new kfunc bpf_kfunc_st_ops_inc10 which does "args->a += 10".
>>
>> This patch uses the test_loader infra to check the __xlated
>> instructions patched after gen_prologue and/or gen_epilogue.
>> The __xlated check is based on Eduard's example (Thanks!) in v1.
>>
>> args->a is returned by the struct_ops prog (either the main prog
>> or the epilogue). Thus, the __retval of the SEC("syscall") prog
>> is checked. For example, when triggering the ops in the
>> 'SEC("struct_ops/test_epilogue_subprog") int test_epilogue_subprog'
>> The expected args->a is +1 (subprog call) + 10000 (.gen_epilogue) = 10001.
>> The expected return value is 2 * 10001 (.gen_epilogue).
>>
>> Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>> ---
> 
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> 
> [...]
> 
>> diff --git a/tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c b/tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c
>> new file mode 100644
>> index 000000000000..7d1124cf4942
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c
>> @@ -0,0 +1,156 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
>> +
>> +#include <vmlinux.h>
>> +#include <bpf/bpf_tracing.h>
>> +#include "bpf_misc.h"
>> +#include "../bpf_testmod/bpf_testmod.h"
>> +#include "../bpf_testmod/bpf_testmod_kfunc.h"
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +void __kfunc_btf_root(void)
>> +{
>> +	struct st_ops_args args = {};
>> +
>> +	bpf_kfunc_st_ops_inc10(&args);
> 
> Nit: 'bpf_kfunc_st_ops_inc10(0);' would also work.

sgtm. I think it will make it obvious that it won't be executed also.

> 
>> +}
> 
> As a side note, I think that kfunc and subprog sets of tests could be
> combined in order to have less code. Probably does not matter.

ok. I will drop the _subprog.c and only keep the _kfunc.c.

The _kfunc.c calls a subprog and a kfunc which should have already covered the 
_subprog.c cases.

[PATCH v4 bpf-next 7/9] selftests/bpf: Add tailcall epilogue test

[Martin KaFai Lau]

From: Martin KaFai Lau <martin.lau@kernel.org>

This patch adds a gen_epilogue test to test a main prog
using a bpf_tail_call.

A non test_loader test is used. The tailcall target program,
"test_epilogue_subprog", needs to be used in a struct_ops map
before it can be loaded. Another struct_ops map is also needed
to host the actual "test_epilogue_tailcall" struct_ops program
that does the bpf_tail_call. The earlier test_loader patch
will attach all struct_ops maps but the bpf_testmod.c does
not support >1 attached struct_ops.

The earlier patch used the test_loader which has already covered
checking for the patched pro/epilogue instructions. This is done
by the __xlated tag.

This patch goes for the regular skel load and syscall test to do
the tailcall test that can also allow to directly pass the
the "struct st_ops_args *args" as ctx_in to the
SEC("syscall") program.

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 .../selftests/bpf/prog_tests/pro_epilogue.c   | 38 ++++++++++++
 .../selftests/bpf/progs/epilogue_tailcall.c   | 58 +++++++++++++++++++
 2 files changed, 96 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/epilogue_tailcall.c

diff --git a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
index 69e4a5a1756d..98de677c55a9 100644
--- a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
+++ b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
@@ -4,9 +4,47 @@
 #include <test_progs.h>
 #include "pro_epilogue_subprog.skel.h"
 #include "pro_epilogue_kfunc.skel.h"
+#include "epilogue_tailcall.skel.h"
+
+struct st_ops_args {
+	int a;
+};
+
+static void test_tailcall(void)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	struct epilogue_tailcall *skel;
+	struct st_ops_args args;
+	int err, prog_fd;
+
+	skel = epilogue_tailcall__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "epilogue_tailcall__open_and_load"))
+		return;
+
+	topts.ctx_in = &args;
+	topts.ctx_size_in = sizeof(args);
+
+	skel->links.epilogue_tailcall =
+		bpf_map__attach_struct_ops(skel->maps.epilogue_tailcall);
+	if (!ASSERT_OK_PTR(skel->links.epilogue_tailcall, "attach_struct_ops"))
+		goto done;
+
+	/* tailcall prog + gen_epilogue */
+	memset(&args, 0, sizeof(args));
+	prog_fd = bpf_program__fd(skel->progs.syscall_epilogue_tailcall);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	ASSERT_OK(err, "bpf_prog_test_run_opts");
+	ASSERT_EQ(args.a, 10001, "args.a");
+	ASSERT_EQ(topts.retval, 10001 * 2, "topts.retval");
+
+done:
+	epilogue_tailcall__destroy(skel);
+}
 
 void test_pro_epilogue(void)
 {
 	RUN_TESTS(pro_epilogue_subprog);
 	RUN_TESTS(pro_epilogue_kfunc);
+	if (test__start_subtest("tailcall"))
+		test_tailcall();
 }
diff --git a/tools/testing/selftests/bpf/progs/epilogue_tailcall.c b/tools/testing/selftests/bpf/progs/epilogue_tailcall.c
new file mode 100644
index 000000000000..7275dd594de0
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/epilogue_tailcall.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+#include "../bpf_testmod/bpf_testmod.h"
+#include "../bpf_testmod/bpf_testmod_kfunc.h"
+
+char _license[] SEC("license") = "GPL";
+
+static __noinline __used int subprog(struct st_ops_args *args)
+{
+	args->a += 1;
+	return args->a;
+}
+
+SEC("struct_ops/test_epilogue_subprog")
+int BPF_PROG(test_epilogue_subprog, struct st_ops_args *args)
+{
+	subprog(args);
+	return args->a;
+}
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+	__uint(max_entries, 1);
+	__uint(key_size, sizeof(__u32));
+	__uint(value_size, sizeof(__u32));
+	__array(values, void (void));
+} epilogue_map SEC(".maps") = {
+	.values = {
+		[0] = (void *)&test_epilogue_subprog,
+	}
+};
+
+SEC("struct_ops/test_epilogue_tailcall")
+int test_epilogue_tailcall(unsigned long long *ctx)
+{
+	bpf_tail_call(ctx, &epilogue_map, 0);
+	return 0;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops epilogue_tailcall = {
+	.test_epilogue = (void *)test_epilogue_tailcall,
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops epilogue_subprog = {
+	.test_epilogue = (void *)test_epilogue_subprog,
+};
+
+SEC("syscall")
+int syscall_epilogue_tailcall(struct st_ops_args *args)
+{
+	return bpf_kfunc_st_ops_test_epilogue(args);
+}
-- 
2.43.5

Re: [PATCH v4 bpf-next 7/9] selftests/bpf: Add tailcall epilogue test

[Eduard Zingerman]

On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> This patch adds a gen_epilogue test to test a main prog
> using a bpf_tail_call.
> 
> A non test_loader test is used. The tailcall target program,
> "test_epilogue_subprog", needs to be used in a struct_ops map
> before it can be loaded. Another struct_ops map is also needed
> to host the actual "test_epilogue_tailcall" struct_ops program
> that does the bpf_tail_call. The earlier test_loader patch
> will attach all struct_ops maps but the bpf_testmod.c does
> not support >1 attached struct_ops.
> 
> The earlier patch used the test_loader which has already covered
> checking for the patched pro/epilogue instructions. This is done
> by the __xlated tag.
> 
> This patch goes for the regular skel load and syscall test to do
> the tailcall test that can also allow to directly pass the
> the "struct st_ops_args *args" as ctx_in to the
> SEC("syscall") program.
> 
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> +static void test_tailcall(void)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, topts);
> +	struct epilogue_tailcall *skel;
> +	struct st_ops_args args;
> +	int err, prog_fd;
> +
> +	skel = epilogue_tailcall__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "epilogue_tailcall__open_and_load"))
> +		return;
> +
> +	topts.ctx_in = &args;
> +	topts.ctx_size_in = sizeof(args);
> +
> +	skel->links.epilogue_tailcall =
> +		bpf_map__attach_struct_ops(skel->maps.epilogue_tailcall);
> +	if (!ASSERT_OK_PTR(skel->links.epilogue_tailcall, "attach_struct_ops"))
> +		goto done;
> +

Nitpick:
Both test_epilogue_tailcall and test_epilogue_subprog would be
augmented with epilogue, and we know that tail call run as expected
because only test_epilogue_subprog does +1, right?

If above is true, could you please update the comment a bit, e.g.:

/* Both test_epilogue_tailcall and test_epilogue_subprog are
 * augmented with epilogue. When syscall_epilogue_tailcall()
 * is run test_epilogue_tailcall() is triggered,
 * it executes a tail call and control is transferred to
 * test_epilogue_subprog(). Only test_epilogue_subprog()
 * does args->a += 1, thus final args.a value of 10001
 * guarantees that tail call was executed as expected.
 */

(For some reason it took me a while to understand what happens in this test)

> +	/* tailcall prog + gen_epilogue */
> +	memset(&args, 0, sizeof(args));
> +	prog_fd = bpf_program__fd(skel->progs.syscall_epilogue_tailcall);
> +	err = bpf_prog_test_run_opts(prog_fd, &topts);
> +	ASSERT_OK(err, "bpf_prog_test_run_opts");
> +	ASSERT_EQ(args.a, 10001, "args.a");
> +	ASSERT_EQ(topts.retval, 10001 * 2, "topts.retval");
> +
> +done:
> +	epilogue_tailcall__destroy(skel);
> +}

[...]

Re: [PATCH v4 bpf-next 7/9] selftests/bpf: Add tailcall epilogue test

[Martin KaFai Lau]

On 8/28/24 11:16 PM, Eduard Zingerman wrote:
> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>
>> This patch adds a gen_epilogue test to test a main prog
>> using a bpf_tail_call.
>>
>> A non test_loader test is used. The tailcall target program,
>> "test_epilogue_subprog", needs to be used in a struct_ops map
>> before it can be loaded. Another struct_ops map is also needed
>> to host the actual "test_epilogue_tailcall" struct_ops program
>> that does the bpf_tail_call. The earlier test_loader patch
>> will attach all struct_ops maps but the bpf_testmod.c does
>> not support >1 attached struct_ops.
>>
>> The earlier patch used the test_loader which has already covered
>> checking for the patched pro/epilogue instructions. This is done
>> by the __xlated tag.
>>
>> This patch goes for the regular skel load and syscall test to do
>> the tailcall test that can also allow to directly pass the
>> the "struct st_ops_args *args" as ctx_in to the
>> SEC("syscall") program.
>>
>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>> ---
> 
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> 
> [...]
> 
>> +static void test_tailcall(void)
>> +{
>> +	LIBBPF_OPTS(bpf_test_run_opts, topts);
>> +	struct epilogue_tailcall *skel;
>> +	struct st_ops_args args;
>> +	int err, prog_fd;
>> +
>> +	skel = epilogue_tailcall__open_and_load();
>> +	if (!ASSERT_OK_PTR(skel, "epilogue_tailcall__open_and_load"))
>> +		return;
>> +
>> +	topts.ctx_in = &args;
>> +	topts.ctx_size_in = sizeof(args);
>> +
>> +	skel->links.epilogue_tailcall =
>> +		bpf_map__attach_struct_ops(skel->maps.epilogue_tailcall);
>> +	if (!ASSERT_OK_PTR(skel->links.epilogue_tailcall, "attach_struct_ops"))
>> +		goto done;
>> +
> 
> Nitpick:
> Both test_epilogue_tailcall and test_epilogue_subprog would be
> augmented with epilogue, and we know that tail call run as expected
> because only test_epilogue_subprog does +1, right?

Yes. and also the epilogue of the test_epilogue_subprog is executed.

> 
> If above is true, could you please update the comment a bit, e.g.:
> 
> /* Both test_epilogue_tailcall and test_epilogue_subprog are
>   * augmented with epilogue. When syscall_epilogue_tailcall()
>   * is run test_epilogue_tailcall() is triggered,
>   * it executes a tail call and control is transferred to
>   * test_epilogue_subprog(). Only test_epilogue_subprog()
>   * does args->a += 1, thus final args.a value of 10001
>   * guarantees that tail call was executed as expected.
>   */

Added. I massaged the wordings a little.

[PATCH v4 bpf-next 8/9] selftests/bpf: A pro/epilogue test when the main prog jumps back to the 1st insn

[Martin KaFai Lau]

From: Martin KaFai Lau <martin.lau@kernel.org>

This patch adds a pro/epilogue test when the main prog has a goto insn
that goes back to the very first instruction of the prog. It is
to test the correctness of the adjust_jmp_off(prog, 0, delta)
after the verifier has applied the prologue and/or epilogue patch.

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 .../selftests/bpf/prog_tests/pro_epilogue.c   |   2 +
 .../bpf/progs/pro_epilogue_goto_start.c       | 149 ++++++++++++++++++
 2 files changed, 151 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/pro_epilogue_goto_start.c

diff --git a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
index 98de677c55a9..b2e467cf15fe 100644
--- a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
+++ b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
@@ -5,6 +5,7 @@
 #include "pro_epilogue_subprog.skel.h"
 #include "pro_epilogue_kfunc.skel.h"
 #include "epilogue_tailcall.skel.h"
+#include "pro_epilogue_goto_start.skel.h"
 
 struct st_ops_args {
 	int a;
@@ -45,6 +46,7 @@ void test_pro_epilogue(void)
 {
 	RUN_TESTS(pro_epilogue_subprog);
 	RUN_TESTS(pro_epilogue_kfunc);
+	RUN_TESTS(pro_epilogue_goto_start);
 	if (test__start_subtest("tailcall"))
 		test_tailcall();
 }
diff --git a/tools/testing/selftests/bpf/progs/pro_epilogue_goto_start.c b/tools/testing/selftests/bpf/progs/pro_epilogue_goto_start.c
new file mode 100644
index 000000000000..0ddc646cc24b
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/pro_epilogue_goto_start.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+#include "../bpf_testmod/bpf_testmod.h"
+#include "../bpf_testmod/bpf_testmod_kfunc.h"
+
+char _license[] SEC("license") = "GPL";
+
+__success
+/* prologue */
+__xlated("0: r6 = *(u64 *)(r1 +0)")
+__xlated("1: r7 = *(u32 *)(r6 +0)")
+__xlated("2: w7 += 1000")
+__xlated("3: *(u32 *)(r6 +0) = r7")
+/* main prog */
+__xlated("4: if r1 == 0x0 goto pc+5")
+__xlated("5: if r1 == 0x1 goto pc+2")
+__xlated("6: r1 = 1")
+__xlated("7: goto pc-3")
+__xlated("8: r1 = 0")
+__xlated("9: goto pc-6")
+__xlated("10: r0 = 0")
+__xlated("11: exit")
+SEC("struct_ops/test_prologue_goto_start")
+__naked int test_prologue_goto_start(void)
+{
+	asm volatile (
+	"if r1 == 0 goto +5;"
+	"if r1 == 1 goto +2;"
+	"r1 = 1;"
+	"goto -3;"
+	"r1 = 0;"
+	"goto -6;"
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+__success
+/* save __u64 *ctx to stack */
+__xlated("0: *(u64 *)(r10 -8) = r1")
+/* main prog */
+__xlated("1: if r1 == 0x0 goto pc+5")
+__xlated("2: if r1 == 0x1 goto pc+2")
+__xlated("3: r1 = 1")
+__xlated("4: goto pc-3")
+__xlated("5: r1 = 0")
+__xlated("6: goto pc-6")
+__xlated("7: r0 = 0")
+/* epilogue */
+__xlated("8: r1 = *(u64 *)(r10 -8)")
+__xlated("9: r1 = *(u64 *)(r1 +0)")
+__xlated("10: r6 = *(u32 *)(r1 +0)")
+__xlated("11: w6 += 10000")
+__xlated("12: *(u32 *)(r1 +0) = r6")
+__xlated("13: w0 = w6")
+__xlated("14: w0 *= 2")
+__xlated("15: exit")
+SEC("struct_ops/test_epilogue_goto_start")
+__naked int test_epilogue_goto_start(void)
+{
+	asm volatile (
+	"if r1 == 0 goto +5;"
+	"if r1 == 1 goto +2;"
+	"r1 = 1;"
+	"goto -3;"
+	"r1 = 0;"
+	"goto -6;"
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+__success
+/* prologue */
+__xlated("0: r6 = *(u64 *)(r1 +0)")
+__xlated("1: r7 = *(u32 *)(r6 +0)")
+__xlated("2: w7 += 1000")
+__xlated("3: *(u32 *)(r6 +0) = r7")
+/* save __u64 *ctx to stack */
+__xlated("4: *(u64 *)(r10 -8) = r1")
+/* main prog */
+__xlated("5: if r1 == 0x0 goto pc+5")
+__xlated("6: if r1 == 0x1 goto pc+2")
+__xlated("7: r1 = 1")
+__xlated("8: goto pc-3")
+__xlated("9: r1 = 0")
+__xlated("10: goto pc-6")
+__xlated("11: r0 = 0")
+/* epilogue */
+__xlated("12: r1 = *(u64 *)(r10 -8)")
+__xlated("13: r1 = *(u64 *)(r1 +0)")
+__xlated("14: r6 = *(u32 *)(r1 +0)")
+__xlated("15: w6 += 10000")
+__xlated("16: *(u32 *)(r1 +0) = r6")
+__xlated("17: w0 = w6")
+__xlated("18: w0 *= 2")
+__xlated("19: exit")
+SEC("struct_ops/test_pro_epilogue_goto_start")
+__naked int test_pro_epilogue_goto_start(void)
+{
+	asm volatile (
+	"if r1 == 0 goto +5;"
+	"if r1 == 1 goto +2;"
+	"r1 = 1;"
+	"goto -3;"
+	"r1 = 0;"
+	"goto -6;"
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops epilogue_goto_start = {
+	.test_prologue = (void *)test_prologue_goto_start,
+	.test_epilogue = (void *)test_epilogue_goto_start,
+	.test_pro_epilogue = (void *)test_pro_epilogue_goto_start,
+};
+
+SEC("syscall")
+__retval(0)
+int syscall_prologue_goto_start(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_prologue(&args);
+}
+
+SEC("syscall")
+__retval(20000) /* (EPILOGUE_A [10000]) * 2 */
+int syscall_epilogue_goto_start(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_epilogue(&args);
+}
+
+SEC("syscall")
+__retval(22000) /* (PROLOGUE_A [1000] + EPILOGUE_A [10000]) * 2 */
+int syscall_pro_epilogue_goto_start(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_pro_epilogue(&args);
+}
-- 
2.43.5

Re: [PATCH v4 bpf-next 8/9] selftests/bpf: A pro/epilogue test when the main prog jumps back to the 1st insn

[Eduard Zingerman]

On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> This patch adds a pro/epilogue test when the main prog has a goto insn
> that goes back to the very first instruction of the prog. It is
> to test the correctness of the adjust_jmp_off(prog, 0, delta)
> after the verifier has applied the prologue and/or epilogue patch.
> 
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

[PATCH v4 bpf-next 9/9] selftests/bpf: Test epilogue patching when the main prog has multiple BPF_EXIT

[Martin KaFai Lau]

From: Martin KaFai Lau <martin.lau@kernel.org>

This patch tests the epilogue patching when the main prog has
multiple BPF_EXIT. The verifier should have patched the 2nd (and
later) BPF_EXIT with a BPF_JA that goes back to the earlier
patched epilogue instructions.

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 .../selftests/bpf/prog_tests/pro_epilogue.c   |  2 +
 .../selftests/bpf/progs/epilogue_exit.c       | 78 +++++++++++++++++++
 2 files changed, 80 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/epilogue_exit.c

diff --git a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
index b2e467cf15fe..58c18529a802 100644
--- a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
+++ b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
@@ -6,6 +6,7 @@
 #include "pro_epilogue_kfunc.skel.h"
 #include "epilogue_tailcall.skel.h"
 #include "pro_epilogue_goto_start.skel.h"
+#include "epilogue_exit.skel.h"
 
 struct st_ops_args {
 	int a;
@@ -47,6 +48,7 @@ void test_pro_epilogue(void)
 	RUN_TESTS(pro_epilogue_subprog);
 	RUN_TESTS(pro_epilogue_kfunc);
 	RUN_TESTS(pro_epilogue_goto_start);
+	RUN_TESTS(epilogue_exit);
 	if (test__start_subtest("tailcall"))
 		test_tailcall();
 }
diff --git a/tools/testing/selftests/bpf/progs/epilogue_exit.c b/tools/testing/selftests/bpf/progs/epilogue_exit.c
new file mode 100644
index 000000000000..8c03256c7491
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/epilogue_exit.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+#include "../bpf_testmod/bpf_testmod.h"
+#include "../bpf_testmod/bpf_testmod_kfunc.h"
+
+char _license[] SEC("license") = "GPL";
+
+__success
+/* save __u64 *ctx to stack */
+__xlated("0: *(u64 *)(r10 -8) = r1")
+/* main prog */
+__xlated("1: r1 = *(u64 *)(r1 +0)")
+__xlated("2: r2 = *(u32 *)(r1 +0)")
+__xlated("3: if r2 == 0x0 goto pc+10")
+__xlated("4: r0 = 0")
+__xlated("5: *(u32 *)(r1 +0) = 0")
+/* epilogue */
+__xlated("6: r1 = *(u64 *)(r10 -8)")
+__xlated("7: r1 = *(u64 *)(r1 +0)")
+__xlated("8: r6 = *(u32 *)(r1 +0)")
+__xlated("9: w6 += 10000")
+__xlated("10: *(u32 *)(r1 +0) = r6")
+__xlated("11: w0 = w6")
+__xlated("12: w0 *= 2")
+__xlated("13: exit")
+/* 2nd part of the main prog after the first exit */
+__xlated("14: *(u32 *)(r1 +0) = 1")
+__xlated("15: r0 = 1")
+/* Clear the r1 to ensure it does not have
+ * off-by-1 error and ensure it jumps back to the
+ * beginning of epilogue which initializes
+ * the r1 with the ctx ptr.
+ */
+__xlated("16: r1 = 0")
+__xlated("17: gotol pc-12")
+SEC("struct_ops/test_epilogue_exit")
+__naked int test_epilogue_exit(void)
+{
+	asm volatile (
+	"r1 = *(u64 *)(r1 +0);"
+	"r2 = *(u32 *)(r1 +0);"
+	"if r2 == 0 goto +3;"
+	"r0 = 0;"
+	"*(u32 *)(r1 + 0) = 0;"
+	"exit;"
+	"*(u32 *)(r1 + 0) = 1;"
+	"r0 = 1;"
+	"r1 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops epilogue_exit = {
+	.test_epilogue = (void *)test_epilogue_exit,
+};
+
+SEC("syscall")
+__retval(20000)
+int syscall_epilogue_exit0(void *ctx)
+{
+	struct st_ops_args args = { .a = 1 };
+
+	return bpf_kfunc_st_ops_test_epilogue(&args);
+}
+
+SEC("syscall")
+__retval(20002)
+int syscall_epilogue_exit1(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_epilogue(&args);
+}
-- 
2.43.5

Re: [PATCH v4 bpf-next 9/9] selftests/bpf: Test epilogue patching when the main prog has multiple BPF_EXIT

[Martin KaFai Lau]

On 8/27/24 12:48 PM, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> This patch tests the epilogue patching when the main prog has
> multiple BPF_EXIT. The verifier should have patched the 2nd (and
> later) BPF_EXIT with a BPF_JA that goes back to the earlier
> patched epilogue instructions.
> 
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---
>   .../selftests/bpf/prog_tests/pro_epilogue.c   |  2 +
>   .../selftests/bpf/progs/epilogue_exit.c       | 78 +++++++++++++++++++
>   2 files changed, 80 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/progs/epilogue_exit.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
> index b2e467cf15fe..58c18529a802 100644
> --- a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
> +++ b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
> @@ -6,6 +6,7 @@
>   #include "pro_epilogue_kfunc.skel.h"
>   #include "epilogue_tailcall.skel.h"
>   #include "pro_epilogue_goto_start.skel.h"
> +#include "epilogue_exit.skel.h"
>   
>   struct st_ops_args {
>   	int a;
> @@ -47,6 +48,7 @@ void test_pro_epilogue(void)
>   	RUN_TESTS(pro_epilogue_subprog);
>   	RUN_TESTS(pro_epilogue_kfunc);
>   	RUN_TESTS(pro_epilogue_goto_start);
> +	RUN_TESTS(epilogue_exit);
>   	if (test__start_subtest("tailcall"))
>   		test_tailcall();
>   }
> diff --git a/tools/testing/selftests/bpf/progs/epilogue_exit.c b/tools/testing/selftests/bpf/progs/epilogue_exit.c
> new file mode 100644
> index 000000000000..8c03256c7491
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/epilogue_exit.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_tracing.h>
> +#include "bpf_misc.h"
> +#include "../bpf_testmod/bpf_testmod.h"
> +#include "../bpf_testmod/bpf_testmod_kfunc.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +__success
> +/* save __u64 *ctx to stack */
> +__xlated("0: *(u64 *)(r10 -8) = r1")
> +/* main prog */
> +__xlated("1: r1 = *(u64 *)(r1 +0)")
> +__xlated("2: r2 = *(u32 *)(r1 +0)")
> +__xlated("3: if r2 == 0x0 goto pc+10")
> +__xlated("4: r0 = 0")
> +__xlated("5: *(u32 *)(r1 +0) = 0")
> +/* epilogue */
> +__xlated("6: r1 = *(u64 *)(r10 -8)")
> +__xlated("7: r1 = *(u64 *)(r1 +0)")
> +__xlated("8: r6 = *(u32 *)(r1 +0)")
> +__xlated("9: w6 += 10000")
> +__xlated("10: *(u32 *)(r1 +0) = r6")
> +__xlated("11: w0 = w6")
> +__xlated("12: w0 *= 2")
> +__xlated("13: exit")
> +/* 2nd part of the main prog after the first exit */
> +__xlated("14: *(u32 *)(r1 +0) = 1")
> +__xlated("15: r0 = 1")
> +/* Clear the r1 to ensure it does not have
> + * off-by-1 error and ensure it jumps back to the
> + * beginning of epilogue which initializes
> + * the r1 with the ctx ptr.
> + */
> +__xlated("16: r1 = 0")
> +__xlated("17: gotol pc-12")
> +SEC("struct_ops/test_epilogue_exit")
> +__naked int test_epilogue_exit(void)
> +{
> +	asm volatile (
> +	"r1 = *(u64 *)(r1 +0);"
> +	"r2 = *(u32 *)(r1 +0);"
> +	"if r2 == 0 goto +3;"
> +	"r0 = 0;"
> +	"*(u32 *)(r1 + 0) = 0;"

llvm17 cannot take "*(u32 *)(r1 +0) = 0".

Instead:

r3 = 0;
*(u32 *)(r1 + 0) = r3;

The above solved the llvm17 error:
https://github.com/kernel-patches/bpf/actions/runs/10586206183/job/29334690461

However, there is still a zext with s390 that added extra insn and failed the 
__xlated check. will try an adjustment in the tests to avoid the zext.

pw-bot: cr


> +	"exit;"
> +	"*(u32 *)(r1 + 0) = 1;"
> +	"r0 = 1;"
> +	"r1 = 0;"
> +	"exit;"
> +	::: __clobber_all);
> +}
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_st_ops epilogue_exit = {
> +	.test_epilogue = (void *)test_epilogue_exit,
> +};
> +
> +SEC("syscall")
> +__retval(20000)
> +int syscall_epilogue_exit0(void *ctx)
> +{
> +	struct st_ops_args args = { .a = 1 };
> +
> +	return bpf_kfunc_st_ops_test_epilogue(&args);
> +}
> +
> +SEC("syscall")
> +__retval(20002)
> +int syscall_epilogue_exit1(void *ctx)
> +{
> +	struct st_ops_args args = {};
> +
> +	return bpf_kfunc_st_ops_test_epilogue(&args);
> +}

Re: [PATCH v4 bpf-next 9/9] selftests/bpf: Test epilogue patching when the main prog has multiple BPF_EXIT

[Eduard Zingerman]

On Tue, 2024-08-27 at 17:58 -0700, Martin KaFai Lau wrote:

[...]

> > +SEC("struct_ops/test_epilogue_exit")
> > +__naked int test_epilogue_exit(void)
> > +{
> > +	asm volatile (
> > +	"r1 = *(u64 *)(r1 +0);"
> > +	"r2 = *(u32 *)(r1 +0);"
> > +	"if r2 == 0 goto +3;"
> > +	"r0 = 0;"
> > +	"*(u32 *)(r1 + 0) = 0;"
> 
> llvm17 cannot take "*(u32 *)(r1 +0) = 0".
> 
> Instead:
> 
> r3 = 0;
> *(u32 *)(r1 + 0) = r3;
> 
> The above solved the llvm17 error:
> https://github.com/kernel-patches/bpf/actions/runs/10586206183/job/29334690461
> 
> However, there is still a zext with s390 that added extra insn and failed the 
> __xlated check. will try an adjustment in the tests to avoid the zext.

Another option would be to limit archs for the test, e.g. use
__arch_x86_64 and __arch_arm64.

Re: [PATCH v4 bpf-next 9/9] selftests/bpf: Test epilogue patching when the main prog has multiple BPF_EXIT

[Martin KaFai Lau]

On 8/28/24 11:28 PM, Eduard Zingerman wrote:
> On Tue, 2024-08-27 at 17:58 -0700, Martin KaFai Lau wrote:
> 
> [...]
> 
>>> +SEC("struct_ops/test_epilogue_exit")
>>> +__naked int test_epilogue_exit(void)
>>> +{
>>> +	asm volatile (
>>> +	"r1 = *(u64 *)(r1 +0);"
>>> +	"r2 = *(u32 *)(r1 +0);"
>>> +	"if r2 == 0 goto +3;"
>>> +	"r0 = 0;"
>>> +	"*(u32 *)(r1 + 0) = 0;"
>>
>> llvm17 cannot take "*(u32 *)(r1 +0) = 0".
>>
>> Instead:
>>
>> r3 = 0;
>> *(u32 *)(r1 + 0) = r3;
>>
>> The above solved the llvm17 error:
>> https://github.com/kernel-patches/bpf/actions/runs/10586206183/job/29334690461
>>
>> However, there is still a zext with s390 that added extra insn and failed the
>> __xlated check. will try an adjustment in the tests to avoid the zext.
> 
> Another option would be to limit archs for the test, e.g. use
> __arch_x86_64 and __arch_arm64.

Ah. good to know.

I have used all 64bits ops to solve it. It passed the earlier bpf CI run:
https://github.com/kernel-patches/bpf/actions/runs/10590714532
I will respin with the 64bits ops solution.

Re: [PATCH v4 bpf-next 9/9] selftests/bpf: Test epilogue patching when the main prog has multiple BPF_EXIT

[Eduard Zingerman]

On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> This patch tests the epilogue patching when the main prog has
> multiple BPF_EXIT. The verifier should have patched the 2nd (and
> later) BPF_EXIT with a BPF_JA that goes back to the earlier
> patched epilogue instructions.
> 
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

[PATCH v4 bpf-next 2/9] bpf: Adjust BPF_JMP that jumps to the 1st insn of the prologue

[Martin KaFai Lau]

From: Martin KaFai Lau <martin.lau@kernel.org>

The next patch will add a ctx ptr saving instruction
"(r1 = *(u64 *)(r10 -8)" at the beginning for the main prog
when there is an epilogue patch (by the .gen_epilogue() verifier
ops added in the next patch).

There is one corner case if the bpf prog has a BPF_JMP that jumps
to the 1st instruction. It needs an adjustment such that
those BPF_JMP instructions won't jump to the newly added
ctx saving instruction.
The commit 5337ac4c9b80 ("bpf: Fix the corner case with may_goto and jump to the 1st insn.")
has the details on this case.

Note that the jump back to 1st instruction is not limited to the
ctx ptr saving instruction. The same also applies to the prologue.
A later test, pro_epilogue_goto_start.c, has a test for the prologue
only case.

Thus, this patch does one adjustment after gen_prologue and
the future ctx ptr saving. It is done by
adjust_jmp_off(env->prog, 0, delta) where delta has the total
number of instructions in the prologue and
the future ctx ptr saving instruction.

The adjust_jmp_off(env->prog, 0, delta) assumes that the
prologue does not have a goto 1st instruction itself.
To accommodate the prologue might have a goto 1st insn itself,
adjust_jmp_off() needs to skip the prologue instructions. This patch
adds a skip_cnt argument to the adjust_jmp_off(). The skip_cnt is the
number of instructions at the beginning that does not need adjustment.
adjust_jmp_off(prog, 0, delta, delta) is used in this patch.

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 kernel/bpf/verifier.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index b408692a12d7..8714b83c5fb8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19277,14 +19277,14 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
  * For all jmp insns in a given 'prog' that point to 'tgt_idx' insn adjust the
  * jump offset by 'delta'.
  */
-static int adjust_jmp_off(struct bpf_prog *prog, u32 tgt_idx, u32 delta)
+static int adjust_jmp_off(struct bpf_prog *prog, u32 tgt_idx, u32 delta, u32 skip_cnt)
 {
-	struct bpf_insn *insn = prog->insnsi;
+	struct bpf_insn *insn = prog->insnsi + skip_cnt;
 	u32 insn_cnt = prog->len, i;
 	s32 imm;
 	s16 off;
 
-	for (i = 0; i < insn_cnt; i++, insn++) {
+	for (i = skip_cnt; i < insn_cnt; i++, insn++) {
 		u8 code = insn->code;
 
 		if ((BPF_CLASS(code) != BPF_JMP && BPF_CLASS(code) != BPF_JMP32) ||
@@ -19705,6 +19705,9 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 		}
 	}
 
+	if (delta)
+		WARN_ON(adjust_jmp_off(env->prog, 0, delta, delta));
+
 	if (bpf_prog_is_offloaded(env->prog->aux))
 		return 0;
 
@@ -21136,7 +21139,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 		 * to insn after BPF_ST that inits may_goto count.
 		 * Adjustment will succeed because bpf_patch_insn_data() didn't fail.
 		 */
-		WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1));
+		WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1, 0));
 	}
 
 	/* Since poke tab is now finalized, publish aux to tracker. */
-- 
2.43.5

Re: [PATCH v4 bpf-next 2/9] bpf: Adjust BPF_JMP that jumps to the 1st insn of the prologue

[Alexei Starovoitov]

On Tue, Aug 27, 2024 at 12:53 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> From: Martin KaFai Lau <martin.lau@kernel.org>
>
> The next patch will add a ctx ptr saving instruction
> "(r1 = *(u64 *)(r10 -8)" at the beginning for the main prog
> when there is an epilogue patch (by the .gen_epilogue() verifier
> ops added in the next patch).
>
> There is one corner case if the bpf prog has a BPF_JMP that jumps
> to the 1st instruction. It needs an adjustment such that
> those BPF_JMP instructions won't jump to the newly added
> ctx saving instruction.
> The commit 5337ac4c9b80 ("bpf: Fix the corner case with may_goto and jump to the 1st insn.")
> has the details on this case.
>
> Note that the jump back to 1st instruction is not limited to the
> ctx ptr saving instruction. The same also applies to the prologue.
> A later test, pro_epilogue_goto_start.c, has a test for the prologue
> only case.
>
> Thus, this patch does one adjustment after gen_prologue and
> the future ctx ptr saving. It is done by
> adjust_jmp_off(env->prog, 0, delta) where delta has the total
> number of instructions in the prologue and
> the future ctx ptr saving instruction.
>
> The adjust_jmp_off(env->prog, 0, delta) assumes that the
> prologue does not have a goto 1st instruction itself.
> To accommodate the prologue might have a goto 1st insn itself,
> adjust_jmp_off() needs to skip the prologue instructions. This patch
> adds a skip_cnt argument to the adjust_jmp_off(). The skip_cnt is the
> number of instructions at the beginning that does not need adjustment.
> adjust_jmp_off(prog, 0, delta, delta) is used in this patch.
>
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---
>  kernel/bpf/verifier.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index b408692a12d7..8714b83c5fb8 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19277,14 +19277,14 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
>   * For all jmp insns in a given 'prog' that point to 'tgt_idx' insn adjust the
>   * jump offset by 'delta'.
>   */
> -static int adjust_jmp_off(struct bpf_prog *prog, u32 tgt_idx, u32 delta)
> +static int adjust_jmp_off(struct bpf_prog *prog, u32 tgt_idx, u32 delta, u32 skip_cnt)
>  {
> -       struct bpf_insn *insn = prog->insnsi;
> +       struct bpf_insn *insn = prog->insnsi + skip_cnt;
>         u32 insn_cnt = prog->len, i;
>         s32 imm;
>         s16 off;
>
> -       for (i = 0; i < insn_cnt; i++, insn++) {
> +       for (i = skip_cnt; i < insn_cnt; i++, insn++) {

Do we really need to add this argument?

> -               WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1));
> +               WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1, 0));

We can always do for (i = delta; ...

The above case of skip_cnt == 0 is lucky to work this way.
It would be less surprising to skip all insns in the patch.
Maybe I'm missing something.

Re: [PATCH v4 bpf-next 2/9] bpf: Adjust BPF_JMP that jumps to the 1st insn of the prologue

[Martin KaFai Lau]

On 8/28/24 9:48 AM, Alexei Starovoitov wrote:
> On Tue, Aug 27, 2024 at 12:53 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>
>> The next patch will add a ctx ptr saving instruction
>> "(r1 = *(u64 *)(r10 -8)" at the beginning for the main prog
>> when there is an epilogue patch (by the .gen_epilogue() verifier
>> ops added in the next patch).
>>
>> There is one corner case if the bpf prog has a BPF_JMP that jumps
>> to the 1st instruction. It needs an adjustment such that
>> those BPF_JMP instructions won't jump to the newly added
>> ctx saving instruction.
>> The commit 5337ac4c9b80 ("bpf: Fix the corner case with may_goto and jump to the 1st insn.")
>> has the details on this case.
>>
>> Note that the jump back to 1st instruction is not limited to the
>> ctx ptr saving instruction. The same also applies to the prologue.
>> A later test, pro_epilogue_goto_start.c, has a test for the prologue
>> only case.
>>
>> Thus, this patch does one adjustment after gen_prologue and
>> the future ctx ptr saving. It is done by
>> adjust_jmp_off(env->prog, 0, delta) where delta has the total
>> number of instructions in the prologue and
>> the future ctx ptr saving instruction.
>>
>> The adjust_jmp_off(env->prog, 0, delta) assumes that the
>> prologue does not have a goto 1st instruction itself.
>> To accommodate the prologue might have a goto 1st insn itself,
>> adjust_jmp_off() needs to skip the prologue instructions. This patch
>> adds a skip_cnt argument to the adjust_jmp_off(). The skip_cnt is the
>> number of instructions at the beginning that does not need adjustment.
>> adjust_jmp_off(prog, 0, delta, delta) is used in this patch.
>>
>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>> ---
>>   kernel/bpf/verifier.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index b408692a12d7..8714b83c5fb8 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -19277,14 +19277,14 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
>>    * For all jmp insns in a given 'prog' that point to 'tgt_idx' insn adjust the
>>    * jump offset by 'delta'.
>>    */
>> -static int adjust_jmp_off(struct bpf_prog *prog, u32 tgt_idx, u32 delta)
>> +static int adjust_jmp_off(struct bpf_prog *prog, u32 tgt_idx, u32 delta, u32 skip_cnt)
>>   {
>> -       struct bpf_insn *insn = prog->insnsi;
>> +       struct bpf_insn *insn = prog->insnsi + skip_cnt;
>>          u32 insn_cnt = prog->len, i;
>>          s32 imm;
>>          s16 off;
>>
>> -       for (i = 0; i < insn_cnt; i++, insn++) {
>> +       for (i = skip_cnt; i < insn_cnt; i++, insn++) {
> 
> Do we really need to add this argument?
> 
>> -               WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1));
>> +               WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1, 0));
> 
> We can always do for (i = delta; ...
> 
> The above case of skip_cnt == 0 is lucky to work this way.
> It would be less surprising to skip all insns in the patch.
> Maybe I'm missing something.

For subprog_start case, tgt_idx (where the patch started) may not be 0. How 
about this:

	for (i = 0; i < insn_cnt; i++, insn++) {
		if (tgt_idx <= i && i < tgt_idx + delta)
			continue;

		/* ... */
	}

Re: [PATCH v4 bpf-next 2/9] bpf: Adjust BPF_JMP that jumps to the 1st insn of the prologue

[Alexei Starovoitov]

On Wed, Aug 28, 2024 at 10:44 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
> >>
> >> -       for (i = 0; i < insn_cnt; i++, insn++) {
> >> +       for (i = skip_cnt; i < insn_cnt; i++, insn++) {
> >
> > Do we really need to add this argument?
> >
> >> -               WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1));
> >> +               WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1, 0));
> >
> > We can always do for (i = delta; ...
> >
> > The above case of skip_cnt == 0 is lucky to work this way.
> > It would be less surprising to skip all insns in the patch.
> > Maybe I'm missing something.
>
> For subprog_start case, tgt_idx (where the patch started) may not be 0. How
> about this:
>
>         for (i = 0; i < insn_cnt; i++, insn++) {
>                 if (tgt_idx <= i && i < tgt_idx + delta)
>                         continue;

Yeah. Right. Same idea, but certainly your way is more correct
instead of my buggy proposal.

In that sense the "for (i = skip_cnt" approach
is also a bit buggy, if tgt_idx != 0.

Re: [PATCH v4 bpf-next 2/9] bpf: Adjust BPF_JMP that jumps to the 1st insn of the prologue

[Martin KaFai Lau]

On 8/28/24 11:43 AM, Alexei Starovoitov wrote:
> On Wed, Aug 28, 2024 at 10:44 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>>>
>>>> -       for (i = 0; i < insn_cnt; i++, insn++) {
>>>> +       for (i = skip_cnt; i < insn_cnt; i++, insn++) {
>>>
>>> Do we really need to add this argument?
>>>
>>>> -               WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1));
>>>> +               WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1, 0));
>>>
>>> We can always do for (i = delta; ...
>>>
>>> The above case of skip_cnt == 0 is lucky to work this way.
>>> It would be less surprising to skip all insns in the patch.
>>> Maybe I'm missing something.
>>
>> For subprog_start case, tgt_idx (where the patch started) may not be 0. How
>> about this:
>>
>>          for (i = 0; i < insn_cnt; i++, insn++) {
>>                  if (tgt_idx <= i && i < tgt_idx + delta)
>>                          continue;
> 
> Yeah. Right. Same idea, but certainly your way is more correct
> instead of my buggy proposal.
> 
> In that sense the "for (i = skip_cnt" approach
> is also a bit buggy, if tgt_idx != 0.

Yep. Adding skip_cnt like this patch 2 is not right. I didn't think hard enough 
what to do with the existing adjust_jmp_off().

I will remove skip_cnt in the next respin.

Thanks for the review!

Re: [PATCH v4 bpf-next 2/9] bpf: Adjust BPF_JMP that jumps to the 1st insn of the prologue

[Eduard Zingerman]

On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:

[...]

> @@ -19705,6 +19705,9 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>  		}
>  	}
>  
> +	if (delta)
> +		WARN_ON(adjust_jmp_off(env->prog, 0, delta, delta));
> +

I double checked my old calculations and agree that this adjustment is necessary.
Alexei's suggestion to use delta == skip_cnt sounds reasonable.


>  	if (bpf_prog_is_offloaded(env->prog->aux))
>  		return 0;
>  

[...]