* [PATCH] perf test sample-parsing: Validate PERF_FORMAT_GROUP values without LOST @ 2026-07-25 8:47 PVS Narasimha Rao 2026-07-25 8:56 ` sashiko-bot 0 siblings, 1 reply; 3+ messages in thread From: PVS Narasimha Rao @ 2026-07-25 8:47 UTC (permalink / raw) To: linux-perf-users Cc: acme, namhyung, irogers, peterz, mingo, PVS Narasimha Rao The sample parsing test only validates grouped read values when PERF_FORMAT_LOST is present. For PERF_FORMAT_GROUP without PERF_FORMAT_LOST, the contents of read.group.values[] are not validated, allowing corruption of parsed value and id fields to go undetected. Validate value and id for grouped reads and continue to validate lost when PERF_FORMAT_LOST is present. Use next_sample_read_value() to walk the variable-sized group entries. Signed-off-by: PVS Narasimha Rao <venkatasuryapala@gmail.com> diff --git a/tools/perf/tests/sample-parsing.c b/tools/perf/tests/sample-parsing.c index 55f0b73ca20e..e44b01e5cd14 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); } } else { COMP(read.one.id); -- 2.43.7 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] perf test sample-parsing: Validate PERF_FORMAT_GROUP values without LOST 2026-07-25 8:47 [PATCH] perf test sample-parsing: Validate PERF_FORMAT_GROUP values without LOST PVS Narasimha Rao @ 2026-07-25 8:56 ` sashiko-bot 2026-08-06 5:40 ` Namhyung Kim 0 siblings, 1 reply; 3+ messages in thread From: sashiko-bot @ 2026-07-25 8:56 UTC (permalink / raw) To: PVS Narasimha Rao; +Cc: linux-perf-users 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] perf test sample-parsing: Validate PERF_FORMAT_GROUP values without LOST 2026-07-25 8:56 ` sashiko-bot @ 2026-08-06 5:40 ` Namhyung Kim 0 siblings, 0 replies; 3+ messages in thread From: Namhyung Kim @ 2026-08-06 5:40 UTC (permalink / raw) To: sashiko-reviews; +Cc: PVS Narasimha Rao, linux-perf-users Hello, On Sat, Jul 25, 2026 at 08:56:50AM +0000, sashiko-bot@kernel.org wrote: > 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? I think it's a valid issue and it seems there's a bug in the perf_event__synthesize_sample(). The input values contain the lost field regardless of the read format. So it should skip the field carefully. Thanks, Namhyung ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-08-06 5:40 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-25 8:47 [PATCH] perf test sample-parsing: Validate PERF_FORMAT_GROUP values without LOST PVS Narasimha Rao 2026-07-25 8:56 ` sashiko-bot 2026-08-06 5:40 ` Namhyung Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox