Re: [PATCH] perf test sample-parsing: Validate PERF_FORMAT_GROUP values without LOST

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The test setup for grouped reads without PERF_FORMAT_LOST validates garbage data because it provides a standard C array of structs rather than a tightly packed buffer.
--

--- Patch [1]: [PATCH] perf test sample-parsing: Validate PERF_FORMAT_GROUP values without LOST ---
commit 803c0b64d4e8c311df29991bbbdd74726b85a1dd
Author: PVS Narasimha Rao <venkatasuryapala@gmail.com>

perf test sample-parsing: Validate PERF_FORMAT_GROUP values without LOST

This patch updates the sample parsing test to validate the value and id
fields in grouped reads even when PERF_FORMAT_LOST is absent. It uses
next_sample_read_value() to properly navigate the variable-sized group
entries during validation.

> diff --git a/tools/perf/tests/sample-parsing.c b/tools/perf/tests/sample-parsing.c
> index 55f0b73ca20e0..e44b01e5cd14c 100644
> --- a/tools/perf/tests/sample-parsing.c
> +++ b/tools/perf/tests/sample-parsing.c
[ ... ]
> @@ -86,10 +86,27 @@ static bool samples_same(struct perf_sample *s1,
>  			COMP(read.time_running);
>  		/* PERF_FORMAT_ID is forced for PERF_SAMPLE_READ */
>  		if (read_format & PERF_FORMAT_GROUP) {
> +			struct sample_read_value *v1 = s1->read.group.values;
> +			struct sample_read_value *v2 = s2->read.group.values;
> +
>  			for (i = 0; i < s1->read.group.nr; i++) {
> -				/* FIXME: check values without LOST */
> -				if (read_format & PERF_FORMAT_LOST)
> -					MCOMP(read.group.values[i]);
> +				if (v1->value != v2->value) {
> +					pr_debug("Samples differ at 'read.group.values[].value'\n");
> +					return false;
> +				}
> +
> +				if (v1->id != v2->id) {
> +					pr_debug("Samples differ at 'read.group.values[].id'\n");
> +					return false;
> +				}
> +
> +				if (read_format & PERF_FORMAT_LOST &&
> +				    v1->lost != v2->lost) {
> +					pr_debug("Samples differ at 'read.group.values[].lost'\n");
> +					return false;
> +				}
> +				v1 = next_sample_read_value(v1, read_format);
> +				v2 = next_sample_read_value(v2, read_format);

[Severity: Medium]
This is a pre-existing issue, but does using next_sample_read_value() here
inadvertently cause the test to assert equality on misaligned garbage data
when PERF_FORMAT_LOST is omitted?

Looking at do_test(), the original sample is initialized with a standard C
array of struct sample_read_value:

tools/perf/tests/sample-parsing.c:do_test() {
    ...
    struct sample_read_value values[] = {{1, 5, 0}, {9, 3, 0}, {2, 7, 0}, {6, 4, 1},};
    ...
    if (read_format & PERF_FORMAT_GROUP) {
        sample.read.group.nr     = 4;
        sample.read.group.values = values;
    }
    ...
}

Since this array always has a 24-byte stride, but next_sample_read_value()
advances by a 16-byte stride when PERF_FORMAT_LOST is absent, both the
synthesizer and this validation loop end up reading overlapping memory
instead of the actual struct fields.

Could the test setup be updated to provide a tightly packed buffer so that
we validate the actual fields rather than falsely passing on identical
misaligned reads?

>  			}
>  		} else {
>  			COMP(read.one.id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725084704.15463-1-venkatasuryapala@gmail.com?part=1