[RFC PATCH bpf-next 0/6] bpf: Add gen_epilogue and allow kfunc call in pro/epilogue

[RFC PATCH bpf-next 0/6] bpf: Add gen_epilogue and allow kfunc call in pro/epilogue

[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 kfunc to handle (e.g. drop) it in
the epilogue. Another use case could be in bpf_tcp_ca.c to
enforce snd_cwnd has sane value (e.g. non zero).

The existing .gen_prologue can call bpf helper.
This set also allows the existing .gen_prologue and the new
.gen_epilogue to call kfunc. Other than the skb drop
mentioned above, the bpf qdisc subsystem can call
kfunc to enforce initializing / releasing resources (e.g.
qdisc_watchdog_init/cancel) in some of the qdisc_ops.

It is under RFC because the .gen_epilogue will need 8 extra
bytes in the stack to save the ctx pointer. It is now
done after the check_max_stack_depth. The ctx pointer
saving will need to be done earlier before check_max_stack_depth.

Martin KaFai Lau (6):
  bpf: Add gen_epilogue to bpf_verifier_ops
  bpf: Export bpf_base_func_proto
  selftests/test: test gen_prologue and gen_epilogue
  bpf: Add module parameter to gen_prologue and gen_epilogue
  bpf: Allow pro/epilogue to call kfunc
  selftests/bpf: Add kfunc call test in gen_prologue and gen_epilogue

 include/linux/bpf.h                           |   4 +-
 include/linux/btf.h                           |   1 +
 kernel/bpf/btf.c                              |   2 +-
 kernel/bpf/cgroup.c                           |   3 +-
 kernel/bpf/helpers.c                          |   1 +
 kernel/bpf/verifier.c                         | 146 ++++++++++-
 net/core/filter.c                             |   6 +-
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 228 ++++++++++++++++++
 .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  11 +
 .../bpf/bpf_testmod/bpf_testmod_kfunc.h       |   6 +
 .../bpf/prog_tests/struct_ops_syscall.c       |  92 +++++++
 .../selftests/bpf/progs/struct_ops_syscall.c  | 113 +++++++++
 12 files changed, 604 insertions(+), 9 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_syscall.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_syscall.c

-- 
2.43.5

[RFC PATCH bpf-next 1/6] 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.

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
The .gen_epilogue will need 8 extra bytes in the stack to save
the ctx pointer. It is now done after the check_max_stack_depth.
The ctx pointer saving will need to be done earlier before
check_max_stack_depth.

 include/linux/bpf.h   |  2 ++
 kernel/bpf/verifier.c | 34 ++++++++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index b9425e410bcb..2de67bc497f4 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/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index df3be12096cf..bbb655f0c7b5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19610,15 +19610,37 @@ 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 insn_buf[16], *insn;
+	struct bpf_insn insn_buf[16], epilogue_buf[16], *insn;
 	u32 target_size, size_default, off;
 	struct bpf_prog *new_prog;
 	enum bpf_access_type type;
 	bool is_narrower_load;
 
+	if (ops->gen_epilogue) {
+		epilogue_cnt = ops->gen_epilogue(epilogue_buf, env->prog,
+						 -(subprogs[0].stack_depth + 8));
+		if (epilogue_cnt >= ARRAY_SIZE(epilogue_buf)) {
+			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) {
 			verbose(env, "bpf verifier is misconfigured\n");
@@ -19671,6 +19693,13 @@ 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 */
+			memcpy(insn_buf, epilogue_buf, sizeof(epilogue_buf));
+			cnt = epilogue_cnt;
+			goto patch_insn_buf;
 		} else {
 			continue;
 		}
@@ -19807,6 +19836,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: [RFC PATCH bpf-next 1/6] bpf: Add gen_epilogue to bpf_verifier_ops

[Eduard Zingerman]

On Tue, 2024-08-13 at 11:49 -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.
> 
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---

Apart from the note below I don't see any obvious problems with this code.

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

[...]

> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19610,15 +19610,37 @@ 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 insn_buf[16], *insn;
> +	struct bpf_insn insn_buf[16], epilogue_buf[16], *insn;
>  	u32 target_size, size_default, off;
>  	struct bpf_prog *new_prog;
>  	enum bpf_access_type type;
>  	bool is_narrower_load;
>  
> +	if (ops->gen_epilogue) {
> +		epilogue_cnt = ops->gen_epilogue(epilogue_buf, env->prog,
> +						 -(subprogs[0].stack_depth + 8));
> +		if (epilogue_cnt >= ARRAY_SIZE(epilogue_buf)) {
> +			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;

Note: two other places that allocate additional stack
      (optimize_bpf_loop(), do_misc_fixups())
      also bump 'env->prog->aux->stack_depth'.

> +			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;
> +		}
> +	}
> +

[...]

Re: [RFC PATCH bpf-next 1/6] bpf: Add gen_epilogue to bpf_verifier_ops

[Martin KaFai Lau]

On 8/14/24 1:56 PM, Eduard Zingerman wrote:
> On Tue, 2024-08-13 at 11:49 -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.
>>
>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>> ---
> 
> Apart from the note below I don't see any obvious problems with this code.
> 
> Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>
> 
> [...]
> 
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -19610,15 +19610,37 @@ 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 insn_buf[16], *insn;
>> +	struct bpf_insn insn_buf[16], epilogue_buf[16], *insn;
>>   	u32 target_size, size_default, off;
>>   	struct bpf_prog *new_prog;
>>   	enum bpf_access_type type;
>>   	bool is_narrower_load;
>>   
>> +	if (ops->gen_epilogue) {
>> +		epilogue_cnt = ops->gen_epilogue(epilogue_buf, env->prog,
>> +						 -(subprogs[0].stack_depth + 8));
>> +		if (epilogue_cnt >= ARRAY_SIZE(epilogue_buf)) {
>> +			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;
> 
> Note: two other places that allocate additional stack
>        (optimize_bpf_loop(), do_misc_fixups())
>        also bump 'env->prog->aux->stack_depth'.

Thanks for pointing it out.

In this case, I will stay with calling .gen_epilogue in convert_ctx_accesses().

> 
>> +			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;
>> +		}
>> +	}
>> +
> 
> [...]
> 

Re: [RFC PATCH bpf-next 1/6] bpf: Add gen_epilogue to bpf_verifier_ops

[Amery Hung]

On Tue, Aug 13, 2024 at 11:49 AM Martin KaFai Lau <martin.lau@linux.dev> 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.
>
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---
> The .gen_epilogue will need 8 extra bytes in the stack to save
> the ctx pointer. It is now done after the check_max_stack_depth.
> The ctx pointer saving will need to be done earlier before
> check_max_stack_depth.
>
>  include/linux/bpf.h   |  2 ++
>  kernel/bpf/verifier.c | 34 ++++++++++++++++++++++++++++++++--
>  2 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b9425e410bcb..2de67bc497f4 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/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index df3be12096cf..bbb655f0c7b5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19610,15 +19610,37 @@ 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 insn_buf[16], *insn;
> +       struct bpf_insn insn_buf[16], epilogue_buf[16], *insn;
>         u32 target_size, size_default, off;
>         struct bpf_prog *new_prog;
>         enum bpf_access_type type;
>         bool is_narrower_load;
>
> +       if (ops->gen_epilogue) {
> +               epilogue_cnt = ops->gen_epilogue(epilogue_buf, env->prog,
> +                                                -(subprogs[0].stack_depth + 8));
> +               if (epilogue_cnt >= ARRAY_SIZE(epilogue_buf)) {
> +                       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) {
>                         verbose(env, "bpf verifier is misconfigured\n");
> @@ -19671,6 +19693,13 @@ 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 */
> +                       memcpy(insn_buf, epilogue_buf, sizeof(epilogue_buf));
> +                       cnt = epilogue_cnt;
> +                       goto patch_insn_buf;
>                 } else {
>                         continue;
>                 }
> @@ -19807,6 +19836,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
>

Hi Martin,

Thanks for working on this set! This patch looks good to me.

Thanks,
Amery

[RFC PATCH bpf-next 2/6] 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
the next 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 d02ae323996b..544f7d171fe6 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

[RFC PATCH bpf-next 3/6] selftests/test: 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(struct st_ops_args *args)
        bpf_kfunc_st_op_test_prologue(args)
	    st_ops->test_prologue(args)

The .gen_prologue and .gen_epilogue of the bpf_testmod_st_ops
will add PROLOGUE_A (1000) and EPILOGUE_A (10000) to args->a.
.gen_prologue adds PROLOGUE_A (1000).
.gen_epilogue adds EPILOGUE_A (10000).
.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 SEC("struct_ops/..") will either
call a global subprog() which does "args->a += 1" or
call another new kfunc bpf_kfunc_st_ops_inc10 which does "args->a += 10".

The prog_tests/struct_ops_syscall.c will test_run the SEC("syscall")
programs. It checks the result by testing the args->a and the retval.

For example, when triggering the ops in the
'SEC("struct_ops/test_epilogue") int BPF_PROG(test_epilogue_subprog..'

the expected args->a is
+1 (because of the subprog calls) + 10000 (.gen_epilogue) = 10001
The expected return value is 2 * 10001 (.gen_epilogue).

Another set of tests is to have the main struct_ops program
to call a subprog and call the inc10 kfunc.

There is also a bpf_tail_call test for epilogue.

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 +
 .../bpf/prog_tests/struct_ops_syscall.c       |  91 +++++++++
 .../selftests/bpf/progs/struct_ops_syscall.c  | 113 +++++++++++
 5 files changed, 411 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_syscall.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_syscall.c

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 3687a40b61c6..7194330bdefc 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"
@@ -920,6 +921,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)
@@ -956,6 +1002,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)
@@ -1075,6 +1125,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)
@@ -1092,8 +1280,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/struct_ops_syscall.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_syscall.c
new file mode 100644
index 000000000000..a293a35b0dcc
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_syscall.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+#include "struct_ops_syscall.skel.h"
+
+#define EPILOGUE_A  10000
+#define PROLOGUE_A   1000
+#define KFUNC_A10      10
+#define SUBPROG_A       1
+
+#define SUBPROG_TEST_MAIN	SUBPROG_A
+#define KFUNC_TEST_MAIN		(KFUNC_A10 + SUBPROG_A)
+
+struct st_ops_args {
+	int a;
+};
+
+static void do_test(struct struct_ops_syscall *skel,
+		    struct bpf_map *st_ops_map, int main_prog_a)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	int err, prog_fd, expected_a;
+	struct st_ops_args args;
+	struct bpf_link *link;
+
+	topts.ctx_in = &args;
+	topts.ctx_size_in = sizeof(args);
+
+	link = bpf_map__attach_struct_ops(st_ops_map);
+	if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
+		return;
+
+	/* gen_prologue + main prog */
+	expected_a = PROLOGUE_A + main_prog_a;
+	memset(&args, 0, sizeof(args));
+	prog_fd = bpf_program__fd(skel->progs.syscall_prologue);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	ASSERT_OK(err, "bpf_prog_test_run_opts");
+	ASSERT_EQ(args.a, expected_a, "args.a");
+	ASSERT_EQ(topts.retval, 0, "topts.retval");
+
+	/* main prog + gen_epilogue */
+	expected_a =  main_prog_a + EPILOGUE_A;
+	memset(&args, 0, sizeof(args));
+	prog_fd = bpf_program__fd(skel->progs.syscall_epilogue);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	ASSERT_OK(err, "bpf_prog_test_run_opts");
+	ASSERT_EQ(args.a, expected_a, "args.a");
+	ASSERT_EQ(topts.retval, expected_a * 2, "topts.retval");
+
+	/* gen_prologue + main prog + gen_epilogue */
+	expected_a = PROLOGUE_A + main_prog_a + EPILOGUE_A;
+	memset(&args, 0, sizeof(args));
+	prog_fd = bpf_program__fd(skel->progs.syscall_pro_epilogue);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	ASSERT_OK(err, "bpf_prog_test_run_opts");
+	ASSERT_EQ(args.a, expected_a, "args.a");
+	ASSERT_EQ(topts.retval, expected_a * 2, "topts.retval");
+	bpf_link__destroy(link);
+}
+
+void test_struct_ops_syscall(void)
+{
+	struct struct_ops_syscall *skel;
+
+	skel = struct_ops_syscall__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
+		return;
+
+	if (test__start_subtest("subprog"))
+		do_test(skel, skel->maps.pro_epilogue_subprog_ops,
+			SUBPROG_TEST_MAIN);
+
+	if (test__start_subtest("kfunc"))
+		do_test(skel, skel->maps.pro_epilogue_kfunc_ops,
+			KFUNC_TEST_MAIN);
+
+	if (test__start_subtest("tailcall")) {
+		const int zero = 0;
+		int prog_fd = bpf_program__fd(skel->progs.test_epilogue_subprog);
+		int map_fd = bpf_map__fd(skel->maps.epilogue_map);
+		int err;
+
+		err = bpf_map_update_elem(map_fd, &zero, &prog_fd, 0);
+		if (ASSERT_OK(err, "map_update(epilogue_map)"))
+			do_test(skel, skel->maps.pro_epilogue_tail_ops,
+				SUBPROG_TEST_MAIN);
+	}
+
+	struct_ops_syscall__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_syscall.c b/tools/testing/selftests/bpf/progs/struct_ops_syscall.c
new file mode 100644
index 000000000000..ee153461d9f8
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_syscall.c
@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "../bpf_testmod/bpf_testmod.h"
+#include "../bpf_testmod/bpf_testmod_kfunc.h"
+
+char _license[] SEC("license") = "GPL";
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+	__uint(max_entries, 1);
+	__uint(key_size, sizeof(__u32));
+	__uint(value_size, sizeof(__u32));
+} epilogue_map SEC(".maps");
+
+static __noinline int subprog(struct st_ops_args *args)
+{
+	args->a += 1;
+	return 0;
+}
+
+SEC("struct_ops/test_prologue_subprog")
+int BPF_PROG(test_prologue_subprog, struct st_ops_args *args)
+{
+	subprog(args);
+	return 0;
+}
+
+SEC("struct_ops/test_epilogue_subprog")
+int BPF_PROG(test_epilogue_subprog, struct st_ops_args *args)
+{
+	subprog(args);
+	return 0;
+}
+
+SEC("struct_ops/test_pro_epilogue_subprog")
+int BPF_PROG(test_pro_epilogue_subprog, struct st_ops_args *args)
+{
+	subprog(args);
+	return 0;
+}
+
+SEC("struct_ops/test_prologue_kfunc")
+int BPF_PROG(test_prologue_kfunc, struct st_ops_args *args)
+{
+	bpf_kfunc_st_ops_inc10(args);
+	subprog(args);
+	return 0;
+}
+
+SEC("struct_ops/test_epilogue_kfunc")
+int BPF_PROG(test_epilogue_kfunc, struct st_ops_args *args)
+{
+	bpf_kfunc_st_ops_inc10(args);
+	subprog(args);
+	return 0;
+}
+
+SEC("struct_ops/test_pro_epilogue_kfunc")
+int BPF_PROG(test_pro_epilogue_kfunc, struct st_ops_args *args)
+{
+	bpf_kfunc_st_ops_inc10(args);
+	subprog(args);
+	return 0;
+}
+
+SEC("struct_ops/test_epilogue_tail")
+int test_epilogue_tail(unsigned long long *ctx)
+{
+	bpf_tail_call_static(ctx, &epilogue_map, 0);
+	return 0;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops pro_epilogue_subprog_ops = {
+	.test_prologue = (void *)test_prologue_subprog,
+	.test_epilogue = (void *)test_epilogue_subprog,
+	.test_pro_epilogue = (void *)test_pro_epilogue_subprog,
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops pro_epilogue_kfunc_ops = {
+	.test_prologue = (void *)test_prologue_kfunc,
+	.test_epilogue = (void *)test_epilogue_kfunc,
+	.test_pro_epilogue = (void *)test_pro_epilogue_kfunc,
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops pro_epilogue_tail_ops = {
+	.test_prologue = (void *)test_prologue_subprog,
+	.test_epilogue = (void *)test_epilogue_tail,
+	.test_pro_epilogue = (void *)test_pro_epilogue_subprog,
+};
+
+SEC("syscall")
+int syscall_prologue(struct st_ops_args *args)
+{
+	return bpf_kfunc_st_ops_test_prologue(args);
+}
+
+SEC("syscall")
+int syscall_epilogue(struct st_ops_args *args)
+{
+	return bpf_kfunc_st_ops_test_epilogue(args);
+}
+
+SEC("syscall")
+int syscall_pro_epilogue(struct st_ops_args *args)
+{
+	return bpf_kfunc_st_ops_test_pro_epilogue(args);
+}
-- 
2.43.5

Re: [RFC PATCH bpf-next 3/6] selftests/test: test gen_prologue and gen_epilogue

[Eduard Zingerman]

[-- Attachment #1: Type: text/plain, Size: 1978 bytes --]

Hi Martin,

Please note that after changes for struct_ops map autoload by libbpf,
test_loader could be use to test struct_ops related changes.
Also, test_loader now supports __xlated macro which allows to verify
rewrites applied by verifier.
For example, the sample below works:

    struct st_ops_args;
    
    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;
    };
    
    __success
    __xlated("0: *(u64 *)(r10 -8) = r1")
    __xlated("1: r0 = 0")
    __xlated("2: r1 = *(u64 *)(r10 -8)")
    __xlated("3: r1 = *(u64 *)(r1 +0)")
    __xlated("4: r6 = *(u32 *)(r1 +0)")
    __xlated("5: w6 += 10000")
    __xlated("6: *(u32 *)(r1 +0) = r6")
    __xlated("7: r6 = r1")
    __xlated("8: call kernel-function")
    __xlated("9: r1 = r6")
    __xlated("10: call kernel-function")
    __xlated("11: w0 *= 2")
    __xlated("12: exit")
    SEC("struct_ops/test_epilogue")
    __naked int test_epilogue(void)
    {
    	asm volatile (
    	"r0 = 0;"
    	"exit;"
    	::: __clobber_all);
    }
    
    SEC(".struct_ops.link")
    struct bpf_testmod_st_ops st_ops = {
    	.test_epilogue = (void *)test_epilogue,
    };

(Complete example is in the attachment).
test_loader based tests can also trigger program execution via __retval() macro.
The only (minor) shortcoming that I see, is that test_loader would
load/unload st_ops map multiple times because of the following
interaction:
- test_loader assumes that each bpf program defines a test;
- test_loader re-creates all maps before each test;
- libbpf struct_ops autocreate logic marks all programs referenced
  from struct_ops map as autoloaded.

I think that writing tests this way is easier to follow,
compared to arithmetic manipulations done currently.
What do you think?

Thanks,
Eduard


[-- Attachment #2: struct_ops-test_loader-example.patch --]
[-- Type: text/x-patch, Size: 2395 bytes --]

diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_epilogue.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_epilogue.c
new file mode 100644
index 000000000000..02825d9107ac
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_epilogue.c
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <test_progs.h>
+#include "struct_ops_epilogue.skel.h"
+
+void test_struct_ops_epilogue(void)
+{
+	RUN_TESTS(struct_ops_epilogue);
+}
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_epilogue.c b/tools/testing/selftests/bpf/progs/struct_ops_epilogue.c
new file mode 100644
index 000000000000..8702c9375023
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_epilogue.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+
+char _license[] SEC("license") = "GPL";
+
+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;
+};
+
+__success
+__xlated("0: *(u64 *)(r10 -8) = r1")
+__xlated("1: r0 = 0")
+__xlated("2: r1 = *(u64 *)(r10 -8)")
+__xlated("3: r1 = *(u64 *)(r1 +0)")
+__xlated("4: r6 = *(u32 *)(r1 +0)")
+__xlated("5: w6 += 10000")
+__xlated("6: *(u32 *)(r1 +0) = r6")
+__xlated("7: r6 = r1")
+__xlated("8: call kernel-function")
+__xlated("9: r1 = r6")
+__xlated("10: call kernel-function")
+__xlated("11: w0 *= 2")
+__xlated("12: exit")
+SEC("struct_ops/test_epilogue")
+__naked int test_epilogue(void)
+{
+	asm volatile (
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+__success
+__xlated("0: r6 = *(u64 *)(r1 +0)")
+__xlated("1: r7 = *(u32 *)(r6 +0)")
+__xlated("2: w7 += 1000")
+__xlated("3: *(u32 *)(r6 +0) = r7")
+__xlated("4: r7 = r1")
+__xlated("5: r1 = r6")
+__xlated("6: call kernel-function")
+__xlated("7: r1 = r6")
+__xlated("8: call kernel-function")
+__xlated("9: r1 = r7")
+__xlated("10: r0 = 0")
+__xlated("11: exit")
+SEC("struct_ops/test_prologue")
+__naked int test_prologue(void)
+{
+	asm volatile (
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops st_ops = {
+	.test_epilogue = (void *)test_epilogue,
+	.test_prologue = (void *)test_prologue,
+};

Re: [RFC PATCH bpf-next 3/6] selftests/test: test gen_prologue and gen_epilogue

[Martin KaFai Lau]

On 8/14/24 1:48 PM, Eduard Zingerman wrote:
> Hi Martin,
> 
> Please note that after changes for struct_ops map autoload by libbpf,
> test_loader could be use to test struct_ops related changes.
> Also, test_loader now supports __xlated macro which allows to verify
> rewrites applied by verifier.
> For example, the sample below works:
> 
>      struct st_ops_args;
>      
>      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;
>      };
>      
>      __success
>      __xlated("0: *(u64 *)(r10 -8) = r1")
>      __xlated("1: r0 = 0")
>      __xlated("2: r1 = *(u64 *)(r10 -8)")
>      __xlated("3: r1 = *(u64 *)(r1 +0)")
>      __xlated("4: r6 = *(u32 *)(r1 +0)")
>      __xlated("5: w6 += 10000")
>      __xlated("6: *(u32 *)(r1 +0) = r6")
>      __xlated("7: r6 = r1")
>      __xlated("8: call kernel-function")
>      __xlated("9: r1 = r6")
>      __xlated("10: call kernel-function")
>      __xlated("11: w0 *= 2")
>      __xlated("12: exit")

It is appealing to be able to check at the xlated instruction level for 
.gen_pro/epilogue.

>      SEC("struct_ops/test_epilogue")
>      __naked int test_epilogue(void)
>      {
>      	asm volatile (
>      	"r0 = 0;"

I also want to test a struct_ops prog making kfunc call, e.g. the 
BPF_PROG(test_epilogue_kfunc) in this patch. I have never tried this in asm, so 
a n00b question. Do you know if there is an example how to call kfunc?

>      	"exit;"
>      	::: __clobber_all);
>      }
>      
>      SEC(".struct_ops.link")
>      struct bpf_testmod_st_ops st_ops = {
>      	.test_epilogue = (void *)test_epilogue,
>      };
> 
> (Complete example is in the attachment).
> test_loader based tests can also trigger program execution via __retval() macro.
> The only (minor) shortcoming that I see, is that test_loader would
> load/unload st_ops map multiple times because of the following
> interaction:
> - test_loader assumes that each bpf program defines a test;
> - test_loader re-creates all maps before each test;
> - libbpf struct_ops autocreate logic marks all programs referenced
>    from struct_ops map as autoloaded.

If I understand correctly, there are redundant works but still work?

Potentially the test_loader can check all the loaded struct_ops progs of a 
st_ops map at once which is an optimization.

Re: __retval(), the struct_ops progs is triggered by a SEC("syscall") prog. 
Before calling this syscall prog, the st_ops map needs to be attached first. I 
think the attach part is missing also? or there is a way?

Re: [RFC PATCH bpf-next 3/6] selftests/test: test gen_prologue and gen_epilogue

[Eduard Zingerman]

On Thu, 2024-08-15 at 16:41 -0700, Martin KaFai Lau wrote:

[...]

> >      SEC("struct_ops/test_epilogue")
> >      __naked int test_epilogue(void)
> >      {
> >      	asm volatile (
> >      	"r0 = 0;"
> 
> I also want to test a struct_ops prog making kfunc call, e.g. the 
> BPF_PROG(test_epilogue_kfunc) in this patch. I have never tried this in asm, so 
> a n00b question. Do you know if there is an example how to call kfunc?

Here is an example:
progs/verifier_ref_tracking.c, specifically take a look at
acquire_release_user_key_reference(). The main trick is to have
__kfunc_btf_root() with dummy calls, so that there are BTF signatures
for kfuncs included in the object file.

> >      	"exit;"
> >      	::: __clobber_all);
> >      }
> >      
> >      SEC(".struct_ops.link")
> >      struct bpf_testmod_st_ops st_ops = {
> >      	.test_epilogue = (void *)test_epilogue,
> >      };
> > 
> > (Complete example is in the attachment).
> > test_loader based tests can also trigger program execution via __retval() macro.
> > The only (minor) shortcoming that I see, is that test_loader would
> > load/unload st_ops map multiple times because of the following
> > interaction:
> > - test_loader assumes that each bpf program defines a test;
> > - test_loader re-creates all maps before each test;
> > - libbpf struct_ops autocreate logic marks all programs referenced
> >    from struct_ops map as autoloaded.
> 
> If I understand correctly, there are redundant works but still work?

Yes.

> Potentially the test_loader can check all the loaded struct_ops progs of a 
> st_ops map at once which is an optimization.

Yes, I should look into this.

> Re: __retval(), the struct_ops progs is triggered by a SEC("syscall") prog. 
> Before calling this syscall prog, the st_ops map needs to be attached first. I 
> think the attach part is missing also? or there is a way?

I think libbpf handles the attachment automatically, I'll double check and reply.

Re: [RFC PATCH bpf-next 3/6] selftests/test: test gen_prologue and gen_epilogue

[Eduard Zingerman]

On Thu, 2024-08-15 at 17:23 -0700, Eduard Zingerman wrote:

[...]

> > Re: __retval(), the struct_ops progs is triggered by a SEC("syscall") prog. 
> > Before calling this syscall prog, the st_ops map needs to be attached first. I 
> > think the attach part is missing also? or there is a way?
> 
> I think libbpf handles the attachment automatically, I'll double check and reply.
> 

In theory, the following addition to the example I've sent already should work:

    struct st_ops_args;
    int bpf_kfunc_st_ops_test_prologue(struct st_ops_args *args) __ksym;
 
    SEC("syscall")
    __retval(0)
    int syscall_prologue(void *ctx)
    {
    	struct st_ops_args args = { -42 };
    	bpf_kfunc_st_ops_test_prologue(&args);
    	return args.a;
    }

However, the initial value of -42 is not changed, e.g. here is the log:

    $ ./test_progs -vvv -t struct_ops_epilogue/syscall_prologue
    ...
    libbpf: loaded kernel BTF from '/sys/kernel/btf/vmlinux'
    libbpf: extern (func ksym) 'bpf_kfunc_st_ops_test_prologue': resolved to bpf_testmod [104486]
    libbpf: struct_ops init_kern st_ops: type_id:44 kern_type_id:104321 kern_vtype_id:104378
    libbpf: struct_ops init_kern st_ops: func ptr test_prologue is set to prog test_prologue from data(+0) to kern_data(+0)
    libbpf: struct_ops init_kern st_ops: func ptr test_epilogue is set to prog test_epilogue from data(+8) to kern_data(+8)
    libbpf: map 'st_ops': created successfully, fd=5
    run_subtest:PASS:unexpected_load_failure 0 nsec
    VERIFIER LOG:
    =============
    ...
    =============
    do_prog_test_run:PASS:bpf_prog_test_run 0 nsec
    run_subtest:FAIL:837 Unexpected retval: -42 != 0
    #321/3   struct_ops_epilogue/syscall_prologue:FAIL
    #321     struct_ops_epilogue:FAIL

So, something goes awry in bpf_kfunc_st_ops_test_prologue():

    __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;
    }

Either st_ops is null or st_ops->test_prologue is null.
However, the log above shows:

    libbpf: struct_ops init_kern st_ops: type_id:44 kern_type_id:104321 kern_vtype_id:104378
    libbpf: struct_ops init_kern st_ops: func ptr test_prologue is set to prog test_prologue from data(+0) to kern_data(+0)
    libbpf: struct_ops init_kern st_ops: func ptr test_epilogue is set to prog test_epilogue from data(+8) to kern_data(+8)

Here libbpf does autoload for st_ops map and populates it, so st_ops->test_prologue should not be null.
Will have some time tomorrow to debug this (or you can give it a shot if you'd like).

Re: [RFC PATCH bpf-next 3/6] selftests/test: test gen_prologue and gen_epilogue

[Martin KaFai Lau]

On 8/15/24 6:50 PM, Eduard Zingerman wrote:
> On Thu, 2024-08-15 at 17:23 -0700, Eduard Zingerman wrote:
> 
> [...]
> 
>>> Re: __retval(), the struct_ops progs is triggered by a SEC("syscall") prog.
>>> Before calling this syscall prog, the st_ops map needs to be attached first. I
>>> think the attach part is missing also? or there is a way?
>>
>> I think libbpf handles the attachment automatically, I'll double check and reply.
>>
> 
> In theory, the following addition to the example I've sent already should work:
> 
>      struct st_ops_args;
>      int bpf_kfunc_st_ops_test_prologue(struct st_ops_args *args) __ksym;
>   
>      SEC("syscall")
>      __retval(0)
>      int syscall_prologue(void *ctx)
>      {
>      	struct st_ops_args args = { -42 };
>      	bpf_kfunc_st_ops_test_prologue(&args);
>      	return args.a;
>      }
> 
> However, the initial value of -42 is not changed, e.g. here is the log:
> 
>      $ ./test_progs -vvv -t struct_ops_epilogue/syscall_prologue
>      ...
>      libbpf: loaded kernel BTF from '/sys/kernel/btf/vmlinux'
>      libbpf: extern (func ksym) 'bpf_kfunc_st_ops_test_prologue': resolved to bpf_testmod [104486]
>      libbpf: struct_ops init_kern st_ops: type_id:44 kern_type_id:104321 kern_vtype_id:104378
>      libbpf: struct_ops init_kern st_ops: func ptr test_prologue is set to prog test_prologue from data(+0) to kern_data(+0)
>      libbpf: struct_ops init_kern st_ops: func ptr test_epilogue is set to prog test_epilogue from data(+8) to kern_data(+8)
>      libbpf: map 'st_ops': created successfully, fd=5
>      run_subtest:PASS:unexpected_load_failure 0 nsec
>      VERIFIER LOG:
>      =============
>      ...
>      =============
>      do_prog_test_run:PASS:bpf_prog_test_run 0 nsec
>      run_subtest:FAIL:837 Unexpected retval: -42 != 0
>      #321/3   struct_ops_epilogue/syscall_prologue:FAIL
>      #321     struct_ops_epilogue:FAIL
> 
> So, something goes awry in bpf_kfunc_st_ops_test_prologue():
> 
>      __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)

Thanks for checking!

I think the bpf_map__attach_struct_ops() is not done such that st_ops is NULL.

It probably needs another tag in the SEC("syscall") program to tell which st_ops 
map should be attached first before executing the "syscall" program.

I like the idea of using the __xlated macro to check the patched prologue, ctx 
pointer saving, and epilogue. I will add this test in the respin. I will keep 
the current way in this patch to exercise syscall and the ops/func in st_ops for 
now. We can iterate on it later and use it as an example on what supports are 
needed on the test_loader side for st_ops map testing. On the repetitive-enough 
to worth test_loader refactoring side, I suspect some of the existing st_ops 
load-success/load-failure tests may be worth to look at also. Thoughts?

>      		ret = st_ops->test_prologue(args);
>      	mutex_unlock(&st_ops_mutex);
>      
>      	return ret;
>      }
> 
> Either st_ops is null or st_ops->test_prologue is null.
> However, the log above shows:
> 
>      libbpf: struct_ops init_kern st_ops: type_id:44 kern_type_id:104321 kern_vtype_id:104378
>      libbpf: struct_ops init_kern st_ops: func ptr test_prologue is set to prog test_prologue from data(+0) to kern_data(+0)
>      libbpf: struct_ops init_kern st_ops: func ptr test_epilogue is set to prog test_epilogue from data(+8) to kern_data(+8)
> 
> Here libbpf does autoload for st_ops map and populates it, so st_ops->test_prologue should not be null.
> Will have some time tomorrow to debug this (or you can give it a shot if you'd like).
> 

Re: [RFC PATCH bpf-next 3/6] selftests/test: test gen_prologue and gen_epilogue

[Eduard Zingerman]

[-- Attachment #1: Type: text/plain, Size: 1296 bytes --]

On Fri, 2024-08-16 at 10:27 -0700, Martin KaFai Lau wrote:

[...]

> Thanks for checking!
> 
> I think the bpf_map__attach_struct_ops() is not done such that st_ops is NULL.
> 
> It probably needs another tag in the SEC("syscall") program to tell which st_ops 
> map should be attached first before executing the "syscall" program.
> 
> I like the idea of using the __xlated macro to check the patched prologue, ctx 
> pointer saving, and epilogue. I will add this test in the respin. I will keep 
> the current way in this patch to exercise syscall and the ops/func in st_ops for 
> now. We can iterate on it later and use it as an example on what supports are 
> needed on the test_loader side for st_ops map testing. On the repetitive-enough 
> to worth test_loader refactoring side, I suspect some of the existing st_ops 
> load-success/load-failure tests may be worth to look at also. Thoughts?

You are correct, this happens because bpf_map__attach_struct_ops() is
not called. Fortunately, the change for test_loader.c is not very big.
Please check two patches in the attachment.

> I suspect some of the existing st_ops load-success/load-failure
> tests may be worth to look at also.

I suspect this is the case, but would prefer not worry about it for now :)


[-- Attachment #2: 0001-selftests-bpf-attach-struct_ops-maps-before-test-pro.patch --]
[-- Type: text/x-patch, Size: 2630 bytes --]

From 7c77dc57572d68e206e2fcf230b3aa1c9403fa93 Mon Sep 17 00:00:00 2001
From: Eduard Zingerman <eddyz87@gmail.com>
Date: Fri, 16 Aug 2024 13:16:46 -0700
Subject: [PATCH bpf-next 1/2] selftests/bpf: attach struct_ops maps before
 test prog runs

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>
---
 tools/testing/selftests/bpf/test_loader.c | 26 +++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 12b0c41e8d64..67f8d427cfb5 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -729,11 +729,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))
@@ -823,6 +825,25 @@ 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) {
@@ -837,9 +858,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.45.2


[-- Attachment #3: 0002-selftests-bpf-example-struct_ops-test-using-test_loa.patch --]
[-- Type: text/x-patch, Size: 3368 bytes --]

From bbc36b2b0ec42f5994bef771bf8a85641aeb969e Mon Sep 17 00:00:00 2001
From: Eduard Zingerman <eddyz87@gmail.com>
Date: Fri, 16 Aug 2024 13:19:39 -0700
Subject: [PATCH bpf-next 2/2] selftests/bpf: example struct_ops test using
 test_loader

This is based on struct_ops_syscall.c and aims to show usage of
__xlated and __retval macros when testing struct_ops related
functionality.
---
 .../bpf/prog_tests/struct_ops_epilogue.c      |  9 ++
 .../selftests/bpf/progs/struct_ops_epilogue.c | 82 +++++++++++++++++++
 2 files changed, 91 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_epilogue.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_epilogue.c

diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_epilogue.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_epilogue.c
new file mode 100644
index 000000000000..02825d9107ac
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_epilogue.c
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <test_progs.h>
+#include "struct_ops_epilogue.skel.h"
+
+void test_struct_ops_epilogue(void)
+{
+	RUN_TESTS(struct_ops_epilogue);
+}
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_epilogue.c b/tools/testing/selftests/bpf/progs/struct_ops_epilogue.c
new file mode 100644
index 000000000000..ca2343e5158a
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_epilogue.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+
+char _license[] SEC("license") = "GPL";
+
+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;
+};
+
+__success
+__xlated("0: *(u64 *)(r10 -8) = r1")
+__xlated("1: r0 = 0")
+__xlated("2: r1 = *(u64 *)(r10 -8)")
+__xlated("3: r1 = *(u64 *)(r1 +0)")
+__xlated("4: r6 = *(u32 *)(r1 +0)")
+__xlated("5: w6 += 10000")
+__xlated("6: *(u32 *)(r1 +0) = r6")
+__xlated("7: r6 = r1")
+__xlated("8: call kernel-function")
+__xlated("9: r1 = r6")
+__xlated("10: call kernel-function")
+__xlated("11: w0 *= 2")
+__xlated("12: exit")
+SEC("struct_ops/test_epilogue")
+__naked int test_epilogue(void)
+{
+	asm volatile (
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+__success
+__xlated("0: r6 = *(u64 *)(r1 +0)")
+__xlated("1: r7 = *(u32 *)(r6 +0)")
+__xlated("2: w7 += 1000")
+__xlated("3: *(u32 *)(r6 +0) = r7")
+__xlated("4: r7 = r1")
+__xlated("5: r1 = r6")
+__xlated("6: call kernel-function")
+__xlated("7: r1 = r6")
+__xlated("8: call kernel-function")
+__xlated("9: r1 = r7")
+__xlated("10: r0 = 0")
+__xlated("11: exit")
+SEC("struct_ops/test_prologue")
+__naked int test_prologue(void)
+{
+	asm volatile (
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+struct st_ops_args;
+int bpf_kfunc_st_ops_test_prologue(struct st_ops_args *args) __ksym;
+
+SEC("syscall")
+__retval(1110)
+int syscall_prologue(void *ctx)
+{
+	struct st_ops_args args = {};
+	bpf_kfunc_st_ops_test_prologue(&args);
+	return args.a;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops st_ops = {
+	.test_epilogue = (void *)test_epilogue,
+	.test_prologue = (void *)test_prologue,
+};
-- 
2.45.2

Re: [RFC PATCH bpf-next 3/6] selftests/test: test gen_prologue and gen_epilogue

[Martin KaFai Lau]

On 8/16/24 1:27 PM, Eduard Zingerman wrote:
> On Fri, 2024-08-16 at 10:27 -0700, Martin KaFai Lau wrote:
> 
> [...]
> 
>> Thanks for checking!
>>
>> I think the bpf_map__attach_struct_ops() is not done such that st_ops is NULL.
>>
>> It probably needs another tag in the SEC("syscall") program to tell which st_ops
>> map should be attached first before executing the "syscall" program.
>>
>> I like the idea of using the __xlated macro to check the patched prologue, ctx
>> pointer saving, and epilogue. I will add this test in the respin. I will keep
>> the current way in this patch to exercise syscall and the ops/func in st_ops for
>> now. We can iterate on it later and use it as an example on what supports are
>> needed on the test_loader side for st_ops map testing. On the repetitive-enough
>> to worth test_loader refactoring side, I suspect some of the existing st_ops
>> load-success/load-failure tests may be worth to look at also. Thoughts?
> 
> You are correct, this happens because bpf_map__attach_struct_ops() is
> not called. Fortunately, the change for test_loader.c is not very big.
> Please check two patches in the attachment.

The patch looks good. I tried and it works. I will add it in the next respin.
That will help to cover the __xlated check on the instructions generated by 
gen_pro/epilogue and also check the syscall return value for the common case.

Except the tail_call test which needs to load a struct_ops program that does 
bpf_tail_call and another struct_ops program that was used in the prog_array. 
These two struct_ops programs need to be used in two separate struct_ops maps to 
be able to load. The way that test_loader attaching all maps in your patch will 
fail because bpf_testmod does not support attaching more than one struct_ops map.

I don't want to further polish on the tail_call testing side. I will stay with 
the current way to do the tail_call test which also allows the more regular 
trampoline "unsigned long long *ctx" for the main struct_ops prog and also 
allows using ctx_in in the SEC("syscall") prog.

Thanks.

[RFC PATCH bpf-next 4/6] bpf: Add module parameter to gen_prologue and gen_epilogue

[Martin KaFai Lau]

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

This patch adds a "struct module **module" arg to the .gen_prologue
and .gen_epilogue. This will allow the .gen_pro/epilogue to
make kfunc call because the verifer needs to know the kfunc's BTF.
The next patch will figure the kfunc's BTF from the module.
It also exposes the "btf_get_module_btf" function to help
figuring out the btf from a module.

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 include/linux/bpf.h                                   | 4 ++--
 include/linux/btf.h                                   | 1 +
 kernel/bpf/btf.c                                      | 2 +-
 kernel/bpf/cgroup.c                                   | 3 ++-
 kernel/bpf/verifier.c                                 | 4 ++--
 net/core/filter.c                                     | 6 +++---
 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 4 ++--
 7 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 2de67bc497f4..9787532813e2 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -973,9 +973,9 @@ struct bpf_verifier_ops {
 				const struct bpf_prog *prog,
 				struct bpf_insn_access_aux *info);
 	int (*gen_prologue)(struct bpf_insn *insn, bool direct_write,
-			    const struct bpf_prog *prog);
+			    const struct bpf_prog *prog, struct module **module);
 	int (*gen_epilogue)(struct bpf_insn *insn, const struct bpf_prog *prog,
-			    s16 ctx_stack_off);
+			    s16 ctx_stack_off, struct module **module);
 	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/btf.h b/include/linux/btf.h
index cffb43133c68..177187fa3819 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -580,6 +580,7 @@ bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
 int get_kern_ctx_btf_id(struct bpf_verifier_log *log, enum bpf_prog_type prog_type);
 bool btf_types_are_same(const struct btf *btf1, u32 id1,
 			const struct btf *btf2, u32 id2);
+struct btf *btf_get_module_btf(const struct module *module);
 #else
 static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
 						    u32 type_id)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 95426d5b634e..b230f7ff9388 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7970,7 +7970,7 @@ struct module *btf_try_get_module(const struct btf *btf)
 /* Returns struct btf corresponding to the struct module.
  * This function can return NULL or ERR_PTR.
  */
-static struct btf *btf_get_module_btf(const struct module *module)
+struct btf *btf_get_module_btf(const struct module *module)
 {
 #ifdef CONFIG_DEBUG_INFO_BTF_MODULES
 	struct btf_module *btf_mod, *tmp;
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 8ba73042a239..0be053d86b56 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -2503,7 +2503,8 @@ static u32 cg_sockopt_convert_ctx_access(enum bpf_access_type type,
 
 static int cg_sockopt_get_prologue(struct bpf_insn *insn_buf,
 				   bool direct_write,
-				   const struct bpf_prog *prog)
+				   const struct bpf_prog *prog,
+				   struct module **module)
 {
 	/* Nothing to do for sockopt argument. The data is kzalloc'ated.
 	 */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index bbb655f0c7b5..5e995b7884fb 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19622,7 +19622,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 
 	if (ops->gen_epilogue) {
 		epilogue_cnt = ops->gen_epilogue(epilogue_buf, env->prog,
-						 -(subprogs[0].stack_depth + 8));
+						 -(subprogs[0].stack_depth + 8), NULL);
 		if (epilogue_cnt >= ARRAY_SIZE(epilogue_buf)) {
 			verbose(env, "bpf verifier is misconfigured\n");
 			return -EINVAL;
@@ -19647,7 +19647,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 			return -EINVAL;
 		}
 		cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
-					env->prog);
+					env->prog, NULL);
 		if (cnt >= ARRAY_SIZE(insn_buf)) {
 			verbose(env, "bpf verifier is misconfigured\n");
 			return -EINVAL;
diff --git a/net/core/filter.c b/net/core/filter.c
index 78a6f746ea0b..65d219e71ae8 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -8844,7 +8844,7 @@ static bool sock_filter_is_valid_access(int off, int size,
 }
 
 static int bpf_noop_prologue(struct bpf_insn *insn_buf, bool direct_write,
-			     const struct bpf_prog *prog)
+			     const struct bpf_prog *prog, struct module **module)
 {
 	/* Neither direct read nor direct write requires any preliminary
 	 * action.
@@ -8927,7 +8927,7 @@ static int bpf_gen_ld_abs(const struct bpf_insn *orig,
 }
 
 static int tc_cls_act_prologue(struct bpf_insn *insn_buf, bool direct_write,
-			       const struct bpf_prog *prog)
+			       const struct bpf_prog *prog, struct module **module)
 {
 	return bpf_unclone_prologue(insn_buf, direct_write, prog, TC_ACT_SHOT);
 }
@@ -9263,7 +9263,7 @@ static bool sock_ops_is_valid_access(int off, int size,
 }
 
 static int sk_skb_prologue(struct bpf_insn *insn_buf, bool direct_write,
-			   const struct bpf_prog *prog)
+			   const struct bpf_prog *prog, struct module **module)
 {
 	return bpf_unclone_prologue(insn_buf, direct_write, prog, SK_DROP);
 }
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 7194330bdefc..4c75346376d9 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -1141,7 +1141,7 @@ static int bpf_test_mod_st_ops__test_pro_epilogue(struct st_ops_args *args)
 }
 
 static int st_ops_gen_prologue(struct bpf_insn *insn_buf, bool direct_write,
-			       const struct bpf_prog *prog)
+			       const struct bpf_prog *prog, struct module **module)
 {
 	struct bpf_insn *insn = insn_buf;
 
@@ -1164,7 +1164,7 @@ static int st_ops_gen_prologue(struct bpf_insn *insn_buf, bool direct_write,
 }
 
 static int st_ops_gen_epilogue(struct bpf_insn *insn_buf, const struct bpf_prog *prog,
-			       s16 ctx_stack_off)
+			       s16 ctx_stack_off, struct module **module)
 {
 	struct bpf_insn *insn = insn_buf;
 
-- 
2.43.5

[RFC PATCH bpf-next 5/6] bpf: Allow pro/epilogue to call kfunc

[Martin KaFai Lau]

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

The existing prologue has been able to call bpf helper but not a kfunc.
This patch allows the prologue/epilogue to call the kfunc.

The subsystem that implements the .gen_prologue and .gen_epilogue
can add the BPF_PSEUDO_KFUNC_CALL instruction with insn->imm
set to the btf func_id of the kfunc call. This part is the same
as the bpf prog loaded from the sys_bpf.

Another piece is to have a way for the subsystem to tell the btf object
of the kfunc func_id. This patch uses the "struct module **module"
argument added to the .gen_prologue and .gen_epilogue
in the previous patch. The verifier will use btf_get_module_btf(module)
to find out the btf object.

The .gen_epi/prologue will usually use THIS_MODULE to initialize
the "*module = THIS_MODULE". Only kfunc(s) from one module (or vmlinux)
can be used in the .gen_epi/prologue now. In the future, the
.gen_epi/prologue can return an array of modules and use the
insn->off as an index into the array.

When the returned module is NULL, the btf is btf_vmlinux. Then the
insn->off stays at 0. This is the same as the sys_bpf.

When the btf is from a module, the btf needs an entry in
prog->aux->kfunc_btf_tab. The kfunc_btf_tab is currently
sorted by insn->off which is the offset to the attr->fd_array.

This module btf may or may not be in the kfunc_btf_tab. A new function
"find_kfunc_desc_btf_offset" is added to search for the existing entry
that has the same btf. If it is found, its offset will be used in
the insn->off. If it is not found, it will find an offset value
that is not used in the kfunc_btf_tab. Add a new entry
to kfunc_btf_tab and set this new offset to the insn->off

Once the insn->off is determined (either reuse an existing one
or an unused one is found), it will call the existing add_kfunc_call()
and everything else should fall through.

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

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5e995b7884fb..2873e1083402 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2787,6 +2787,61 @@ static struct btf *find_kfunc_desc_btf(struct bpf_verifier_env *env, s16 offset)
 	return btf_vmlinux ?: ERR_PTR(-ENOENT);
 }
 
+static int find_kfunc_desc_btf_offset(struct bpf_verifier_env *env, struct btf *btf,
+				      struct module *module, s16 *offset)
+{
+	struct bpf_kfunc_btf_tab *tab;
+	struct bpf_kfunc_btf *b;
+	s16 new_offset = S16_MAX;
+	u32 i;
+
+	if (btf_is_vmlinux(btf)) {
+		*offset = 0;
+		return 0;
+	}
+
+	tab = env->prog->aux->kfunc_btf_tab;
+	if (!tab) {
+		tab = kzalloc(sizeof(*tab), GFP_KERNEL);
+		if (!tab)
+			return -ENOMEM;
+		env->prog->aux->kfunc_btf_tab = tab;
+	}
+
+	b = tab->descs;
+	for (i = tab->nr_descs; i > 0; i--) {
+		if (b[i - 1].btf == btf) {
+			*offset = b[i - 1].offset;
+			return 0;
+		}
+		/* Search new_offset from backward S16_MAX, S16_MAX-1, ...
+		 * tab->nr_descs max out at MAX_KFUNC_BTFS which is
+		 * smaller than S16_MAX, so it will be able to find
+		 * a non-zero new_offset to use.
+		 */
+		if (new_offset == b[i - 1].offset)
+			new_offset--;
+	}
+
+	if (tab->nr_descs == MAX_KFUNC_BTFS) {
+		verbose(env, "too many different module BTFs\n");
+		return -E2BIG;
+	}
+
+	if (!try_module_get(module))
+		return -ENXIO;
+
+	b = &tab->descs[tab->nr_descs++];
+	btf_get(btf);
+	b->btf = btf;
+	b->module = module;
+	b->offset = new_offset;
+	*offset = new_offset;
+	sort(tab->descs, tab->nr_descs, sizeof(tab->descs[0]),
+	     kfunc_btf_cmp_by_off, NULL);
+	return 0;
+}
+
 static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
 {
 	const struct btf_type *func, *func_proto;
@@ -19603,6 +19658,50 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
 	return 0;
 }
 
+static int fixup_pro_epilogue_kfunc(struct bpf_verifier_env *env, struct bpf_insn *insns,
+				    int cnt, struct module *module)
+{
+	struct btf *btf;
+	u32 func_id;
+	int i, err;
+	s16 offset;
+
+	for (i = 0; i < cnt; i++) {
+		if (!bpf_pseudo_kfunc_call(&insns[i]))
+			continue;
+
+		/* The kernel may not have BTF available, so only
+		 * try to get a btf if the pro/epilogue calls a kfunc.
+		 */
+		btf = btf_get_module_btf(module);
+		if (IS_ERR_OR_NULL(btf)) {
+			verbose(env, "cannot find BTF from %s for kfunc used in pro/epilogue\n",
+				module_name(module));
+			return -EINVAL;
+		}
+
+		func_id = insns[i].imm;
+		if (btf_is_vmlinux(btf) &&
+		    btf_id_set_contains(&special_kfunc_set, func_id)) {
+			verbose(env, "pro/epilogue cannot use special kfunc\n");
+			btf_put(btf);
+			return -EINVAL;
+		}
+
+		err = find_kfunc_desc_btf_offset(env, btf, module, &offset);
+		btf_put(btf);
+		if (err)
+			return err;
+
+		insns[i].off = offset;
+		err = add_kfunc_call(env, func_id, offset);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 /* convert load instructions that access fields of a context type into a
  * sequence of instructions that access fields of the underlying structure:
  *     struct __sk_buff    -> struct sk_buff
@@ -19612,21 +19711,27 @@ 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, epilogue_cnt = 0;
+	int err, i, cnt, size, ctx_field_size, delta = 0, epilogue_cnt = 0;
 	const int insn_cnt = env->prog->len;
 	struct bpf_insn insn_buf[16], epilogue_buf[16], *insn;
 	u32 target_size, size_default, off;
 	struct bpf_prog *new_prog;
 	enum bpf_access_type type;
 	bool is_narrower_load;
+	struct module *module;
 
 	if (ops->gen_epilogue) {
+		module = NULL;
 		epilogue_cnt = ops->gen_epilogue(epilogue_buf, env->prog,
-						 -(subprogs[0].stack_depth + 8), NULL);
+						 -(subprogs[0].stack_depth + 8), &module);
 		if (epilogue_cnt >= ARRAY_SIZE(epilogue_buf)) {
 			verbose(env, "bpf verifier is misconfigured\n");
 			return -EINVAL;
 		} else if (epilogue_cnt) {
+			err = fixup_pro_epilogue_kfunc(env, epilogue_buf, epilogue_cnt, module);
+			if (err)
+				return err;
+
 			/* Save the ARG_PTR_TO_CTX for the epilogue to use */
 			cnt = 0;
 			subprogs[0].stack_depth += 8;
@@ -19646,12 +19751,17 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 			verbose(env, "bpf verifier is misconfigured\n");
 			return -EINVAL;
 		}
+		module = NULL;
 		cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
-					env->prog, NULL);
+					env->prog, &module);
 		if (cnt >= ARRAY_SIZE(insn_buf)) {
 			verbose(env, "bpf verifier is misconfigured\n");
 			return -EINVAL;
 		} else if (cnt) {
+			err = fixup_pro_epilogue_kfunc(env, insn_buf, cnt, module);
+			if (err)
+				return err;
+
 			new_prog = bpf_patch_insn_data(env, 0, insn_buf, cnt);
 			if (!new_prog)
 				return -ENOMEM;
-- 
2.43.5

Re: [RFC PATCH bpf-next 5/6] bpf: Allow pro/epilogue to call kfunc

[Eduard Zingerman]

On Tue, 2024-08-13 at 11:49 -0700, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> The existing prologue has been able to call bpf helper but not a kfunc.
> This patch allows the prologue/epilogue to call the kfunc.

[...]

> Once the insn->off is determined (either reuse an existing one
> or an unused one is found), it will call the existing add_kfunc_call()
> and everything else should fall through.
> 
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---

fwiw, don't see any obvious problems with this patch.

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

>  kernel/bpf/verifier.c | 116 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 113 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5e995b7884fb..2873e1083402 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2787,6 +2787,61 @@ static struct btf *find_kfunc_desc_btf(struct bpf_verifier_env *env, s16 offset)
>  	return btf_vmlinux ?: ERR_PTR(-ENOENT);
>  }
>  
> +static int find_kfunc_desc_btf_offset(struct bpf_verifier_env *env, struct btf *btf,
> +				      struct module *module, s16 *offset)
> +{
> +	struct bpf_kfunc_btf_tab *tab;
> +	struct bpf_kfunc_btf *b;
> +	s16 new_offset = S16_MAX;
> +	u32 i;
> +
> +	if (btf_is_vmlinux(btf)) {
> +		*offset = 0;
> +		return 0;
> +	}
> +
> +	tab = env->prog->aux->kfunc_btf_tab;
> +	if (!tab) {
> +		tab = kzalloc(sizeof(*tab), GFP_KERNEL);
> +		if (!tab)
> +			return -ENOMEM;
> +		env->prog->aux->kfunc_btf_tab = tab;
> +	}
> +
> +	b = tab->descs;
> +	for (i = tab->nr_descs; i > 0; i--) {

Question: why iterating in reverse here?

> +		if (b[i - 1].btf == btf) {
> +			*offset = b[i - 1].offset;
> +			return 0;
> +		}
> +		/* Search new_offset from backward S16_MAX, S16_MAX-1, ...
> +		 * tab->nr_descs max out at MAX_KFUNC_BTFS which is
> +		 * smaller than S16_MAX, so it will be able to find
> +		 * a non-zero new_offset to use.
> +		 */
> +		if (new_offset == b[i - 1].offset)
> +			new_offset--;
> +	}
> +
> +	if (tab->nr_descs == MAX_KFUNC_BTFS) {
> +		verbose(env, "too many different module BTFs\n");
> +		return -E2BIG;
> +	}
> +
> +	if (!try_module_get(module))
> +		return -ENXIO;
> +
> +	b = &tab->descs[tab->nr_descs++];
> +	btf_get(btf);
> +	b->btf = btf;
> +	b->module = module;
> +	b->offset = new_offset;
> +	*offset = new_offset;
> +	sort(tab->descs, tab->nr_descs, sizeof(tab->descs[0]),
> +	     kfunc_btf_cmp_by_off, NULL);
> +	return 0;
> +}
> +
>  static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
>  {
>  	const struct btf_type *func, *func_proto;

[...]

Re: [RFC PATCH bpf-next 5/6] bpf: Allow pro/epilogue to call kfunc

[Martin KaFai Lau]

On 8/14/24 3:17 PM, Eduard Zingerman wrote:
> On Tue, 2024-08-13 at 11:49 -0700, Martin KaFai Lau wrote:
>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>
>> The existing prologue has been able to call bpf helper but not a kfunc.
>> This patch allows the prologue/epilogue to call the kfunc.
> 
> [...]
> 
>> Once the insn->off is determined (either reuse an existing one
>> or an unused one is found), it will call the existing add_kfunc_call()
>> and everything else should fall through.
>>
>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>> ---
> 
> fwiw, don't see any obvious problems with this patch.
> 
> Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>
> 
>>   kernel/bpf/verifier.c | 116 ++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 113 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 5e995b7884fb..2873e1083402 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -2787,6 +2787,61 @@ static struct btf *find_kfunc_desc_btf(struct bpf_verifier_env *env, s16 offset)
>>   	return btf_vmlinux ?: ERR_PTR(-ENOENT);
>>   }
>>   
>> +static int find_kfunc_desc_btf_offset(struct bpf_verifier_env *env, struct btf *btf,
>> +				      struct module *module, s16 *offset)
>> +{
>> +	struct bpf_kfunc_btf_tab *tab;
>> +	struct bpf_kfunc_btf *b;
>> +	s16 new_offset = S16_MAX;
>> +	u32 i;
>> +
>> +	if (btf_is_vmlinux(btf)) {
>> +		*offset = 0;
>> +		return 0;
>> +	}
>> +
>> +	tab = env->prog->aux->kfunc_btf_tab;
>> +	if (!tab) {
>> +		tab = kzalloc(sizeof(*tab), GFP_KERNEL);
>> +		if (!tab)
>> +			return -ENOMEM;
>> +		env->prog->aux->kfunc_btf_tab = tab;
>> +	}
>> +
>> +	b = tab->descs;
>> +	for (i = tab->nr_descs; i > 0; i--) {
> 
> Question: why iterating in reverse here?

Agreed. It is unnecessary. I will change it to iterate forward in the next re-spin.

Thanks for the review!

[RFC PATCH bpf-next 6/6] selftests/bpf: Add kfunc call test in gen_prologue and gen_epilogue

[Martin KaFai Lau]

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

This patch changes the .gen_pro/epilogue of the bpf_testmod_st_ops
to call kfunc. It will call the inc10 and inc100 kfunc.
The value of the PROLOGUE_A and EPILOGUE_A macro are adjusted
to reflect this change also.

The inc100 kfunc is newly added in this patch which does
args->a += 100. Note that it is not in the register_btf_kfunc_id_set(),
so no need to declare in the bpf_testmod_kfunc.h.
It is enclosed with __bpf_kfunc_{start,edn}_defs to avoid the
compiler warning.

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 42 ++++++++++++++++++-
 .../bpf/prog_tests/struct_ops_syscall.c       |  5 ++-
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 4c75346376d9..6f745d29e124 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -966,6 +966,16 @@ __bpf_kfunc int bpf_kfunc_st_ops_inc10(struct st_ops_args *args)
 	return args->a;
 }
 
+__bpf_kfunc_start_defs();
+
+__bpf_kfunc int bpf_kfunc_st_ops_inc100(struct st_ops_args *args)
+{
+	args->a += 100;
+	return args->a;
+}
+
+__bpf_kfunc_end_defs();
+
 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)
@@ -1140,6 +1150,10 @@ static int bpf_test_mod_st_ops__test_pro_epilogue(struct st_ops_args *args)
 	return 0;
 }
 
+BTF_ID_LIST(st_ops_epilogue_kfunc_list)
+BTF_ID(func, bpf_kfunc_st_ops_inc10)
+BTF_ID(func, bpf_kfunc_st_ops_inc100)
+
 static int st_ops_gen_prologue(struct bpf_insn *insn_buf, bool direct_write,
 			       const struct bpf_prog *prog, struct module **module)
 {
@@ -1153,13 +1167,28 @@ static int st_ops_gen_prologue(struct bpf_insn *insn_buf, bool direct_write,
 	 * r7 = r6->a;
 	 * r7 += 1000;
 	 * r6->a = r7;
+	 * r7 = r1;
+	 * r1 = r6;
+	 * bpf_kfunc_st_ops_in10(r1)
+	 * r1 = r6;
+	 * bpf_kfunc_st_ops_in100(r1)
+	 * r1 = 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++ = BPF_MOV64_REG(BPF_REG_7, BPF_REG_1);
+	*insn++ = BPF_MOV64_REG(BPF_REG_1, BPF_REG_6);
+	*insn++ = BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0,
+			       st_ops_epilogue_kfunc_list[0]);
+	*insn++ = BPF_MOV64_REG(BPF_REG_1, BPF_REG_6);
+	*insn++ = BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0,
+			       st_ops_epilogue_kfunc_list[1]);
+	*insn++ = BPF_MOV64_REG(BPF_REG_1, BPF_REG_7);
 	*insn++ = prog->insnsi[0];
 
+	*module = THIS_MODULE;
 	return insn - insn_buf;
 }
 
@@ -1177,7 +1206,10 @@ static int st_ops_gen_epilogue(struct bpf_insn *insn_buf, const struct bpf_prog
 	 * r6 = r1->a;
 	 * r6 += 10000;
 	 * r1->a = r6;
-	 * r0 = r6;
+	 * r6 = r1;
+	 * bpf_kfunc_st_ops_in10(r1)
+	 * r1 = r6;
+	 * bpf_kfunc_st_ops_in100(r1)
 	 * r0 *= 2;
 	 * BPF_EXIT;
 	 */
@@ -1186,10 +1218,16 @@ static int st_ops_gen_epilogue(struct bpf_insn *insn_buf, const struct bpf_prog
 	*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_MOV64_REG(BPF_REG_6, BPF_REG_1);
+	*insn++ = BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0,
+			       st_ops_epilogue_kfunc_list[0]);
+	*insn++ = BPF_MOV64_REG(BPF_REG_1, BPF_REG_6);
+	*insn++ = BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0,
+			       st_ops_epilogue_kfunc_list[1]);
 	*insn++ = BPF_ALU32_IMM(BPF_MUL, BPF_REG_0, 2);
 	*insn++ = BPF_EXIT_INSN();
 
+	*module = THIS_MODULE;
 	return insn - insn_buf;
 }
 
diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_syscall.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_syscall.c
index a293a35b0dcc..2a73066adbf5 100644
--- a/tools/testing/selftests/bpf/prog_tests/struct_ops_syscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_syscall.c
@@ -3,10 +3,11 @@
 #include <test_progs.h>
 #include "struct_ops_syscall.skel.h"
 
-#define EPILOGUE_A  10000
-#define PROLOGUE_A   1000
+#define KFUNC_A100    100
 #define KFUNC_A10      10
 #define SUBPROG_A       1
+#define EPILOGUE_A (10000 + KFUNC_A100 + KFUNC_A10)
+#define PROLOGUE_A  (1000 + KFUNC_A100 + KFUNC_A10)
 
 #define SUBPROG_TEST_MAIN	SUBPROG_A
 #define KFUNC_TEST_MAIN		(KFUNC_A10 + SUBPROG_A)
-- 
2.43.5