Re: [PATCH 4/5] alpha/perf: attach ProfileMe status as PERF_SAMPLE_RAW data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] alpha/perf: misaligned u64 fields and unwritten pad in alpha_profileme_sample
--

commit 6d712b2296772549d16528b224255d9e1a7bde43
Author: Matt Turner <mattst88@gmail.com>

alpha/perf: attach ProfileMe status as PERF_SAMPLE_RAW data

This commit attaches the PMPC, I_STAT, and PCTR_CTL register values to
ProfileMe samples as PERF_SAMPLE_RAW data using a versioned UAPI struct
alpha_profileme_sample.

> diff --git a/arch/alpha/include/uapi/asm/perf_event.h b/arch/alpha/include/uapi/asm/perf_event.h
> new file mode 100644
> index 0000000000000..0cb7d6f3cb154
> --- /dev/null
> +++ b/arch/alpha/include/uapi/asm/perf_event.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_ASM_ALPHA_PERF_EVENT_H
> +#define _UAPI_ASM_ALPHA_PERF_EVENT_H
> +
> +#include <linux/types.h>
> +
> +/*
> + * PERF_SAMPLE_RAW payload of a ProfileMe sample from the cpu PMU.
> + *
> + * pmpc, i_stat and pctr_ctl are the values of the PMPC, I_STAT and
> + * PCTR_CTL registers when the ProfileMe window closed. i_stat holds only
> + * bits 40:30 and pctr_ctl only bits 47:0. See the EV67/EV68 Hardware
> + * Reference Manuals for the field layout.
> + *
> + * Fields may be added to the end in later versions.
> + */
> +#define ALPHA_PROFILEME_SAMPLE_VERSION	1
> +
> +struct alpha_profileme_sample {
> +	__u32 version;
> +	__u32 reserved;
> +	__u64 pmpc;
> +	__u64 i_stat;
> +	__u64 pctr_ctl;
> +};

[ ... ]

> diff --git a/arch/alpha/kernel/perf_event.c b/arch/alpha/kernel/perf_event.c
> index 24c96feda9424..06c9afeb7c05e 100644
> --- a/arch/alpha/kernel/perf_event.c
> +++ b/arch/alpha/kernel/perf_event.c

[ ... ]

> @@ -955,6 +958,20 @@ static void alpha_perf_event_profileme(struct perf_event *event,
>  
>  	perf_sample_data_init(&data, 0, hwc->last_period);
>  
> +	pm.version = ALPHA_PROFILEME_SAMPLE_VERSION;
> +	pm.reserved = 0;
> +	pm.pmpc = pmpc;
> +	pm.i_stat = i_stat;
> +	/* PALcode sign-extends bit 47 of PCTR_CTL. */
> +	pm.pctr_ctl = pctr_ctl & GENMASK(47, 0);
> +	raw = (struct perf_raw_record) {
> +		.frag = {
> +			.size = sizeof(pm),
> +			.data = &pm,
> +		},
> +	};
> +	perf_sample_save_raw_data(&data, event, &raw);

[Severity: Medium]
Does including the __u32 reserved field in struct alpha_profileme_sample
misalign pmpc, i_stat, and pctr_ctl in the perf ring buffer and leave 4 bytes
of trailing padding unwritten?

In a PERF_RECORD_SAMPLE record, all fields preceding PERF_SAMPLE_RAW are
multiples of sizeof(u64). When perf_output_sample() writes the
PERF_SAMPLE_RAW section, it begins on an 8-byte boundary by outputting the
4-byte u32 raw->size field, which places the start of the raw payload at a 4
(mod 8) byte offset in the ring buffer.

Because struct alpha_profileme_sample has both __u32 version and __u32
reserved before __u64 pmpc, its total size is 32 bytes and pmpc is at offset
8 within the struct. Placed after the 4-byte raw->size field in the sample
record, pmpc, i_stat, and pctr_ctl land at 4 (mod 8) offsets rather than
8-byte aligned offsets, triggering unaligned 64-bit load traps on Alpha when
dereferenced in place and breaking 64-bit byte-swapping in
evsel__parse_sample().

In addition, when alpha_perf_event_profileme() passes sizeof(pm) (32 bytes) to
perf_sample_save_raw_data():

include/linux/perf_event.h:perf_sample_save_raw_data() {
	...
	size = round_up(sum + sizeof(u32), sizeof(u64));
	raw->size = size - sizeof(u32);
	frag->pad = raw->size - sum;
	...
}

round_up(32 + 4, 8) evaluates to 40, which sets raw->size to 36 and
frag->pad to 4. When perf_element overflows into
alpha_perf_event_profileme() -> perf_event_overflow() ->
__perf_event_overflow() -> perf_output_sample():

kernel/events/core.c:perf_output_sample() {
	...
			if (frag->pad)
				__output_skip(handle, NULL, frag->pad);
	...
}

__output_skip() advances the ring buffer position by 4 bytes without writing
to the buffer, leaving 4 trailing bytes of the 36-byte raw payload unwritten
(exposing stale ring buffer data after wrap-around).

Would it make sense to drop __u32 reserved from struct alpha_profileme_sample
so that the struct is 28 bytes (4 mod 8), keeping pmpc, i_stat, and pctr_ctl
8-byte aligned in the sample record with raw->size == 28 and frag->pad == 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-alpha-profileme-v1-0-d44b52bf2500@gmail.com?part=4