Re: [PATCH v5 01/23] perf trace: Set the augmented arg header in the augmenters that omit it

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Sep 23, 2026 at 12:13:41AM -0700, Ian Rogers wrote:
> An augmented argument is a struct augmented_arg header, holding the
> length of the payload and an error code, followed by the payload. The
> augmenters build it in augmented_args_tmp, a single entry per-CPU array
> reused by every syscall on that CPU, so a field left unassigned holds
> whatever the previous syscall on that CPU put there rather than anything
> about this one.
> 
> The string augmenters get both fields from augmented_arg__read_str(),
> and sys_enter_perf_event_open() bails out when its read fails, but
> sys_enter_connect(), sys_enter_sendto(), sys_enter_clock_nanosleep() and
> sys_enter_nanosleep() copy the payload and output the record without
> ever describing it, and augment_arg() sets the length but not the error.
> 
> This has gone unnoticed because the beautifiers for those payloads take
> a fixed sized type and so read the value without consulting the header:
> syscall_arg__scnprintf_augmented_sockaddr(),
> syscall_arg__scnprintf_augmented_timespec() and
> syscall_arg__scnprintf_augmented_perf_event_attr() all cast
> augmented.args->value directly. Nothing has yet read a length that was
> never written.
> 
> Describe the payload everywhere one is produced, so that a reader can
> bound it by its length.
> 
> These reads can fail, and none of the four checked whether they had.
> Rather than claim a payload that was not read, report a length of zero
> and the error, so the record describes what it holds and leaves the
> scratch out of it. A reader that bounds the payload by the header then
> shows the pointer, as it does for a syscall with no augmentation at
> all, rather than another task's data.
> 
> The error is set for the same reason the length is, that the header is
> in a buffer the next syscall on this CPU will reuse and so carries the
> previous one's value if it is not written, rather than because anything
> reads it yet: beauty.h still calls the field int_arg and no beautifier
> looks at it. augment_arg() reported a string it could not read as an
> empty one before this and still does.
> 
> Assisted-by: Antigravity:gemini-3.1-pro
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  .../bpf_skel/augmented_raw_syscalls.bpf.c     | 53 ++++++++++++++++---
>  1 file changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> index 3bc9e28a9b8a..dd3aa5bd910b 100644
> --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> @@ -210,6 +210,7 @@ int sys_enter_connect(struct syscall_enter_args *args)
>  	const void *sockaddr_arg = (const void *)args->args[1];
>  	unsigned int socklen = args->args[2];
>  	unsigned int len = sizeof(u64) + sizeof(augmented_args->args); // the size + err in all 'augmented_arg' structs
> +	int err;
>  
>          if (augmented_args == NULL)
>                  return 1; /* Failure: don't filter */
> @@ -217,9 +218,16 @@ int sys_enter_connect(struct syscall_enter_args *args)
>  	_Static_assert(is_power_of_2(sizeof(augmented_args->arg.saddr)), "sizeof(augmented_args->arg.saddr) needs to be a power of two");
>  	socklen &= sizeof(augmented_args->arg.saddr) - 1;
>  
> -	bpf_probe_read_user(&augmented_args->arg.saddr, socklen, sockaddr_arg);
> +	err = bpf_probe_read_user(&augmented_args->arg.saddr, socklen, sockaddr_arg);
> +	/*
> +	 * A failed read leaves the scratch holding whatever the previous
> +	 * syscall on this CPU put there, so say there is no payload and why,
> +	 * rather than describing another task's data as this task's sockaddr.
> +	 */
> +	if (err < 0)
> +		socklen = 0;
>  	augmented_args->arg.size = socklen;
> -	augmented_args->arg.err = 0;
> +	augmented_args->arg.err = err < 0 ? err : 0;

It seems we can simply use the return value of bpf_probe_read_user()
as it only returns 0 or a negative error according to the doc.

https://docs.ebpf.io/linux/helper-function/bpf_probe_read_user/

>  
>  	return augmented__output(args, augmented_args, len + socklen);
>  }
> @@ -231,13 +239,19 @@ int sys_enter_sendto(struct syscall_enter_args *args)
>  	const void *sockaddr_arg = (const void *)args->args[4];
>  	unsigned int socklen = args->args[5];
>  	unsigned int len = sizeof(u64) + sizeof(augmented_args->args); // the size + err in all 'augmented_arg' structs
> +	int err;
>  
>          if (augmented_args == NULL)
>                  return 1; /* Failure: don't filter */
>  
>  	socklen &= sizeof(augmented_args->arg.saddr) - 1;
>  
> -	bpf_probe_read_user(&augmented_args->arg.saddr, socklen, sockaddr_arg);
> +	err = bpf_probe_read_user(&augmented_args->arg.saddr, socklen, sockaddr_arg);
> +	/* As in sys_enter_connect(), do not describe scratch as a sockaddr. */
> +	if (err < 0)
> +		socklen = 0;
> +	augmented_args->arg.size = socklen;
> +	augmented_args->arg.err = err < 0 ? err : 0;
>  
>  	return augmented__output(args, augmented_args, len + socklen);
>  }
> @@ -372,6 +386,9 @@ int sys_enter_perf_event_open(struct syscall_enter_args *args)
>  	if (bpf_probe_read_user(&augmented_args->arg.value, size, attr) < 0)
>  		goto failure;
>  
> +	augmented_args->arg.size = size;
> +	augmented_args->arg.err = 0;
> +
>  	return augmented__output(args, augmented_args, len + size);
>  failure:
>  	return 1; /* Failure: don't filter */
> @@ -384,6 +401,7 @@ int sys_enter_clock_nanosleep(struct syscall_enter_args *args)
>  	const void *rqtp_arg = (const void *)args->args[2];
>  	unsigned int len = sizeof(u64) + sizeof(augmented_args->args); // the size + err in all 'augmented_arg' structs
>  	__u32 size = sizeof(struct timespec64);
> +	int err;
>  
>          if (augmented_args == NULL)
>  		goto failure;
> @@ -391,7 +409,12 @@ int sys_enter_clock_nanosleep(struct syscall_enter_args *args)
>  	if (size > sizeof(augmented_args->arg.value))
>                  goto failure;
>  
> -	bpf_probe_read_user(&augmented_args->arg.value, size, rqtp_arg);
> +	err = bpf_probe_read_user(&augmented_args->arg.value, size, rqtp_arg);
> +	/* As in sys_enter_connect(), do not describe scratch as a timespec. */
> +	if (err < 0)
> +		size = 0;
> +	augmented_args->arg.size = size;
> +	augmented_args->arg.err = err < 0 ? err : 0;
>  
>  	return augmented__output(args, augmented_args, len + size);
>  failure:
> @@ -405,6 +428,7 @@ int sys_enter_nanosleep(struct syscall_enter_args *args)
>  	const void *req_arg = (const void *)args->args[0];
>  	unsigned int len = sizeof(augmented_args->args);
>  	__u32 size = sizeof(struct timespec64);
> +	int err;
>  
>          if (augmented_args == NULL)
>  		goto failure;
> @@ -412,7 +436,12 @@ int sys_enter_nanosleep(struct syscall_enter_args *args)
>  	if (size > sizeof(augmented_args->arg.value))
>                  goto failure;
>  
> -	bpf_probe_read_user(&augmented_args->arg.value, size, req_arg);
> +	err = bpf_probe_read_user(&augmented_args->arg.value, size, req_arg);
> +	/* As in sys_enter_connect(), do not describe scratch as a timespec. */
> +	if (err < 0)
> +		size = 0;
> +	augmented_args->arg.size = size;
> +	augmented_args->arg.err = err < 0 ? err : 0;
>  
>  	return augmented__output(args, augmented_args, len + size);
>  failure:
> @@ -445,6 +474,7 @@ static inline int augment_arg(struct syscall_enter_args *args, int i,
>  			      struct beauty_payload_enter *payload, u64 offset)
>  {
>  	int index, value_size = sizeof(struct augmented_arg) - offsetof(struct augmented_arg, value);
> +	int read_err = 0;
>  	struct augmented_arg *payload_offset;
>  	s64 aug_size, size;
>  	bool augmented;
> @@ -467,8 +497,18 @@ static inline int augment_arg(struct syscall_enter_args *args, int i,
>  	if (size == 1) { /* string */
>  		aug_size = bpf_probe_read_user_str(payload_offset->value, value_size, arg);
>  		/* minimum of 0 to pass the verifier */
> -		if (aug_size < 0)
> +		if (aug_size < 0) {
> +			/*
> +			 * Record why nothing was read. The header sits in
> +			 * scratch that the next syscall on this CPU reuses,
> +			 * so an error left unwritten is the previous one's.
> +			 * No beautifier reads it yet, beauty.h still calls
> +			 * the field int_arg, so the string is still shown
> +			 * as an empty one.
> +			 */

Do we really need this comment?  Looks too verbose.

Thanks,
Namhyung


> +			read_err = aug_size;
>  			aug_size = 0;
> +		}
>  
>  		augmented = true;
>  	} else if (size > 0 && size <= value_size) { /* struct */
> @@ -498,6 +538,7 @@ static inline int augment_arg(struct syscall_enter_args *args, int i,
>  			return -1;
>  
>  		payload_offset->size = aug_size;
> +		payload_offset->err = read_err;
>  		return written;
>  	}
>  
> -- 
> 2.56.0.rc1.315.gc6ed9934b7-goog
>