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.

Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 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

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@amd.com>
> ---
>  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
>

>> 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.

I also could not find any reason to allow {freq=1, sample_freq=0}, but:

1) perf_event_open() allows it.
2) ioctl(PERF_EVENT_IOC_PERIOD) allows it.
3) all generic code explicitly checks for ->sample_freq != 0 wherever
   ->freq == 1.

I will prepare and post a patch to reject such event and see if there
are any objections.

Thanks,
Ravi

On Tue, Oct 08, 2024 at 11:00:37AM +0530, Ravi Bangoria wrote:
> >> 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.
> 
> I also could not find any reason to allow {freq=1, sample_freq=0}, but:
> 
> 1) perf_event_open() allows it.
> 2) ioctl(PERF_EVENT_IOC_PERIOD) allows it.
> 3) all generic code explicitly checks for ->sample_freq != 0 wherever
>    ->freq == 1.
> 
> I will prepare and post a patch to reject such event and see if there
> are any objections.

Hmm.. now I think that the kernel won't treat it as a sampling event and
would ignore the attr.freq value.  Setting IOC_PERIOD to 0 would disable
sampling then.  Sorry for the noise.

Thanks,
Namhyung