Re: [Patch v10 06/10] perf regs: Support x86 SIMD registers sampling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The sample parser fails to skip the SIMD register data payload, corrupting subsequent sample fields.
- [Medium] Silently synchronizing SIMD register masks between intr and user modes bypasses architectural capability checks.
- [Medium] Bypassing hardware event check for SIMD register sampling enables it for unsupported software events.
--

--- Patch [6]: [Patch v10 06/10] perf regs: Support x86 SIMD registers sampling ---
commit 511dc4506d241eccda5990483afe081741ffbdb5
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf regs: Support x86 SIMD registers sampling

This commit adds support for the SIMD register sampling format on x86 platforms.
It introduces functions to retrieve register class masks, names, and lengths for 
both interrupt and user registers, and updates command-line parsing to handle
groups like XMM, YMM, ZMM, and OPMASK.

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index da0ad06446091..52249316709d7 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
[ ... ]
> @@ -1669,6 +1670,16 @@ void evsel__config(struct evsel *evsel, const struct record_opts *opts,
>  		evsel__set_sample_bit(evsel, REGS_INTR);
>  	}
>  
> +	if ((opts->sample_intr_vec_regs || opts->sample_intr_pred_regs) &&
> +	    !evsel->no_aux_samples && !evsel__is_dummy_event(evsel)) {
> +		attr->sample_simd_regs_enabled = !!opts->sample_simd_regs_enabled;

[Severity: Medium]
When SIMD registers are enabled here, should there be a check to verify it is a
hardware event?

For regular registers, this is guarded with evsel__is_hardware_event(evsel),
but here it seems to unconditionally enable SIMD registers for software events.
Could this cause the kernel to reject the event with -EINVAL?

> +		attr->sample_simd_vec_reg_intr = opts->sample_intr_vec_regs;
> +		attr->sample_simd_vec_reg_qwords = opts->sample_vec_reg_qwords;
> +		attr->sample_simd_pred_reg_intr = opts->sample_intr_pred_regs;
> +		attr->sample_simd_pred_reg_qwords = opts->sample_pred_reg_qwords;
> +		evsel__set_sample_bit(evsel, REGS_INTR);
> +	}

[Severity: High]
If SIMD sampling is enabled, does the parser in __evsel__parse_sample() properly
skip the SIMD data payload?

It appears that __evsel__parse_sample() only skips the 4 header elements but
misses the variable-length SIMD data array that follows:

__evsel__parse_sample() {
    ...
    if (regs->abi & PERF_SAMPLE_REGS_ABI_SIMD) {
        /* Skip SIMD-regs header. */
        sz = 4 * sizeof(u64);
        OVERFLOW_CHECK(array, sz, max_size);
        array = (void *)array + sz;
    }
    ...
}

Will this leave the array pointer pointing at register data instead of the next
field (like PERF_SAMPLE_STACK_USER), potentially corrupting subsequent fields
and failing to parse the event?

> +
>  	if (opts->sample_user_regs && !evsel->no_aux_samples &&
>  	    !evsel__is_dummy_event(evsel)) {
>  		attr->sample_regs_user |= opts->sample_user_regs;
> @@ -1677,6 +1688,16 @@ void evsel__config(struct evsel *evsel, const struct record_opts *opts,
>  		evsel__set_sample_bit(evsel, REGS_USER);
>  	}
>  
> +	if ((opts->sample_user_vec_regs || opts->sample_user_pred_regs) &&
> +	    !evsel->no_aux_samples && !evsel__is_dummy_event(evsel)) {
> +		attr->sample_simd_regs_enabled = !!opts->sample_simd_regs_enabled;
> +		attr->sample_simd_vec_reg_user = opts->sample_user_vec_regs;
> +		attr->sample_simd_vec_reg_qwords = opts->sample_vec_reg_qwords;
> +		attr->sample_simd_pred_reg_user = opts->sample_user_pred_regs;
> +		attr->sample_simd_pred_reg_qwords = opts->sample_pred_reg_qwords;
> +		evsel__set_sample_bit(evsel, REGS_USER);
> +	}
> +
>  	if (target__has_cpu(&opts->target) || opts->sample_cpu)
>  		evsel__set_sample_bit(evsel, CPU);
>  
> diff --git a/tools/perf/util/parse-regs-options.c b/tools/perf/util/parse-regs-options.c
> index 70a1cc90b2c13..709fcfb077a2b 100644
> --- a/tools/perf/util/parse-regs-options.c
> +++ b/tools/perf/util/parse-regs-options.c
[ ... ]
> @@ -53,26 +106,116 @@ name_to_perf_reg_mask(const char *to_match, uint64_t mask, int abi)
>  	return reg_mask;
>  }
>  
> +static bool
> +name_to_simd_reg_mask(struct record_opts *opts, const char *to_match,
> +		      uint64_t mask, bool intr, bool pred)
> +{
[ ... ]
> +		/*
> +		 * sample_intr_pred_regs and sample_user_pred_regs share
> +		 * sample_pred_reg_qwords. If both are set, keep their bitmaps identical.
> +		 */
> +		if (opts->sample_intr_pred_regs && opts->sample_user_pred_regs) {
> +			uint64_t max_pred_regs = MAX(opts->sample_intr_pred_regs,
> +						     opts->sample_user_pred_regs);
> +
> +			opts->sample_intr_pred_regs = max_pred_regs;
> +			opts->sample_user_pred_regs = max_pred_regs;
> +		}
> +	} else {
> +		opts->sample_vec_reg_qwords =
> +			MAX(qwords, opts->sample_vec_reg_qwords);
[ ... ]
> +		/*
> +		 * sample_intr_vec_regs and sample_user_vec_regs share
> +		 * sample_vec_reg_qwords. If both are set, keep their bitmaps identical.
> +		 */
> +		if (opts->sample_intr_vec_regs && opts->sample_user_vec_regs) {
> +			uint64_t max_vec_regs = MAX(opts->sample_intr_vec_regs,
> +						    opts->sample_user_vec_regs);
> +
> +			opts->sample_intr_vec_regs = max_vec_regs;
> +			opts->sample_user_vec_regs = max_vec_regs;
> +		}
> +	}

[Severity: Medium]
Does synchronizing the SIMD register masks between intr and user modes bypass
architectural capability checks?

If a user specifies different sets for INTR and USER mode SIMD registers
(e.g., -I XMM --user-regs ZMM), forcing both masks to the maximum might
violate the capability check retrieved by
perf_intr_simd_reg_class_bitmap_qwords().

Will this cause perf record to pass an unsupported register bitmask and fail
with -EINVAL from the kernel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721072707.3787848-1-dapeng1.mi@linux.intel.com?part=6