Add arm64 support to fnargs.c #89

aetimmes · 2025-02-27T05:33:27Z

The README states:

NOTE: Function arguments capture feature is currently only supported on x86-64 (amd64) architecture. Most probably arm64 support will be added as well, please request this using Github issues, if you are interested in this.

I am interested in the above (as I'm in the middle of an ARM migration so the opportunity to trace on x86 hardware is slowly vanishing), but don't know how to implement it. However, I do have access to Cursor/Claude 3.7 and am curious as to how close it comes to getting this correct.

However, I won't be offended if the answer is "I don't have time to read random unflitered LLM output".

Signed-off-by: Andrew E. Timmes <andrew.timmes@gmail.com>

anakryiko · 2025-02-27T17:50:46Z

src/fnargs.c

+#ifdef __aarch64__
+			/* Special handling for structs in ARM64 */
+			if (btf_is_struct(t) && t->size <= 16) {
+				if (t->size <= 8 && is_arg_in_reg(reg_idx, &reg1_name)) {
+					/* Fits in one register */
+					spec->arg_flags |= data_len;
+					spec->arg_flags |= FNARGS_REG << FNARGS_LOC_SHIFT;
+					spec->arg_flags |= reg_idx << FNARGS_REGIDX_SHIFT;
+					dlog("%s", reg1_name);
+					reg_idx += 1;
+				} else if (t->size <= 16 && 
+						is_arg_in_reg(reg_idx, &reg1_name) && 
+						is_arg_in_reg(reg_idx + 1, &reg2_name)) {
+					/* Passed in a pair of registers */
+					spec->arg_flags |= data_len;
+					spec->arg_flags |= FNARGS_REG_PAIR << FNARGS_LOC_SHIFT;
+					spec->arg_flags |= reg_idx << FNARGS_REGIDX_SHIFT;
+					reg_idx += 2;
+					dlog("%s:%s", reg1_name, reg2_name);
+				} else {
+					/* Fallback to stack */
+					goto use_stack;
+				}
+			} else
+#endif


we have exactly the same logic for x86-64, no need for this duplication

addressed in 0aacc58

anakryiko · 2025-02-27T17:54:00Z

src/fnargs.c

 	int stack_off = 8; /* 8 bytes for return address */
+#elif defined(__aarch64__)
+	int stack_off = 0; /* ARM64 uses x0-x7 for first 8 args */


not sure what this comment has to do with stack offset... first few args will be in registers, but the rest might be on the stack, just like for x86-64

addressed in 0aacc58

anakryiko · 2025-02-27T17:56:03Z

src/retsnoop.bpf.c

+static __always_inline u64 get_stack_pointer(void *ctx)
+{
+	u64 sp;
+
+	if (use_kprobes) {
+		sp = PT_REGS_SP((struct pt_regs *)ctx);
+		barrier_var(sp);
+	} else {
+		/* current FENTRY doesn't support attaching to functions that
+		 * pass arguments on the stack, so we don't really need to
+		 * implement this
+		 */
+		sp = 0;
+		barrier_var(sp);
+	}
+
+	return sp;
+}
+#else /* !__TARGET_ARCH_x86 && !__TARGET_ARCH_arm64 */


this is an exact copy of get_stack_pointer() for x86-64, let's avoid duplication (and really, it will be the same for all architectures, probably)

Addressed in 7cfd2c0, but I employed some nested #ifdefery - I'm not a native C speaker so I don't know if that's idiomatic or idiotic

Signed-off-by: Andrew E. Timmes <andrew.timmes@gmail.com>

aetimmes · 2025-02-28T04:35:16Z

A quirk I noticed while testing:

When tracing syscalls, the values of user-controlled register args aren't labeled by name - possibly because arm64's pt_regs struct is an anonymous array where x86-64 has a struct of named fields?

Is this something I can/should improve on in this PR? Do we want to try to line up the register names out-of-band?

Sample syscall output below:

01:11:15.115687 -> 01:11:15.115688 TID/PID 658/658 (systemd-oomd/systemd-oomd):

FUNCTION CALLS        RESULT     DURATION
-------------------   ---------  --------
↔ __arm64_sys_ioctl   [-ENOTTY]   0.361us
                      › regs = &{
                          {
                              .user_regs = {
                                  .regs = [
                                      12,
                                      21505,
                                      0xffffd76edeb0,
                                      0xffffd76edef8,
                                      0xffffa067db20,
                                      0,
                                      0xfffffffffffffff0,
                                      4,
                                      29,
                                      0x7f7f7f7f7f7f7f7f,
                                      115,
                                      0x101010101010101,
                                      1,
                                      6,
                                      0xffffffffffffffff,
                                      40,
                                      0xffffa045a540,
                                      0xffff9ff16040,
                                      0,
                                      12,
                                      0,
                                      0xaaaae776a2c0,
                                      1048576,
                                      0xffffa067db20,
                                      0xffffa067db20,
                                      0xffffd76ee030,
                                      0x7fffffff,
                                      47,
                                      46,
                                      0xffffd76edee0,
                                      0x54ffff9ff1606c
                                  ],
                                  .sp = 0xffffd76edeb0…

                    el0t_64_sync+0x194
                    el0t_64_sync_handler+0x120
                    el0_svc+0x3c
                    do_el0_svc+0x24
                    el0_svc_common.constprop.0+0xc8
                    invoke_syscall+0x6c

Compare this to the named x86-64 registers shown in the output of the commands in the current readme.

anakryiko · 2025-03-04T00:37:00Z

Is this something I can/should improve on in this PR? Do we want to try to line up the register names out-of-band?

I don't think so. It is what it is in kernel, so unless we add custom rendered for arm64 pt_regs, there isn't much to do here.

anakryiko

Looks pretty good, a few small things remains, thanks!

anakryiko · 2025-03-04T00:39:23Z

src/fnargs.c

+	 * address (NSAA) is set to the current stack-pointer value (SP)."
+	 */
+	int stack_off = 0; 
+


nit: unnecessary empty line, please drop

anakryiko · 2025-03-04T00:43:04Z

src/retsnoop.c

@@ -411,7 +411,7 @@ int main(int argc, char **argv, char **envp)
 	skel->rodata->use_kprobes = env.attach_mode != ATTACH_FENTRY;
 	memset(skel->rodata->spaces, ' ', sizeof(skel->rodata->spaces) - 1);

-#ifdef __x86_64__
+#if defined(__x86_64__) || defined(__aarch64__)


we should probably have a properly named #define for this at this point, something like:

#if defined(__x86_64__) || defined(__aarch64__) #define RETSNOOP_SUPPORTS_ARG_CAPTURE 1 #endif

and use #ifdef RETSNOOP_SUPPORTS_ARG_CAPTURE in all relevant places?

anakryiko · 2025-03-04T00:46:53Z

src/fnargs.c

+
+#else
+	int stack_off = 0;
+#endif


can you please also adjust

fn_args->arg_spec_cnt = 6;

below to take into account architecture's maximum in register argument count? We probably need #define for that

Signed-off-by: Andrew E. Timmes <andrew.timmes@gmail.com>

aetimmes · 2025-03-06T00:40:05Z

I did some refactoring of arch-specific fnarg bits into retsnoop.h - I didn't adjust retsnoop.bpf.c because I didn't quite grok the distinction between #ifdef __${ARCH}__ and #ifdef __TARGET_ARCH_${ARCH}; open to suggestions there and on the var names - there are some similarly-named pre-existing vars being #defined and I wasn't sure how best to name them to disambiguate them.

aetimmes force-pushed the aet/arm-function-args branch 2 times, most recently from 4f57c94 to 3c90974 Compare February 27, 2025 13:31

first pass at ARM funcargs support

2a3401c

Signed-off-by: Andrew E. Timmes <andrew.timmes@gmail.com>

aetimmes force-pushed the aet/arm-function-args branch from 3c90974 to 2a3401c Compare February 27, 2025 13:33

anakryiko requested changes Feb 27, 2025

View reviewed changes

aetimmes force-pushed the aet/arm-function-args branch from 162b27a to 2a3401c Compare February 28, 2025 03:52

Dedupe src/fnargs.c

0aacc58

Signed-off-by: Andrew E. Timmes <andrew.timmes@gmail.com>

aetimmes force-pushed the aet/arm-function-args branch from 03ed912 to 0aacc58 Compare February 28, 2025 04:01

Dedupe src/retsnoop.bpf.c

7cfd2c0

Signed-off-by: Andrew E. Timmes <andrew.timmes@gmail.com>

aetimmes changed the title ~~first pass at ARM funcargs support~~ Add arm64 support to fnargs.c Feb 28, 2025

aetimmes marked this pull request as ready for review February 28, 2025 04:40

anakryiko requested changes Mar 4, 2025

View reviewed changes

refactor

e59ff3a

Signed-off-by: Andrew E. Timmes <andrew.timmes@gmail.com>

aetimmes force-pushed the aet/arm-function-args branch from 8cbf59d to e59ff3a Compare March 6, 2025 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add arm64 support to fnargs.c #89

Add arm64 support to fnargs.c #89

aetimmes commented Feb 27, 2025

anakryiko Feb 27, 2025

aetimmes Feb 28, 2025

anakryiko Feb 27, 2025

aetimmes Feb 28, 2025

anakryiko Feb 27, 2025

aetimmes Feb 28, 2025

aetimmes commented Feb 28, 2025 •

edited

Loading

anakryiko commented Mar 4, 2025

anakryiko left a comment

anakryiko Mar 4, 2025

anakryiko Mar 4, 2025

anakryiko Mar 4, 2025

aetimmes commented Mar 6, 2025 •

edited

Loading

Add arm64 support to fnargs.c #89

Are you sure you want to change the base?

Add arm64 support to fnargs.c #89

Conversation

aetimmes commented Feb 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aetimmes commented Feb 28, 2025 • edited Loading

anakryiko commented Mar 4, 2025

anakryiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aetimmes commented Mar 6, 2025 • edited Loading

aetimmes commented Feb 28, 2025 •

edited

Loading

aetimmes commented Mar 6, 2025 •

edited

Loading