[PATCH 0/3] perf/amd/ibs: Assorted fixes

[PATCH 0/3] perf/amd/ibs: Assorted fixes

[Ravi Bangoria]

#1 Set IBS MSRs in syscore->resume() callback
#2 Fix kernel address leaking through REGS_USER
#3 Fix races by reducing SW states when IBS_CAPS_DIS is present

Ravi Bangoria (3):
  perf/amd/ibs: Reset CPU0 IBS state on syscore resume
  perf/amd/ibs: Fix kernel address leak via REGS_USER
  perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS

 arch/x86/events/amd/ibs.c | 156 +++++++++++++++++++++++++++++++-------
 1 file changed, 129 insertions(+), 27 deletions(-)

-- 
2.55.0

[PATCH 1/3] perf/amd/ibs: Reset CPU0 IBS state on syscore resume

[Ravi Bangoria]

During a suspend/resume cycle, all CPUs except CPU0 go through hotplug
callbacks, which sets up IBS state. CPU0 bypasses this path, leaving its
IBS state stale after resume. Fix this by explicitly setting CPU0's IBS
state in the syscore ->resume() callback.

Fixes: efa5700ec0da ("perf/amd/ibs: Support IBS_{FETCH|OP}_CTL2[Dis] to eliminate RMW race")
Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/ibs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 3531f9c23b8c..bfb51f8e184f 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -1948,7 +1948,7 @@ static int perf_ibs_suspend(void *data)
 static void perf_ibs_resume(void *data)
 {
 	ibs_eilvt_setup();
-	setup_APIC_ibs();
+	x86_pmu_amd_ibs_starting_cpu(smp_processor_id());
 }
 
 static const struct syscore_ops perf_ibs_syscore_ops = {
-- 
2.55.0

Re: [PATCH 1/3] perf/amd/ibs: Reset CPU0 IBS state on syscore resume

[sashiko-bot]

> During a suspend/resume cycle, all CPUs except CPU0 go through hotplug
> callbacks, which sets up IBS state. CPU0 bypasses this path, leaving its
> IBS state stale after resume. Fix this by explicitly setting CPU0's IBS
> state in the syscore ->resume() callback.
> 
> Fixes: efa5700ec0da ("perf/amd/ibs: Support IBS_{FETCH|OP}_CTL2[Dis] to eliminate RMW race")
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015060430.5877-1-ravi.bangoria@amd.com?part=1

[PATCH 2/3] perf/amd/ibs: Fix kernel address leak via REGS_USER

[Ravi Bangoria]

IBS hardware captures a sample and raises an NMI. Since the NMI can be
delivered with a delay, a privilege change may occur between sample
capture and NMI delivery. Because IBS only provides the sample RIP, the
kernel records GPRs from the NMI pt_regs, which may reflect kernel mode
if a privilege switch occurred in the interim. This can leak kernel
addresses to unprivileged users through PERF_SAMPLE_REGS_USER.

Fix this by discarding samples where the NMI arrives in kernel mode and
the user has set exclude_kernel.

Reported-by: Jayapal Ashwin Nair <janair@amd.com>
Fixes: d29e744c7167 ("perf/x86: Relax privilege filter restriction on AMD IBS")
Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/ibs.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index bfb51f8e184f..3cb46c775eaa 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -1373,12 +1373,15 @@ static bool perf_ibs_is_kernel_br_target(struct perf_event *event,
 }
 
 static bool perf_ibs_discard_sample(struct perf_ibs *perf_ibs, struct perf_event *event,
-				    struct pt_regs *regs, struct perf_ibs_data *ibs_data,
-				    int br_target_idx)
+				    struct pt_regs *regs, struct pt_regs *iregs,
+				    struct perf_ibs_data *ibs_data, int br_target_idx)
 {
 	if (perf_exclude_event(event, regs))
 		return true;
 
+	if (perf_exclude_event(event, iregs))
+		return true;
+
 	if (perf_ibs != &perf_ibs_op || !event->attr.exclude_kernel)
 		return false;
 
@@ -1529,7 +1532,7 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
 
 	if (((ibs_caps & IBS_CAPS_BIT63_FILTER) ||
 	     (event->attr.config2 & IBS_SW_FILTER_MASK)) &&
-	    perf_ibs_discard_sample(perf_ibs, event, &regs, &ibs_data, br_target_idx)) {
+	    perf_ibs_discard_sample(perf_ibs, event, &regs, iregs, &ibs_data, br_target_idx)) {
 		throttle = perf_event_account_interrupt(event);
 		goto out;
 	}
-- 
2.55.0

Re: [PATCH 2/3] perf/amd/ibs: Fix kernel address leak via REGS_USER

[sashiko-bot]

> IBS hardware captures a sample and raises an NMI. Since the NMI can be
> delivered with a delay, a privilege change may occur between sample
> capture and NMI delivery. Because IBS only provides the sample RIP, the
> kernel records GPRs from the NMI pt_regs, which may reflect kernel mode
> if a privilege switch occurred in the interim. This can leak kernel
> addresses to unprivileged users through PERF_SAMPLE_REGS_USER.
> 
> Fix this by discarding samples where the NMI arrives in kernel mode and
> the user has set exclude_kernel.
> 
> Reported-by: Jayapal Ashwin Nair <janair@amd.com>
> Fixes: d29e744c7167 ("perf/x86: Relax privilege filter restriction on AMD IBS")
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015060430.5877-1-ravi.bangoria@amd.com?part=2

[PATCH 3/3] perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS

[Ravi Bangoria]

The IBS_{FETCH|OP}_CTL2[Dis] bit allows IBS sampling to be stopped without
a RMW of IBS_{FETCH,OP}_CTL. Systems providing this bit therefore do not
require the IBS_STARTED and IBS_STOPPED states used to coordinate control
MSR updates with late NMIs.

Add a separate stop path for these systems. Preserve Val while disabling
the event so that a delayed NMI can be identified and consumed, and wait
briefly for such an NMI before restarting the event.

Leave the existing handling unchanged on systems without IBS_CAPS_DIS.

Reported-by: Jim Mattson <jmattson@google.com>
Closes: https://lore.kernel.org/r/20261007234815.4026028-1-jmattson@google.com
Fixes: efa5700ec0da ("perf/amd/ibs: Support IBS_{FETCH|OP}_CTL2[Dis] to eliminate RMW race")
Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/ibs.c | 145 ++++++++++++++++++++++++++++++++------
 1 file changed, 122 insertions(+), 23 deletions(-)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 3cb46c775eaa..f49f30f53399 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -28,6 +28,7 @@ static u32 ibs_caps;
 #include <linux/hardirq.h>
 
 #include <asm/nmi.h>
+#include <asm/delay.h>
 #include <asm/amd/ibs.h>
 
 /* attr.config2 */
@@ -73,7 +74,12 @@ static u32 ibs_caps;
  *
  * XXX: we could probably be using !atomic bitops for all this.
  */
-
+/*
+ * NOTE: IBS_CAPS_DIS eliminates the RMW race on the IBS_{FETCH|OP}_CTL, making
+ * IBS_STARTED and IBS_STOPPED states unnecessary. We use only IBS_ENABLED and
+ * IBS_STOPPING states when IBS_CAPS_DIS is present. Semantics of IBS_STOPPING
+ * changes a bit as well.
+ */
 enum ibs_states {
 	IBS_ENABLED	= 0,
 	IBS_STARTED	= 1,
@@ -557,7 +563,9 @@ static void perf_ibs_start(struct perf_event *event, int flags)
 	struct hw_perf_event *hwc = &event->hw;
 	struct perf_ibs *perf_ibs = container_of(event->pmu, struct perf_ibs, pmu);
 	struct cpu_perf_ibs *pcpu = this_cpu_ptr(perf_ibs->pcpu);
+	unsigned int delay = 50;
 	u64 period, config = 0;
+	u64 ctl;
 
 	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
 		return;
@@ -575,23 +583,87 @@ static void perf_ibs_start(struct perf_event *event, int flags)
 	}
 	config |= period >> 4;
 
-	/*
-	 * Reset the IBS_{FETCH|OP}_CTL MSR before updating pcpu->state.
-	 * Doing so prevents a race condition in which an NMI due to other
-	 * source might accidentally activate the event before we enable
-	 * it ourselves.
-	 */
-	perf_ibs_disable_event(perf_ibs, hwc, 0);
+	if (ibs_caps & IBS_CAPS_DIS) {
+		/*
+		 *      IBS HW                     SW
+		 *
+		 *  o Capture sample       o ->stop()
+		 *  o Set VAL bit            CTL2[Dis] = 1
+		 *  o Raise NMI --.        o ...
+		 *                 \       o ->start()
+		 *                  \        wrmsr(IBS_{FETCH|OP}_CTL, new value);
+		 *                   \       (wrmsr clears the VAL bit)
+		 *                    `--> o Receives delayed NMI
+		 *                           (Unknown NMI since VAL bit is cleared)
+		 *
+		 * Handle this with induced delay.
+		 */
+		rdmsrq(hwc->config_base, ctl);
+		while ((ctl & perf_ibs->valid_mask) && delay--) {
+			udelay(1);
+			rdmsrq(hwc->config_base, ctl);
+		}
+	} else {
+		/*
+		 * Reset the IBS_{FETCH|OP}_CTL MSR before updating pcpu->state.
+		 * Doing so prevents a race condition in which an NMI due to
+		 * other source might accidentally activate the event before we
+		 * enable it ourselves.
+		 */
+		perf_ibs_disable_event(perf_ibs, hwc, 0);
+		/*
+		 * Set STARTED before enabling the hardware, such that a
+		 * subsequent NMI must observe it.
+		 */
+		set_bit(IBS_STARTED,    pcpu->state);
+		clear_bit(IBS_STOPPING, pcpu->state);
+	}
+	perf_ibs_enable_event(perf_ibs, hwc, config);
+
+	perf_event_update_userpage(event);
+}
+
+/* ->stop() for platforms with additional CTL2[Dis] bit. */
+static void perf_ibs_stop_dis(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct perf_ibs *perf_ibs = container_of(event->pmu, struct perf_ibs, pmu);
+	struct cpu_perf_ibs *pcpu = this_cpu_ptr(perf_ibs->pcpu);
+	u64 config;
+	u64 ctl2;
+
+	WARN_ON_ONCE(test_bit(IBS_STOPPING, pcpu->state));
+
+	set_bit(IBS_STOPPING, pcpu->state);
+
+	rdmsrq(hwc->extra_reg.reg, ctl2);
+	if (ctl2 & perf_ibs->disable_mask)
+		goto reset_state;
+
+	wrmsrq(hwc->extra_reg.reg, perf_ibs->disable_mask);
+
+	WARN_ON_ONCE(hwc->state & PERF_HES_STOPPED);
+	hwc->state |= PERF_HES_STOPPED;
+
+	if (hwc->state & PERF_HES_UPTODATE)
+		goto reset_state;
+
+	rdmsrq(hwc->config_base, config);
+
+	config &= ~perf_ibs->valid_mask;
 
 	/*
-	 * Set STARTED before enabling the hardware, such that a subsequent NMI
-	 * must observe it.
+	 * Clear the Val bit otherwise ->start() will unnecessarily
+	 * wait for 50us.
 	 */
-	set_bit(IBS_STARTED,    pcpu->state);
-	clear_bit(IBS_STOPPING, pcpu->state);
-	perf_ibs_enable_event(perf_ibs, hwc, config);
+	if (in_nmi())
+		wrmsrq(hwc->config_base, config);
 
-	perf_event_update_userpage(event);
+	perf_ibs_event_update(perf_ibs, event, &config);
+	hwc->state |= PERF_HES_UPTODATE;
+
+reset_state:
+	clear_bit(IBS_STOPPING, pcpu->state);
 }
 
 static void perf_ibs_stop(struct perf_event *event, int flags)
@@ -602,6 +674,9 @@ static void perf_ibs_stop(struct perf_event *event, int flags)
 	u64 config;
 	int stopping;
 
+	if (ibs_caps & IBS_CAPS_DIS)
+		return perf_ibs_stop_dis(event, flags);
+
 	if (test_and_set_bit(IBS_STOPPING, pcpu->state))
 		return;
 
@@ -1419,19 +1494,43 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
 	unsigned int msr;
 	u64 *buf, *config, period, new_config = 0;
 	int br_target_idx = -1;
+	u64 ctl2, ctl;
 
-	if (!test_bit(IBS_STARTED, pcpu->state)) {
-fail:
+	if (ibs_caps & IBS_CAPS_DIS) {
 		/*
-		 * Catch spurious interrupts after stopping IBS: After
-		 * disabling IBS there could be still incoming NMIs
-		 * with samples that even have the valid bit cleared.
-		 * Mark all this NMIs as handled.
+		 * ->stop() {
+		 *     set_bit(IBS_STOPPING);
+		 *                               <-- NMI (race 1)
+		 *     CTL2[Dis]=1;
+		 *     ...                       <-- NMI (race 2)
+		 * }
 		 */
-		if (test_and_clear_bit(IBS_STOPPED, pcpu->state))
-			return 1;
+		rdmsrq(perf_ibs->msr2, ctl2);
+		if (test_bit(IBS_STOPPING, pcpu->state) /* race 1 */ ||
+		    (ctl2 & perf_ibs->disable_mask) /* race 2 */) {
+			rdmsrq(perf_ibs->msr, ctl);
+			if (ctl & perf_ibs->valid_mask) {
+				ctl &= ~(perf_ibs->enable_mask |
+					 perf_ibs->valid_mask);
+				wrmsrq(perf_ibs->msr, ctl);
+				return 1;
+			}
+			return 0;
+		}
+	} else {
+		if (!test_bit(IBS_STARTED, pcpu->state)) {
+fail:
+			/*
+			 * Catch spurious interrupts after stopping IBS: After
+			 * disabling IBS there could be still incoming NMIs
+			 * with samples that even have the valid bit cleared.
+			 * Mark all this NMIs as handled.
+			 */
+			if (test_and_clear_bit(IBS_STOPPED, pcpu->state))
+				return 1;
 
-		return 0;
+			return 0;
+		}
 	}
 
 	if (WARN_ON_ONCE(!event))
-- 
2.55.0

Re: [PATCH 3/3] perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf/amd/ibs: Unhandled NMI panic due to 50us timeout
--

commit be4cf40ba3750348ffc77933b0cfe40c48e335e7
Author: Ravi Bangoria <ravi.bangoria@amd.com>
Subject: perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS

This commit simplifies event state handling on systems supporting
IBS_CAPS_DIS. It introduces a separate stop path that preserves the
Val bit while disabling the event to catch delayed NMIs.

Link: https://lore.kernel.org/r/20261007234815.4026028-1-jmattson@google.com

> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
> --- a/arch/x86/events/amd/ibs.c
> +++ b/arch/x86/events/amd/ibs.c

[ ... ]

> @@ -575,23 +583,87 @@ static void perf_ibs_start(struct perf_event *event, int flags)

[ ... ]

> +	if (ibs_caps & IBS_CAPS_DIS) {
> +		/*
> +		 *      IBS HW                     SW
> +		 *
> +		 *  o Capture sample       o ->stop()
> +		 *  o Set VAL bit            CTL2[Dis] = 1
> +		 *  o Raise NMI --.        o ...
> +		 *                 \       o ->start()
> +		 *                  \        wrmsr(IBS_{FETCH|OP}_CTL, new value);
> +		 *                   \       (wrmsr clears the VAL bit)
> +		 *                    `--> o Receives delayed NMI
> +		 *                           (Unknown NMI since VAL bit is cleared)
> +		 *
> +		 * Handle this with induced delay.
> +		 */
> +		rdmsrq(hwc->config_base, ctl);
> +		while ((ctl & perf_ibs->valid_mask) && delay--) {
> +			udelay(1);
> +			rdmsrq(hwc->config_base, ctl);
> +		}

[Severity: High]
What happens if the delayed NMI takes longer than 50us to arrive, for
example due to hypervisor scheduling, nested virtualization, or SMIs?

If this delay loop expires, the code proceeds to call
perf_ibs_enable_event() which clears the VAL and CTL2[Dis] bits. When the
delayed NMI eventually arrives, it appears it will be treated as an
unhandled NMI.

[ ... ]

> @@ -1419,19 +1494,43 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)

[ ... ]

> +	} else {
> +		if (!test_bit(IBS_STARTED, pcpu->state)) {
> +fail:
> +			/*
> +			 * Catch spurious interrupts after stopping IBS: After
> +			 * disabling IBS there could be still incoming NMIs
> +			 * with samples that even have the valid bit cleared.
> +			 * Mark all this NMIs as handled.
> +			 */
> +			if (test_and_clear_bit(IBS_STOPPED, pcpu->state))
> +				return 1;
>  
> -		return 0;
> +			return 0;
> +		}
> +	}

[Severity: High]
If the NMI is delayed past the 50us timeout in perf_ibs_start(), won't
perf_ibs_handle_irq() return 0 here?

Since IBS_STOPPED is intentionally not set for systems with IBS_CAPS_DIS,
when the delayed NMI arrives, CTL2[Dis] == 0 and VAL == 0. If the code
jumps to the 'fail' label due to the cleared VAL bit, IBS_STOPPED will be
clear, causing it to return 0.

Could this result in an unhandled NMI and a subsequent kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015060430.5877-1-ravi.bangoria@amd.com?part=3

Re: [PATCH 3/3] perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS

[Ravi Bangoria]

>> +     if (ibs_caps & IBS_CAPS_DIS) {
>> +             /*
>> +              *      IBS HW                     SW
>> +              *
>> +              *  o Capture sample       o ->stop()
>> +              *  o Set VAL bit            CTL2[Dis] = 1
>> +              *  o Raise NMI --.        o ...
>> +              *                 \       o ->start()
>> +              *                  \        wrmsr(IBS_{FETCH|OP}_CTL, new value);
>> +              *                   \       (wrmsr clears the VAL bit)
>> +              *                    `--> o Receives delayed NMI
>> +              *                           (Unknown NMI since VAL bit is cleared)
>> +              *
>> +              * Handle this with induced delay.
>> +              */
>> +             rdmsrq(hwc->config_base, ctl);
>> +             while ((ctl & perf_ibs->valid_mask) && delay--) {
>> +                     udelay(1);
>> +                     rdmsrq(hwc->config_base, ctl);
>> +             }
> 
> [Severity: High]
> What happens if the delayed NMI takes longer than 50us to arrive, for
> example due to hypervisor scheduling, nested virtualization, or SMIs?
> 
> If this delay loop expires, the code proceeds to call
> perf_ibs_enable_event() which clears the VAL and CTL2[Dis] bits. When the
> delayed NMI eventually arrives, it appears it will be treated as an
> unhandled NMI.

A delay exceeding 50us is highly unlikely in practice, and even if it
occurs, the consequence is "Unknown NMI", which is not fatal.

Thanks,
Ravi