Re: [RFC v1 16/19] ptwrite uprobes / perf tools pt: Improve FUP error handling for ptwrite

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect ptwrite IP reporting and failure to prevent trace abort in full instruction tracing mode.
- [Medium] Bypassing `intel_pt_fup_event()` causes incomplete manual state reconstruction, leaking instruction metadata and asynchronous FUP flags.
--

commit f901af3d0d6f6651e120a5aaaf5502b7aa3f6612
Author: Andi Kleen <ak@kernel.org>

ptwrite uprobes / perf tools pt: Improve FUP error handling for ptwrite

Special case the decoder to handle ptwrite FUPs targeting unmapped code
like uprobe stubs. When the decoder can't resolve the instruction page,
report the ptwrite with its IP instead of erroring out and failing the walk.

> diff --git a/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c b/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c
> index e733f6b1f7ac5..bd31d65dbe035 100644
> --- a/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c
> +++ b/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c
> @@ -1440,8 +1440,29 @@ static int intel_pt_walk_fup(struct intel_pt_decoder *decoder)
>  			return -EAGAIN;
>  		}
>  		decoder->set_fup_tx_flags = false;
> -		if (err)
> +		if (err) {
> +			/*
> +			 * A ptwrite's FUP can target an address whose
> +			 * instruction cannot be resolved (e.g. the
> +			 * [uprobes-ptwrite] stub is an anonymous special
> +			 * mapping invisible to the machine). The FUP is
> +			 * still the ptwrite's IP: report it rather than
> +			 * failing the whole walk.
> +			 */
> +			if (decoder->set_fup_ptw) {
> +				decoder->set_fup_ptw = false;
> +				decoder->pkt_state = INTEL_PT_STATE_IN_SYNC;
> +				decoder->state.type &= ~INTEL_PT_BRANCH;
> +				decoder->state.type |= INTEL_PT_PTW;

[Severity: Medium]
Will bypassing intel_pt_fup_event() here leak instruction metadata and
asynchronous FUP flags?

Because intel_pt_walk_insn() returned an error early, decoder->state.insn_op
and insn_len retain the metadata from the last successfully decoded
instruction. By skipping intel_pt_fup_event() which usually clears these
fields and handles asynchronous flags, could stale instruction op/len and
pending event flags be incorrectly attributed to subsequent events?

> +				decoder->state.flags |= INTEL_PT_FUP_IP;
> +				decoder->state.from_ip = decoder->ip;

[Severity: High]
Does this code assign the wrong IP for the PTWRITE event and fail to prevent
the trace abort it intends to fix?

When intel_pt_walk_insn() encounters an unmapped region like the uprobe stub,
it returns an error without advancing decoder->ip. The true PTWRITE IP from
the hardware FUP packet is passed as the ip parameter (from
decoder->last_ip) to intel_pt_walk_insn().

Assigning decoder->state.from_ip = decoder->ip instead of using the ip
parameter attributes the event to the stuck failure address. Additionally,
because decoder->ip is never advanced out of the unmapped region, subsequent
packets could trigger functions like intel_pt_walk_tip() to call
intel_pt_walk_insn(..., 0) starting from the same stuck decoder->ip, failing
again and aborting the trace.

> +				decoder->state.to_ip = 0;
> +				decoder->state.ptw_payload =
> +							decoder->fup_ptw_payload;
> +				return 0;
> +			}
>  			return err;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831150651.1134594-1-ak@kernel.org?part=16