From: Menglong Dong <[email protected]>
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
friendly at all, as too many functions have arguments count more than 6.
Therefore, let's enhance it by increasing the function arguments count
allowed in arch_prepare_bpf_trampoline(), for now, only x86_64.
In the 1st patch, we make arch_prepare_bpf_trampoline() support to copy
function arguments in stack for x86 arch. Therefore, the maximum
arguments can be up to MAX_BPF_FUNC_ARGS for FENTRY and FEXIT.
In the 2nd patch, we clean garbage value in upper bytes of the trampoline
when we store the arguments from regs into stack.
And the 3rd patches are for the testcases of the 1st patch.
Changes since v2:
- keep MAX_BPF_FUNC_ARGS still
- clean garbage value in upper bytes in the 2nd patch
- move bpf_fentry_test{7,12} to bpf_testmod.c and rename them to
bpf_testmod_fentry_test{7,12} meanwhile in the 3rd patch
Changes since v1:
- change the maximun function arguments to 14 from 12
- add testcases (Jiri Olsa)
- instead EMIT4 with EMIT3_off32 for "lea" to prevent overflow
Menglong Dong (3):
bpf, x86: allow function arguments up to 12 for TRACING
bpf, x86: clean garbage value in the stack of trampoline
selftests/bpf: add testcase for FENTRY/FEXIT with 6+ arguments
arch/x86/net/bpf_jit_comp.c | 105 +++++++++++++++---
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 19 +++-
.../selftests/bpf/prog_tests/fentry_fexit.c | 4 +-
.../selftests/bpf/prog_tests/fentry_test.c | 2 +
.../selftests/bpf/prog_tests/fexit_test.c | 2 +
.../testing/selftests/bpf/progs/fentry_test.c | 21 ++++
.../testing/selftests/bpf/progs/fexit_test.c | 33 ++++++
7 files changed, 169 insertions(+), 17 deletions(-)
--
2.40.1
From: Menglong Dong <[email protected]>
There are garbage values in upper bytes when we store the arguments
into stack in save_regs() if the size of the argument less then 8.
As we already reserve 8 byte for the arguments in regs and stack,
it is ok to store/restore the regs in BPF_DW size. Then, the garbage
values in upper bytes will be cleaned.
Reviewed-by: Jiang Biao <[email protected]>
Signed-off-by: Menglong Dong <[email protected]>
---
arch/x86/net/bpf_jit_comp.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 413b986b5afd..e9bc0b50656b 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1878,20 +1878,16 @@ static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
if (i <= 5) {
/* copy function arguments from regs into stack */
- emit_stx(prog, bytes_to_bpf_size(arg_size),
- BPF_REG_FP,
+ emit_stx(prog, BPF_DW, BPF_REG_FP,
i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
-(stack_size - i * 8));
} else {
/* copy function arguments from origin stack frame
* into current stack frame.
*/
- emit_ldx(prog, bytes_to_bpf_size(arg_size),
- BPF_REG_0, BPF_REG_FP,
+ emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
(i - 6) * 8 + 0x18);
- emit_stx(prog, bytes_to_bpf_size(arg_size),
- BPF_REG_FP,
- BPF_REG_0,
+ emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0,
-(stack_size - i * 8));
}
@@ -1918,7 +1914,7 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
next_same_struct = !next_same_struct;
}
- emit_ldx(prog, bytes_to_bpf_size(arg_size),
+ emit_ldx(prog, BPF_DW,
i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
BPF_REG_FP,
-(stack_size - i * 8));
@@ -1949,12 +1945,9 @@ static void prepare_origin_stack(const struct btf_func_model *m, u8 **prog,
}
if (i > 5) {
- emit_ldx(prog, bytes_to_bpf_size(arg_size),
- BPF_REG_0, BPF_REG_FP,
+ emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
(i - 6) * 8 + 0x18);
- emit_stx(prog, bytes_to_bpf_size(arg_size),
- BPF_REG_FP,
- BPF_REG_0,
+ emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0,
-(stack_size - (i - 6) * 8));
}
--
2.40.1
On Wed, Jun 07, 2023 at 08:59:10PM +0800, [email protected] wrote:
> From: Menglong Dong <[email protected]>
>
> There are garbage values in upper bytes when we store the arguments
> into stack in save_regs() if the size of the argument less then 8.
>
> As we already reserve 8 byte for the arguments in regs and stack,
> it is ok to store/restore the regs in BPF_DW size. Then, the garbage
> values in upper bytes will be cleaned.
>
> Reviewed-by: Jiang Biao <[email protected]>
> Signed-off-by: Menglong Dong <[email protected]>
> ---
> arch/x86/net/bpf_jit_comp.c | 19 ++++++-------------
> 1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 413b986b5afd..e9bc0b50656b 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1878,20 +1878,16 @@ static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
>
> if (i <= 5) {
> /* copy function arguments from regs into stack */
> - emit_stx(prog, bytes_to_bpf_size(arg_size),
> - BPF_REG_FP,
> + emit_stx(prog, BPF_DW, BPF_REG_FP,
> i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
> -(stack_size - i * 8));
This is ok,
> } else {
> /* copy function arguments from origin stack frame
> * into current stack frame.
> */
> - emit_ldx(prog, bytes_to_bpf_size(arg_size),
> - BPF_REG_0, BPF_REG_FP,
> + emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
> (i - 6) * 8 + 0x18);
> - emit_stx(prog, bytes_to_bpf_size(arg_size),
> - BPF_REG_FP,
> - BPF_REG_0,
> + emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0,
> -(stack_size - i * 8));
But this is not.
See https://godbolt.org/z/qW17f6cYe
mov dword ptr [rsp], 6
the compiler will store 32-bit only. The upper 32-bit are still garbage.
> }
>
> @@ -1918,7 +1914,7 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
> next_same_struct = !next_same_struct;
> }
>
> - emit_ldx(prog, bytes_to_bpf_size(arg_size),
> + emit_ldx(prog, BPF_DW,
> i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
> BPF_REG_FP,
> -(stack_size - i * 8));
> @@ -1949,12 +1945,9 @@ static void prepare_origin_stack(const struct btf_func_model *m, u8 **prog,
> }
>
> if (i > 5) {
> - emit_ldx(prog, bytes_to_bpf_size(arg_size),
> - BPF_REG_0, BPF_REG_FP,
> + emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
> (i - 6) * 8 + 0x18);
> - emit_stx(prog, bytes_to_bpf_size(arg_size),
> - BPF_REG_FP,
> - BPF_REG_0,
> + emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0,
> -(stack_size - (i - 6) * 8));
> }
>
> --
> 2.40.1
>
On Thu, Jun 8, 2023 at 4:03 AM Alexei Starovoitov
<[email protected]> wrote:
>
> On Wed, Jun 07, 2023 at 08:59:10PM +0800, [email protected] wrote:
> > From: Menglong Dong <[email protected]>
> >
> > There are garbage values in upper bytes when we store the arguments
> > into stack in save_regs() if the size of the argument less then 8.
> >
> > As we already reserve 8 byte for the arguments in regs and stack,
> > it is ok to store/restore the regs in BPF_DW size. Then, the garbage
> > values in upper bytes will be cleaned.
> >
> > Reviewed-by: Jiang Biao <[email protected]>
> > Signed-off-by: Menglong Dong <[email protected]>
> > ---
> > arch/x86/net/bpf_jit_comp.c | 19 ++++++-------------
> > 1 file changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index 413b986b5afd..e9bc0b50656b 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -1878,20 +1878,16 @@ static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
> >
> > if (i <= 5) {
> > /* copy function arguments from regs into stack */
> > - emit_stx(prog, bytes_to_bpf_size(arg_size),
> > - BPF_REG_FP,
> > + emit_stx(prog, BPF_DW, BPF_REG_FP,
> > i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
> > -(stack_size - i * 8));
>
> This is ok,
>
> > } else {
> > /* copy function arguments from origin stack frame
> > * into current stack frame.
> > */
> > - emit_ldx(prog, bytes_to_bpf_size(arg_size),
> > - BPF_REG_0, BPF_REG_FP,
> > + emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
> > (i - 6) * 8 + 0x18);
> > - emit_stx(prog, bytes_to_bpf_size(arg_size),
> > - BPF_REG_FP,
> > - BPF_REG_0,
> > + emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0,
> > -(stack_size - i * 8));
>
> But this is not.
> See https://godbolt.org/z/qW17f6cYe
> mov dword ptr [rsp], 6
>
> the compiler will store 32-bit only. The upper 32-bit are still garbage.
Enn......I didn't expect this case, and it seems
that this only happens on clang. With gcc,
"push 6" is used.
I haven't found a solution for this case, and it seems
not worth it to add an extra insn to clean the garbage
values.
Does anyone have any ideas here?
Thanks!
Menglong Dong
>
> > }
> >
> > @@ -1918,7 +1914,7 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
> > next_same_struct = !next_same_struct;
> > }
> >
> > - emit_ldx(prog, bytes_to_bpf_size(arg_size),
> > + emit_ldx(prog, BPF_DW,
> > i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
> > BPF_REG_FP,
> > -(stack_size - i * 8));
> > @@ -1949,12 +1945,9 @@ static void prepare_origin_stack(const struct btf_func_model *m, u8 **prog,
> > }
> >
> > if (i > 5) {
> > - emit_ldx(prog, bytes_to_bpf_size(arg_size),
> > - BPF_REG_0, BPF_REG_FP,
> > + emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
> > (i - 6) * 8 + 0x18);
> > - emit_stx(prog, bytes_to_bpf_size(arg_size),
> > - BPF_REG_FP,
> > - BPF_REG_0,
> > + emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0,
> > -(stack_size - (i - 6) * 8));
> > }
> >
> > --
> > 2.40.1
> >