Re: [PATCH 5/8] perf/amd/ibs: Don't allow freq mode event creation through ->config interface

[Namhyung Kim]

On Mon, Oct 07, 2024 at 03:48:07AM +0000, Ravi Bangoria wrote:
> Most perf_event_attr->config bits directly maps to IBS_{FETCH|OP}_CTL
> MSR. Since the sample period is programmed in these control registers,
> IBS PMU driver allows opening an IBS event by setting sample period
> value directly in perf_event_attr->config instead of using explicit
> perf_event_attr->sample_period interface.
> 
> However, this logic is not applicable for freq mode events since the
> semantics of control register fields are applicable only to fixed
> sample period whereas the freq mode event adjusts sample period after
> each and every sample. Currently, IBS driver (unintentionally) allows
> creating freq mode event via ->config interface, which is semantically
> wrong as well as detrimental because it can be misused to bypass
> perf_event_max_sample_rate checks.
> 
> Don't allow freq mode event creation through perf_event_attr->config
> interface.

Sounds reasonable.  I agree the freq mode should use the standard
interface using attr->sample_freq.  But I'm not sure if the behaivor is
defined when attr->freq is set and attr->sample_freq is 0.  Maybe this
should be handled in the generic code.

Thanks,
Namhyung

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@xxxxxxx>
> ---
>  arch/x86/events/amd/ibs.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
> index 152f9116af1e..368ed839b612 100644
> --- a/arch/x86/events/amd/ibs.c
> +++ b/arch/x86/events/amd/ibs.c
> @@ -302,6 +302,9 @@ static int perf_ibs_init(struct perf_event *event)
>  	} else {
>  		u64 period = 0;
>  
> +		if (event->attr.freq)
> +			return -EINVAL;
> +
>  		if (perf_ibs == &perf_ibs_op) {
>  			period = (config & IBS_OP_MAX_CNT) << 4;
>  			if (ibs_caps & IBS_CAPS_OPCNTEXT)
> -- 
> 2.46.2
>