On Thu, Jun 22, 2023 at 12:44 AM Yonghong Song <yhs@xxxxxxxx> wrote:
>
>
>
> On 6/19/23 4:49 AM, menglong8.dong@xxxxxxxxx wrote:
> > From: Menglong Dong <imagedong@xxxxxxxxxxx>
> >
> > For now, the BPF program of type BPF_PROG_TYPE_TRACING can only be used
> > on the kernel functions whose arguments count less than 6. This is not
>
> less than or equal to 6, if not considering '> 8 bytes' struct
> argument.
>
> > friendly at all, as too many functions have arguments count more than 6.
> >
> > According to the current kernel version, below is a statistics of the
> > function arguments count:
> >
> > argument count | function count
> > 7 | 704
> > 8 | 270
> > 9 | 84
> > 10 | 47
> > 11 | 47
> > 12 | 27
> > 13 | 22
> > 14 | 5
> > 15 | 0
> > 16 | 1
> >
> > Therefore, let's enhance it by increasing the function arguments count
> > allowed in arch_prepare_bpf_trampoline(), for now, only x86_64.
> >
> > For the case that we don't need to call origin function, which means
> > without BPF_TRAMP_F_CALL_ORIG, we need only copy the function arguments
> > that stored in the frame of the caller to current frame. The arguments
> > of arg6-argN are stored in "$rbp + 0x18", we need copy them to
> > "$rbp - regs_off + (6 * 8)".
>
> The 7th and later arguments are stored in "$rbp + 0x18", and they will
> be copied to the stack area following where register values are saved.
>
> >
> > For the case with BPF_TRAMP_F_CALL_ORIG, we need prepare the arguments
> > in stack before call origin function, which means we need alloc extra
> > "8 * (arg_count - 6)" memory in the top of the stack. Note, there should
> > not be any data be pushed to the stack before call the origin function.
>
> call -> calling
>
> > Then, we have to store rbx with 'mov' instead of 'push'.
>
> So 'rbx' value will be stored on a stack position higher than
> where stack arguments are stored for BPF_TRAMP_F_CALL_ORIG.
>
> >
> > According to the research of Yonghong, struct members should be all in
> > register or all on the stack. Meanwhile, the compiler will pass the
> > argument on regs if the remaining regs can hold the argument. Therefore,
> > we need save the arguments in order. Otherwise, disorder of the args can
> > happen. For example:
> >
> > struct foo_struct {
> > long a;
> > int b;
> > };
> > int foo(char, char, char, char, char, struct foo_struct,
> > char);
> >
> > the arg1-5,arg7 will be passed by regs, and arg6 will by stack. Therefore,
> > we should save/restore the arguments in the same order with the
> > declaration of foo(). And the args used as ctx in stack will be like this:
> >
> > reg_arg6 -- copy from regs
> > stack_arg2 -- copy from stack
> > stack_arg1
> > reg_arg5 -- copy from regs
> > reg_arg4
> > reg_arg3
> > reg_arg2
> > reg_arg1
> >
> > We use EMIT3_off32() or EMIT4() for "lea" and "sub". The range of the
> > imm in "lea" and "sub" is [-128, 127] if EMIT4() is used. Therefore,
> > we use EMIT3_off32() instead if the imm out of the range.
> >
> > It works well for the FENTRY/FEXIT/MODIFY_RETURN.
> >
> > Signed-off-by: Menglong Dong <imagedong@xxxxxxxxxxx>
>
> LGTM with some nits for commit messages and below codes.
>
Thank you for correcting these typos! I'll send the
next version with them fixed.
> Acked-by: Yonghong Song <yhs@xxxxxx>
>
> > ---
> > v6:
> > - introduce get_nr_regs() to get the space that used to pass args on
> > stack correct
> > - rename some args and fix some spelling mistake
> > v5:
> > - consider the case of the struct in arguments can't be hold by regs
> > v4:
> > - make the stack 16-byte aligned if passing args on-stack is needed
> > - add the function arguments statistics to the commit log
> > v3:
> > - use EMIT3_off32() for "lea" and "sub" only on necessary
> > - make 12 as the maximum arguments count
> > v2:
> > - instead EMIT4 with EMIT3_off32 for "lea" to prevent overflow
> > - make MAX_BPF_FUNC_ARGS as the maximum argument count
> > ---
> > arch/x86/net/bpf_jit_comp.c | 238 ++++++++++++++++++++++++++++++++----
> > 1 file changed, 212 insertions(+), 26 deletions(-)
> >
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index a407fbbffecd..c0637f2b5398 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -1857,37 +1857,181 @@ st: if (is_imm8(insn->off))
> > return proglen;
> > }
> >
> > -static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
> > - int stack_size)
> > +static inline void clean_stack_garbage(const struct btf_func_model *m,
> > + u8 **pprog, int nr_stack_slots,
> > + int stack_size)
>
> Please remove 'inline' here. Let us compiler to decide
> whether inlining is needed or not.
>
> > {
> > - int i;
> > + int arg_size, off;
> > + u8 *prog;
> > +
> > + /* Generally speaking, the compiler will pass the arguments
> > + * on-stack with "push" instruction, which will take 8-byte
> > + * on the stack. In this case, there won't be garbage values
> > + * while we copy the arguments from origin stack frame to current
> > + * in BPF_DW.
> > + *
> > + * However, sometimes the compiler will only allocate 4-byte on
> > + * the stack for the arguments. For now, this case will only
> > + * happen if there is only one argument on-stack and its size
> > + * not more than 4 byte. In this case, there will be garbage
> > + * values on the upper 4-byte where we store the argument on
> > + * current stack frame.
> > + *
> > + * arguments on origin stack:
> > + *
> > + * stack_arg_1(4-byte) xxx(4-byte)
> > + *
> > + * what we copy:
> > + *
> > + * stack_arg_1(8-byte): stack_arg_1(origin) xxx
> > + *
> > + * and the xxx is the garbage values which we should clean here.
> > + */
> > + if (nr_stack_slots != 1)
> > + return;
> > +
> > + /* the size of the last argument */
> > + arg_size = m->arg_size[m->nr_args - 1];
> > + if (arg_size <= 4) {
> > + off = -(stack_size - 4);
> > + prog = *pprog;
> > + /* mov DWORD PTR [rbp + off], 0 */
> > + if (!is_imm8(off))
> > + EMIT2_off32(0xC7, 0x85, off);
> > + else
> > + EMIT3(0xC7, 0x45, off);
> > + EMIT(0, 4);
> > + *pprog = prog;
> > + }
> > +}
> > +
> > +/* get the count of the regs that are used to pass arguments * > +static inline int get_nr_regs(const struct btf_func_model *m)
>
> Again, remove 'inline' keyword here.
> Also rename function name 'get_nr_regs' to 'get_nr_used_regs'
> to reflect that it counts for the number of registers
> used to pass arguments?
>
> > +{
> > + int i, arg_regs, nr_regs = 0;
>
> nr_regs => nr_used_regs for clarity?
>
> > +
> > + for (i = 0; i < min_t(int, m->nr_args, MAX_BPF_FUNC_ARGS); i++) {
> > + arg_regs = (m->arg_size[i] + 7) / 8;
> > + if (nr_regs + arg_regs <= 6)
> > + nr_regs += arg_regs;
> > +
> > + if (nr_regs >= 6)
> > + break;
> > + }
> > +
> > + return nr_regs;
> > +}
> > +
> > +static void save_args(const struct btf_func_model *m, u8 **prog,
> > + int stack_size, bool for_call_origin)
> > +{
> > + int arg_regs, first_off, nr_regs = 0, nr_stack = 0;
>
> To be consistent with clean_stack_garbage(),
> nr_stack -> nr_stack_slots?
>
Sounds great!
> > + int i, j;
> >
> > /* 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_regs, 6); i++)
> > - emit_stx(prog, BPF_DW, BPF_REG_FP,
> > - i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
> > - -(stack_size - i * 8));
> > + for (i = 0; i < min_t(int, m->nr_args, MAX_BPF_FUNC_ARGS); i++) {
> > + arg_regs = (m->arg_size[i] + 7) / 8;
> > +
> > + /* According to the research of Yonghong, struct members
> > + * should be all in register or all on the stack.
> > + * Meanwhile, the compiler will pass the argument on regs
> > + * if the remaining regs can hold the argument.
> > + *
> > + * Disorder of the args can happen. For example:
> > + *
> > + * struct foo_struct {
> > + * long a;
> > + * int b;
> > + * };
> > + * int foo(char, char, char, char, char, struct foo_struct,
> > + * char);
> > + *
> > + * the arg1-5,arg7 will be passed by regs, and arg6 will
> > + * by stack.
> > + *
> > + * Therefore, we should keep the same logic as here when
> > + * we restore the regs in restore_regs.
>
> The above two line comments are not needed. A similar comments exists
> in restore_regs() already.
Okay!
Thanks!
Menglong Dong
