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.

Acked-by: Namhyung Kim <namhyung@kernel.org>
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 11123c174a3b..a687ffb09905 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.47.0

* Ravi Bangoria <ravi.bangoria@amd.com> 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.

Then let's fix those rate checks?

AFAICS this patch limits functionality because the IBS driver would 
have to be fixed/enhanced to support frequency based events?

I'd strongly favor fixing/enhancing the driver instead, as 'perf top -F 
1000' is easy to use and it is a useful concept.

Thanks,

	Ingo

Hi Ingo,

>> 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.
> 
> Then let's fix those rate checks?
> 
> AFAICS this patch limits functionality because the IBS driver would 
> have to be fixed/enhanced to support frequency based events?
> 
> I'd strongly favor fixing/enhancing the driver instead, as 'perf top -F 
> 1000' is easy to use and it is a useful concept.

No. This patch does not prevent opening an IBS event in freq mode. User
can still open freq mode IBS event with usual interface attr->freq=1 and
attr->sample_freq=<freq>. i.e. 'perf top -F 1000' with IBS event will
work fine with this patch.

Thanks,
Ravi

* Ravi Bangoria <ravi.bangoria@amd.com> wrote:

> Hi Ingo,
> 
> >> 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.
> > 
> > Then let's fix those rate checks?
> > 
> > AFAICS this patch limits functionality because the IBS driver would 
> > have to be fixed/enhanced to support frequency based events?
> > 
> > I'd strongly favor fixing/enhancing the driver instead, as 'perf top -F 
> > 1000' is easy to use and it is a useful concept.
> 
> No. This patch does not prevent opening an IBS event in freq mode. User
> can still open freq mode IBS event with usual interface attr->freq=1 and
> attr->sample_freq=<freq>. i.e. 'perf top -F 1000' with IBS event will
> work fine with this patch.

Oh, I see - the regular tooling uses perf_event_attr->sample_period, right?

Never mind then.

Thanks,

	Ingo

>>>> 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.
>>>
>>> Then let's fix those rate checks?
>>>
>>> AFAICS this patch limits functionality because the IBS driver would 
>>> have to be fixed/enhanced to support frequency based events?
>>>
>>> I'd strongly favor fixing/enhancing the driver instead, as 'perf top -F 
>>> 1000' is easy to use and it is a useful concept.
>>
>> No. This patch does not prevent opening an IBS event in freq mode. User
>> can still open freq mode IBS event with usual interface attr->freq=1 and
>> attr->sample_freq=<freq>. i.e. 'perf top -F 1000' with IBS event will
>> work fine with this patch.
> 
> Oh, I see - the regular tooling uses perf_event_attr->sample_period, right?

Correct.

> Never mind then.

No worries. Thanks for the review :)

Thanks,
Ravi