Re: [PATCH bpf-next 1/3] bpf, x64: Replace some stack_size usage with offset variables

On Mon, Dec 06, 2021 at 01:41:15PM -0800, Andrii Nakryiko wrote:

On Sat, Dec 4, 2021 at 6:07 AM Jiri Olsa [off-list ref] wrote:

quoted

As suggested by Andrii, adding variables for registers and ip
address offsets, which makes the code more clear, rather than
abusing single stack_size variable for everything.

Also describing the stack layout in the comment.

There is no function change.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/net/bpf_jit_comp.c | 42 ++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 1d7b0c69b644..b106e80e8d9c 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c

@@ -1941,7 +1941,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
                                void *orig_call)
 {
        int ret, i, nr_args = m->nr_args;
-       int stack_size = nr_args * 8;
+       int regs_off, ip_off, stack_size = nr_args * 8;
        struct bpf_tramp_progs *fentry = &tprogs[BPF_TRAMP_FENTRY];
        struct bpf_tramp_progs *fexit = &tprogs[BPF_TRAMP_FEXIT];
        struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN];

@@ -1956,14 +1956,33 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
        if (!is_valid_bpf_tramp_flags(flags))
                return -EINVAL;

+       /* Generated trampoline stack layout:
+        *
+        * RBP + 8         [ return address  ]
+        * RBP + 0         [ RBP             ]
+        *
+        * RBP - 8         [ return value    ]  BPF_TRAMP_F_CALL_ORIG or
+        *                                      BPF_TRAMP_F_RET_FENTRY_RET flags
+        *
+        *                 [ reg_argN        ]  always
+        *                 [ ...             ]
+        * RBP - regs_off  [ reg_arg1        ]
+        *

I think it's also worth mentioning that context passed into
fentry/fexit programs are pointing here (makes it a bit easier to
track those ctx[-1] and ctx[-2] in the next patch.

ok, jirka

quoted

+        * RBP - ip_off    [ traced function ]  BPF_TRAMP_F_IP_ARG flag
+        */
+
        /* room for return value of orig_call or fentry prog */
        save_ret = flags & (BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_RET_FENTRY_RET);
        if (save_ret)
                stack_size += 8;

+       regs_off = stack_size;
+
        if (flags & BPF_TRAMP_F_IP_ARG)
                stack_size += 8; /* room for IP address argument */

+       ip_off = stack_size;
+

[...]