Re: [PATCH v1] perf/core: Restore header fields in sideband output callbacks

[bot+bpf-ci]

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index db7b76d6b68a..2be915a293ce 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -9032,6 +9032,11 @@ static void perf_iterate_sb_cpu(perf_iterate_f output, void *data)
>   *
>   * For new callers; ensure that account_pmu_sb_event() includes
>   * your event, otherwise it might not get delivered.
> + *
> + * Note: @data is shared across all @output calls, so any fields modified
> + * incrementally or conditionally (e.g. header.size via
> + * perf_event_header__init_id()) must be saved and restored by @output,
> + * or unconditionally re-initialized on each call.
>   */
>  static void
>  perf_iterate_sb(perf_iterate_f output, void *data,

Peter Zijlstra identified that perf_event_switch_output() also modifies
header.type and header.size before calling perf_event_header__init_id()
and suggested it might need similar treatment.

Looking at perf_event_switch_output():

    if (event->ctx->task) {
        se->event_id.header.type = PERF_RECORD_SWITCH;
        se->event_id.header.size = sizeof(se->event_id.header);
    } else {
        se->event_id.header.type = PERF_RECORD_SWITCH_CPU_WIDE;
        se->event_id.header.size = sizeof(se->event_id);
        ...
    }
    perf_event_header__init_id(&se->event_id.header, &sample, event);

The new comment mentions that fields must be "saved and restored by
@output, or unconditionally re-initialized on each call."
perf_event_switch_output() does unconditionally reinitialize both
header.type and header.size at the start of each callback invocation,
which should prevent the accumulation bug.

Is this analysis correct, or does perf_event_switch_output() need the
save/restore pattern as well?

> @@ -9726,6 +9731,7 @@ static void perf_event_mmap_output(struct perf_event *event,
>  	struct perf_sample_data sample;
>  	int size = mmap_event->event_id.header.size;
>  	u32 type = mmap_event->event_id.header.type;
> +	u16 misc = mmap_event->event_id.header.misc;
>  	bool use_build_id;
>  	int ret;
>

[ ... ]

> @@ -9783,6 +9789,7 @@ static void perf_event_mmap_output(struct perf_event *event,
>  out:
>  	mmap_event->event_id.header.size = size;
>  	mmap_event->event_id.header.type = type;
> +	mmap_event->event_id.header.misc = misc;
>  }

[ ... ]

> @@ -10247,6 +10254,7 @@ static void perf_event_ksymbol_output(struct perf_event *event, void *data)
>  	struct perf_ksymbol_event *ksymbol_event = data;
>  	struct perf_output_handle handle;
>  	struct perf_sample_data sample;
> +	u16 header_size = ksymbol_event->event_id.header.size;
>  	int ret;
>
>  	if (!perf_event_ksymbol_match(event))
> @@ -10257,13 +10265,15 @@ static void perf_event_ksymbol_output(struct perf_event *event, void *data)
>  	ret = perf_output_begin(&handle, &sample, event,
>  				ksymbol_event->event_id.header.size);
>  	if (ret)
> -		return;
> +		goto out;
>
>  	perf_output_put(&handle, ksymbol_event->event_id);
>  	__output_copy(&handle, ksymbol_event->name, ksymbol_event->name_len);
>  	perf_event__output_id_sample(event, &handle, &sample);
>
>  	perf_output_end(&handle);
> +out:
> +	ksymbol_event->event_id.header.size = header_size;
>  }

[ ... ]

> @@ -10337,6 +10347,7 @@ static void perf_event_bpf_output(struct perf_event *event, void *data)
>  	struct perf_bpf_event *bpf_event = data;
>  	struct perf_output_handle handle;
>  	struct perf_sample_data sample;
> +	u16 header_size = bpf_event->event_id.header.size;
>  	int ret;
>
>  	if (!perf_event_bpf_match(event))
> @@ -10347,12 +10358,14 @@ static void perf_event_bpf_output(struct perf_event *event, void *data)
>  	ret = perf_output_begin(&handle, &sample, event,
>  				bpf_event->event_id.header.size);
>  	if (ret)
> -		return;
> +		goto out;
>
>  	perf_output_put(&handle, bpf_event->event_id);
>  	perf_event__output_id_sample(event, &handle, &sample);
>
>  	perf_output_end(&handle);
> +out:
> +	bpf_event->event_id.header.size = header_size;
>  }

[ ... ]

> @@ -10499,6 +10512,7 @@ static void perf_event_text_poke_output(struct perf_event *event, void *data)
>  	struct perf_text_poke_event *text_poke_event = data;
>  	struct perf_output_handle handle;
>  	struct perf_sample_data sample;
> +	u16 header_size = text_poke_event->event_id.header.size;
>  	u64 padding = 0;
>  	int ret;
>
> @@ -10510,7 +10524,7 @@ static void perf_event_text_poke_output(struct perf_event *event, void *data)
>  	ret = perf_output_begin(&handle, &sample, event,
>  				text_poke_event->event_id.header.size);
>  	if (ret)
> -		return;
> +		goto out;
>
>  	perf_output_put(&handle, text_poke_event->event_id);
>  	perf_output_put(&handle, text_poke_event->old_len);
> @@ -10525,6 +10539,8 @@ static void perf_event_text_poke_output(struct perf_event *event, void *data)
>  	perf_event__output_id_sample(event, &handle, &sample);
>
>  	perf_output_end(&handle);
> +out:
> +	text_poke_event->event_id.header.size = header_size;
>  }


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/36643934404