[PATCH bpf-next v3 0/7] bpf: Support struct argument for trampoline base progs

[PATCH bpf-next v3 0/7] bpf: Support struct argument for trampoline base progs

[Yonghong Song]

Currently struct arguments are not supported for trampoline based progs.
One of major reason is that struct argument may pass by value which may
use more than one registers. This breaks trampoline progs where
each argument is assumed to take one register. bcc community reported the
issue ([1]) where struct argument is not supported for fentry program.
  typedef struct {
        uid_t val;
  } kuid_t;
  typedef struct {
        gid_t val;
  } kgid_t;
  int security_path_chown(struct path *path, kuid_t uid, kgid_t gid);
Inside Meta, we also have a use case to attach to tcp_setsockopt()
  typedef struct {
        union { 
                void            *kernel;
                void __user     *user;
        };
        bool            is_kernel : 1;
  } sockptr_t;
  int tcp_setsockopt(struct sock *sk, int level, int optname,
                     sockptr_t optval, unsigned int optlen);

This patch added struct value support for bpf tracing programs which
uses trampoline. Only <= 16 byte struct size is supported for now
which covers use cases in the above. For x86/arm64/bpf, <= 16
struct value will be passed in registers instead of by reference.
Only x86_64 is supported in this patch. arm64 support can be
added later.

 [1] https://github.com/iovisor/bcc/issues/3657

Changelog:
  v2 -> v3:
   - previously struct arguments (<= 16 bytes) are passed
     by reference for bpf programs. Suggested by Alexei,
     it is passed by value now.
   - in order to support passing <= 16 struct value, a
     new macro BPF_PROG2 is invented.
  rfc v1 -> v2:
   - changed bpf_func_model struct info fields to
     arg_flags[] to make it easy to iterate arguments
     in arch specific {save|restore}_regs() functions.
   - added fexit tests to test return values with
     struct arguments.

Yonghong Song (7):
  bpf: Allow struct argument in trampoline based programs
  bpf: x86: Support in-register struct arguments in trampoline programs
  bpf: Update descriptions for helpers bpf_get_func_arg[_cnt]()
  bpf: arm64: No support of struct argument in trampoline programs
  libbpf: Add new BPF_PROG2 macro
  selftests/bpf: Add struct argument tests with fentry/fexit programs.
  selftests/bpf: Use BPF_PROG2 for some fentry programs without struct
    arguments

 arch/arm64/net/bpf_jit_comp.c                 |   8 +-
 arch/x86/net/bpf_jit_comp.c                   |  68 ++++++++---
 include/linux/bpf.h                           |   4 +
 include/uapi/linux/bpf.h                      |   9 +-
 kernel/bpf/btf.c                              |  42 ++++++-
 tools/include/uapi/linux/bpf.h                |   9 +-
 tools/lib/bpf/bpf_tracing.h                   |  82 +++++++++++++
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  48 ++++++++
 .../selftests/bpf/prog_tests/tracing_struct.c |  63 ++++++++++
 tools/testing/selftests/bpf/progs/timer.c     |   4 +-
 .../selftests/bpf/progs/tracing_struct.c      | 114 ++++++++++++++++++
 11 files changed, 417 insertions(+), 34 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/tracing_struct.c
 create mode 100644 tools/testing/selftests/bpf/progs/tracing_struct.c

-- 
2.30.2

[PATCH bpf-next v3 1/7] bpf: Allow struct argument in trampoline based programs

[Yonghong Song]

Allow struct argument in trampoline based programs where
the struct size should be <= 16 bytes. In such cases, the argument
will be put into up to 2 registers for bpf, x86_64 and arm64
architectures.

To support arch-specific trampoline manipulation,
add arg_flags for additional struct information about arguments
in btf_func_model. Such information will be used in arch specific
function arch_prepare_bpf_trampoline() to prepare argument access
properly in trampoline.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/linux/bpf.h |  4 ++++
 kernel/bpf/btf.c    | 42 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 9c1674973e03..4d32f125f4af 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -727,10 +727,14 @@ enum bpf_cgroup_storage_type {
  */
 #define MAX_BPF_FUNC_REG_ARGS 5
 
+/* The argument is a structure. */
+#define BTF_FMODEL_STRUCT_ARG		BIT(0)
+
 struct btf_func_model {
 	u8 ret_size;
 	u8 nr_args;
 	u8 arg_size[MAX_BPF_FUNC_ARGS];
+	u8 arg_flags[MAX_BPF_FUNC_ARGS];
 };
 
 /* Restore arguments before returning from trampoline to let original function
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 903719b89238..4a081bfb4c8a 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5328,6 +5328,31 @@ static bool is_int_ptr(struct btf *btf, const struct btf_type *t)
 	return btf_type_is_int(t);
 }
 
+static u32 get_ctx_arg_idx(struct btf *btf, const struct btf_type *func_proto,
+			   int off)
+{
+	const struct btf_param *args;
+	const struct btf_type *t;
+	u32 offset = 0, nr_args;
+	int i;
+
+	nr_args = btf_type_vlen(func_proto);
+	args = (const struct btf_param *)(func_proto + 1);
+	for (i = 0; i < nr_args; i++) {
+		t = btf_type_skip_modifiers(btf, args[i].type, NULL);
+		offset += btf_type_is_ptr(t) ? 8 : roundup(t->size, 8);
+		if (off < offset)
+			return i;
+	}
+
+	t = btf_type_skip_modifiers(btf, func_proto->type, NULL);
+	offset += btf_type_is_ptr(t) ? 8 : roundup(t->size, 8);
+	if (off < offset)
+		return nr_args;
+
+	return nr_args + 1;
+}
+
 bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		    const struct bpf_prog *prog,
 		    struct bpf_insn_access_aux *info)
@@ -5347,7 +5372,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 			tname, off);
 		return false;
 	}
-	arg = off / 8;
+	arg = t == NULL ? (off / 8) :  get_ctx_arg_idx(btf, t, off);
 	args = (const struct btf_param *)(t + 1);
 	/* if (t == NULL) Fall back to default BPF prog with
 	 * MAX_BPF_FUNC_REG_ARGS u64 arguments.
@@ -5417,7 +5442,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	/* skip modifiers */
 	while (btf_type_is_modifier(t))
 		t = btf_type_by_id(btf, t->type);
-	if (btf_type_is_small_int(t) || btf_is_any_enum(t))
+	if (btf_type_is_small_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
 		/* accessing a scalar */
 		return true;
 	if (!btf_type_is_ptr(t)) {
@@ -5881,7 +5906,7 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
 	if (btf_type_is_ptr(t))
 		/* kernel size of pointer. Not BPF's size of pointer*/
 		return sizeof(void *);
-	if (btf_type_is_int(t) || btf_is_any_enum(t))
+	if (btf_type_is_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
 		return t->size;
 	return -EINVAL;
 }
@@ -5901,8 +5926,10 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 		/* BTF function prototype doesn't match the verifier types.
 		 * Fall back to MAX_BPF_FUNC_REG_ARGS u64 args.
 		 */
-		for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++)
+		for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
 			m->arg_size[i] = 8;
+			m->arg_flags[i] = 0;
+		}
 		m->ret_size = 8;
 		m->nr_args = MAX_BPF_FUNC_REG_ARGS;
 		return 0;
@@ -5916,7 +5943,7 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 		return -EINVAL;
 	}
 	ret = __get_type_size(btf, func->type, &t);
-	if (ret < 0) {
+	if (ret < 0 || __btf_type_is_struct(t)) {
 		bpf_log(log,
 			"The function %s return type %s is unsupported.\n",
 			tname, btf_kind_str[BTF_INFO_KIND(t->info)]);
@@ -5932,7 +5959,9 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 			return -EINVAL;
 		}
 		ret = __get_type_size(btf, args[i].type, &t);
-		if (ret < 0) {
+
+		/* No support of struct argument size greater than 16 bytes */
+		if (ret < 0 || ret > 16) {
 			bpf_log(log,
 				"The function %s arg%d type %s is unsupported.\n",
 				tname, i, btf_kind_str[BTF_INFO_KIND(t->info)]);
@@ -5945,6 +5974,7 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 			return -EINVAL;
 		}
 		m->arg_size[i] = ret;
+		m->arg_flags[i] = __btf_type_is_struct(t) ? BTF_FMODEL_STRUCT_ARG : 0;
 	}
 	m->nr_args = nargs;
 	return 0;
-- 
2.30.2

Re: [PATCH bpf-next v3 1/7] bpf: Allow struct argument in trampoline based programs

[Jiri Olsa]

On Sat, Aug 27, 2022 at 07:54:43PM -0700, Yonghong Song wrote:

SNIP

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 903719b89238..4a081bfb4c8a 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -5328,6 +5328,31 @@ static bool is_int_ptr(struct btf *btf, const struct btf_type *t)
>  	return btf_type_is_int(t);
>  }
>  
> +static u32 get_ctx_arg_idx(struct btf *btf, const struct btf_type *func_proto,
> +			   int off)
> +{
> +	const struct btf_param *args;
> +	const struct btf_type *t;
> +	u32 offset = 0, nr_args;
> +	int i;
> +
> +	nr_args = btf_type_vlen(func_proto);
> +	args = (const struct btf_param *)(func_proto + 1);
> +	for (i = 0; i < nr_args; i++) {
> +		t = btf_type_skip_modifiers(btf, args[i].type, NULL);
> +		offset += btf_type_is_ptr(t) ? 8 : roundup(t->size, 8);
> +		if (off < offset)
> +			return i;
> +	}
> +
> +	t = btf_type_skip_modifiers(btf, func_proto->type, NULL);
> +	offset += btf_type_is_ptr(t) ? 8 : roundup(t->size, 8);
> +	if (off < offset)
> +		return nr_args;
> +
> +	return nr_args + 1;
> +}
> +
>  bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>  		    const struct bpf_prog *prog,
>  		    struct bpf_insn_access_aux *info)
> @@ -5347,7 +5372,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>  			tname, off);
>  		return false;
>  	}
> -	arg = off / 8;
> +	arg = t == NULL ? (off / 8) :  get_ctx_arg_idx(btf, t, off);

is the t == NULL check needed? we relied on t being defined later in the code

jirka

>  	args = (const struct btf_param *)(t + 1);
>  	/* if (t == NULL) Fall back to default BPF prog with
>  	 * MAX_BPF_FUNC_REG_ARGS u64 arguments.
> @@ -5417,7 +5442,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>  	/* skip modifiers */
>  	while (btf_type_is_modifier(t))
>  		t = btf_type_by_id(btf, t->type);
> -	if (btf_type_is_small_int(t) || btf_is_any_enum(t))
> +	if (btf_type_is_small_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
>  		/* accessing a scalar */
>  		return true;
>  	if (!btf_type_is_ptr(t)) {
> @@ -5881,7 +5906,7 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
>  	if (btf_type_is_ptr(t))
>  		/* kernel size of pointer. Not BPF's size of pointer*/
>  		return sizeof(void *);
> -	if (btf_type_is_int(t) || btf_is_any_enum(t))
> +	if (btf_type_is_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
>  		return t->size;
>  	return -EINVAL;

SNIP

Re: [PATCH bpf-next v3 1/7] bpf: Allow struct argument in trampoline based programs

[Yonghong Song]

On 8/30/22 5:11 AM, Jiri Olsa wrote:
> On Sat, Aug 27, 2022 at 07:54:43PM -0700, Yonghong Song wrote:
> 
> SNIP
> 
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 903719b89238..4a081bfb4c8a 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -5328,6 +5328,31 @@ static bool is_int_ptr(struct btf *btf, const struct btf_type *t)
>>   	return btf_type_is_int(t);
>>   }
>>   
>> +static u32 get_ctx_arg_idx(struct btf *btf, const struct btf_type *func_proto,
>> +			   int off)
>> +{
>> +	const struct btf_param *args;
>> +	const struct btf_type *t;
>> +	u32 offset = 0, nr_args;
>> +	int i;
>> +
>> +	nr_args = btf_type_vlen(func_proto);
>> +	args = (const struct btf_param *)(func_proto + 1);
>> +	for (i = 0; i < nr_args; i++) {
>> +		t = btf_type_skip_modifiers(btf, args[i].type, NULL);
>> +		offset += btf_type_is_ptr(t) ? 8 : roundup(t->size, 8);
>> +		if (off < offset)
>> +			return i;
>> +	}
>> +
>> +	t = btf_type_skip_modifiers(btf, func_proto->type, NULL);
>> +	offset += btf_type_is_ptr(t) ? 8 : roundup(t->size, 8);
>> +	if (off < offset)
>> +		return nr_args;
>> +
>> +	return nr_args + 1;
>> +}
>> +
>>   bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>>   		    const struct bpf_prog *prog,
>>   		    struct bpf_insn_access_aux *info)
>> @@ -5347,7 +5372,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>>   			tname, off);
>>   		return false;
>>   	}
>> -	arg = off / 8;
>> +	arg = t == NULL ? (off / 8) :  get_ctx_arg_idx(btf, t, off);
> 
> is the t == NULL check needed? we relied on t being defined later in the code

t is defined earlier in
    const struct btf_type *t = prog->aux->attach_func_proto;
so we should be fine.

I guess I will fold
	if (!t)
		return off / 8;
inside get_ctx_arg_idx() to make interface cleaner.
Will do this in the next revision.

> 
> jirka
> 
>>   	args = (const struct btf_param *)(t + 1);
>>   	/* if (t == NULL) Fall back to default BPF prog with
>>   	 * MAX_BPF_FUNC_REG_ARGS u64 arguments.
>> @@ -5417,7 +5442,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>>   	/* skip modifiers */
>>   	while (btf_type_is_modifier(t))
>>   		t = btf_type_by_id(btf, t->type);
>> -	if (btf_type_is_small_int(t) || btf_is_any_enum(t))
>> +	if (btf_type_is_small_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
>>   		/* accessing a scalar */
>>   		return true;
>>   	if (!btf_type_is_ptr(t)) {
>> @@ -5881,7 +5906,7 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
>>   	if (btf_type_is_ptr(t))
>>   		/* kernel size of pointer. Not BPF's size of pointer*/
>>   		return sizeof(void *);
>> -	if (btf_type_is_int(t) || btf_is_any_enum(t))
>> +	if (btf_type_is_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
>>   		return t->size;
>>   	return -EINVAL;
> 
> SNIP

[PATCH bpf-next v3 2/7] bpf: x86: Support in-register struct arguments in trampoline programs

[Yonghong Song]

In C, struct value can be passed as a function argument.
For small structs, struct value may be passed in
one or more registers. For trampoline based bpf programs,
This would cause complication since one-to-one mapping between
function argument and arch argument register is not valid
any more.

The latest llvm16 added bpf support to pass by values
for struct up to 16 bytes ([1]). This is also true for
x86_64 architecture where two registers will hold
the struct value if the struct size is >8 and <= 16.
This may not be true if one of struct member is 'double'
type but in current linux source code we don't have
such instance yet, so we assume all >8 && <= 16 struct
holds two general purpose argument registers.

Also change on-stack nr_args value to the number
of registers holding the arguments. This will
permit bpf_get_func_arg() helper to get all
argument values.

 [1] https://reviews.llvm.org/D132144

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 arch/x86/net/bpf_jit_comp.c | 68 +++++++++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 17 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index c1f6c1c51d99..ae89f4143eb4 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1751,34 +1751,60 @@ st:			if (is_imm8(insn->off))
 static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
 		      int stack_size)
 {
-	int i;
+	int i, j, arg_size, nr_regs;
 	/* Store function arguments to stack.
 	 * For a function that accepts two pointers the sequence will be:
 	 * mov QWORD PTR [rbp-0x10],rdi
 	 * mov QWORD PTR [rbp-0x8],rsi
 	 */
-	for (i = 0; i < min(nr_args, 6); i++)
-		emit_stx(prog, bytes_to_bpf_size(m->arg_size[i]),
-			 BPF_REG_FP,
-			 i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
-			 -(stack_size - i * 8));
+	for (i = 0, j = 0; i < min(nr_args, 6); i++) {
+		if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG) {
+			nr_regs = (m->arg_size[i] + 7) / 8;
+			arg_size = 8;
+		} else {
+			nr_regs = 1;
+			arg_size = m->arg_size[i];
+		}
+
+		while (nr_regs) {
+			emit_stx(prog, bytes_to_bpf_size(arg_size),
+				 BPF_REG_FP,
+				 j == 5 ? X86_REG_R9 : BPF_REG_1 + j,
+				 -(stack_size - j * 8));
+			nr_regs--;
+			j++;
+		}
+	}
 }
 
 static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
 			 int stack_size)
 {
-	int i;
+	int i, j, arg_size, nr_regs;
 
 	/* Restore function arguments from stack.
 	 * For a function that accepts two pointers the sequence will be:
 	 * EMIT4(0x48, 0x8B, 0x7D, 0xF0); mov rdi,QWORD PTR [rbp-0x10]
 	 * EMIT4(0x48, 0x8B, 0x75, 0xF8); mov rsi,QWORD PTR [rbp-0x8]
 	 */
-	for (i = 0; i < min(nr_args, 6); i++)
-		emit_ldx(prog, bytes_to_bpf_size(m->arg_size[i]),
-			 i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
-			 BPF_REG_FP,
-			 -(stack_size - i * 8));
+	for (i = 0, j = 0; i < min(nr_args, 6); i++) {
+		if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG) {
+			nr_regs = (m->arg_size[i] + 7) / 8;
+			arg_size = 8;
+		} else {
+			nr_regs = 1;
+			arg_size = m->arg_size[i];
+		}
+
+		while (nr_regs) {
+			emit_ldx(prog, bytes_to_bpf_size(arg_size),
+				 j == 5 ? X86_REG_R9 : BPF_REG_1 + j,
+				 BPF_REG_FP,
+				 -(stack_size - j * 8));
+			nr_regs--;
+			j++;
+		}
+	}
 }
 
 static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
@@ -2015,7 +2041,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 				struct bpf_tramp_links *tlinks,
 				void *orig_call)
 {
-	int ret, i, nr_args = m->nr_args;
+	int ret, i, nr_args = m->nr_args, extra_nregs = 0;
 	int regs_off, ip_off, args_off, stack_size = nr_args * 8, run_ctx_off;
 	struct bpf_tramp_links *fentry = &tlinks[BPF_TRAMP_FENTRY];
 	struct bpf_tramp_links *fexit = &tlinks[BPF_TRAMP_FEXIT];
@@ -2028,6 +2054,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	if (nr_args > 6)
 		return -ENOTSUPP;
 
+	for (i = 0; i < MAX_BPF_FUNC_ARGS; i++) {
+		if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG)
+			extra_nregs += (m->arg_size[i] + 7) / 8 - 1;
+	}
+	if (nr_args + extra_nregs > 6)
+		return -ENOTSUPP;
+	stack_size += extra_nregs * 8;
+
 	/* Generated trampoline stack layout:
 	 *
 	 * RBP + 8         [ return address  ]
@@ -2040,7 +2074,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	 *                 [ ...             ]
 	 * RBP - regs_off  [ reg_arg1        ]  program's ctx pointer
 	 *
-	 * RBP - args_off  [ args count      ]  always
+	 * RBP - args_off  [ arg regs count  ]  always
 	 *
 	 * RBP - ip_off    [ traced function ]  BPF_TRAMP_F_IP_ARG flag
 	 *
@@ -2083,11 +2117,11 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	EMIT4(0x48, 0x83, 0xEC, stack_size); /* sub rsp, stack_size */
 	EMIT1(0x53);		 /* push rbx */
 
-	/* Store number of arguments of the traced function:
-	 *   mov rax, nr_args
+	/* Store number of argument registers of the traced function:
+	 *   mov rax, nr_args + extra_nregs
 	 *   mov QWORD PTR [rbp - args_off], rax
 	 */
-	emit_mov_imm64(&prog, BPF_REG_0, 0, (u32) nr_args);
+	emit_mov_imm64(&prog, BPF_REG_0, 0, (u32) nr_args + extra_nregs);
 	emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -args_off);
 
 	if (flags & BPF_TRAMP_F_IP_ARG) {
-- 
2.30.2

[PATCH bpf-next v3 3/7] bpf: Update descriptions for helpers bpf_get_func_arg[_cnt]()

[Yonghong Song]

Now instead of the number of arguments, the number of registers
holding argument values are stored in trampoline. Update
the description of bpf_get_func_arg[_cnt]() helpers. Previous
programs without struct arguments should continue to work
as usual.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/uapi/linux/bpf.h       | 9 +++++----
 tools/include/uapi/linux/bpf.h | 9 +++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 962960a98835..f9f43343ef93 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5079,12 +5079,12 @@ union bpf_attr {
  *
  * long bpf_get_func_arg(void *ctx, u32 n, u64 *value)
  *	Description
- *		Get **n**-th argument (zero based) of the traced function (for tracing programs)
+ *		Get **n**-th argument register (zero based) of the traced function (for tracing programs)
  *		returned in **value**.
  *
  *	Return
  *		0 on success.
- *		**-EINVAL** if n >= arguments count of traced function.
+ *		**-EINVAL** if n >= argument register count of traced function.
  *
  * long bpf_get_func_ret(void *ctx, u64 *value)
  *	Description
@@ -5097,10 +5097,11 @@ union bpf_attr {
  *
  * long bpf_get_func_arg_cnt(void *ctx)
  *	Description
- *		Get number of arguments of the traced function (for tracing programs).
+ *		Get number of registers of the traced function (for tracing programs) where
+ *		function arguments are stored in these registers.
  *
  *	Return
- *		The number of arguments of the traced function.
+ *		The number of argument registers of the traced function.
  *
  * int bpf_get_retval(void)
  *	Description
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index f4ba82a1eace..f13fa71822f4 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5079,12 +5079,12 @@ union bpf_attr {
  *
  * long bpf_get_func_arg(void *ctx, u32 n, u64 *value)
  *	Description
- *		Get **n**-th argument (zero based) of the traced function (for tracing programs)
+ *		Get **n**-th argument register (zero based) of the traced function (for tracing programs)
  *		returned in **value**.
  *
  *	Return
  *		0 on success.
- *		**-EINVAL** if n >= arguments count of traced function.
+ *		**-EINVAL** if n >= argument register count of traced function.
  *
  * long bpf_get_func_ret(void *ctx, u64 *value)
  *	Description
@@ -5097,10 +5097,11 @@ union bpf_attr {
  *
  * long bpf_get_func_arg_cnt(void *ctx)
  *	Description
- *		Get number of arguments of the traced function (for tracing programs).
+ *		Get number of registers of the traced function (for tracing programs) where
+ *		function arguments are stored in these registers.
  *
  *	Return
- *		The number of arguments of the traced function.
+ *		The number of argument registers of the traced function.
  *
  * int bpf_get_retval(void)
  *	Description
-- 
2.30.2

[PATCH bpf-next v3 4/7] bpf: arm64: No support of struct argument in trampoline programs

[Yonghong Song]

ARM64 does not support struct argument for trampoline based
bpf programs yet.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 arch/arm64/net/bpf_jit_comp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 389623ae5a91..30f76178608b 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -1970,7 +1970,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
 				u32 flags, struct bpf_tramp_links *tlinks,
 				void *orig_call)
 {
-	int ret;
+	int i, ret;
 	int nargs = m->nr_args;
 	int max_insns = ((long)image_end - (long)image) / AARCH64_INSN_SIZE;
 	struct jit_ctx ctx = {
@@ -1982,6 +1982,12 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
 	if (nargs > 8)
 		return -ENOTSUPP;
 
+	/* don't support struct argument */
+	for (i = 0; i < MAX_BPF_FUNC_ARGS; i++) {
+		if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG)
+			return -ENOTSUPP;
+	}
+
 	ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags);
 	if (ret < 0)
 		return ret;
-- 
2.30.2

[PATCH bpf-next v3 5/7] libbpf: Add new BPF_PROG2 macro

[Yonghong Song]

To support struct arguments in trampoline based programs,
existing BPF_PROG doesn't work any more since
the type size is needed to find whether a parameter
takes one or two registers. So this patch added a new
BPF_PROG2 macro to support such trampoline programs.

The idea is suggested by Andrii. For example, if the
to-be-traced function has signature like
  typedef struct {
       void *x;
       int t;
  } sockptr;
  int blah(sockptr x, char y);

In the new BPF_PROG2 macro, the argument can be
represented as
  __bpf_prog_call(
     ({ union {
          struct { __u64 x, y; } ___z;
          sockptr x;
        } ___tmp = { .___z = { ctx[0], ctx[1] }};
        ___tmp.x;
     }),
     ({ union {
          struct { __u8 x; } ___z;
          char y;
        } ___tmp = { .___z = { ctx[2] }};
        ___tmp.y;
     }));
In the above, the values stored on the stack are properly
assigned to the actual argument type value by using 'union'
magic. Note that the macro also works even if no arguments
are with struct types.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/lib/bpf/bpf_tracing.h | 82 +++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 5fdb93da423b..c59ae9c8ccbd 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -438,6 +438,88 @@ typeof(name(0)) name(unsigned long long *ctx)				    \
 static __always_inline typeof(name(0))					    \
 ____##name(unsigned long long *ctx, ##args)
 
+#ifndef ____bpf_nth
+#define ____bpf_nth(_, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _13, _14, _15, _16, _17, _18, _19, _20, _21, _22, _23, _24, N, ...) N
+#endif
+#ifndef ____bpf_narg
+#define ____bpf_narg(...) ____bpf_nth(_, ##__VA_ARGS__, 12, 12, 11, 11, 10, 10, 9, 9, 8, 8, 7, 7, 6, 6, 5, 5, 4, 4, 3, 3, 2, 2, 1, 1, 0)
+#endif
+
+#define BPF_REG_CNT(t) \
+	(__builtin_choose_expr(sizeof(t) == 1, 1,	\
+	 __builtin_choose_expr(sizeof(t) == 2, 1,	\
+	 __builtin_choose_expr(sizeof(t) == 4, 1,	\
+	 __builtin_choose_expr(sizeof(t) == 8, 1,	\
+	 __builtin_choose_expr(sizeof(t) == 16, 2,	\
+			       (void)0))))))
+
+#define ____bpf_reg_cnt0()			(0)
+#define ____bpf_reg_cnt1(t, x)			(____bpf_reg_cnt0() + BPF_REG_CNT(t))
+#define ____bpf_reg_cnt2(t, x, args...)		(____bpf_reg_cnt1(args) + BPF_REG_CNT(t))
+#define ____bpf_reg_cnt3(t, x, args...)		(____bpf_reg_cnt2(args) + BPF_REG_CNT(t))
+#define ____bpf_reg_cnt4(t, x, args...)		(____bpf_reg_cnt3(args) + BPF_REG_CNT(t))
+#define ____bpf_reg_cnt5(t, x, args...)		(____bpf_reg_cnt4(args) + BPF_REG_CNT(t))
+#define ____bpf_reg_cnt6(t, x, args...)		(____bpf_reg_cnt5(args) + BPF_REG_CNT(t))
+#define ____bpf_reg_cnt7(t, x, args...)		(____bpf_reg_cnt6(args) + BPF_REG_CNT(t))
+#define ____bpf_reg_cnt8(t, x, args...)		(____bpf_reg_cnt7(args) + BPF_REG_CNT(t))
+#define ____bpf_reg_cnt9(t, x, args...)		(____bpf_reg_cnt8(args) + BPF_REG_CNT(t))
+#define ____bpf_reg_cnt10(t, x, args...)	(____bpf_reg_cnt9(args) + BPF_REG_CNT(t))
+#define ____bpf_reg_cnt11(t, x, args...)	(____bpf_reg_cnt10(args) + BPF_REG_CNT(t))
+#define ____bpf_reg_cnt12(t, x, args...)	(____bpf_reg_cnt11(args) + BPF_REG_CNT(t))
+#define ____bpf_reg_cnt(args...)	 ___bpf_apply(____bpf_reg_cnt, ____bpf_narg(args))(args)
+
+#define ____bpf_union_arg(t, x, n) \
+	__builtin_choose_expr(sizeof(t) == 1, ({ union { struct { __u8 x; } ___z; t x; } ___tmp = { .___z = {ctx[n]}}; ___tmp.x; }), \
+	__builtin_choose_expr(sizeof(t) == 2, ({ union { struct { __u16 x; } ___z; t x; } ___tmp = { .___z = {ctx[n]} }; ___tmp.x; }), \
+	__builtin_choose_expr(sizeof(t) == 4, ({ union { struct { __u32 x; } ___z; t x; } ___tmp = { .___z = {ctx[n]} }; ___tmp.x; }), \
+	__builtin_choose_expr(sizeof(t) == 8, ({ union { struct { __u64 x; } ___z; t x; } ___tmp = {.___z = {ctx[n]} }; ___tmp.x; }), \
+	__builtin_choose_expr(sizeof(t) == 16, ({ union { struct { __u64 x, y; } ___z; t x; } ___tmp = {.___z = {ctx[n], ctx[n + 1]} }; ___tmp.x; }), \
+			      (void)0)))))
+
+#define ____bpf_ctx_arg0(n, args...)
+#define ____bpf_ctx_arg1(n, t, x)		, ____bpf_union_arg(t, x, n - ____bpf_reg_cnt1(t, x))
+#define ____bpf_ctx_arg2(n, t, x, args...)	, ____bpf_union_arg(t, x, n - ____bpf_reg_cnt2(t, x, args)) ____bpf_ctx_arg1(n, args)
+#define ____bpf_ctx_arg3(n, t, x, args...)	, ____bpf_union_arg(t, x, n - ____bpf_reg_cnt3(t, x, args)) ____bpf_ctx_arg2(n, args)
+#define ____bpf_ctx_arg4(n, t, x, args...)	, ____bpf_union_arg(t, x, n - ____bpf_reg_cnt4(t, x, args)) ____bpf_ctx_arg3(n, args)
+#define ____bpf_ctx_arg5(n, t, x, args...)	, ____bpf_union_arg(t, x, n - ____bpf_reg_cnt5(t, x, args)) ____bpf_ctx_arg4(n, args)
+#define ____bpf_ctx_arg6(n, t, x, args...)	, ____bpf_union_arg(t, x, n - ____bpf_reg_cnt6(t, x, args)) ____bpf_ctx_arg5(n, args)
+#define ____bpf_ctx_arg7(n, t, x, args...)	, ____bpf_union_arg(t, x, n - ____bpf_reg_cnt7(t, x, args)) ____bpf_ctx_arg6(n, args)
+#define ____bpf_ctx_arg8(n, t, x, args...)	, ____bpf_union_arg(t, x, n - ____bpf_reg_cnt8(t, x, args)) ____bpf_ctx_arg7(n, args)
+#define ____bpf_ctx_arg9(n, t, x, args...)	, ____bpf_union_arg(t, x, n - ____bpf_reg_cnt9(t, x, args)) ____bpf_ctx_arg8(n, args)
+#define ____bpf_ctx_arg10(n, t, x, args...)	, ____bpf_union_arg(t, x, n - ____bpf_reg_cnt10(t, x, args)) ____bpf_ctx_arg9(n, args)
+#define ____bpf_ctx_arg11(n, t, x, args...)	, ____bpf_union_arg(t, x, n - ____bpf_reg_cnt11(t, x, args)) ____bpf_ctx_arg10(n, args)
+#define ____bpf_ctx_arg12(n, t, x, args...)	, ____bpf_union_arg(t, x, n - ____bpf_reg_cnt12(t, x, args)) ____bpf_ctx_arg11(n, args)
+#define ____bpf_ctx_arg(n, args...)	___bpf_apply(____bpf_ctx_arg, ____bpf_narg(args))(n, args)
+
+#define ____bpf_ctx_decl0()
+#define ____bpf_ctx_decl1(t, x)			, t x
+#define ____bpf_ctx_decl2(t, x, args...)	, t x ____bpf_ctx_decl1(args)
+#define ____bpf_ctx_decl3(t, x, args...)	, t x ____bpf_ctx_decl2(args)
+#define ____bpf_ctx_decl4(t, x, args...)	, t x ____bpf_ctx_decl3(args)
+#define ____bpf_ctx_decl5(t, x, args...)	, t x ____bpf_ctx_decl4(args)
+#define ____bpf_ctx_decl6(t, x, args...)	, t x ____bpf_ctx_decl5(args)
+#define ____bpf_ctx_decl7(t, x, args...)	, t x ____bpf_ctx_decl6(args)
+#define ____bpf_ctx_decl8(t, x, args...)	, t x ____bpf_ctx_decl7(args)
+#define ____bpf_ctx_decl9(t, x, args...)	, t x ____bpf_ctx_decl8(args)
+#define ____bpf_ctx_decl10(t, x, args...)	, t x ____bpf_ctx_decl9(args)
+#define ____bpf_ctx_decl11(t, x, args...)	, t x ____bpf_ctx_decl10(args)
+#define ____bpf_ctx_decl12(t, x, args...)	, t x ____bpf_ctx_decl11(args)
+#define ____bpf_ctx_decl(args...)	___bpf_apply(____bpf_ctx_decl, ____bpf_narg(args))(args)
+
+/*
+ * BPF_PROG2 can handle struct arguments.
+ */
+#define BPF_PROG2(name, args...)						\
+name(unsigned long long *ctx);							\
+static __always_inline typeof(name(0))						\
+____##name(unsigned long long *ctx ____bpf_ctx_decl(args));			\
+typeof(name(0)) name(unsigned long long *ctx)					\
+{										\
+	return ____##name(ctx ____bpf_ctx_arg(____bpf_reg_cnt(args), args));	\
+}										\
+static __always_inline typeof(name(0))						\
+____##name(unsigned long long *ctx ____bpf_ctx_decl(args))
+
 struct pt_regs;
 
 #define ___bpf_kprobe_args0()           ctx
-- 
2.30.2

Re: [PATCH bpf-next v3 5/7] libbpf: Add new BPF_PROG2 macro

[Kui-Feng Lee]

On Sat, 2022-08-27 at 19:55 -0700, Yonghong Song wrote:
> To support struct arguments in trampoline based programs,
> existing BPF_PROG doesn't work any more since
> the type size is needed to find whether a parameter
> takes one or two registers. So this patch added a new
> BPF_PROG2 macro to support such trampoline programs.
> 
> The idea is suggested by Andrii. For example, if the
> to-be-traced function has signature like
>   typedef struct {
>        void *x;
>        int t;
>   } sockptr;
>   int blah(sockptr x, char y);
> 
> In the new BPF_PROG2 macro, the argument can be
> represented as
>   __bpf_prog_call(
>      ({ union {
>           struct { __u64 x, y; } ___z;
>           sockptr x;
>         } ___tmp = { .___z = { ctx[0], ctx[1] }};
>         ___tmp.x;
>      }),
>      ({ union {
>           struct { __u8 x; } ___z;
>           char y;
>         } ___tmp = { .___z = { ctx[2] }};
>         ___tmp.y;
>      }));
> In the above, the values stored on the stack are properly
> assigned to the actual argument type value by using 'union'
> magic. Note that the macro also works even if no arguments
> are with struct types.
> 
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  tools/lib/bpf/bpf_tracing.h | 82
> +++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/tools/lib/bpf/bpf_tracing.h
> b/tools/lib/bpf/bpf_tracing.h
> index 5fdb93da423b..c59ae9c8ccbd 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -438,6 +438,88 @@ typeof(name(0)) name(unsigned long long
> *ctx)                                  \
>  static __always_inline
> typeof(name(0))                                     \
>  ____##name(unsigned long long *ctx, ##args)
>  
> +#ifndef ____bpf_nth
> +#define ____bpf_nth(_, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11,
> _12, _13, _14, _15, _16, _17, _18, _19, _20, _21, _22, _23, _24, N,
> ...) N

____bpf_nth() is a confusing name.  I will expect to give any number n
to return the nth argument.  However, here it return the value of an
argument at a specific/fixed position.  ____bpf_25th() would make more
sense for me.

> +#endif
> +#ifndef ____bpf_narg
> +#define ____bpf_narg(...) ____bpf_nth(_, ##__VA_ARGS__, 12, 12, 11,
> 11, 10, 10, 9, 9, 8, 8, 7, 7, 6, 6, 5, 5, 4, 4, 3, 3, 2, 2, 1, 1, 0)
> +#endif
> +
> +#define BPF_REG_CNT(t) \
> +       (__builtin_choose_expr(sizeof(t) == 1, 1,       \
> +        __builtin_choose_expr(sizeof(t) == 2, 1,       \
> +        __builtin_choose_expr(sizeof(t) == 4, 1,       \
> +        __builtin_choose_expr(sizeof(t) == 8, 1,       \
> +        __builtin_choose_expr(sizeof(t) == 16, 2,      \
> +                              (void)0))))))

nit: Using ternary operator will still work, right?
For example,

 (sizeof(t) == 1 || sizeof(t) == 2 || sizeof(t) == 4 || sizeof(t) == 8)
?  1 : (sizeof(t) == 16 ? 2 : 0)

Compilers should be able to optimize it as a const value.

> +
> +#define ____bpf_reg_cnt0()                     (0)
> +#define ____bpf_reg_cnt1(t, x)                 (____bpf_reg_cnt0() +
> BPF_REG_CNT(t))
> +#define ____bpf_reg_cnt2(t, x,
> args...)                (____bpf_reg_cnt1(args) + BPF_REG_CNT(t))
> +#define ____bpf_reg_cnt3(t, x,
> args...)                (____bpf_reg_cnt2(args) + BPF_REG_CNT(t))
> +#define ____bpf_reg_cnt4(t, x,
> args...)                (____bpf_reg_cnt3(args) + BPF_REG_CNT(t))
> +#define ____bpf_reg_cnt5(t, x,
> args...)                (____bpf_reg_cnt4(args) + BPF_REG_CNT(t))
> +#define ____bpf_reg_cnt6(t, x,
> args...)                (____bpf_reg_cnt5(args) + BPF_REG_CNT(t))
> +#define ____bpf_reg_cnt7(t, x,
> args...)                (____bpf_reg_cnt6(args) + BPF_REG_CNT(t))
> +#define ____bpf_reg_cnt8(t, x,
> args...)                (____bpf_reg_cnt7(args) + BPF_REG_CNT(t))
> +#define ____bpf_reg_cnt9(t, x,
> args...)                (____bpf_reg_cnt8(args) + BPF_REG_CNT(t))
> +#define ____bpf_reg_cnt10(t, x,
> args...)       (____bpf_reg_cnt9(args) + BPF_REG_CNT(t))
> +#define ____bpf_reg_cnt11(t, x,
> args...)       (____bpf_reg_cnt10(args) + BPF_REG_CNT(t))
> +#define ____bpf_reg_cnt12(t, x,
> args...)       (____bpf_reg_cnt11(args) + BPF_REG_CNT(t))
> +#define ____bpf_reg_cnt(args...)       
> ___bpf_apply(____bpf_reg_cnt, ____bpf_narg(args))(args)
> +
> +#define ____bpf_union_arg(t, x, n) \
> +       __builtin_choose_expr(sizeof(t) == 1, ({ union { struct {
> __u8 x; } ___z; t x; } ___tmp = { .___z = {ctx[n]}}; ___tmp.x; }), \
> +       __builtin_choose_expr(sizeof(t) == 2, ({ union { struct {
> __u16 x; } ___z; t x; } ___tmp = { .___z = {ctx[n]} }; ___tmp.x; }),
> \
> +       __builtin_choose_expr(sizeof(t) == 4, ({ union { struct {
> __u32 x; } ___z; t x; } ___tmp = { .___z = {ctx[n]} }; ___tmp.x; }),
> \
> +       __builtin_choose_expr(sizeof(t) == 8, ({ union { struct {
> __u64 x; } ___z; t x; } ___tmp = {.___z = {ctx[n]} }; ___tmp.x; }), \
> +       __builtin_choose_expr(sizeof(t) == 16, ({ union { struct {
> __u64 x, y; } ___z; t x; } ___tmp = {.___z = {ctx[n], ctx[n + 1]} };
> ___tmp.x; }), \
> +                             (void)0)))))

Is it possible to cast &ctx[n] to the pointer of the target type?
For example,

  *(t*)&ctx[n]

Did I miss any thing?

> +
> +#define ____bpf_ctx_arg0(n, args...)
> +#define ____bpf_ctx_arg1(n, t, x)              ,
> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt1(t, x))
> +#define ____bpf_ctx_arg2(n, t, x, args...)     ,
> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt2(t, x, args))
> ____bpf_ctx_arg1(n, args)
> +#define ____bpf_ctx_arg3(n, t, x, args...)     ,
> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt3(t, x, args))
> ____bpf_ctx_arg2(n, args)
> +#define ____bpf_ctx_arg4(n, t, x, args...)     ,
> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt4(t, x, args))
> ____bpf_ctx_arg3(n, args)
> +#define ____bpf_ctx_arg5(n, t, x, args...)     ,
> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt5(t, x, args))
> ____bpf_ctx_arg4(n, args)
> +#define ____bpf_ctx_arg6(n, t, x, args...)     ,
> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt6(t, x, args))
> ____bpf_ctx_arg5(n, args)
> +#define ____bpf_ctx_arg7(n, t, x, args...)     ,
> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt7(t, x, args))
> ____bpf_ctx_arg6(n, args)
> +#define ____bpf_ctx_arg8(n, t, x, args...)     ,
> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt8(t, x, args))
> ____bpf_ctx_arg7(n, args)
> +#define ____bpf_ctx_arg9(n, t, x, args...)     ,
> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt9(t, x, args))
> ____bpf_ctx_arg8(n, args)
> +#define ____bpf_ctx_arg10(n, t, x, args...)    ,
> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt10(t, x, args))
> ____bpf_ctx_arg9(n, args)
> +#define ____bpf_ctx_arg11(n, t, x, args...)    ,
> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt11(t, x, args))
> ____bpf_ctx_arg10(n, args)
> +#define ____bpf_ctx_arg12(n, t, x, args...)    ,
> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt12(t, x, args))
> ____bpf_ctx_arg11(n, args)
> +#define ____bpf_ctx_arg(n, args...)    ___bpf_apply(____bpf_ctx_arg,
> ____bpf_narg(args))(n, args)
> +
> +#define ____bpf_ctx_decl0()
> +#define ____bpf_ctx_decl1(t, x)                        , t x
> +#define ____bpf_ctx_decl2(t, x, args...)       , t x
> ____bpf_ctx_decl1(args)
> +#define ____bpf_ctx_decl3(t, x, args...)       , t x
> ____bpf_ctx_decl2(args)
> +#define ____bpf_ctx_decl4(t, x, args...)       , t x
> ____bpf_ctx_decl3(args)
> +#define ____bpf_ctx_decl5(t, x, args...)       , t x
> ____bpf_ctx_decl4(args)
> +#define ____bpf_ctx_decl6(t, x, args...)       , t x
> ____bpf_ctx_decl5(args)
> +#define ____bpf_ctx_decl7(t, x, args...)       , t x
> ____bpf_ctx_decl6(args)
> +#define ____bpf_ctx_decl8(t, x, args...)       , t x
> ____bpf_ctx_decl7(args)
> +#define ____bpf_ctx_decl9(t, x, args...)       , t x
> ____bpf_ctx_decl8(args)
> +#define ____bpf_ctx_decl10(t, x, args...)      , t x
> ____bpf_ctx_decl9(args)
> +#define ____bpf_ctx_decl11(t, x, args...)      , t x
> ____bpf_ctx_decl10(args)
> +#define ____bpf_ctx_decl12(t, x, args...)      , t x
> ____bpf_ctx_decl11(args)
> +#define
> ____bpf_ctx_decl(args...)      ___bpf_apply(____bpf_ctx_decl,
> ____bpf_narg(args))(args)
> +
> +/*
> + * BPF_PROG2 can handle struct arguments.
> + */
> +#define BPF_PROG2(name,
> args...)                                               \
> +name(unsigned long long
> *ctx);                                                 \
> +static __always_inline
> typeof(name(0))                                         \
> +____##name(unsigned long long *ctx
> ____bpf_ctx_decl(args));                    \
> +typeof(name(0)) name(unsigned long long
> *ctx)                                  \
> +{                                                                   
>            \
> +       return ____##name(ctx ____bpf_ctx_arg(____bpf_reg_cnt(args),
> args));    \
> +}                                                                   
>            \
> +static __always_inline
> typeof(name(0))                                         \
> +____##name(unsigned long long *ctx ____bpf_ctx_decl(args))
> +
>  struct pt_regs;
>  
>  #define ___bpf_kprobe_args0()           ctx

Re: [PATCH bpf-next v3 5/7] libbpf: Add new BPF_PROG2 macro

[Yonghong Song]

On 8/29/22 5:32 PM, Kui-Feng Lee wrote:
> On Sat, 2022-08-27 at 19:55 -0700, Yonghong Song wrote:
>> To support struct arguments in trampoline based programs,
>> existing BPF_PROG doesn't work any more since
>> the type size is needed to find whether a parameter
>> takes one or two registers. So this patch added a new
>> BPF_PROG2 macro to support such trampoline programs.
>>
>> The idea is suggested by Andrii. For example, if the
>> to-be-traced function has signature like
>>    typedef struct {
>>         void *x;
>>         int t;
>>    } sockptr;
>>    int blah(sockptr x, char y);
>>
>> In the new BPF_PROG2 macro, the argument can be
>> represented as
>>    __bpf_prog_call(
>>       ({ union {
>>            struct { __u64 x, y; } ___z;
>>            sockptr x;
>>          } ___tmp = { .___z = { ctx[0], ctx[1] }};
>>          ___tmp.x;
>>       }),
>>       ({ union {
>>            struct { __u8 x; } ___z;
>>            char y;
>>          } ___tmp = { .___z = { ctx[2] }};
>>          ___tmp.y;
>>       }));
>> In the above, the values stored on the stack are properly
>> assigned to the actual argument type value by using 'union'
>> magic. Note that the macro also works even if no arguments
>> are with struct types.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   tools/lib/bpf/bpf_tracing.h | 82
>> +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 82 insertions(+)
>>
>> diff --git a/tools/lib/bpf/bpf_tracing.h
>> b/tools/lib/bpf/bpf_tracing.h
>> index 5fdb93da423b..c59ae9c8ccbd 100644
>> --- a/tools/lib/bpf/bpf_tracing.h
>> +++ b/tools/lib/bpf/bpf_tracing.h
>> @@ -438,6 +438,88 @@ typeof(name(0)) name(unsigned long long
>> *ctx)                                  \
>>   static __always_inline
>> typeof(name(0))                                     \
>>   ____##name(unsigned long long *ctx, ##args)
>>   
>> +#ifndef ____bpf_nth
>> +#define ____bpf_nth(_, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11,
>> _12, _13, _14, _15, _16, _17, _18, _19, _20, _21, _22, _23, _24, N,
>> ...) N
> 
> ____bpf_nth() is a confusing name.  I will expect to give any number n
> to return the nth argument.  However, here it return the value of an
> argument at a specific/fixed position.  ____bpf_25th() would make more
> sense for me.

In bpf_tracing.h, we have
#define ___bpf_nth(_, _1, _2, _3, _4, _5, _6, _7, _8, _9, _a, _b, _c, N, 
...) N
for 12 arguments.

The above is similar to ___bpf_nth (with one less _ compared to 
____bpf_nth).

On the other hand, I can just modify the existing ___bpf_nth for 24 
arguments so we don't need ____bpf_nth any more.
Will do this in the next revision.

> 
>> +#endif
>> +#ifndef ____bpf_narg
>> +#define ____bpf_narg(...) ____bpf_nth(_, ##__VA_ARGS__, 12, 12, 11,
>> 11, 10, 10, 9, 9, 8, 8, 7, 7, 6, 6, 5, 5, 4, 4, 3, 3, 2, 2, 1, 1, 0)
>> +#endif
>> +
>> +#define BPF_REG_CNT(t) \
>> +       (__builtin_choose_expr(sizeof(t) == 1, 1,       \
>> +        __builtin_choose_expr(sizeof(t) == 2, 1,       \
>> +        __builtin_choose_expr(sizeof(t) == 4, 1,       \
>> +        __builtin_choose_expr(sizeof(t) == 8, 1,       \
>> +        __builtin_choose_expr(sizeof(t) == 16, 2,      \
>> +                              (void)0))))))
> 
> nit: Using ternary operator will still work, right?
> For example,
> 
>   (sizeof(t) == 1 || sizeof(t) == 2 || sizeof(t) == 4 || sizeof(t) == 8)
> ?  1 : (sizeof(t) == 16 ? 2 : 0)
> 
> Compilers should be able to optimize it as a const value.

Good idea. Will change in the next revision.

> 
>> +
>> +#define ____bpf_reg_cnt0()                     (0)
>> +#define ____bpf_reg_cnt1(t, x)                 (____bpf_reg_cnt0() +
>> BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt2(t, x,
>> args...)                (____bpf_reg_cnt1(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt3(t, x,
>> args...)                (____bpf_reg_cnt2(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt4(t, x,
>> args...)                (____bpf_reg_cnt3(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt5(t, x,
>> args...)                (____bpf_reg_cnt4(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt6(t, x,
>> args...)                (____bpf_reg_cnt5(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt7(t, x,
>> args...)                (____bpf_reg_cnt6(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt8(t, x,
>> args...)                (____bpf_reg_cnt7(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt9(t, x,
>> args...)                (____bpf_reg_cnt8(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt10(t, x,
>> args...)       (____bpf_reg_cnt9(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt11(t, x,
>> args...)       (____bpf_reg_cnt10(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt12(t, x,
>> args...)       (____bpf_reg_cnt11(args) + BPF_REG_CNT(t))
>> +#define ____bpf_reg_cnt(args...)
>> ___bpf_apply(____bpf_reg_cnt, ____bpf_narg(args))(args)
>> +
>> +#define ____bpf_union_arg(t, x, n) \
>> +       __builtin_choose_expr(sizeof(t) == 1, ({ union { struct {
>> __u8 x; } ___z; t x; } ___tmp = { .___z = {ctx[n]}}; ___tmp.x; }), \
>> +       __builtin_choose_expr(sizeof(t) == 2, ({ union { struct {
>> __u16 x; } ___z; t x; } ___tmp = { .___z = {ctx[n]} }; ___tmp.x; }),
>> \
>> +       __builtin_choose_expr(sizeof(t) == 4, ({ union { struct {
>> __u32 x; } ___z; t x; } ___tmp = { .___z = {ctx[n]} }; ___tmp.x; }),
>> \
>> +       __builtin_choose_expr(sizeof(t) == 8, ({ union { struct {
>> __u64 x; } ___z; t x; } ___tmp = {.___z = {ctx[n]} }; ___tmp.x; }), \
>> +       __builtin_choose_expr(sizeof(t) == 16, ({ union { struct {
>> __u64 x, y; } ___z; t x; } ___tmp = {.___z = {ctx[n], ctx[n + 1]} };
>> ___tmp.x; }), \
>> +                             (void)0)))))
> 
> Is it possible to cast &ctx[n] to the pointer of the target type?
> For example,
> 
>    *(t*)&ctx[n]
> 
> Did I miss any thing?

This won't work.

ctx[n] represents a u64 value.
Let us say type is 'u32'.   *(u32 *)&ctx[n]
works for little endian, but won't work for big endian
system.

> 
>> +
>> +#define ____bpf_ctx_arg0(n, args...)
>> +#define ____bpf_ctx_arg1(n, t, x)              ,
>> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt1(t, x))
>> +#define ____bpf_ctx_arg2(n, t, x, args...)     ,
>> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt2(t, x, args))
>> ____bpf_ctx_arg1(n, args)
>> +#define ____bpf_ctx_arg3(n, t, x, args...)     ,
>> ____bpf_union_arg(t, x, n - ____bpf_reg_cnt3(t, x, args))
>> ____bpf_ctx_arg2(n, args)
[...]

[PATCH bpf-next v3 6/7] selftests/bpf: Add struct argument tests with fentry/fexit programs.

[Yonghong Song]

Add various struct argument tests with fentry/fexit programs.
Also add one test with a kernel func which does not have any
argument to test BPF_PROG2 macro in such situation.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  48 ++++++++
 .../selftests/bpf/prog_tests/tracing_struct.c |  63 ++++++++++
 .../selftests/bpf/progs/tracing_struct.c      | 114 ++++++++++++++++++
 3 files changed, 225 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/tracing_struct.c
 create mode 100644 tools/testing/selftests/bpf/progs/tracing_struct.c

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 792cb15bac40..a6021d6117b5 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -18,6 +18,46 @@ typedef int (*func_proto_typedef_nested1)(func_proto_typedef);
 typedef int (*func_proto_typedef_nested2)(func_proto_typedef_nested1);
 
 DEFINE_PER_CPU(int, bpf_testmod_ksym_percpu) = 123;
+long bpf_testmod_test_struct_arg_result;
+
+struct bpf_testmod_struct_arg_1 {
+	int a;
+};
+struct bpf_testmod_struct_arg_2 {
+	long a;
+	long b;
+};
+
+noinline int
+bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int b, int c) {
+	bpf_testmod_test_struct_arg_result = a.a + a.b  + b + c;
+	return bpf_testmod_test_struct_arg_result;
+}
+
+noinline int
+bpf_testmod_test_struct_arg_2(int a, struct bpf_testmod_struct_arg_2 b, int c) {
+	bpf_testmod_test_struct_arg_result = a + b.a + b.b + c;
+	return bpf_testmod_test_struct_arg_result;
+}
+
+noinline int
+bpf_testmod_test_struct_arg_3(int a, int b, struct bpf_testmod_struct_arg_2 c) {
+	bpf_testmod_test_struct_arg_result = a + b + c.a + c.b;
+	return bpf_testmod_test_struct_arg_result;
+}
+
+noinline int
+bpf_testmod_test_struct_arg_4(struct bpf_testmod_struct_arg_1 a, int b,
+			      int c, int d, struct bpf_testmod_struct_arg_2 e) {
+	bpf_testmod_test_struct_arg_result = a.a + b + c + d + e.a + e.b;
+	return bpf_testmod_test_struct_arg_result;
+}
+
+noinline int
+bpf_testmod_test_struct_arg_5(void) {
+	bpf_testmod_test_struct_arg_result = 1;
+	return bpf_testmod_test_struct_arg_result;
+}
 
 noinline void
 bpf_testmod_test_mod_kfunc(int i)
@@ -98,11 +138,19 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
 		.off = off,
 		.len = len,
 	};
+	struct bpf_testmod_struct_arg_1 struct_arg1 = {10};
+	struct bpf_testmod_struct_arg_2 struct_arg2 = {2, 3};
 	int i = 1;
 
 	while (bpf_testmod_return_ptr(i))
 		i++;
 
+	(void)bpf_testmod_test_struct_arg_1(struct_arg2, 1, 4);
+	(void)bpf_testmod_test_struct_arg_2(1, struct_arg2, 4);
+	(void)bpf_testmod_test_struct_arg_3(1, 4, struct_arg2);
+	(void)bpf_testmod_test_struct_arg_4(struct_arg1, 1, 2, 3, struct_arg2);
+	(void)bpf_testmod_test_struct_arg_5();
+
 	/* This is always true. Use the check to make sure the compiler
 	 * doesn't remove bpf_testmod_loop_test.
 	 */
diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
new file mode 100644
index 000000000000..d5022b91d1e4
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
+
+#include <test_progs.h>
+#include "tracing_struct.skel.h"
+
+static void test_fentry(void)
+{
+	struct tracing_struct *skel;
+	int err;
+
+	skel = tracing_struct__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "tracing_struct__open_and_load"))
+		return;
+
+	err = tracing_struct__attach(skel);
+	if (!ASSERT_OK(err, "tracing_struct__attach"))
+		return;
+
+	ASSERT_OK(trigger_module_test_read(256), "trigger_read");
+
+	ASSERT_EQ(skel->bss->t1_a_a, 2, "t1:a.a");
+	ASSERT_EQ(skel->bss->t1_a_b, 3, "t1:a.b");
+	ASSERT_EQ(skel->bss->t1_b, 1, "t1:b");
+	ASSERT_EQ(skel->bss->t1_c, 4, "t1:c");
+
+	ASSERT_EQ(skel->bss->t1_nregs, 4, "t1 nregs");
+	ASSERT_EQ(skel->bss->t1_reg0, 2, "t1 reg0");
+	ASSERT_EQ(skel->bss->t1_reg1, 3, "t1 reg1");
+	ASSERT_EQ(skel->bss->t1_reg2, 1, "t1 reg2");
+	ASSERT_EQ(skel->bss->t1_reg3, 4, "t1 reg3");
+	ASSERT_EQ(skel->bss->t1_ret, 10, "t1 ret");
+
+	ASSERT_EQ(skel->bss->t2_a, 1, "t2:a");
+	ASSERT_EQ(skel->bss->t2_b_a, 2, "t2:b.a");
+	ASSERT_EQ(skel->bss->t2_b_b, 3, "t2:b.b");
+	ASSERT_EQ(skel->bss->t2_c, 4, "t2:c");
+	ASSERT_EQ(skel->bss->t2_ret, 10, "t2 ret");
+
+	ASSERT_EQ(skel->bss->t3_a, 1, "t3:a");
+	ASSERT_EQ(skel->bss->t3_b, 4, "t3:b");
+	ASSERT_EQ(skel->bss->t3_c_a, 2, "t3:c.a");
+	ASSERT_EQ(skel->bss->t3_c_b, 3, "t3:c.b");
+	ASSERT_EQ(skel->bss->t3_ret, 10, "t3 ret");
+
+	ASSERT_EQ(skel->bss->t4_a_a, 10, "t4:a.a");
+	ASSERT_EQ(skel->bss->t4_b, 1, "t4:b");
+	ASSERT_EQ(skel->bss->t4_c, 2, "t4:c");
+	ASSERT_EQ(skel->bss->t4_d, 3, "t4:d");
+	ASSERT_EQ(skel->bss->t4_e_a, 2, "t4:e.a");
+	ASSERT_EQ(skel->bss->t4_e_b, 3, "t4:e.b");
+	ASSERT_EQ(skel->bss->t4_ret, 21, "t4 ret");
+
+	ASSERT_EQ(skel->bss->t5_ret, 1, "t5 ret");
+
+	tracing_struct__detach(skel);
+	tracing_struct__destroy(skel);
+}
+
+void test_tracing_struct(void)
+{
+	test_fentry();
+}
diff --git a/tools/testing/selftests/bpf/progs/tracing_struct.c b/tools/testing/selftests/bpf/progs/tracing_struct.c
new file mode 100644
index 000000000000..26d47a0ab616
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tracing_struct.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_helpers.h>
+
+struct bpf_testmod_struct_arg_1 {
+	int a;
+};
+struct bpf_testmod_struct_arg_2 {
+	long a;
+	long b;
+};
+
+long t1_a_a, t1_a_b, t1_b, t1_c, t1_ret, t1_nregs;
+__u64 t1_reg0, t1_reg1, t1_reg2, t1_reg3;
+long t2_a, t2_b_a, t2_b_b, t2_c, t2_ret;
+long t3_a, t3_b, t3_c_a, t3_c_b, t3_ret;
+long t4_a_a, t4_b, t4_c, t4_d, t4_e_a, t4_e_b, t4_ret;
+long t5_ret;
+
+SEC("fentry/bpf_testmod_test_struct_arg_1")
+int BPF_PROG2(test_struct_arg_1, struct bpf_testmod_struct_arg_2, a, int, b, int, c)
+{
+	t1_a_a = a.a;
+	t1_a_b = a.b;
+	t1_b = b;
+	t1_c = c;
+	return 0;
+}
+
+SEC("fexit/bpf_testmod_test_struct_arg_1")
+int BPF_PROG2(test_struct_arg_2, struct bpf_testmod_struct_arg_2, a, int, b, int, c, int, ret)
+{
+	t1_nregs =  bpf_get_func_arg_cnt(ctx);
+	bpf_get_func_arg(ctx, 0, &t1_reg0);
+	bpf_get_func_arg(ctx, 1, &t1_reg1);
+	bpf_get_func_arg(ctx, 2, &t1_reg2);
+	bpf_get_func_arg(ctx, 3, &t1_reg3);
+
+	t1_ret = ret;
+	return 0;
+}
+
+SEC("fentry/bpf_testmod_test_struct_arg_2")
+int BPF_PROG2(test_struct_arg_3, int, a, struct bpf_testmod_struct_arg_2, b, int, c)
+{
+	t2_a = a;
+	t2_b_a = b.a;
+	t2_b_b = b.b;
+	t2_c = c;
+	return 0;
+}
+
+SEC("fexit/bpf_testmod_test_struct_arg_2")
+int BPF_PROG2(test_struct_arg_4, int, a, struct bpf_testmod_struct_arg_2, b, int, c, int, ret)
+{
+	t2_ret = ret;
+	return 0;
+}
+
+SEC("fentry/bpf_testmod_test_struct_arg_3")
+int BPF_PROG2(test_struct_arg_5, int, a, int, b, struct bpf_testmod_struct_arg_2, c)
+{
+	t3_a = a;
+	t3_b = b;
+	t3_c_a = c.a;
+	t3_c_b = c.b;
+	return 0;
+}
+
+SEC("fexit/bpf_testmod_test_struct_arg_3")
+int BPF_PROG2(test_struct_arg_6, int, a, int, b, struct bpf_testmod_struct_arg_2, c, int, ret)
+{
+	t3_ret = ret;
+	return 0;
+}
+
+SEC("fentry/bpf_testmod_test_struct_arg_4")
+int BPF_PROG2(test_struct_arg_7, struct bpf_testmod_struct_arg_1, a, int, b,
+	     int, c, int, d, struct bpf_testmod_struct_arg_2, e)
+{
+	t4_a_a = a.a;
+	t4_b = b;
+	t4_c = c;
+	t4_d = d;
+	t4_e_a = e.a;
+	t4_e_b = e.b;
+	return 0;
+}
+
+SEC("fexit/bpf_testmod_test_struct_arg_4")
+int BPF_PROG2(test_struct_arg_8, struct bpf_testmod_struct_arg_1, a, int, b,
+	     int, c, int, d, struct bpf_testmod_struct_arg_2, e, int, ret)
+{
+	t4_ret = ret;
+	return 0;
+}
+
+SEC("fentry/bpf_testmod_test_struct_arg_5")
+int BPF_PROG2(test_struct_arg_9)
+{
+	return 0;
+}
+
+SEC("fexit/bpf_testmod_test_struct_arg_5")
+int BPF_PROG2(test_struct_arg_10, int, ret)
+{
+	t5_ret = ret;
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.30.2

Re: [PATCH bpf-next v3 6/7] selftests/bpf: Add struct argument tests with fentry/fexit programs.

[Daniel Borkmann]

On 8/28/22 4:55 AM, Yonghong Song wrote:
> Add various struct argument tests with fentry/fexit programs.
> Also add one test with a kernel func which does not have any
> argument to test BPF_PROG2 macro in such situation.
> 
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>   .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  48 ++++++++
>   .../selftests/bpf/prog_tests/tracing_struct.c |  63 ++++++++++
>   .../selftests/bpf/progs/tracing_struct.c      | 114 ++++++++++++++++++
>   3 files changed, 225 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/tracing_struct.c
>   create mode 100644 tools/testing/selftests/bpf/progs/tracing_struct.c
> 

For s390x these tests need to be deny-listed due to missing trampoline support..

   All error logs:
   test_fentry:PASS:tracing_struct__open_and_load 0 nsec
   libbpf: prog 'test_struct_arg_1': failed to attach: ERROR: strerror_r(-524)=22
   libbpf: prog 'test_struct_arg_1': failed to auto-attach: -524
   test_fentry:FAIL:tracing_struct__attach unexpected error: -524 (errno 524)
   #209     tracing_struct:FAIL
   Summary: 189/972 PASSED, 27 SKIPPED, 1 FAILED

However, looks like the no_alu32 ones on x86 fail:

   [...]
   #207     trace_printk:OK
   #208     trace_vprintk:OK
   test_fentry:PASS:tracing_struct__open_and_load 0 nsec
   test_fentry:PASS:tracing_struct__attach 0 nsec
   trigger_module_test_read:PASS:testmod_file_open 0 nsec
   test_fentry:PASS:trigger_read 0 nsec
   test_fentry:PASS:t1:a.a 0 nsec
   test_fentry:PASS:t1:a.b 0 nsec
   test_fentry:PASS:t1:b 0 nsec
   test_fentry:PASS:t1:c 0 nsec
   test_fentry:PASS:t1 nregs 0 nsec
   test_fentry:PASS:t1 reg0 0 nsec
   test_fentry:PASS:t1 reg1 0 nsec
   test_fentry:FAIL:t1 reg2 unexpected t1 reg2: actual 7327499336969879553 != expected 1
   test_fentry:PASS:t1 reg3 0 nsec
   test_fentry:PASS:t1 ret 0 nsec
   test_fentry:PASS:t2:a 0 nsec
   test_fentry:PASS:t2:b.a 0 nsec
   test_fentry:PASS:t2:b.b 0 nsec
   test_fentry:PASS:t2:c 0 nsec
   test_fentry:PASS:t2 ret 0 nsec
   test_fentry:PASS:t3:a 0 nsec
   test_fentry:PASS:t3:b 0 nsec
   test_fentry:PASS:t3:c.a 0 nsec
   test_fentry:PASS:t3:c.b 0 nsec
   test_fentry:PASS:t3 ret 0 nsec
   test_fentry:PASS:t4:a.a 0 nsec
   test_fentry:PASS:t4:b 0 nsec
   test_fentry:PASS:t4:c 0 nsec
   test_fentry:PASS:t4:d 0 nsec
   test_fentry:PASS:t4:e.a 0 nsec
   test_fentry:PASS:t4:e.b 0 nsec
   test_fentry:PASS:t4 ret 0 nsec
   test_fentry:PASS:t5 ret 0 nsec
   #209     tracing_struct:FAIL
   #210     trampoline_count:OK
   [...]

Re: [PATCH bpf-next v3 6/7] selftests/bpf: Add struct argument tests with fentry/fexit programs.

[Jiri Olsa]

On Tue, Aug 30, 2022 at 12:12:08AM +0200, Daniel Borkmann wrote:
> On 8/28/22 4:55 AM, Yonghong Song wrote:
> > Add various struct argument tests with fentry/fexit programs.
> > Also add one test with a kernel func which does not have any
> > argument to test BPF_PROG2 macro in such situation.
> > 
> > Signed-off-by: Yonghong Song <yhs@fb.com>
> > ---
> >   .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  48 ++++++++
> >   .../selftests/bpf/prog_tests/tracing_struct.c |  63 ++++++++++
> >   .../selftests/bpf/progs/tracing_struct.c      | 114 ++++++++++++++++++
> >   3 files changed, 225 insertions(+)
> >   create mode 100644 tools/testing/selftests/bpf/prog_tests/tracing_struct.c
> >   create mode 100644 tools/testing/selftests/bpf/progs/tracing_struct.c
> > 
> 
> For s390x these tests need to be deny-listed due to missing trampoline support..
> 
>   All error logs:
>   test_fentry:PASS:tracing_struct__open_and_load 0 nsec
>   libbpf: prog 'test_struct_arg_1': failed to attach: ERROR: strerror_r(-524)=22
>   libbpf: prog 'test_struct_arg_1': failed to auto-attach: -524
>   test_fentry:FAIL:tracing_struct__attach unexpected error: -524 (errno 524)
>   #209     tracing_struct:FAIL
>   Summary: 189/972 PASSED, 27 SKIPPED, 1 FAILED
> 
> However, looks like the no_alu32 ones on x86 fail:
> 
>   [...]
>   #207     trace_printk:OK
>   #208     trace_vprintk:OK
>   test_fentry:PASS:tracing_struct__open_and_load 0 nsec
>   test_fentry:PASS:tracing_struct__attach 0 nsec
>   trigger_module_test_read:PASS:testmod_file_open 0 nsec
>   test_fentry:PASS:trigger_read 0 nsec
>   test_fentry:PASS:t1:a.a 0 nsec
>   test_fentry:PASS:t1:a.b 0 nsec
>   test_fentry:PASS:t1:b 0 nsec
>   test_fentry:PASS:t1:c 0 nsec
>   test_fentry:PASS:t1 nregs 0 nsec
>   test_fentry:PASS:t1 reg0 0 nsec
>   test_fentry:PASS:t1 reg1 0 nsec
>   test_fentry:FAIL:t1 reg2 unexpected t1 reg2: actual 7327499336969879553 != expected 1

I'm getting the same, I think it's because the argument is int (4 bytes)
while the register is 8, we need to cast to int before we check for the
argument value

jirka

>   test_fentry:PASS:t1 reg3 0 nsec
>   test_fentry:PASS:t1 ret 0 nsec
>   test_fentry:PASS:t2:a 0 nsec
>   test_fentry:PASS:t2:b.a 0 nsec
>   test_fentry:PASS:t2:b.b 0 nsec
>   test_fentry:PASS:t2:c 0 nsec
>   test_fentry:PASS:t2 ret 0 nsec
>   test_fentry:PASS:t3:a 0 nsec
>   test_fentry:PASS:t3:b 0 nsec
>   test_fentry:PASS:t3:c.a 0 nsec
>   test_fentry:PASS:t3:c.b 0 nsec
>   test_fentry:PASS:t3 ret 0 nsec
>   test_fentry:PASS:t4:a.a 0 nsec
>   test_fentry:PASS:t4:b 0 nsec
>   test_fentry:PASS:t4:c 0 nsec
>   test_fentry:PASS:t4:d 0 nsec
>   test_fentry:PASS:t4:e.a 0 nsec
>   test_fentry:PASS:t4:e.b 0 nsec
>   test_fentry:PASS:t4 ret 0 nsec
>   test_fentry:PASS:t5 ret 0 nsec
>   #209     tracing_struct:FAIL
>   #210     trampoline_count:OK
>   [...]

Re: [PATCH bpf-next v3 6/7] selftests/bpf: Add struct argument tests with fentry/fexit programs.

[Yonghong Song]

On 8/30/22 5:11 AM, Jiri Olsa wrote:
> On Tue, Aug 30, 2022 at 12:12:08AM +0200, Daniel Borkmann wrote:
>> On 8/28/22 4:55 AM, Yonghong Song wrote:
>>> Add various struct argument tests with fentry/fexit programs.
>>> Also add one test with a kernel func which does not have any
>>> argument to test BPF_PROG2 macro in such situation.
>>>
>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>> ---
>>>    .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  48 ++++++++
>>>    .../selftests/bpf/prog_tests/tracing_struct.c |  63 ++++++++++
>>>    .../selftests/bpf/progs/tracing_struct.c      | 114 ++++++++++++++++++
>>>    3 files changed, 225 insertions(+)
>>>    create mode 100644 tools/testing/selftests/bpf/prog_tests/tracing_struct.c
>>>    create mode 100644 tools/testing/selftests/bpf/progs/tracing_struct.c
>>>
>>
>> For s390x these tests need to be deny-listed due to missing trampoline support..
>>
>>    All error logs:
>>    test_fentry:PASS:tracing_struct__open_and_load 0 nsec
>>    libbpf: prog 'test_struct_arg_1': failed to attach: ERROR: strerror_r(-524)=22
>>    libbpf: prog 'test_struct_arg_1': failed to auto-attach: -524
>>    test_fentry:FAIL:tracing_struct__attach unexpected error: -524 (errno 524)
>>    #209     tracing_struct:FAIL
>>    Summary: 189/972 PASSED, 27 SKIPPED, 1 FAILED
>>
>> However, looks like the no_alu32 ones on x86 fail:
>>
>>    [...]
>>    #207     trace_printk:OK
>>    #208     trace_vprintk:OK
>>    test_fentry:PASS:tracing_struct__open_and_load 0 nsec
>>    test_fentry:PASS:tracing_struct__attach 0 nsec
>>    trigger_module_test_read:PASS:testmod_file_open 0 nsec
>>    test_fentry:PASS:trigger_read 0 nsec
>>    test_fentry:PASS:t1:a.a 0 nsec
>>    test_fentry:PASS:t1:a.b 0 nsec
>>    test_fentry:PASS:t1:b 0 nsec
>>    test_fentry:PASS:t1:c 0 nsec
>>    test_fentry:PASS:t1 nregs 0 nsec
>>    test_fentry:PASS:t1 reg0 0 nsec
>>    test_fentry:PASS:t1 reg1 0 nsec
>>    test_fentry:FAIL:t1 reg2 unexpected t1 reg2: actual 7327499336969879553 != expected 1
> 
> I'm getting the same, I think it's because the argument is int (4 bytes)
> while the register is 8, we need to cast to int before we check for the
> argument value

Good point! Indeed bpf_get_func_arg() gets a 64bit value and
I need to cast it to 32bit.


> 
> jirka
> 
>>    test_fentry:PASS:t1 reg3 0 nsec
>>    test_fentry:PASS:t1 ret 0 nsec
>>    test_fentry:PASS:t2:a 0 nsec
>>    test_fentry:PASS:t2:b.a 0 nsec
>>    test_fentry:PASS:t2:b.b 0 nsec
>>    test_fentry:PASS:t2:c 0 nsec
>>    test_fentry:PASS:t2 ret 0 nsec
>>    test_fentry:PASS:t3:a 0 nsec
>>    test_fentry:PASS:t3:b 0 nsec
>>    test_fentry:PASS:t3:c.a 0 nsec
>>    test_fentry:PASS:t3:c.b 0 nsec
>>    test_fentry:PASS:t3 ret 0 nsec
>>    test_fentry:PASS:t4:a.a 0 nsec
>>    test_fentry:PASS:t4:b 0 nsec
>>    test_fentry:PASS:t4:c 0 nsec
>>    test_fentry:PASS:t4:d 0 nsec
>>    test_fentry:PASS:t4:e.a 0 nsec
>>    test_fentry:PASS:t4:e.b 0 nsec
>>    test_fentry:PASS:t4 ret 0 nsec
>>    test_fentry:PASS:t5 ret 0 nsec
>>    #209     tracing_struct:FAIL
>>    #210     trampoline_count:OK
>>    [...]

Re: [PATCH bpf-next v3 6/7] selftests/bpf: Add struct argument tests with fentry/fexit programs.

[Yonghong Song]

On 8/29/22 3:12 PM, Daniel Borkmann wrote:
> On 8/28/22 4:55 AM, Yonghong Song wrote:
>> Add various struct argument tests with fentry/fexit programs.
>> Also add one test with a kernel func which does not have any
>> argument to test BPF_PROG2 macro in such situation.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  48 ++++++++
>>   .../selftests/bpf/prog_tests/tracing_struct.c |  63 ++++++++++
>>   .../selftests/bpf/progs/tracing_struct.c      | 114 ++++++++++++++++++
>>   3 files changed, 225 insertions(+)
>>   create mode 100644 
>> tools/testing/selftests/bpf/prog_tests/tracing_struct.c
>>   create mode 100644 tools/testing/selftests/bpf/progs/tracing_struct.c
>>
> 
> For s390x these tests need to be deny-listed due to missing trampoline 
> support..
> 
>    All error logs:
>    test_fentry:PASS:tracing_struct__open_and_load 0 nsec
>    libbpf: prog 'test_struct_arg_1': failed to attach: ERROR: 
> strerror_r(-524)=22
>    libbpf: prog 'test_struct_arg_1': failed to auto-attach: -524
>    test_fentry:FAIL:tracing_struct__attach unexpected error: -524 (errno 
> 524)
>    #209     tracing_struct:FAIL
>    Summary: 189/972 PASSED, 27 SKIPPED, 1 FAILED

Thanks. will add the test to deny list.

> 
> However, looks like the no_alu32 ones on x86 fail:
> 
>    [...]
>    #207     trace_printk:OK
>    #208     trace_vprintk:OK
>    test_fentry:PASS:tracing_struct__open_and_load 0 nsec
>    test_fentry:PASS:tracing_struct__attach 0 nsec
>    trigger_module_test_read:PASS:testmod_file_open 0 nsec
>    test_fentry:PASS:trigger_read 0 nsec
>    test_fentry:PASS:t1:a.a 0 nsec
>    test_fentry:PASS:t1:a.b 0 nsec
>    test_fentry:PASS:t1:b 0 nsec
>    test_fentry:PASS:t1:c 0 nsec
>    test_fentry:PASS:t1 nregs 0 nsec
>    test_fentry:PASS:t1 reg0 0 nsec
>    test_fentry:PASS:t1 reg1 0 nsec
>    test_fentry:FAIL:t1 reg2 unexpected t1 reg2: actual 
> 7327499336969879553 != expected 1
>    test_fentry:PASS:t1 reg3 0 nsec
>    test_fentry:PASS:t1 ret 0 nsec
>    test_fentry:PASS:t2:a 0 nsec
>    test_fentry:PASS:t2:b.a 0 nsec
>    test_fentry:PASS:t2:b.b 0 nsec
>    test_fentry:PASS:t2:c 0 nsec
>    test_fentry:PASS:t2 ret 0 nsec
>    test_fentry:PASS:t3:a 0 nsec
>    test_fentry:PASS:t3:b 0 nsec
>    test_fentry:PASS:t3:c.a 0 nsec
>    test_fentry:PASS:t3:c.b 0 nsec
>    test_fentry:PASS:t3 ret 0 nsec
>    test_fentry:PASS:t4:a.a 0 nsec
>    test_fentry:PASS:t4:b 0 nsec
>    test_fentry:PASS:t4:c 0 nsec
>    test_fentry:PASS:t4:d 0 nsec
>    test_fentry:PASS:t4:e.a 0 nsec
>    test_fentry:PASS:t4:e.b 0 nsec
>    test_fentry:PASS:t4 ret 0 nsec
>    test_fentry:PASS:t5 ret 0 nsec
>    #209     tracing_struct:FAIL
>    #210     trampoline_count:OK
>    [...]

Thanks for the headsup. Will check.

[PATCH bpf-next v3 7/7] selftests/bpf: Use BPF_PROG2 for some fentry programs without struct arguments

[Yonghong Song]

Use BPF_PROG2 instead of BPF_PROG for programs in progs/timer.c
to test BPF_PROG2 for cases without struct arguments.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/testing/selftests/bpf/progs/timer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/timer.c b/tools/testing/selftests/bpf/progs/timer.c
index 5f5309791649..560a63e95dc3 100644
--- a/tools/testing/selftests/bpf/progs/timer.c
+++ b/tools/testing/selftests/bpf/progs/timer.c
@@ -120,7 +120,7 @@ static int timer_cb1(void *map, int *key, struct bpf_timer *timer)
 }
 
 SEC("fentry/bpf_fentry_test1")
-int BPF_PROG(test1, int a)
+int BPF_PROG2(test1, int, a)
 {
 	struct bpf_timer *arr_timer, *lru_timer;
 	struct elem init = {};
@@ -247,7 +247,7 @@ int bpf_timer_test(void)
 }
 
 SEC("fentry/bpf_fentry_test2")
-int BPF_PROG(test2, int a, int b)
+int BPF_PROG2(test2, int, a, int, b)
 {
 	struct hmap_elem init = {}, *val;
 	int key = HTAB, key_malloc = HTAB_MALLOC;
-- 
2.30.2