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

[RFC PATCH bpf-next 0/7] bpf: Support struct value 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. struct argument size needs to be 16 or less so
it can fit in one or two registers. Based on analysis on llvm and
experiments, atruct argument size greater than 16 will be passed
as pointer to the struct.

Please see patch #4 and selftests for specific examples.

I labelled the patch as RFC so I can get some comments before proceeding.
The patch set is not complete as:
  (1). it does not support struct arguments which are passed in as pointers.
  (2). there might be some corner cases where on x86_64 even 16 bytes may
       pass by pointers. This needs further investigation.
  (3). tests are imcomplete, no fexit or fmod tests.

  [1] https://github.com/iovisor/bcc/issues/3657
  [2] https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/TargetInfo.cpp

Yonghong Song (7):
  bpf: Always return corresponding btf_type in __get_type_size()
  bpf: Add struct argument info in btf_func_model
  bpf: x86: Rename stack_size to regs_off in {save,restore}_regs()
  bpf: x86: Support in-register struct arguments
  bpf: arm64: No support of struct value argument
  bpf: Populate struct value info in btf_func_model
  selftests/bpf: Add struct value tests with fentry programs.

 arch/arm64/net/bpf_jit_comp.c                 |   4 +
 arch/x86/net/bpf_jit_comp.c                   | 173 ++++++++++++++++--
 include/linux/bpf.h                           |   9 +
 kernel/bpf/btf.c                              |  54 +++++-
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  37 ++++
 .../selftests/bpf/prog_tests/tracing_struct.c |  51 ++++++
 .../selftests/bpf/progs/tracing_struct.c      |  64 +++++++
 7 files changed, 365 insertions(+), 27 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

[RFC PATCH bpf-next 1/7] bpf: Always return corresponding btf_type in __get_type_size()

[Yonghong Song]

Currently in funciton __get_type_size(), the corresponding
btf_type is returned only in invalid cases. Let us always
return btf_type regardless of valid or invalid cases.
Such a new functionality will be used in subsequent patches.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 kernel/bpf/btf.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 7ac971ea98d1..3bbcc985a651 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5864,26 +5864,25 @@ bool btf_struct_ids_match(struct bpf_verifier_log *log,
 }
 
 static int __get_type_size(struct btf *btf, u32 btf_id,
-			   const struct btf_type **bad_type)
+			   const struct btf_type **ret_type)
 {
 	const struct btf_type *t;
 
+	*ret_type = btf_type_by_id(btf, 0);
 	if (!btf_id)
 		/* void */
 		return 0;
 	t = btf_type_by_id(btf, btf_id);
 	while (t && btf_type_is_modifier(t))
 		t = btf_type_by_id(btf, t->type);
-	if (!t) {
-		*bad_type = btf_type_by_id(btf, 0);
+	if (!t)
 		return -EINVAL;
-	}
+	*ret_type = t;
 	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))
 		return t->size;
-	*bad_type = t;
 	return -EINVAL;
 }
 
-- 
2.30.2

[RFC PATCH bpf-next 2/7] bpf: Add struct argument info in btf_func_model

[Yonghong Song]

Add struct argument information in btf_func_model and 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 20c26aed7896..173b42cf3940 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -726,10 +726,19 @@ enum bpf_cgroup_storage_type {
  */
 #define MAX_BPF_FUNC_REG_ARGS 5
 
+/* The maximum number of struct arguments a single function may have. */
+#define MAX_BPF_FUNC_STRUCT_ARGS 2
+
 struct btf_func_model {
 	u8 ret_size;
 	u8 nr_args;
 	u8 arg_size[MAX_BPF_FUNC_ARGS];
+	/* The struct_arg_idx should be in increasing order like (0, 2, ...).
+	 * The struct_arg_bsize encodes the struct field byte size
+	 * for the corresponding struct argument index.
+	 */
+	u8 struct_arg_idx[MAX_BPF_FUNC_STRUCT_ARGS];
+	u8 struct_arg_bsize[MAX_BPF_FUNC_STRUCT_ARGS];
 };
 
 /* Restore arguments before returning from trampoline to let original function
-- 
2.30.2

[RFC PATCH bpf-next 3/7] bpf: x86: Rename stack_size to regs_off in {save,restore}_regs()

[Yonghong Song]

Rename argument 'stack_size' to 'regs_off' in save_regs() and
restore_regs() since in reality 'stack_size' represents a particular
stack offset and 'regs_off' is the one in the caller argument.
Leter, another stack offset will be added to these two functions
for saving and restoring struct argument values.

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

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index c1f6c1c51d99..2657b58001cf 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1749,7 +1749,7 @@ 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 regs_off)
 {
 	int i;
 	/* Store function arguments to stack.
@@ -1761,11 +1761,11 @@ static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
 		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));
+			 -(regs_off - i * 8));
 }
 
 static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
-			 int stack_size)
+			 int regs_off)
 {
 	int i;
 
@@ -1778,7 +1778,7 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
 		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));
+			 -(regs_off - i * 8));
 }
 
 static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
-- 
2.30.2

[RFC PATCH bpf-next 4/7] bpf: x86: Support in-register struct arguments

[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.

To support struct value argument and make bpf programs
easy to write, the bpf program function parameter is
changed from struct type to a pointer to struct type.
The following is a simplified example.

In one of later selftests, we have a bpf_testmod function:
    struct bpf_testmod_struct_arg_2 {
        long a;
        long b;
    };
    noinline void
    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;
    }

When a bpf program is attached to the bpf_testmod function
bpf_testmod_test_struct_arg_2(), the bpf program may look like
    SEC("fentry/bpf_testmod_test_struct_arg_2")
    int BPF_PROG(test_struct_arg_2, 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;
    }

Basically struct value becomes a pointer to the struct.
The trampoline stack will be increased to store the stack values and
the pointer to these values will be saved in the stack slot corresponding
to that argument.

The main changes are in save_regs() and restore_regs(). The following
illustrated the trampoline asm codes for save_regs() and restore_regs().
save_regs():
    /* first argument */
    mov    DWORD PTR [rbp-0x18],edi
    /* second argument: struct, save actual values and put the pointer to the slot */
    lea    rax,[rbp-0x40]
    mov    QWORD PTR [rbp-0x10],rax
    mov    QWORD PTR [rbp-0x40],rsi
    mov    QWORD PTR [rbp-0x38],rdx
    /* third argument */
    mov    DWORD PTR [rbp-0x8],esi
restore_regs():
    mov    edi,DWORD PTR [rbp-0x18]
    mov    rsi,QWORD PTR [rbp-0x40]
    mov    rdx,QWORD PTR [rbp-0x38]
    mov    esi,DWORD PTR [rbp-0x8]

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

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 2657b58001cf..55521964ee3c 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -37,6 +37,11 @@ static u8 *emit_code(u8 *ptr, u32 bytes, unsigned int len)
 #define EMIT3(b1, b2, b3)	EMIT((b1) + ((b2) << 8) + ((b3) << 16), 3)
 #define EMIT4(b1, b2, b3, b4)   EMIT((b1) + ((b2) << 8) + ((b3) << 16) + ((b4) << 24), 4)
 
+#define EMIT2_UPDATE_PROG(bytes, len) \
+	do { *prog = emit_code(*prog, bytes, len); } while (0)
+#define EMIT4_UPDATE_PROG(b1, b2, b3, b4) \
+	EMIT2_UPDATE_PROG((b1) + ((b2) << 8) + ((b3) << 16) + ((b4) << 24), 4)
+
 #define EMIT1_off32(b1, off) \
 	do { EMIT1(b1); EMIT(off, 4); } while (0)
 #define EMIT2_off32(b1, b2, off) \
@@ -1748,37 +1753,156 @@ st:			if (is_imm8(insn->off))
 	return proglen;
 }
 
-static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
-		      int regs_off)
+static void __save_normal_arg_regs(const struct btf_func_model *m, u8 **prog,
+				   int curr_arg_idx, int nr_args,
+				   int curr_reg_idx, int regs_off)
 {
-	int i;
+	int i, arg_idx;
+
 	/* 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]),
+	for (i = 0; i < nr_args; i++) {
+		arg_idx = curr_arg_idx + i;
+		emit_stx(prog, bytes_to_bpf_size(m->arg_size[arg_idx]),
 			 BPF_REG_FP,
-			 i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
-			 -(regs_off - i * 8));
+			 curr_reg_idx == 5 ? X86_REG_R9 : BPF_REG_1 + curr_reg_idx,
+			 -(regs_off - arg_idx * 8));
+		curr_reg_idx++;
+	}
 }
 
-static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
-			 int regs_off)
+static void __save_struct_arg_regs(u8 **prog, int curr_reg_idx, int nr_regs,
+				   int struct_val_off, int stack_start_idx)
 {
-	int i;
+	int i, reg_idx;
+
+	/* Save struct registers to stack.
+	 * For example, argument 1 (second argument) size is 16 which occupies two
+	 * registers, these two register values will be saved in stack.
+	 * mov QWORD PTR [rbp-0x40],rsi
+	 * mov QWORD PTR [rbp-0x38],rdx
+	 */
+	for (i = 0; i < nr_regs; i++) {
+		reg_idx = curr_reg_idx + i;
+		emit_stx(prog, bytes_to_bpf_size(8),
+			 BPF_REG_FP,
+			 reg_idx == 5 ? X86_REG_R9 : BPF_REG_1 + reg_idx,
+			 -(struct_val_off - stack_start_idx * 8));
+		stack_start_idx++;
+	}
+}
+
+static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
+		      int regs_off, int struct_val_off)
+{
+	int curr_arg_idx, curr_reg_idx, curr_s_stack_off;
+	int s_size, s_arg_idx, s_arg_nregs;
+
+	curr_arg_idx = curr_reg_idx = curr_s_stack_off = 0;
+	for (int i = 0; i < MAX_BPF_FUNC_STRUCT_ARGS; i++) {
+		s_size = m->struct_arg_bsize[i];
+		if (!s_size)
+			return __save_normal_arg_regs(m, prog, curr_arg_idx, nr_args - curr_arg_idx,
+						      curr_reg_idx, regs_off);
+
+		s_arg_idx = m->struct_arg_idx[i];
+		s_arg_nregs = (s_size + 7) / 8;
+
+		__save_normal_arg_regs(m, prog, curr_arg_idx, s_arg_idx - curr_arg_idx,
+				       curr_reg_idx, regs_off);
+
+		/* lea rax, [rbp - struct_val_off] */
+		EMIT4_UPDATE_PROG(0x48, 0x8D, 0x45, -(struct_val_off - curr_s_stack_off * 8));
+		/* The struct value is converted to a pointer argument.
+		 * mov QWORD PTR [rbp - s_arg_idx * 8],rax
+		 */
+		emit_stx(prog, bytes_to_bpf_size(8),
+			 BPF_REG_FP, BPF_REG_0, -(regs_off - s_arg_idx * 8));
+
+		__save_struct_arg_regs(prog, curr_reg_idx + s_arg_idx - curr_arg_idx, s_arg_nregs,
+				       struct_val_off, curr_s_stack_off);
+		curr_reg_idx += s_arg_idx - curr_arg_idx + s_arg_nregs;
+		curr_s_stack_off += s_arg_nregs;
+		curr_arg_idx = s_arg_idx + 1;
+	}
+
+	__save_normal_arg_regs(m, prog, curr_arg_idx, nr_args - curr_arg_idx, curr_reg_idx,
+			       regs_off);
+}
+
+static void __restore_normal_arg_regs(const struct btf_func_model *m, u8 **prog, int curr_arg_idx,
+				      int nr_args, int curr_reg_idx, int regs_off)
+{
+	int i, arg_idx;
 
 	/* 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,
+	for (i = 0; i < nr_args; i++) {
+		arg_idx = curr_arg_idx + i;
+		emit_ldx(prog, bytes_to_bpf_size(m->arg_size[arg_idx]),
+			 curr_reg_idx  == 5 ? X86_REG_R9 : BPF_REG_1 + curr_reg_idx,
+			 BPF_REG_FP,
+			 -(regs_off - arg_idx * 8));
+		curr_reg_idx++;
+	}
+}
+
+static void __restore_struct_arg_regs(u8 **prog, int curr_reg_idx, int nr_regs,
+				      int struct_val_off, int stack_start_idx)
+{
+	int i, reg_idx;
+
+	/* Restore struct values from stack.
+	 * For example, argument 1 (second argument) size is 16 which occupies two
+	 * registers, these two register values will be restored to the original
+	 * registers.
+	 * mov rsi,QWORD PTR [rbp-0x40]
+	 * mov rdx,QWORD PTR [rbp-0x38]
+	 */
+	for (i = 0; i < nr_regs; i++) {
+		reg_idx = curr_reg_idx + i;
+		emit_ldx(prog, bytes_to_bpf_size(8),
+			 i == reg_idx ? X86_REG_R9 : BPF_REG_1 + reg_idx,
 			 BPF_REG_FP,
-			 -(regs_off - i * 8));
+			 -(struct_val_off - stack_start_idx * 8));
+		stack_start_idx++;
+	}
+}
+
+static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
+			 int regs_off, int struct_val_off)
+{
+	int curr_arg_idx, curr_reg_idx, curr_s_stack_off;
+	int s_size, s_arg_idx, s_arg_nregs;
+
+	curr_arg_idx = curr_reg_idx = curr_s_stack_off = 0;
+	for (int i = 0; i < MAX_BPF_FUNC_STRUCT_ARGS; i++) {
+		s_size = m->struct_arg_bsize[i];
+		if (!s_size)
+			return __restore_normal_arg_regs(m, prog, curr_arg_idx,
+							 nr_args - curr_arg_idx,
+							 curr_reg_idx, regs_off);
+
+		s_arg_idx = m->struct_arg_idx[i];
+		s_arg_nregs = (s_size + 7) / 8;
+
+		__restore_normal_arg_regs(m, prog, curr_arg_idx, s_arg_idx - curr_arg_idx,
+					  curr_reg_idx, regs_off);
+		__restore_struct_arg_regs(prog, curr_reg_idx + s_arg_idx - curr_arg_idx,
+					  s_arg_nregs, struct_val_off, curr_s_stack_off);
+		curr_reg_idx += s_arg_idx - curr_arg_idx + s_arg_nregs;
+		curr_s_stack_off += s_arg_nregs;
+		curr_arg_idx = s_arg_idx + 1;
+	}
+
+	__restore_normal_arg_regs(m, prog, curr_arg_idx, nr_args - curr_arg_idx, curr_reg_idx,
+				  regs_off);
 }
 
 static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
@@ -2020,6 +2144,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	struct bpf_tramp_links *fentry = &tlinks[BPF_TRAMP_FENTRY];
 	struct bpf_tramp_links *fexit = &tlinks[BPF_TRAMP_FEXIT];
 	struct bpf_tramp_links *fmod_ret = &tlinks[BPF_TRAMP_MODIFY_RETURN];
+	int struct_val_off, extra_nregs = 0, s_bsize;
 	u8 **branches = NULL;
 	u8 *prog;
 	bool save_ret;
@@ -2028,6 +2153,15 @@ 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_STRUCT_ARGS; i++) {
+		s_bsize = m->struct_arg_bsize[i];
+		if (!s_bsize)
+			break;
+		extra_nregs += (s_bsize + 7) / 8 - 1;
+	}
+	if (nr_args + extra_nregs > 6)
+		return -ENOTSUPP;
+
 	/* Generated trampoline stack layout:
 	 *
 	 * RBP + 8         [ return address  ]
@@ -2066,6 +2200,11 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	stack_size += (sizeof(struct bpf_tramp_run_ctx) + 7) & ~0x7;
 	run_ctx_off = stack_size;
 
+	/* For structure argument */
+	for (i = 0; i < MAX_BPF_FUNC_STRUCT_ARGS; i++)
+		stack_size += (m->struct_arg_bsize[i] + 7) & ~0x7;
+	struct_val_off = stack_size;
+
 	if (flags & BPF_TRAMP_F_SKIP_FRAME) {
 		/* skip patched call instruction and point orig_call to actual
 		 * body of the kernel function.
@@ -2101,7 +2240,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 		emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -ip_off);
 	}
 
-	save_regs(m, &prog, nr_args, regs_off);
+	save_regs(m, &prog, nr_args, regs_off, struct_val_off);
 
 	if (flags & BPF_TRAMP_F_CALL_ORIG) {
 		/* arg1: mov rdi, im */
@@ -2131,7 +2270,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	}
 
 	if (flags & BPF_TRAMP_F_CALL_ORIG) {
-		restore_regs(m, &prog, nr_args, regs_off);
+		restore_regs(m, &prog, nr_args, regs_off, struct_val_off);
 
 		if (flags & BPF_TRAMP_F_ORIG_STACK) {
 			emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
@@ -2172,7 +2311,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 		}
 
 	if (flags & BPF_TRAMP_F_RESTORE_REGS)
-		restore_regs(m, &prog, nr_args, regs_off);
+		restore_regs(m, &prog, nr_args, regs_off, struct_val_off);
 
 	/* This needs to be done regardless. If there were fmod_ret programs,
 	 * the return value is only updated on the stack and still needs to be
-- 
2.30.2

[RFC PATCH bpf-next 5/7] bpf: arm64: No support of struct value argument

[Yonghong Song]

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

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

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 7ca8779ae34f..27bda6b755ae 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -1982,6 +1982,10 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
 	if (nargs > 8)
 		return -ENOTSUPP;
 
+	/* don't support struct argument */
+	if (m->struct_arg_bsize[0])
+		return -ENOTSUPP;
+
 	ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags);
 	if (ret < 0)
 		return ret;
-- 
2.30.2

[RFC PATCH bpf-next 6/7] bpf: Populate struct value info in btf_func_model

[Yonghong Song]

Add struct value support in btf_ctx_access() and btf_distill_func_proto().
Reject if a struct argument size is greater than 16 as struct size greater than
16 likely passed in memory ([1], see function X86_64ABIInfo::postMerge()).

 [1] https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/TargetInfo.cpp

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 kernel/bpf/btf.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 3bbcc985a651..c4c19c89611b 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5339,7 +5339,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	struct bpf_verifier_log *log = info->log;
 	const struct btf_param *args;
 	const char *tag_value;
-	u32 nr_args, arg;
+	u32 nr_args, arg, curr_tid = 0;
 	int i, ret;
 
 	if (off % 8) {
@@ -5385,6 +5385,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 			 */
 			if (!t)
 				return true;
+			curr_tid = t->type;
 			t = btf_type_by_id(btf, t->type);
 			break;
 		case BPF_MODIFY_RETURN:
@@ -5394,7 +5395,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 			if (!t)
 				return false;
 
-			t = btf_type_skip_modifiers(btf, t->type, NULL);
+			t = btf_type_skip_modifiers(btf, t->type, &curr_tid);
 			if (!btf_type_is_small_int(t)) {
 				bpf_log(log,
 					"ret type %s not allowed for fmod_ret\n",
@@ -5411,15 +5412,25 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		if (!t)
 			/* Default prog with MAX_BPF_FUNC_REG_ARGS args */
 			return true;
+		curr_tid = args[arg].type;
 		t = btf_type_by_id(btf, args[arg].type);
 	}
 
 	/* skip modifiers */
-	while (btf_type_is_modifier(t))
+	while (btf_type_is_modifier(t)) {
+		curr_tid = t->type;
 		t = btf_type_by_id(btf, t->type);
+	}
 	if (btf_type_is_small_int(t) || btf_is_any_enum(t))
 		/* accessing a scalar */
 		return true;
+	if (__btf_type_is_struct(t) && curr_tid) {
+		info->reg_type = PTR_TO_BTF_ID;
+		info->btf = btf;
+		info->btf_id = curr_tid;
+		return true;
+	}
+
 	if (!btf_type_is_ptr(t)) {
 		bpf_log(log,
 			"func '%s' arg%d '%s' has type %s. Only pointer access is allowed\n",
@@ -5878,7 +5889,7 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
 	if (!t)
 		return -EINVAL;
 	*ret_type = t;
-	if (btf_type_is_ptr(t))
+	if (btf_type_is_ptr(t) || __btf_type_is_struct(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))
@@ -5894,9 +5905,14 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 {
 	const struct btf_param *args;
 	const struct btf_type *t;
-	u32 i, nargs;
+	u32 i, j = 0, nargs;
 	int ret;
 
+	for (i = 0; i < MAX_BPF_FUNC_STRUCT_ARGS; i++) {
+		m->struct_arg_idx[i] = 0;
+		m->struct_arg_bsize[i] = 0;
+	}
+
 	if (!func) {
 		/* BTF function prototype doesn't match the verifier types.
 		 * Fall back to MAX_BPF_FUNC_REG_ARGS u64 args.
@@ -5944,6 +5960,25 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 				tname);
 			return -EINVAL;
 		}
+		if (__btf_type_is_struct(t)) {
+			if (t->size > 16) {
+				bpf_log(log,
+					"The function %s arg%d struct size exceeds 16 bytes.\n",
+					tname, i);
+				return -EINVAL;
+			}
+
+			if (j == MAX_BPF_FUNC_STRUCT_ARGS) {
+				bpf_log(log,
+					"The function %s has more than %d struct/union args.\n",
+					tname, MAX_BPF_FUNC_STRUCT_ARGS);
+				return -EINVAL;
+			}
+
+			m->struct_arg_idx[j] = i;
+			m->struct_arg_bsize[j] = t->size;
+			j++;
+		}
 		m->arg_size[i] = ret;
 	}
 	m->nr_args = nargs;
-- 
2.30.2

[RFC PATCH bpf-next 7/7] selftests/bpf: Add struct value tests with fentry programs.

[Yonghong Song]

The fexit tests will be added later.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 37 +++++++++++
 .../selftests/bpf/prog_tests/tracing_struct.c | 51 +++++++++++++++
 .../selftests/bpf/progs/tracing_struct.c      | 64 +++++++++++++++++++
 3 files changed, 152 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..8ba5391023df 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -18,6 +18,36 @@ 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 void
+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;
+}
+
+noinline void
+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;
+}
+
+noinline void
+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;
+}
+
+noinline void
+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;
+}
 
 noinline void
 bpf_testmod_test_mod_kfunc(int i)
@@ -98,11 +128,18 @@ 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++;
 
+	bpf_testmod_test_struct_arg_1(struct_arg2, 1, 4);
+	bpf_testmod_test_struct_arg_2(1, struct_arg2, 4);
+	bpf_testmod_test_struct_arg_3(1, 4, struct_arg2);
+	bpf_testmod_test_struct_arg_4(struct_arg1, 1, 2, 3, struct_arg2);
+
 	/* 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..7c0ee156644f
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
@@ -0,0 +1,51 @@
+// 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->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->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->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");
+
+	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..8c2591aa93c3
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tracing_struct.c
@@ -0,0 +1,64 @@
+// 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;
+long t2_a, t2_b_a, t2_b_b, t2_c;
+long t3_a, t3_b, t3_c_a, t3_c_b;
+long t4_a_a, t4_b, t4_c, t4_d, t4_e_a, t4_e_b;
+
+SEC("fentry/bpf_testmod_test_struct_arg_1")
+int BPF_PROG(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("fentry/bpf_testmod_test_struct_arg_2")
+int BPF_PROG(test_struct_arg_2, 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("fentry/bpf_testmod_test_struct_arg_3")
+int BPF_PROG(test_struct_arg_3, 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("fentry/bpf_testmod_test_struct_arg_4")
+int BPF_PROG(test_struct_arg_4, 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;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.30.2

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

[Kui-Feng Lee]

On Tue, 2022-07-26 at 10:11 -0700, Yonghong Song wrote:
> 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. struct argument size needs to be 16 or less so
> it can fit in one or two registers. Based on analysis on llvm and
> experiments, atruct argument size greater than 16 will be passed
> as pointer to the struct.

Is it possible to force llvm to always pass a pointer to a struct over
8 bytes (the size of single register) for the BPF traget?

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

[Yonghong Song]

On 7/28/22 8:46 AM, Kui-Feng Lee wrote:
> On Tue, 2022-07-26 at 10:11 -0700, Yonghong Song wrote:
>> 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. struct argument size needs to be 16 or less so
>> it can fit in one or two registers. Based on analysis on llvm and
>> experiments, atruct argument size greater than 16 will be passed
>> as pointer to the struct.
> 
> Is it possible to force llvm to always pass a pointer to a struct over
> 8 bytes (the size of single register) for the BPF traget?

This is already the case for bpf target. Any struct parameter (1 byte, 2 
bytes, ..., 8 types, ..., 16 bytes, ...) will be passed as a reference.

But this is not the case for most other architectures. For example, for
x86_64, in most cases, struct size <= 16 will be passed with two
registers instead of as a reference.

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

[Kui-Feng Lee]

On Thu, 2022-07-28 at 10:46 -0700, Yonghong Song wrote:
> 
> 
> On 7/28/22 8:46 AM, Kui-Feng Lee wrote:
> > On Tue, 2022-07-26 at 10:11 -0700, Yonghong Song wrote:
> > > 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. struct argument size needs to be 16 or less so
> > > it can fit in one or two registers. Based on analysis on llvm and
> > > experiments, atruct argument size greater than 16 will be passed
> > > as pointer to the struct.
> > 
> > Is it possible to force llvm to always pass a pointer to a struct
> > over
> > 8 bytes (the size of single register) for the BPF traget?
> 
> This is already the case for bpf target. Any struct parameter (1
> byte, 2 
> bytes, ..., 8 types, ..., 16 bytes, ...) will be passed as a
> reference.
> 
> But this is not the case for most other architectures. For example,
> for
> x86_64, in most cases, struct size <= 16 will be passed with two
> registers instead of as a reference.

I ask this question because you modify the signature of a bpf program
to a pointer to a struct in patch #4.  Is that necessary if the
compiler passes a struct paramter as a reference?

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

[Yonghong Song]

On 7/28/22 12:57 PM, Kui-Feng Lee wrote:
> On Thu, 2022-07-28 at 10:46 -0700, Yonghong Song wrote:
>>
>>
>> On 7/28/22 8:46 AM, Kui-Feng Lee wrote:
>>> On Tue, 2022-07-26 at 10:11 -0700, Yonghong Song wrote:
>>>> 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. struct argument size needs to be 16 or less so
>>>> it can fit in one or two registers. Based on analysis on llvm and
>>>> experiments, atruct argument size greater than 16 will be passed
>>>> as pointer to the struct.
>>>
>>> Is it possible to force llvm to always pass a pointer to a struct
>>> over
>>> 8 bytes (the size of single register) for the BPF traget?
>>
>> This is already the case for bpf target. Any struct parameter (1
>> byte, 2
>> bytes, ..., 8 types, ..., 16 bytes, ...) will be passed as a
>> reference.
>>
>> But this is not the case for most other architectures. For example,
>> for
>> x86_64, in most cases, struct size <= 16 will be passed with two
>> registers instead of as a reference.
> 
> I ask this question because you modify the signature of a bpf program
> to a pointer to a struct in patch #4.  Is that necessary if the
> compiler passes a struct paramter as a reference?

Note that The true bpf program signature is only one.
long bpf_prog(<ctx_type> *ctx)
BPF_PROG is a macro for user friendly purpose.

For example,
+int BPF_PROG(test_struct_arg_1, struct bpf_testmod_struct_arg_2 *a, int 
b, int c)

after macro expansion:
int test_struct_arg_1(unsigned long long *ctx);
static __attribute__((always_inline))
typeof(test_struct_arg_1(0)) ____test_struct_arg_1(
   unsigned long long *ctx,
   struct bpf_testmod_struct_arg_2 *a, int b, int c);
...

Re: [RFC PATCH bpf-next 4/7] bpf: x86: Support in-register struct arguments

[Jiri Olsa]

On Tue, Jul 26, 2022 at 10:11:51AM -0700, Yonghong Song wrote:

SNIP

>  
> -static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
> -			 int regs_off)
> +static void __save_struct_arg_regs(u8 **prog, int curr_reg_idx, int nr_regs,
> +				   int struct_val_off, int stack_start_idx)
>  {
> -	int i;
> +	int i, reg_idx;
> +
> +	/* Save struct registers to stack.
> +	 * For example, argument 1 (second argument) size is 16 which occupies two
> +	 * registers, these two register values will be saved in stack.
> +	 * mov QWORD PTR [rbp-0x40],rsi
> +	 * mov QWORD PTR [rbp-0x38],rdx
> +	 */
> +	for (i = 0; i < nr_regs; i++) {
> +		reg_idx = curr_reg_idx + i;
> +		emit_stx(prog, bytes_to_bpf_size(8),
> +			 BPF_REG_FP,
> +			 reg_idx == 5 ? X86_REG_R9 : BPF_REG_1 + reg_idx,
> +			 -(struct_val_off - stack_start_idx * 8));
> +		stack_start_idx++;
> +	}
> +}
> +
> +static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
> +		      int regs_off, int struct_val_off)
> +{
> +	int curr_arg_idx, curr_reg_idx, curr_s_stack_off;
> +	int s_size, s_arg_idx, s_arg_nregs;
> +
> +	curr_arg_idx = curr_reg_idx = curr_s_stack_off = 0;
> +	for (int i = 0; i < MAX_BPF_FUNC_STRUCT_ARGS; i++) {
> +		s_size = m->struct_arg_bsize[i];
> +		if (!s_size)
> +			return __save_normal_arg_regs(m, prog, curr_arg_idx, nr_args - curr_arg_idx,
> +						      curr_reg_idx, regs_off);

could we just do break in here instead?

SNIP

> +
> +static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
> +			 int regs_off, int struct_val_off)
> +{
> +	int curr_arg_idx, curr_reg_idx, curr_s_stack_off;
> +	int s_size, s_arg_idx, s_arg_nregs;
> +
> +	curr_arg_idx = curr_reg_idx = curr_s_stack_off = 0;
> +	for (int i = 0; i < MAX_BPF_FUNC_STRUCT_ARGS; i++) {
> +		s_size = m->struct_arg_bsize[i];
> +		if (!s_size)
> +			return __restore_normal_arg_regs(m, prog, curr_arg_idx,
> +							 nr_args - curr_arg_idx,
> +							 curr_reg_idx, regs_off);

same here

jirka

> +
> +		s_arg_idx = m->struct_arg_idx[i];
> +		s_arg_nregs = (s_size + 7) / 8;
> +
> +		__restore_normal_arg_regs(m, prog, curr_arg_idx, s_arg_idx - curr_arg_idx,
> +					  curr_reg_idx, regs_off);
> +		__restore_struct_arg_regs(prog, curr_reg_idx + s_arg_idx - curr_arg_idx,
> +					  s_arg_nregs, struct_val_off, curr_s_stack_off);
> +		curr_reg_idx += s_arg_idx - curr_arg_idx + s_arg_nregs;
> +		curr_s_stack_off += s_arg_nregs;
> +		curr_arg_idx = s_arg_idx + 1;
> +	}
> +
> +	__restore_normal_arg_regs(m, prog, curr_arg_idx, nr_args - curr_arg_idx, curr_reg_idx,
> +				  regs_off);
>  }
>  

SNIP

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

[Kui-Feng Lee]

On Thu, 2022-07-28 at 16:30 -0700, Yonghong Song wrote:
> 
> 
> On 7/28/22 12:57 PM, Kui-Feng Lee wrote:
> > On Thu, 2022-07-28 at 10:46 -0700, Yonghong Song wrote:
> > > 
> > > 
> > > On 7/28/22 8:46 AM, Kui-Feng Lee wrote:
> > > > On Tue, 2022-07-26 at 10:11 -0700, Yonghong Song wrote:
> > > > > 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. struct argument size needs to be 16 or less
> > > > > so
> > > > > it can fit in one or two registers. Based on analysis on llvm
> > > > > and
> > > > > experiments, atruct argument size greater than 16 will be
> > > > > passed
> > > > > as pointer to the struct.
> > > > 
> > > > Is it possible to force llvm to always pass a pointer to a
> > > > struct
> > > > over
> > > > 8 bytes (the size of single register) for the BPF traget?
> > > 
> > > This is already the case for bpf target. Any struct parameter (1
> > > byte, 2
> > > bytes, ..., 8 types, ..., 16 bytes, ...) will be passed as a
> > > reference.
> > > 
> > > But this is not the case for most other architectures. For
> > > example,
> > > for
> > > x86_64, in most cases, struct size <= 16 will be passed with two
> > > registers instead of as a reference.
> > 
> > I ask this question because you modify the signature of a bpf
> > program
> > to a pointer to a struct in patch #4.  Is that necessary if the
> > compiler passes a struct paramter as a reference?
> 
> Note that The true bpf program signature is only one.
> long bpf_prog(<ctx_type> *ctx)
> BPF_PROG is a macro for user friendly purpose.
> 
> For example,
> +int BPF_PROG(test_struct_arg_1, struct bpf_testmod_struct_arg_2 *a,
> int 
> b, int c)
> 
> after macro expansion:
> int test_struct_arg_1(unsigned long long *ctx);
> static __attribute__((always_inline))
> typeof(test_struct_arg_1(0)) ____test_struct_arg_1(
>    unsigned long long *ctx,
>    struct bpf_testmod_struct_arg_2 *a, int b, int c);
> ...

If cast the pointer of __test_struct_arg_1() to the type (int
(*)(void*, void*, void*, void*)), test_struct_arg_1() can pass all
arguments to __tst_struct_arg_1() in the type (void *) without changing
the types of struct arguments to the pointer to a struct since all
struct types will be passed as a reference for the BPF target.

For example, the macro above can be expanded into the following block.
  ......
  int test_struct_arg_1(unsigned long long *ctx) {
    ......
    return ((typeof(name(0)) (*)(void*, void*, void*,
void*))&____test_struct_arg_1)(ctx[0], ctx[1], ctx[2], ctx[3]);
    ......
  }
  ......

Re: [RFC PATCH bpf-next 4/7] bpf: x86: Support in-register struct arguments

[Yonghong Song]

On 7/29/22 4:10 AM, Jiri Olsa wrote:
> On Tue, Jul 26, 2022 at 10:11:51AM -0700, Yonghong Song wrote:
> 
> SNIP
> 
>>   
>> -static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
>> -			 int regs_off)
>> +static void __save_struct_arg_regs(u8 **prog, int curr_reg_idx, int nr_regs,
>> +				   int struct_val_off, int stack_start_idx)
>>   {
>> -	int i;
>> +	int i, reg_idx;
>> +
>> +	/* Save struct registers to stack.
>> +	 * For example, argument 1 (second argument) size is 16 which occupies two
>> +	 * registers, these two register values will be saved in stack.
>> +	 * mov QWORD PTR [rbp-0x40],rsi
>> +	 * mov QWORD PTR [rbp-0x38],rdx
>> +	 */
>> +	for (i = 0; i < nr_regs; i++) {
>> +		reg_idx = curr_reg_idx + i;
>> +		emit_stx(prog, bytes_to_bpf_size(8),
>> +			 BPF_REG_FP,
>> +			 reg_idx == 5 ? X86_REG_R9 : BPF_REG_1 + reg_idx,
>> +			 -(struct_val_off - stack_start_idx * 8));
>> +		stack_start_idx++;
>> +	}
>> +}
>> +
>> +static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
>> +		      int regs_off, int struct_val_off)
>> +{
>> +	int curr_arg_idx, curr_reg_idx, curr_s_stack_off;
>> +	int s_size, s_arg_idx, s_arg_nregs;
>> +
>> +	curr_arg_idx = curr_reg_idx = curr_s_stack_off = 0;
>> +	for (int i = 0; i < MAX_BPF_FUNC_STRUCT_ARGS; i++) {
>> +		s_size = m->struct_arg_bsize[i];
>> +		if (!s_size)
>> +			return __save_normal_arg_regs(m, prog, curr_arg_idx, nr_args - curr_arg_idx,
>> +						      curr_reg_idx, regs_off);
> 
> could we just do break in here instead?

Thanks for pointing out. Yes, we can just do break and later call
__save_normal_arg_regs(...) will handle this automatically.
The same for another place you pointed below.

> 
> SNIP
> 
>> +
>> +static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
>> +			 int regs_off, int struct_val_off)
>> +{
>> +	int curr_arg_idx, curr_reg_idx, curr_s_stack_off;
>> +	int s_size, s_arg_idx, s_arg_nregs;
>> +
>> +	curr_arg_idx = curr_reg_idx = curr_s_stack_off = 0;
>> +	for (int i = 0; i < MAX_BPF_FUNC_STRUCT_ARGS; i++) {
>> +		s_size = m->struct_arg_bsize[i];
>> +		if (!s_size)
>> +			return __restore_normal_arg_regs(m, prog, curr_arg_idx,
>> +							 nr_args - curr_arg_idx,
>> +							 curr_reg_idx, regs_off);
> 
> same here
> 
> jirka
> 
>> +
>> +		s_arg_idx = m->struct_arg_idx[i];
>> +		s_arg_nregs = (s_size + 7) / 8;
>> +
>> +		__restore_normal_arg_regs(m, prog, curr_arg_idx, s_arg_idx - curr_arg_idx,
>> +					  curr_reg_idx, regs_off);
>> +		__restore_struct_arg_regs(prog, curr_reg_idx + s_arg_idx - curr_arg_idx,
>> +					  s_arg_nregs, struct_val_off, curr_s_stack_off);
>> +		curr_reg_idx += s_arg_idx - curr_arg_idx + s_arg_nregs;
>> +		curr_s_stack_off += s_arg_nregs;
>> +		curr_arg_idx = s_arg_idx + 1;
>> +	}
>> +
>> +	__restore_normal_arg_regs(m, prog, curr_arg_idx, nr_args - curr_arg_idx, curr_reg_idx,
>> +				  regs_off);
>>   }
>>   
> 
> SNIP

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

[Yonghong Song]

On 7/29/22 11:04 AM, Kui-Feng Lee wrote:
> On Thu, 2022-07-28 at 16:30 -0700, Yonghong Song wrote:
>>
>>
>> On 7/28/22 12:57 PM, Kui-Feng Lee wrote:
>>> On Thu, 2022-07-28 at 10:46 -0700, Yonghong Song wrote:
>>>>
>>>>
>>>> On 7/28/22 8:46 AM, Kui-Feng Lee wrote:
>>>>> On Tue, 2022-07-26 at 10:11 -0700, Yonghong Song wrote:
>>>>>> 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. struct argument size needs to be 16 or less
>>>>>> so
>>>>>> it can fit in one or two registers. Based on analysis on llvm
>>>>>> and
>>>>>> experiments, atruct argument size greater than 16 will be
>>>>>> passed
>>>>>> as pointer to the struct.
>>>>>
>>>>> Is it possible to force llvm to always pass a pointer to a
>>>>> struct
>>>>> over
>>>>> 8 bytes (the size of single register) for the BPF traget?
>>>>
>>>> This is already the case for bpf target. Any struct parameter (1
>>>> byte, 2
>>>> bytes, ..., 8 types, ..., 16 bytes, ...) will be passed as a
>>>> reference.
>>>>
>>>> But this is not the case for most other architectures. For
>>>> example,
>>>> for
>>>> x86_64, in most cases, struct size <= 16 will be passed with two
>>>> registers instead of as a reference.
>>>
>>> I ask this question because you modify the signature of a bpf
>>> program
>>> to a pointer to a struct in patch #4.  Is that necessary if the
>>> compiler passes a struct paramter as a reference?
>>
>> Note that The true bpf program signature is only one.
>> long bpf_prog(<ctx_type> *ctx)
>> BPF_PROG is a macro for user friendly purpose.
>>
>> For example,
>> +int BPF_PROG(test_struct_arg_1, struct bpf_testmod_struct_arg_2 *a,
>> int
>> b, int c)
>>
>> after macro expansion:
>> int test_struct_arg_1(unsigned long long *ctx);
>> static __attribute__((always_inline))
>> typeof(test_struct_arg_1(0)) ____test_struct_arg_1(
>>     unsigned long long *ctx,
>>     struct bpf_testmod_struct_arg_2 *a, int b, int c);
>> ...
> 
> If cast the pointer of __test_struct_arg_1() to the type (int
> (*)(void*, void*, void*, void*)), test_struct_arg_1() can pass all
> arguments to __tst_struct_arg_1() in the type (void *) without changing
> the types of struct arguments to the pointer to a struct since all
> struct types will be passed as a reference for the BPF target.
> 
> For example, the macro above can be expanded into the following block.
>    ......
>    int test_struct_arg_1(unsigned long long *ctx) {
>      ......
>      return ((typeof(name(0)) (*)(void*, void*, void*,
> void*))&____test_struct_arg_1)(ctx[0], ctx[1], ctx[2], ctx[3]);
>      ......

This doesn't really work. Here ctx[0]/ctx[1]/... has type
'unsigned long long' since ctx has type 'unsigned long long *'.

But if later parameter type is 'struct bpf_testmod_struct_arg_2'
in the function prototype and definition. Then implicitly we will
have a type conversion like
    (struct bpf_testmod_struct_arg_2)(unsigned long long)ctx[...]
and the above won't work as you cannot convert a 'unsigned long long'
to a struct.

>    }
>    ......
> 

Re: [RFC PATCH bpf-next 2/7] bpf: Add struct argument info in btf_func_model

[Andrii Nakryiko]

On Tue, Jul 26, 2022 at 10:11 AM Yonghong Song <yhs@fb.com> wrote:
>
> Add struct argument information in btf_func_model and 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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 20c26aed7896..173b42cf3940 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -726,10 +726,19 @@ enum bpf_cgroup_storage_type {
>   */
>  #define MAX_BPF_FUNC_REG_ARGS 5
>
> +/* The maximum number of struct arguments a single function may have. */
> +#define MAX_BPF_FUNC_STRUCT_ARGS 2
> +
>  struct btf_func_model {
>         u8 ret_size;
>         u8 nr_args;
>         u8 arg_size[MAX_BPF_FUNC_ARGS];
> +       /* The struct_arg_idx should be in increasing order like (0, 2, ...).
> +        * The struct_arg_bsize encodes the struct field byte size
> +        * for the corresponding struct argument index.
> +        */
> +       u8 struct_arg_idx[MAX_BPF_FUNC_STRUCT_ARGS];
> +       u8 struct_arg_bsize[MAX_BPF_FUNC_STRUCT_ARGS];

Few questions here. It might be a bad idea, but I thought I'd bring it
up anyway.

So, is there any benefit into having these separate struct_arg_idx and
struct_arg_bsize fields and then processing arg_size completely
separate from struct_arg_idx/struct_arg_bsize in patch #4? Reading
patch #4 it felt like it would be much easier to keep track of things
if we had a single loop going over all the arguments, and then if some
argument is a struct -- do some extra step to copy up to 16 bytes onto
stack and store the pointer there (and skip up to one extra argument).
And if it's not a struct arg -- do what we do right now.

What if instead we keep btf_func_mode definition as is, but for struct
argument we add extra flag to arg_size[struct_arg_idx] value to mark
that it is a struct argument. This limits arg_size to 128 bytes, but I
think it's more than enough for both struct and non-struct cases,
right? Distill function would make sure that nr_args matches number of
logical arguments and not number of registers.

Would that work? Would that make anything harder to implement in
arch-specific code? I don't see what, but I haven't grokked all the
details of patch #4, so I'm sorry if I missed something obvious. The
way I see it, it will make overall logic for saving/restoring
registers more uniform, roughly:

for (int arg_idx = 0; arg_idx < model->arg_size; arg_idx++) {
  if (arg & BTF_FMODEL_STRUCT_ARG) {
    /* handle struct, calc extra registers "consumed" from
arg_size[arg_idx] ~BTF_FMODEL_STRUCT_ARG */
  } else {
    /* just a normal register */
  }
}


If we do stick to current approach, though, let's please
s/struct_arg_bsize/struct_arg_size/. Isn't arg_size also and already
in bytes? It will keep naming and meaning consistent across struct and
non-struct args.

BTW, by not having btf_func_model encode register indices in
struct_arg_idx we keep btf_func_model pretty architecture-agnostic,
right? It will be per each architecture specific implementation to
perform mapping this *model* into actual registers used?




>  };
>
>  /* Restore arguments before returning from trampoline to let original function
> --
> 2.30.2
>

Re: [RFC PATCH bpf-next 2/7] bpf: Add struct argument info in btf_func_model

[Yonghong Song]

On 8/8/22 5:02 PM, Andrii Nakryiko wrote:
> On Tue, Jul 26, 2022 at 10:11 AM Yonghong Song <yhs@fb.com> wrote:
>>
>> Add struct argument information in btf_func_model and 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 | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 20c26aed7896..173b42cf3940 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -726,10 +726,19 @@ enum bpf_cgroup_storage_type {
>>    */
>>   #define MAX_BPF_FUNC_REG_ARGS 5
>>
>> +/* The maximum number of struct arguments a single function may have. */
>> +#define MAX_BPF_FUNC_STRUCT_ARGS 2
>> +
>>   struct btf_func_model {
>>          u8 ret_size;
>>          u8 nr_args;
>>          u8 arg_size[MAX_BPF_FUNC_ARGS];
>> +       /* The struct_arg_idx should be in increasing order like (0, 2, ...).
>> +        * The struct_arg_bsize encodes the struct field byte size
>> +        * for the corresponding struct argument index.
>> +        */
>> +       u8 struct_arg_idx[MAX_BPF_FUNC_STRUCT_ARGS];
>> +       u8 struct_arg_bsize[MAX_BPF_FUNC_STRUCT_ARGS];
> 
> Few questions here. It might be a bad idea, but I thought I'd bring it
> up anyway.
> 
> So, is there any benefit into having these separate struct_arg_idx and
> struct_arg_bsize fields and then processing arg_size completely
> separate from struct_arg_idx/struct_arg_bsize in patch #4? Reading
> patch #4 it felt like it would be much easier to keep track of things
> if we had a single loop going over all the arguments, and then if some
> argument is a struct -- do some extra step to copy up to 16 bytes onto
> stack and store the pointer there (and skip up to one extra argument).
> And if it's not a struct arg -- do what we do right now.
> 
> What if instead we keep btf_func_mode definition as is, but for struct
> argument we add extra flag to arg_size[struct_arg_idx] value to mark
> that it is a struct argument. This limits arg_size to 128 bytes, but I
> think it's more than enough for both struct and non-struct cases,
> right? Distill function would make sure that nr_args matches number of
> logical arguments and not number of registers.
> 
> Would that work? Would that make anything harder to implement in
> arch-specific code? I don't see what, but I haven't grokked all the
> details of patch #4, so I'm sorry if I missed something obvious. The
> way I see it, it will make overall logic for saving/restoring
> registers more uniform, roughly:
> 
> for (int arg_idx = 0; arg_idx < model->arg_size; arg_idx++) {
>    if (arg & BTF_FMODEL_STRUCT_ARG) {
>      /* handle struct, calc extra registers "consumed" from
> arg_size[arg_idx] ~BTF_FMODEL_STRUCT_ARG */
>    } else {
>      /* just a normal register */
>    }
> }

Your suggestion sounds good to me. Yes, we already have
arg_size array. We can add a
	bool is_struct_arg[MAX_BPF_FUNC_ARGS];
to indicate whether the argument is a struct or not.
In this case, we can avoid duplication between
arg_size and struct_arg_bsize.

> 
> 
> If we do stick to current approach, though, let's please
> s/struct_arg_bsize/struct_arg_size/. Isn't arg_size also and already
> in bytes? It will keep naming and meaning consistent across struct and
> non-struct args.
> 
> BTW, by not having btf_func_model encode register indices in
> struct_arg_idx we keep btf_func_model pretty architecture-agnostic,
> right? It will be per each architecture specific implementation to
> perform mapping this *model* into actual registers used?
> 
> 
> 
> 
>>   };
>>
>>   /* Restore arguments before returning from trampoline to let original function
>> --
>> 2.30.2
>>

Re: [RFC PATCH bpf-next 2/7] bpf: Add struct argument info in btf_func_model

[Andrii Nakryiko]

On Tue, Aug 9, 2022 at 10:38 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 8/8/22 5:02 PM, Andrii Nakryiko wrote:
> > On Tue, Jul 26, 2022 at 10:11 AM Yonghong Song <yhs@fb.com> wrote:
> >>
> >> Add struct argument information in btf_func_model and 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 | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >> index 20c26aed7896..173b42cf3940 100644
> >> --- a/include/linux/bpf.h
> >> +++ b/include/linux/bpf.h
> >> @@ -726,10 +726,19 @@ enum bpf_cgroup_storage_type {
> >>    */
> >>   #define MAX_BPF_FUNC_REG_ARGS 5
> >>
> >> +/* The maximum number of struct arguments a single function may have. */
> >> +#define MAX_BPF_FUNC_STRUCT_ARGS 2
> >> +
> >>   struct btf_func_model {
> >>          u8 ret_size;
> >>          u8 nr_args;
> >>          u8 arg_size[MAX_BPF_FUNC_ARGS];
> >> +       /* The struct_arg_idx should be in increasing order like (0, 2, ...).
> >> +        * The struct_arg_bsize encodes the struct field byte size
> >> +        * for the corresponding struct argument index.
> >> +        */
> >> +       u8 struct_arg_idx[MAX_BPF_FUNC_STRUCT_ARGS];
> >> +       u8 struct_arg_bsize[MAX_BPF_FUNC_STRUCT_ARGS];
> >
> > Few questions here. It might be a bad idea, but I thought I'd bring it
> > up anyway.
> >
> > So, is there any benefit into having these separate struct_arg_idx and
> > struct_arg_bsize fields and then processing arg_size completely
> > separate from struct_arg_idx/struct_arg_bsize in patch #4? Reading
> > patch #4 it felt like it would be much easier to keep track of things
> > if we had a single loop going over all the arguments, and then if some
> > argument is a struct -- do some extra step to copy up to 16 bytes onto
> > stack and store the pointer there (and skip up to one extra argument).
> > And if it's not a struct arg -- do what we do right now.
> >
> > What if instead we keep btf_func_mode definition as is, but for struct
> > argument we add extra flag to arg_size[struct_arg_idx] value to mark
> > that it is a struct argument. This limits arg_size to 128 bytes, but I
> > think it's more than enough for both struct and non-struct cases,
> > right? Distill function would make sure that nr_args matches number of
> > logical arguments and not number of registers.
> >
> > Would that work? Would that make anything harder to implement in
> > arch-specific code? I don't see what, but I haven't grokked all the
> > details of patch #4, so I'm sorry if I missed something obvious. The
> > way I see it, it will make overall logic for saving/restoring
> > registers more uniform, roughly:
> >
> > for (int arg_idx = 0; arg_idx < model->arg_size; arg_idx++) {
> >    if (arg & BTF_FMODEL_STRUCT_ARG) {
> >      /* handle struct, calc extra registers "consumed" from
> > arg_size[arg_idx] ~BTF_FMODEL_STRUCT_ARG */
> >    } else {
> >      /* just a normal register */
> >    }
> > }
>
> Your suggestion sounds good to me. Yes, we already have
> arg_size array. We can add a
>         bool is_struct_arg[MAX_BPF_FUNC_ARGS];
> to indicate whether the argument is a struct or not.
> In this case, we can avoid duplication between
> arg_size and struct_arg_bsize.
>

I was imagining that we'll just use the existing arg_size and define
that the upper bit is a struct/non-struct bit. But if that's too
confusing and cryptic, I wonder if it's better to have

u8 arg_flags[MAX_BPF_FUNC_ARGS];

instead and define the BPF_FNARG_STRUCT flag.

For what you did in your other patch set (u8/u16 handling for func
result), we can then define ret_flags and have a flag whether the
argument is integer and whether it's signed in such flags (instead of
bit fields).

This way we have a unified and more extendable "size+flags" approach
both for input arguments and return result.

WDYT?

> >
> >
> > If we do stick to current approach, though, let's please
> > s/struct_arg_bsize/struct_arg_size/. Isn't arg_size also and already
> > in bytes? It will keep naming and meaning consistent across struct and
> > non-struct args.
> >
> > BTW, by not having btf_func_model encode register indices in
> > struct_arg_idx we keep btf_func_model pretty architecture-agnostic,
> > right? It will be per each architecture specific implementation to
> > perform mapping this *model* into actual registers used?
> >
> >
> >
> >
> >>   };
> >>
> >>   /* Restore arguments before returning from trampoline to let original function
> >> --
> >> 2.30.2
> >>

Re: [RFC PATCH bpf-next 2/7] bpf: Add struct argument info in btf_func_model

[Yonghong Song]

On 8/9/22 5:25 PM, Andrii Nakryiko wrote:
> On Tue, Aug 9, 2022 at 10:38 AM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 8/8/22 5:02 PM, Andrii Nakryiko wrote:
>>> On Tue, Jul 26, 2022 at 10:11 AM Yonghong Song <yhs@fb.com> wrote:
>>>>
>>>> Add struct argument information in btf_func_model and 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 | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>>> index 20c26aed7896..173b42cf3940 100644
>>>> --- a/include/linux/bpf.h
>>>> +++ b/include/linux/bpf.h
>>>> @@ -726,10 +726,19 @@ enum bpf_cgroup_storage_type {
>>>>     */
>>>>    #define MAX_BPF_FUNC_REG_ARGS 5
>>>>
>>>> +/* The maximum number of struct arguments a single function may have. */
>>>> +#define MAX_BPF_FUNC_STRUCT_ARGS 2
>>>> +
>>>>    struct btf_func_model {
>>>>           u8 ret_size;
>>>>           u8 nr_args;
>>>>           u8 arg_size[MAX_BPF_FUNC_ARGS];
>>>> +       /* The struct_arg_idx should be in increasing order like (0, 2, ...).
>>>> +        * The struct_arg_bsize encodes the struct field byte size
>>>> +        * for the corresponding struct argument index.
>>>> +        */
>>>> +       u8 struct_arg_idx[MAX_BPF_FUNC_STRUCT_ARGS];
>>>> +       u8 struct_arg_bsize[MAX_BPF_FUNC_STRUCT_ARGS];
>>>
>>> Few questions here. It might be a bad idea, but I thought I'd bring it
>>> up anyway.
>>>
>>> So, is there any benefit into having these separate struct_arg_idx and
>>> struct_arg_bsize fields and then processing arg_size completely
>>> separate from struct_arg_idx/struct_arg_bsize in patch #4? Reading
>>> patch #4 it felt like it would be much easier to keep track of things
>>> if we had a single loop going over all the arguments, and then if some
>>> argument is a struct -- do some extra step to copy up to 16 bytes onto
>>> stack and store the pointer there (and skip up to one extra argument).
>>> And if it's not a struct arg -- do what we do right now.
>>>
>>> What if instead we keep btf_func_mode definition as is, but for struct
>>> argument we add extra flag to arg_size[struct_arg_idx] value to mark
>>> that it is a struct argument. This limits arg_size to 128 bytes, but I
>>> think it's more than enough for both struct and non-struct cases,
>>> right? Distill function would make sure that nr_args matches number of
>>> logical arguments and not number of registers.
>>>
>>> Would that work? Would that make anything harder to implement in
>>> arch-specific code? I don't see what, but I haven't grokked all the
>>> details of patch #4, so I'm sorry if I missed something obvious. The
>>> way I see it, it will make overall logic for saving/restoring
>>> registers more uniform, roughly:
>>>
>>> for (int arg_idx = 0; arg_idx < model->arg_size; arg_idx++) {
>>>     if (arg & BTF_FMODEL_STRUCT_ARG) {
>>>       /* handle struct, calc extra registers "consumed" from
>>> arg_size[arg_idx] ~BTF_FMODEL_STRUCT_ARG */
>>>     } else {
>>>       /* just a normal register */
>>>     }
>>> }
>>
>> Your suggestion sounds good to me. Yes, we already have
>> arg_size array. We can add a
>>          bool is_struct_arg[MAX_BPF_FUNC_ARGS];
>> to indicate whether the argument is a struct or not.
>> In this case, we can avoid duplication between
>> arg_size and struct_arg_bsize.
>>
> 
> I was imagining that we'll just use the existing arg_size and define
> that the upper bit is a struct/non-struct bit. But if that's too
> confusing and cryptic, I wonder if it's better to have
> 
> u8 arg_flags[MAX_BPF_FUNC_ARGS];

I prefer a separate array, which seems cleanly.

> 
> instead and define the BPF_FNARG_STRUCT flag.
> 
> For what you did in your other patch set (u8/u16 handling for func
> result), we can then define ret_flags and have a flag whether the
> argument is integer and whether it's signed in such flags (instead of
> bit fields).

Currently, we only have use case for whether it is a struct. But
agree we can make it extensible with an enum to indicate what kind
of info for the argument.

> 
> This way we have a unified and more extendable "size+flags" approach
> both for input arguments and return result.
> 
> WDYT?
> 
>>>
>>>
>>> If we do stick to current approach, though, let's please
>>> s/struct_arg_bsize/struct_arg_size/. Isn't arg_size also and already
>>> in bytes? It will keep naming and meaning consistent across struct and
>>> non-struct args.
>>>
>>> BTW, by not having btf_func_model encode register indices in
>>> struct_arg_idx we keep btf_func_model pretty architecture-agnostic,
>>> right? It will be per each architecture specific implementation to
>>> perform mapping this *model* into actual registers used?
>>>
>>>
>>>
>>>
>>>>    };
>>>>
>>>>    /* Restore arguments before returning from trampoline to let original function
>>>> --
>>>> 2.30.2
>>>>