public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] complete perf_allow_* trio and use in drm/xe
@ 2026-05-23  1:33 John Hubbard
  2026-05-23  1:33 ` [PATCH v2 1/2] perf/core: out-of-line and export perf_allow_cpu/tracepoint() John Hubbard
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: John Hubbard @ 2026-05-23  1:33 UTC (permalink / raw)
  To: Matthew Brost, Thomas Hellström, Rodrigo Vivi, David Airlie,
	Simona Vetter, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Namhyung Kim
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, James Clark, intel-xe, dri-devel, linux-perf-users,
	LKML, John Hubbard

perf_allow_cpu() and perf_allow_tracepoint() are static inline and
reach into a non-exported sysctl, so modular drivers that want the
same permission model as system-wide perf end up writing partial
copies of this code. Let's instead export these properly so that modules
can call them.

Commit 5e9629d0ae97 ("drivers/perf: arm_spe: Use perf_allow_kernel()
for permissions") already moved perf_allow_kernel() out of line and
exported it. Patch 1 does the same for the other two, and provides
!CONFIG_PERF_EVENTS stubs so the helpers stay callable when perf is
compiled out.

Patch 2 converts drm/xe's OA and EU stall paths to call
perf_allow_cpu(), so xe observation now respects the system
perf_event_paranoid policy and consults the LSM hook. Sites that have
already configured an LSM perf policy or tuned the paranoid sysctl will
now see those settings honored on xe as well.

Changes since v1:

* Patch 1: add !CONFIG_PERF_EVENTS stubs that fall back to
  perfmon_capable() so the helpers remain callable when perf is
  compiled out. The sashiko-bot AI review caught that the v1 code
  would otherwise fail to build with PERF_EVENTS=n.

John Hubbard (2):
  perf/core: out-of-line and export perf_allow_cpu/tracepoint()
  drm/xe: gate observation streams with perf_allow_cpu()

 drivers/gpu/drm/xe/xe_eu_stall.c    |  5 +++--
 drivers/gpu/drm/xe/xe_oa.c          | 25 +++++++++++++---------
 drivers/gpu/drm/xe/xe_observation.c | 32 ++++++++++++++++++++++++-----
 drivers/gpu/drm/xe/xe_observation.h |  3 +--
 include/linux/perf_event.h          | 31 ++++++++++++++--------------
 kernel/events/core.c                | 18 ++++++++++++++++
 6 files changed, 79 insertions(+), 35 deletions(-)


base-commit: 79bd2dded182b1d458b18e62684b7f82ffc682e5
-- 
2.54.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] perf/core: out-of-line and export perf_allow_cpu/tracepoint()
  2026-05-23  1:33 [PATCH v2 0/2] complete perf_allow_* trio and use in drm/xe John Hubbard
@ 2026-05-23  1:33 ` John Hubbard
  2026-05-25  8:04   ` Claude review: " Claude Code Review Bot
  2026-05-23  1:33 ` [PATCH v2 2/2] drm/xe: gate observation streams with perf_allow_cpu() John Hubbard
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: John Hubbard @ 2026-05-23  1:33 UTC (permalink / raw)
  To: Matthew Brost, Thomas Hellström, Rodrigo Vivi, David Airlie,
	Simona Vetter, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Namhyung Kim
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, James Clark, intel-xe, dri-devel, linux-perf-users,
	LKML, John Hubbard

These helpers are static inline in <linux/perf_event.h> and reach
into sysctl_perf_event_paranoid and security_perf_event_open(),
neither of which is itself exported. The perf_allow_* trio is
therefore asymmetric: built-in callers can use any of the three, but
modular code can only call perf_allow_kernel().

Move both bodies into kernel/events/core.c next to perf_allow_kernel()
and export them with EXPORT_SYMBOL_GPL, following the shape of
commit 5e9629d0ae97 ("drivers/perf: arm_spe: Use perf_allow_kernel()
for permissions"). Existing in-tree callers live in built-in arch and
tracing code, so the change is invisible to them.

Provide !CONFIG_PERF_EVENTS stubs that fall back to perfmon_capable(),
so the helpers stay callable when perf is compiled out.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 include/linux/perf_event.h | 31 +++++++++++++++----------------
 kernel/events/core.c       | 18 ++++++++++++++++++
 2 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 48d851fbd8ea..5842552294c1 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1791,22 +1791,8 @@ static inline int perf_is_paranoid(void)
 }
 
 extern int perf_allow_kernel(void);
-
-static inline int perf_allow_cpu(void)
-{
-	if (sysctl_perf_event_paranoid > 0 && !perfmon_capable())
-		return -EACCES;
-
-	return security_perf_event_open(PERF_SECURITY_CPU);
-}
-
-static inline int perf_allow_tracepoint(void)
-{
-	if (sysctl_perf_event_paranoid > -1 && !perfmon_capable())
-		return -EPERM;
-
-	return security_perf_event_open(PERF_SECURITY_TRACEPOINT);
-}
+extern int perf_allow_cpu(void);
+extern int perf_allow_tracepoint(void);
 
 extern int perf_exclude_event(struct perf_event *event, struct pt_regs *regs);
 
@@ -2023,6 +2009,19 @@ perf_event_pause(struct perf_event *event, bool reset)			{ return 0; }
 static inline int
 perf_exclude_event(struct perf_event *event, struct pt_regs *regs)	{ return 0; }
 
+static inline int perf_allow_kernel(void)
+{
+	return perfmon_capable() ? 0 : -EACCES;
+}
+static inline int perf_allow_cpu(void)
+{
+	return perfmon_capable() ? 0 : -EACCES;
+}
+static inline int perf_allow_tracepoint(void)
+{
+	return perfmon_capable() ? 0 : -EPERM;
+}
+
 #endif /* !CONFIG_PERF_EVENTS */
 
 #if defined(CONFIG_PERF_EVENTS) && defined(CONFIG_CPU_SUP_INTEL)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7935d5663944..cb13f3ad11a3 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -14731,6 +14731,24 @@ int perf_allow_kernel(void)
 }
 EXPORT_SYMBOL_GPL(perf_allow_kernel);
 
+int perf_allow_cpu(void)
+{
+	if (sysctl_perf_event_paranoid > 0 && !perfmon_capable())
+		return -EACCES;
+
+	return security_perf_event_open(PERF_SECURITY_CPU);
+}
+EXPORT_SYMBOL_GPL(perf_allow_cpu);
+
+int perf_allow_tracepoint(void)
+{
+	if (sysctl_perf_event_paranoid > -1 && !perfmon_capable())
+		return -EPERM;
+
+	return security_perf_event_open(PERF_SECURITY_TRACEPOINT);
+}
+EXPORT_SYMBOL_GPL(perf_allow_tracepoint);
+
 /*
  * Inherit an event from parent task to child task.
  *
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] drm/xe: gate observation streams with perf_allow_cpu()
  2026-05-23  1:33 [PATCH v2 0/2] complete perf_allow_* trio and use in drm/xe John Hubbard
  2026-05-23  1:33 ` [PATCH v2 1/2] perf/core: out-of-line and export perf_allow_cpu/tracepoint() John Hubbard
@ 2026-05-23  1:33 ` John Hubbard
  2026-05-25  8:04   ` Claude review: " Claude Code Review Bot
  2026-05-23  8:40 ` [PATCH v2 0/2] complete perf_allow_* trio and use in drm/xe Peter Zijlstra
  2026-05-25  8:04 ` Claude review: " Claude Code Review Bot
  3 siblings, 1 reply; 9+ messages in thread
From: John Hubbard @ 2026-05-23  1:33 UTC (permalink / raw)
  To: Matthew Brost, Thomas Hellström, Rodrigo Vivi, David Airlie,
	Simona Vetter, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Namhyung Kim
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, James Clark, intel-xe, dri-devel, linux-perf-users,
	LKML, John Hubbard

xe OA and EU-stall paths open-code a partial copy of the system-wide
perf CPU-event permission check:

    if (xe_observation_paranoid && !perfmon_capable())
            return -EACCES;

This open-coded check skips two things perf_allow_cpu() handles: the
graduated kernel.perf_event_paranoid policy that an administrator
may have tuned, and the security_perf_event_open() LSM hook.

Introduce xe_observation_paranoid_check() to wrap perf_allow_cpu(),
and convert the open-coded sites in xe_oa.c and xe_eu_stall.c. The
dev.xe.observation_paranoid sysctl still acts as an escape hatch
when cleared.

xe observation now consults kernel.perf_event_paranoid and the LSM
perf hook on every open. Sites that have already configured an LSM
perf policy or tuned the paranoid sysctl will see those settings
extend to xe.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/gpu/drm/xe/xe_eu_stall.c    |  5 +++--
 drivers/gpu/drm/xe/xe_oa.c          | 25 +++++++++++++---------
 drivers/gpu/drm/xe/xe_observation.c | 32 ++++++++++++++++++++++++-----
 drivers/gpu/drm/xe/xe_observation.h |  3 +--
 4 files changed, 46 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_eu_stall.c b/drivers/gpu/drm/xe/xe_eu_stall.c
index dddcdd0bb7a3..ede8e3c98b2b 100644
--- a/drivers/gpu/drm/xe/xe_eu_stall.c
+++ b/drivers/gpu/drm/xe/xe_eu_stall.c
@@ -963,9 +963,10 @@ int xe_eu_stall_stream_open(struct drm_device *dev, u64 data, struct drm_file *f
 		return -ENODEV;
 	}
 
-	if (xe_observation_paranoid && !perfmon_capable()) {
+	ret = xe_observation_paranoid_check();
+	if (ret) {
 		drm_dbg(&xe->drm,  "Insufficient privileges for EU stall monitoring\n");
-		return -EACCES;
+		return ret;
 	}
 
 	/* Initialize and set default values */
diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
index d908f4e03906..f3dcff66b336 100644
--- a/drivers/gpu/drm/xe/xe_oa.c
+++ b/drivers/gpu/drm/xe/xe_oa.c
@@ -1676,9 +1676,10 @@ static int xe_oa_mmap(struct file *file, struct vm_area_struct *vma)
 	unsigned long start = vma->vm_start;
 	int i, ret;
 
-	if (xe_observation_paranoid && !perfmon_capable()) {
+	ret = xe_observation_paranoid_check();
+	if (ret) {
 		drm_dbg(&stream->oa->xe->drm, "Insufficient privilege to map OA buffer\n");
-		return -EACCES;
+		return ret;
 	}
 
 	/* Can mmap the entire OA buffer or nothing (no partial OA buffer mmaps) */
@@ -2054,10 +2055,12 @@ int xe_oa_stream_open_ioctl(struct drm_device *dev, u64 data, struct drm_file *f
 		privileged_op = true;
 	}
 
-	if (privileged_op && xe_observation_paranoid && !perfmon_capable()) {
-		drm_dbg(&oa->xe->drm, "Insufficient privileges to open xe OA stream\n");
-		ret = -EACCES;
-		goto err_exec_q;
+	if (privileged_op) {
+		ret = xe_observation_paranoid_check();
+		if (ret) {
+			drm_dbg(&oa->xe->drm, "Insufficient privileges to open xe OA stream\n");
+			goto err_exec_q;
+		}
 	}
 
 	if (!param.exec_q && !param.sample) {
@@ -2336,9 +2339,10 @@ int xe_oa_add_config_ioctl(struct drm_device *dev, u64 data, struct drm_file *fi
 		return -ENODEV;
 	}
 
-	if (xe_observation_paranoid && !perfmon_capable()) {
+	err = xe_observation_paranoid_check();
+	if (err) {
 		drm_dbg(&oa->xe->drm, "Insufficient privileges to add xe OA config\n");
-		return -EACCES;
+		return err;
 	}
 
 	err = copy_from_user(&param, u64_to_user_ptr(data), sizeof(param));
@@ -2438,9 +2442,10 @@ int xe_oa_remove_config_ioctl(struct drm_device *dev, u64 data, struct drm_file
 		return -ENODEV;
 	}
 
-	if (xe_observation_paranoid && !perfmon_capable()) {
+	ret = xe_observation_paranoid_check();
+	if (ret) {
 		drm_dbg(&oa->xe->drm, "Insufficient privileges to remove xe OA config\n");
-		return -EACCES;
+		return ret;
 	}
 
 	ret = get_user(arg, ptr);
diff --git a/drivers/gpu/drm/xe/xe_observation.c b/drivers/gpu/drm/xe/xe_observation.c
index e3f9b546207e..39e05b9131a7 100644
--- a/drivers/gpu/drm/xe/xe_observation.c
+++ b/drivers/gpu/drm/xe/xe_observation.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/errno.h>
+#include <linux/perf_event.h>
 #include <linux/sysctl.h>
 
 #include <uapi/drm/xe_drm.h>
@@ -12,9 +13,28 @@
 #include "xe_oa.h"
 #include "xe_observation.h"
 
-u32 xe_observation_paranoid = true;
+static u32 xe_observation_paranoid = true;
 static struct ctl_table_header *sysctl_header;
 
+/**
+ * xe_observation_paranoid_check - Gate access to xe observation streams.
+ *
+ * When the xe-specific observation_paranoid sysctl is enabled (the
+ * default), defer to perf_allow_cpu() so that access is governed by the
+ * same policy as system-wide perf CPU events: kernel.perf_event_paranoid
+ * plus the security_perf_event_open() LSM hook. When the sysctl has been
+ * cleared by a privileged user, observation is open to all callers.
+ *
+ * Return: 0 if access is permitted, a negative errno otherwise.
+ */
+int xe_observation_paranoid_check(void)
+{
+	if (!xe_observation_paranoid)
+		return 0;
+
+	return perf_allow_cpu();
+}
+
 static int xe_oa_ioctl(struct drm_device *dev, struct drm_xe_observation_param *arg,
 		       struct drm_file *file)
 {
@@ -83,11 +103,13 @@ static const struct ctl_table observation_ctl_table[] = {
 };
 
 /**
- * xe_observation_sysctl_register - Register xe_observation_paranoid sysctl
+ * xe_observation_sysctl_register - Register the observation_paranoid sysctl
  *
- * Normally only superuser/root can access observation stream
- * data. However, superuser can set xe_observation_paranoid sysctl to 0 to
- * allow non-privileged users to also access observation data.
+ * When dev.xe.observation_paranoid is set (the default), access to
+ * observation streams follows the system-wide perf_allow_cpu() policy:
+ * kernel.perf_event_paranoid plus the security_perf_event_open() LSM
+ * hook. A privileged user can clear the sysctl to bypass that gate and
+ * allow unprivileged access to observation data.
  *
  * Return: always returns 0
  */
diff --git a/drivers/gpu/drm/xe/xe_observation.h b/drivers/gpu/drm/xe/xe_observation.h
index 17816998e966..73a03e03c96a 100644
--- a/drivers/gpu/drm/xe/xe_observation.h
+++ b/drivers/gpu/drm/xe/xe_observation.h
@@ -11,8 +11,7 @@
 struct drm_device;
 struct drm_file;
 
-extern u32 xe_observation_paranoid;
-
+int xe_observation_paranoid_check(void);
 int xe_observation_ioctl(struct drm_device *dev, void *data, struct drm_file *file);
 int xe_observation_sysctl_register(void);
 void xe_observation_sysctl_unregister(void);
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/2] complete perf_allow_* trio and use in drm/xe
  2026-05-23  1:33 [PATCH v2 0/2] complete perf_allow_* trio and use in drm/xe John Hubbard
  2026-05-23  1:33 ` [PATCH v2 1/2] perf/core: out-of-line and export perf_allow_cpu/tracepoint() John Hubbard
  2026-05-23  1:33 ` [PATCH v2 2/2] drm/xe: gate observation streams with perf_allow_cpu() John Hubbard
@ 2026-05-23  8:40 ` Peter Zijlstra
  2026-05-23 17:52   ` John Hubbard
  2026-05-25  8:04 ` Claude review: " Claude Code Review Bot
  3 siblings, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2026-05-23  8:40 UTC (permalink / raw)
  To: John Hubbard
  Cc: Matthew Brost, Thomas Hellström, Rodrigo Vivi, David Airlie,
	Simona Vetter, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, James Clark, intel-xe, dri-devel,
	linux-perf-users, LKML

On Fri, May 22, 2026 at 06:33:24PM -0700, John Hubbard wrote:
> perf_allow_cpu() and perf_allow_tracepoint() are static inline and
> reach into a non-exported sysctl, so modular drivers that want the
> same permission model as system-wide perf end up writing partial
> copies of this code. Let's instead export these properly so that modules
> can call them.
> 
> Commit 5e9629d0ae97 ("drivers/perf: arm_spe: Use perf_allow_kernel()
> for permissions") already moved perf_allow_kernel() out of line and
> exported it. Patch 1 does the same for the other two, and provides
> !CONFIG_PERF_EVENTS stubs so the helpers stay callable when perf is
> compiled out.
> 
> Patch 2 converts drm/xe's OA and EU stall paths to call
> perf_allow_cpu(), so xe observation now respects the system
> perf_event_paranoid policy and consults the LSM hook. Sites that have
> already configured an LSM perf policy or tuned the paranoid sysctl will
> now see those settings honored on xe as well.
> 
> Changes since v1:
> 
> * Patch 1: add !CONFIG_PERF_EVENTS stubs that fall back to
>   perfmon_capable() so the helpers remain callable when perf is
>   compiled out. The sashiko-bot AI review caught that the v1 code
>   would otherwise fail to build with PERF_EVENTS=n.
> 
> John Hubbard (2):
>   perf/core: out-of-line and export perf_allow_cpu/tracepoint()
>   drm/xe: gate observation streams with perf_allow_cpu()
> 
>  drivers/gpu/drm/xe/xe_eu_stall.c    |  5 +++--
>  drivers/gpu/drm/xe/xe_oa.c          | 25 +++++++++++++---------
>  drivers/gpu/drm/xe/xe_observation.c | 32 ++++++++++++++++++++++++-----
>  drivers/gpu/drm/xe/xe_observation.h |  3 +--
>  include/linux/perf_event.h          | 31 ++++++++++++++--------------
>  kernel/events/core.c                | 18 ++++++++++++++++
>  6 files changed, 79 insertions(+), 35 deletions(-)

Sure, works for me. How do you want to route these things, Xe tree or
-tip?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/2] complete perf_allow_* trio and use in drm/xe
  2026-05-23  8:40 ` [PATCH v2 0/2] complete perf_allow_* trio and use in drm/xe Peter Zijlstra
@ 2026-05-23 17:52   ` John Hubbard
  0 siblings, 0 replies; 9+ messages in thread
From: John Hubbard @ 2026-05-23 17:52 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Matthew Brost, Thomas Hellström, Rodrigo Vivi, David Airlie,
	Simona Vetter, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, James Clark, intel-xe, dri-devel,
	linux-perf-users, LKML

On 5/23/26 1:40 AM, Peter Zijlstra wrote:
> On Fri, May 22, 2026 at 06:33:24PM -0700, John Hubbard wrote:
...
>>    perf/core: out-of-line and export perf_allow_cpu/tracepoint()
>>    drm/xe: gate observation streams with perf_allow_cpu()
>>
>>   drivers/gpu/drm/xe/xe_eu_stall.c    |  5 +++--
>>   drivers/gpu/drm/xe/xe_oa.c          | 25 +++++++++++++---------
>>   drivers/gpu/drm/xe/xe_observation.c | 32 ++++++++++++++++++++++++-----
>>   drivers/gpu/drm/xe/xe_observation.h |  3 +--
>>   include/linux/perf_event.h          | 31 ++++++++++++++--------------
>>   kernel/events/core.c                | 18 ++++++++++++++++
>>   6 files changed, 79 insertions(+), 35 deletions(-)
> 
> Sure, works for me. How do you want to route these things, Xe tree or
> -tip?

Hi Peter,

Maybe -tip, but really, either way is perfectly OK with me.

thanks,
-- 
John Hubbard


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Claude review: complete perf_allow_* trio and use in drm/xe
  2026-05-23  1:33 [PATCH v2 0/2] complete perf_allow_* trio and use in drm/xe John Hubbard
                   ` (2 preceding siblings ...)
  2026-05-23  8:40 ` [PATCH v2 0/2] complete perf_allow_* trio and use in drm/xe Peter Zijlstra
@ 2026-05-25  8:04 ` Claude Code Review Bot
  3 siblings, 0 replies; 9+ messages in thread
From: Claude Code Review Bot @ 2026-05-25  8:04 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: complete perf_allow_* trio and use in drm/xe
Author: John Hubbard <jhubbard@nvidia.com>
Patches: 5
Reviewed: 2026-05-25T18:04:51.127105

---

This is a clean, well-motivated 2-patch series that completes the `perf_allow_*` API for modular callers and then uses it in drm/xe to replace open-coded permission checks. The series follows the precedent set by commit 5e9629d0ae97 for `perf_allow_kernel()` and extends it consistently to the remaining two helpers.

The code is correct and the structure is sound. The main behavioral change to note is that xe observation streams will now additionally consult the `security_perf_event_open()` LSM hook and respect the graduated `kernel.perf_event_paranoid` policy rather than doing a simple `perfmon_capable()` check. This is a deliberate improvement and is well-documented in the commit messages.

One subtle point worth calling out: when `kernel.perf_event_paranoid` is set to 0 (not the default, which is 2), `perf_allow_cpu()` will skip the capability check entirely and only consult the LSM hook. This means xe observation could become *less* restrictive than before in that specific admin-configured scenario. This is consistent with the stated goal of aligning with the system perf policy, but the series might benefit from a note in the commit message about this edge case for anyone bisecting a permission change.

No blocking issues. Reviewed-by-worthy with minor observations below.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Claude review: perf/core: out-of-line and export perf_allow_cpu/tracepoint()
  2026-05-23  1:33 ` [PATCH v2 1/2] perf/core: out-of-line and export perf_allow_cpu/tracepoint() John Hubbard
@ 2026-05-25  8:04   ` Claude Code Review Bot
  0 siblings, 0 replies; 9+ messages in thread
From: Claude Code Review Bot @ 2026-05-25  8:04 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Good:**

The out-of-line implementations are exact copies of the previous static inline bodies, preserving the distinct threshold levels and error codes:

```c
int perf_allow_cpu(void)
{
	if (sysctl_perf_event_paranoid > 0 && !perfmon_capable())
		return -EACCES;
	return security_perf_event_open(PERF_SECURITY_CPU);
}

int perf_allow_tracepoint(void)
{
	if (sysctl_perf_event_paranoid > -1 && !perfmon_capable())
		return -EPERM;
	return security_perf_event_open(PERF_SECURITY_TRACEPOINT);
}
```

The graduated thresholds (`> 0` for CPU, `> -1` for tracepoint, `> 1` for kernel in the existing code) and distinct error codes (`-EACCES` vs `-EPERM`) are all faithfully preserved.

**Good: !CONFIG_PERF_EVENTS stubs are complete.** The patch adds stubs for all three helpers:

```c
static inline int perf_allow_kernel(void)
{
	return perfmon_capable() ? 0 : -EACCES;
}
static inline int perf_allow_cpu(void)
{
	return perfmon_capable() ? 0 : -EACCES;
}
static inline int perf_allow_tracepoint(void)
{
	return perfmon_capable() ? 0 : -EPERM;
}
```

Note that `perf_allow_kernel()` didn't have a `!CONFIG_PERF_EVENTS` stub before this patch either -- the existing code declared it `extern` in the `CONFIG_PERF_EVENTS` block with no fallback. Adding it here is a bonus fix that makes the trio complete. The cover letter v2 changelog correctly attributes this to AI review feedback catching the build failure with `PERF_EVENTS=n`.

**Minor observation:** The stubs use `perfmon_capable()` as the fallback, which is a reasonable conservative default -- it requires `CAP_PERFMON` or `CAP_SYS_ADMIN`. The LSM hook is naturally unavailable without `CONFIG_PERF_EVENTS`, so this is the best approximation.

**Placement:** The new functions are placed in `kernel/events/core.c` right after the existing `perf_allow_kernel()` / `EXPORT_SYMBOL_GPL`, which is the natural location. No issues.

No problems with this patch.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Claude review: drm/xe: gate observation streams with perf_allow_cpu()
  2026-05-23  1:33 ` [PATCH v2 2/2] drm/xe: gate observation streams with perf_allow_cpu() John Hubbard
@ 2026-05-25  8:04   ` Claude Code Review Bot
  0 siblings, 0 replies; 9+ messages in thread
From: Claude Code Review Bot @ 2026-05-25  8:04 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Good design:** The introduction of `xe_observation_paranoid_check()` as a single gating function is clean:

```c
int xe_observation_paranoid_check(void)
{
	if (!xe_observation_paranoid)
		return 0;

	return perf_allow_cpu();
}
```

The `xe_observation_paranoid` sysctl remains as an escape hatch (when cleared, bypass all checks), but when enabled it delegates to the proper perf infrastructure instead of open-coding a partial check.

**Good: variable made static.** Since `xe_observation_paranoid` is now only accessed inside `xe_observation.c`, making it `static` and removing the `extern` from the header is the right cleanup:

```c
-u32 xe_observation_paranoid = true;
+static u32 xe_observation_paranoid = true;
```

**All call sites converted consistently.** The five open-coded sites (1 in `xe_eu_stall.c`, 4 in `xe_oa.c`) are all converted to `xe_observation_paranoid_check()`, and the return value is properly propagated instead of always using `-EACCES`. This is correct because `perf_allow_cpu()` itself returns `-EACCES` on the capability check, so the common case is unchanged, but the LSM hook could return a different error code.

**Behavioral change to note:** The `xe_oa_mmap()` path previously checked `xe_observation_paranoid && !perfmon_capable()` at mmap time. Now it calls `perf_allow_cpu()` which also invokes the LSM hook. This is appropriate -- the mmap exposes the same sensitive observation data, so gating it with the same policy is consistent. However, it's worth noting that a process that successfully opened an OA stream might then fail the mmap if the LSM policy changed between open and mmap. This is a theoretical TOCTOU edge case that also exists in the current perf infrastructure and isn't a concern in practice.

**Error code subtlety in xe_oa_stream_open_ioctl:** The restructuring of the privileged_op check is clean:

```c
-	if (privileged_op && xe_observation_paranoid && !perfmon_capable()) {
-		...
-		ret = -EACCES;
-		goto err_exec_q;
+	if (privileged_op) {
+		ret = xe_observation_paranoid_check();
+		if (ret) {
+			...
+			goto err_exec_q;
+		}
 	}
```

This correctly separates the "is this a privileged operation" decision from the "does the caller have permission" decision.

**Doc comment:** The function-level comment on `xe_observation_paranoid_check()` is thorough and accurately describes the behavior. It's on the verbose side for kernel style but not objectionable.

No blocking issues with this patch.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Claude review: drm/xe: gate observation streams with perf_allow_cpu()
  2026-05-21  2:49 ` [PATCH 2/2] drm/xe: gate observation streams with perf_allow_cpu() John Hubbard
@ 2026-05-25 11:03   ` Claude Code Review Bot
  0 siblings, 0 replies; 9+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 11:03 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Design: Good.** The new `xe_observation_paranoid_check()` wrapper is well-designed:

```c
int xe_observation_paranoid_check(void)
{
	if (!xe_observation_paranoid)
		return 0;

	return perf_allow_cpu();
}
```

When the xe-specific sysctl is cleared, observation is open to all — preserving the existing escape hatch. When set (the default), it defers to the system-wide perf policy.

**Making `xe_observation_paranoid` static:** Good call. Since all access is now through `xe_observation_paranoid_check()`, there's no need to export the variable. The sysctl table in the same file still references it directly, which is fine.

**Call-site conversions:** All five sites are correctly converted:
- `xe_eu_stall.c`: 1 site (line 976)
- `xe_oa.c`: 4 sites (mmap, stream open, add config, remove config)

Each now propagates the actual error code from `perf_allow_cpu()` rather than hardcoding `-EACCES`, which is correct behavior.

**Behavioral change — worth documenting to reviewers:**

The old check:
```c
if (xe_observation_paranoid && !perfmon_capable())
    return -EACCES;
```

The new check via `perf_allow_cpu()`:
```c
if (sysctl_perf_event_paranoid > 0 && !perfmon_capable())
    return -EACCES;
return security_perf_event_open(PERF_SECURITY_CPU);
```

This means:
1. If an admin sets `kernel.perf_event_paranoid=0` (or `-1`), non-root users gain xe observation access they previously didn't have (previously, only `perfmon_capable()` was checked). The commit message acknowledges this as the intent.
2. Conversely, LSM policies restricting `PERF_SECURITY_CPU` will now also block xe observation — a tightening that seems correct.

**Potential build issue — missing `CONFIG_PERF_EVENTS` dependency:** The current xe observation code uses `perfmon_capable()` from `<linux/capability.h>`, which is available regardless of `CONFIG_PERF_EVENTS`. After this patch, `xe_observation.c` calls `perf_allow_cpu()`, which is only declared inside `#ifdef CONFIG_PERF_EVENTS` in `<linux/perf_event.h>` and only defined in `kernel/events/core.c` (guarded by the same config). There is no stub in the `#else` block. If someone builds a kernel with `CONFIG_DRM_XE=y` but `CONFIG_PERF_EVENTS=n` (unusual but possible), this will fail to link. Consider adding `depends on PERF_EVENTS` or `select PERF_EVENTS` to `drivers/gpu/drm/xe/Kconfig`, or adding a `#ifndef CONFIG_PERF_EVENTS` fallback that returns `-EACCES` unconditionally.

**Documentation:** The kerneldoc for `xe_observation_paranoid_check()` and the updated sysctl_register doc are clear and accurate.

**Minor nit:** The `xe_oa_stream_open_ioctl` conversion restructures the conditional nicely by pulling `privileged_op` out of the combined `if`:

```c
if (privileged_op) {
    ret = xe_observation_paranoid_check();
    if (ret) {
        ...
        goto err_exec_q;
    }
}
```

This is cleaner than the original three-way `&&`. Good.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-05-25 11:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-23  1:33 [PATCH v2 0/2] complete perf_allow_* trio and use in drm/xe John Hubbard
2026-05-23  1:33 ` [PATCH v2 1/2] perf/core: out-of-line and export perf_allow_cpu/tracepoint() John Hubbard
2026-05-25  8:04   ` Claude review: " Claude Code Review Bot
2026-05-23  1:33 ` [PATCH v2 2/2] drm/xe: gate observation streams with perf_allow_cpu() John Hubbard
2026-05-25  8:04   ` Claude review: " Claude Code Review Bot
2026-05-23  8:40 ` [PATCH v2 0/2] complete perf_allow_* trio and use in drm/xe Peter Zijlstra
2026-05-23 17:52   ` John Hubbard
2026-05-25  8:04 ` Claude review: " Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-05-21  2:49 [PATCH 0/2] perf: " John Hubbard
2026-05-21  2:49 ` [PATCH 2/2] drm/xe: gate observation streams with perf_allow_cpu() John Hubbard
2026-05-25 11:03   ` Claude review: " Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox