Re: [PATCH v2 1/2] perf trace: Factor out BPF loop body

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Zero-extension of map values makes the condition `size < 0` always false, resulting in dead code that completely breaks tracepoint buffer augmentation.
--

commit d586728f909e7c096ce4ad924848b781a274c29f
Author: Viktor Malik <vmalik@redhat.com>

perf trace: Factor out BPF loop body

This commit factors out the loop body in augmented_raw_syscalls into a
separate function. This is intended to simplify the code and help address
problems with the BPF verifier.

> diff --git a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c

[ ... ]

> +static inline int augment_arg(struct syscall_enter_args *args, int i,
> +			      unsigned int *beauty_map, void *payload_offset)
> +{
> +	int index, value_size = sizeof(struct augmented_arg) - offsetof(struct augmented_arg, value);
> +	s64 aug_size, size;
> +	bool augmented;
> +	void *arg;
> +
> +	arg = (void *)args->args[i];
> +	augmented = false;
> +	size = beauty_map[i];

[ ... ]

> +	} else if (size > 0 && size <= value_size) { /* struct */
> +		if (!bpf_probe_read_user(((struct augmented_arg *)payload_offset)->value, size, arg))
> +			augmented = true;
> +	} else if ((int)size < 0 && size >= -6) { /* buffer */

[Severity: Medium]
Will this condition evaluate correctly given that beauty_map contains
unsigned integers?

When the userspace perf trace tool populates beauty_map_enter with negative
values (e.g., -1) to indicate buffer arguments, they are stored as 32-bit
unsigned integers (e.g., 4294967295).

Since size is an s64, reading from the map via size = beauty_map[i]
zero-extends this unsigned value, making it strictly non-negative.

It appears a subsequent patch in this series removes the (int) cast to
address an out-of-bounds array access caused by the size >= -6 check
(which was a pre-existing issue). 

However, removing that cast leaves the condition checking if size < 0,
which will always be false because of the zero-extension.

Could this inadvertently turn the entire buffer augmentation branch into
dead code, systematically failing to augment every buffer tracepoint
argument?

> +		index = -(size + 1);
> +		barrier_var(index); // Prevent clang (noticed with v18) from removing the &= 7 trick.
> +		index &= 7;	    // Satisfy the bounds checking with the verifier in some kernels.
> +		aug_size = args->args[index] > TRACE_AUG_MAX_BUF ? TRACE_AUG_MAX_BUF : args->args[index];

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783070132.git.vmalik@redhat.com?part=1