* [PATCH linux-next 0/2] Fix perf security check problem @ 2024-12-23 7:06 Luo Gengkun 2024-12-23 7:06 ` [PATCH linux-next 1/2] perf: Remove unnecessary parameter of security check Luo Gengkun 2024-12-23 7:06 ` [PATCH linux-next 2/2] perf: Return EACCESS when need perfmon capability Luo Gengkun 0 siblings, 2 replies; 9+ messages in thread From: Luo Gengkun @ 2024-12-23 7:06 UTC (permalink / raw) To: peterz Cc: mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen, x86, hpa, will, paul, jmorris, serge, rostedt, mhiramat, mathieu.desnoyers, stephen.smalley.work, omosnace, linux-perf-users, linux-kernel, linux-arm-kernel, linux-security-module, linux-trace-kernel, selinux, luogengkun From: Luo Gengkun <luogengkun2@huawei.com> Luo Gengkun (2): perf: Remove unnecessary parameter of security check perf: Return EACCESS when need perfmon capability arch/x86/events/intel/bts.c | 2 +- arch/x86/events/intel/core.c | 2 +- arch/x86/events/intel/p4.c | 2 +- drivers/perf/arm_spe_pmu.c | 4 ++-- include/linux/lsm_hook_defs.h | 2 +- include/linux/perf_event.h | 12 ++++++------ include/linux/security.h | 5 ++--- kernel/events/core.c | 14 +++++++------- kernel/trace/trace_event_perf.c | 4 ++-- security/security.c | 5 ++--- security/selinux/hooks.c | 2 +- 11 files changed, 26 insertions(+), 28 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH linux-next 1/2] perf: Remove unnecessary parameter of security check 2024-12-23 7:06 [PATCH linux-next 0/2] Fix perf security check problem Luo Gengkun @ 2024-12-23 7:06 ` Luo Gengkun 2025-01-05 2:45 ` Paul Moore 2025-02-26 19:13 ` [PATCH " Paul Moore 2024-12-23 7:06 ` [PATCH linux-next 2/2] perf: Return EACCESS when need perfmon capability Luo Gengkun 1 sibling, 2 replies; 9+ messages in thread From: Luo Gengkun @ 2024-12-23 7:06 UTC (permalink / raw) To: peterz Cc: mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen, x86, hpa, will, paul, jmorris, serge, rostedt, mhiramat, mathieu.desnoyers, stephen.smalley.work, omosnace, linux-perf-users, linux-kernel, linux-arm-kernel, linux-security-module, linux-trace-kernel, selinux, luogengkun It seems that the attr parameter was never been used in security checks since it was first introduced by: commit da97e18458fb ("perf_event: Add support for LSM and SELinux checks") so remove it. Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com> --- arch/x86/events/intel/bts.c | 2 +- arch/x86/events/intel/core.c | 2 +- arch/x86/events/intel/p4.c | 2 +- drivers/perf/arm_spe_pmu.c | 4 ++-- include/linux/lsm_hook_defs.h | 2 +- include/linux/perf_event.h | 10 +++++----- include/linux/security.h | 5 ++--- kernel/events/core.c | 14 +++++++------- kernel/trace/trace_event_perf.c | 4 ++-- security/security.c | 5 ++--- security/selinux/hooks.c | 2 +- 11 files changed, 25 insertions(+), 27 deletions(-) diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c index 8f78b0c900ef..485a09d61adf 100644 --- a/arch/x86/events/intel/bts.c +++ b/arch/x86/events/intel/bts.c @@ -559,7 +559,7 @@ static int bts_event_init(struct perf_event *event) * to the user in a zero-copy fashion. */ if (event->attr.exclude_kernel) { - ret = perf_allow_kernel(&event->attr); + ret = perf_allow_kernel(); if (ret) return ret; } diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c index 103ee41907c7..3b65df32daee 100644 --- a/arch/x86/events/intel/core.c +++ b/arch/x86/events/intel/core.c @@ -4150,7 +4150,7 @@ static int intel_pmu_hw_config(struct perf_event *event) if (x86_pmu.version < 3) return -EINVAL; - ret = perf_allow_cpu(&event->attr); + ret = perf_allow_cpu(); if (ret) return ret; diff --git a/arch/x86/events/intel/p4.c b/arch/x86/events/intel/p4.c index 844bc4fc4724..78d78b8a1530 100644 --- a/arch/x86/events/intel/p4.c +++ b/arch/x86/events/intel/p4.c @@ -776,7 +776,7 @@ static int p4_validate_raw_event(struct perf_event *event) * the user needs special permissions to be able to use it */ if (p4_ht_active() && p4_event_bind_map[v].shared) { - v = perf_allow_cpu(&event->attr); + v = perf_allow_cpu(); if (v) return v; } diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c index fd5b78732603..a55d10956ff6 100644 --- a/drivers/perf/arm_spe_pmu.c +++ b/drivers/perf/arm_spe_pmu.c @@ -50,7 +50,7 @@ static_assert((PERF_EVENT_FLAG_ARCH & SPE_PMU_HW_FLAGS_CX) == SPE_PMU_HW_FLAGS_C static void set_spe_event_has_cx(struct perf_event *event) { - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && !perf_allow_kernel(&event->attr)) + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && !perf_allow_kernel()) event->hw.flags |= SPE_PMU_HW_FLAGS_CX; } @@ -746,7 +746,7 @@ static int arm_spe_pmu_event_init(struct perf_event *event) set_spe_event_has_cx(event); reg = arm_spe_event_to_pmscr(event); if (reg & (PMSCR_EL1_PA | PMSCR_EL1_PCT)) - return perf_allow_kernel(&event->attr); + return perf_allow_kernel(); return 0; } diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index e2f1ce37c41e..e349e585cc07 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -445,7 +445,7 @@ LSM_HOOK(int, 0, bpf_token_capable, const struct bpf_token *token, int cap) LSM_HOOK(int, 0, locked_down, enum lockdown_reason what) #ifdef CONFIG_PERF_EVENTS -LSM_HOOK(int, 0, perf_event_open, struct perf_event_attr *attr, int type) +LSM_HOOK(int, 0, perf_event_open, int type) LSM_HOOK(int, 0, perf_event_alloc, struct perf_event *event) LSM_HOOK(int, 0, perf_event_read, struct perf_event *event) LSM_HOOK(int, 0, perf_event_write, struct perf_event *event) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 8333f132f4a9..5d2ec4283ebf 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -1672,22 +1672,22 @@ static inline int perf_is_paranoid(void) return sysctl_perf_event_paranoid > -1; } -int perf_allow_kernel(struct perf_event_attr *attr); +int perf_allow_kernel(void); -static inline int perf_allow_cpu(struct perf_event_attr *attr) +static inline int perf_allow_cpu(void) { if (sysctl_perf_event_paranoid > 0 && !perfmon_capable()) return -EACCES; - return security_perf_event_open(attr, PERF_SECURITY_CPU); + return security_perf_event_open(PERF_SECURITY_CPU); } -static inline int perf_allow_tracepoint(struct perf_event_attr *attr) +static inline int perf_allow_tracepoint(void) { if (sysctl_perf_event_paranoid > -1 && !perfmon_capable()) return -EPERM; - return security_perf_event_open(attr, PERF_SECURITY_TRACEPOINT); + return security_perf_event_open(PERF_SECURITY_TRACEPOINT); } extern int perf_exclude_event(struct perf_event *event, struct pt_regs *regs); diff --git a/include/linux/security.h b/include/linux/security.h index 980b6c207cad..a3b35a699256 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -2324,14 +2324,13 @@ struct perf_event_attr; struct perf_event; #ifdef CONFIG_SECURITY -extern int security_perf_event_open(struct perf_event_attr *attr, int type); +extern int security_perf_event_open(int type); extern int security_perf_event_alloc(struct perf_event *event); extern void security_perf_event_free(struct perf_event *event); extern int security_perf_event_read(struct perf_event *event); extern int security_perf_event_write(struct perf_event *event); #else -static inline int security_perf_event_open(struct perf_event_attr *attr, - int type) +static inline int security_perf_event_open(int type) { return 0; } diff --git a/kernel/events/core.c b/kernel/events/core.c index b2bc67791f84..f2cb450eb134 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -4883,7 +4883,7 @@ find_get_context(struct task_struct *task, struct perf_event *event) if (!task) { /* Must be root to operate on a CPU event: */ - err = perf_allow_cpu(&event->attr); + err = perf_allow_cpu(); if (err) return ERR_PTR(err); @@ -12488,7 +12488,7 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr, } /* privileged levels capture (kernel, hv): check permissions */ if (mask & PERF_SAMPLE_BRANCH_PERM_PLM) { - ret = perf_allow_kernel(attr); + ret = perf_allow_kernel(); if (ret) return ret; } @@ -12745,12 +12745,12 @@ SYSCALL_DEFINE5(perf_event_open, return err; /* Do we allow access to perf_event_open(2) ? */ - err = security_perf_event_open(&attr, PERF_SECURITY_OPEN); + err = security_perf_event_open(PERF_SECURITY_OPEN); if (err) return err; if (!attr.exclude_kernel) { - err = perf_allow_kernel(&attr); + err = perf_allow_kernel(); if (err) return err; } @@ -12770,7 +12770,7 @@ SYSCALL_DEFINE5(perf_event_open, /* Only privileged users can get physical addresses */ if ((attr.sample_type & PERF_SAMPLE_PHYS_ADDR)) { - err = perf_allow_kernel(&attr); + err = perf_allow_kernel(); if (err) return err; } @@ -13603,12 +13603,12 @@ const struct perf_event_attr *perf_event_attrs(struct perf_event *event) return &event->attr; } -int perf_allow_kernel(struct perf_event_attr *attr) +int perf_allow_kernel(void) { if (sysctl_perf_event_paranoid > 1 && !perfmon_capable()) return -EACCES; - return security_perf_event_open(attr, PERF_SECURITY_KERNEL); + return security_perf_event_open(PERF_SECURITY_KERNEL); } EXPORT_SYMBOL_GPL(perf_allow_kernel); diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c index 3ff9caa4a71b..a6bb7577e8c5 100644 --- a/kernel/trace/trace_event_perf.c +++ b/kernel/trace/trace_event_perf.c @@ -49,7 +49,7 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event, /* The ftrace function trace is allowed only for root. */ if (ftrace_event_is_function(tp_event)) { - ret = perf_allow_tracepoint(&p_event->attr); + ret = perf_allow_tracepoint(); if (ret) return ret; @@ -86,7 +86,7 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event, * ...otherwise raw tracepoint data can be a severe data leak, * only allow root to have these. */ - ret = perf_allow_tracepoint(&p_event->attr); + ret = perf_allow_tracepoint(); if (ret) return ret; diff --git a/security/security.c b/security/security.c index 1db835c05a78..ad9655aa1322 100644 --- a/security/security.c +++ b/security/security.c @@ -5885,16 +5885,15 @@ EXPORT_SYMBOL(security_bdev_setintegrity); #ifdef CONFIG_PERF_EVENTS /** * security_perf_event_open() - Check if a perf event open is allowed - * @attr: perf event attribute * @type: type of event * * Check whether the @type of perf_event_open syscall is allowed. * * Return: Returns 0 if permission is granted. */ -int security_perf_event_open(struct perf_event_attr *attr, int type) +int security_perf_event_open(int type) { - return call_int_hook(perf_event_open, attr, type); + return call_int_hook(perf_event_open, type); } /** diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index a503b8fd6611..14c7d331c3b6 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -7040,7 +7040,7 @@ struct lsm_blob_sizes selinux_blob_sizes __ro_after_init = { }; #ifdef CONFIG_PERF_EVENTS -static int selinux_perf_event_open(struct perf_event_attr *attr, int type) +static int selinux_perf_event_open(int type) { u32 requested, sid = current_sid(); -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH linux-next 1/2] perf: Remove unnecessary parameter of security check 2024-12-23 7:06 ` [PATCH linux-next 1/2] perf: Remove unnecessary parameter of security check Luo Gengkun @ 2025-01-05 2:45 ` Paul Moore 2025-01-07 1:52 ` Luo Gengkun 2025-02-25 21:59 ` Ingo Molnar 2025-02-26 19:13 ` [PATCH " Paul Moore 1 sibling, 2 replies; 9+ messages in thread From: Paul Moore @ 2025-01-05 2:45 UTC (permalink / raw) To: Luo Gengkun Cc: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen, x86, hpa, will, jmorris, serge, rostedt, mhiramat, mathieu.desnoyers, stephen.smalley.work, omosnace, linux-perf-users, linux-kernel, linux-arm-kernel, linux-security-module, linux-trace-kernel, selinux On Mon, Dec 23, 2024 at 1:57 AM Luo Gengkun <luogengkun@huaweicloud.com> wrote: > > It seems that the attr parameter was never been used in security > checks since it was first introduced by: > > commit da97e18458fb ("perf_event: Add support for LSM and SELinux checks") > > so remove it. > > Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com> > --- > arch/x86/events/intel/bts.c | 2 +- > arch/x86/events/intel/core.c | 2 +- > arch/x86/events/intel/p4.c | 2 +- > drivers/perf/arm_spe_pmu.c | 4 ++-- > include/linux/lsm_hook_defs.h | 2 +- > include/linux/perf_event.h | 10 +++++----- > include/linux/security.h | 5 ++--- > kernel/events/core.c | 14 +++++++------- > kernel/trace/trace_event_perf.c | 4 ++-- > security/security.c | 5 ++--- > security/selinux/hooks.c | 2 +- > 11 files changed, 25 insertions(+), 27 deletions(-) This patch seems fine to me from a LSM and SELinux perspective, but I'd want to see an ACK from the perf folks before I merge this. I'll also leave patch 2/2 for the perf folks as it seems largely unrelated to this patch. -- paul-moore.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH linux-next 1/2] perf: Remove unnecessary parameter of security check 2025-01-05 2:45 ` Paul Moore @ 2025-01-07 1:52 ` Luo Gengkun 2025-02-25 21:59 ` Ingo Molnar 1 sibling, 0 replies; 9+ messages in thread From: Luo Gengkun @ 2025-01-07 1:52 UTC (permalink / raw) To: Paul Moore Cc: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen, x86, hpa, will, jmorris, serge, rostedt, mhiramat, mathieu.desnoyers, stephen.smalley.work, omosnace, linux-perf-users, linux-kernel, linux-arm-kernel, linux-security-module, linux-trace-kernel, selinux On 2025/1/5 10:45, Paul Moore wrote: > On Mon, Dec 23, 2024 at 1:57 AM Luo Gengkun <luogengkun@huaweicloud.com> wrote: >> It seems that the attr parameter was never been used in security >> checks since it was first introduced by: >> >> commit da97e18458fb ("perf_event: Add support for LSM and SELinux checks") >> >> so remove it. >> >> Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com> >> --- >> arch/x86/events/intel/bts.c | 2 +- >> arch/x86/events/intel/core.c | 2 +- >> arch/x86/events/intel/p4.c | 2 +- >> drivers/perf/arm_spe_pmu.c | 4 ++-- >> include/linux/lsm_hook_defs.h | 2 +- >> include/linux/perf_event.h | 10 +++++----- >> include/linux/security.h | 5 ++--- >> kernel/events/core.c | 14 +++++++------- >> kernel/trace/trace_event_perf.c | 4 ++-- >> security/security.c | 5 ++--- >> security/selinux/hooks.c | 2 +- >> 11 files changed, 25 insertions(+), 27 deletions(-) > This patch seems fine to me from a LSM and SELinux perspective, but > I'd want to see an ACK from the perf folks before I merge this. > > I'll also leave patch 2/2 for the perf folks as it seems largely > unrelated to this patch. Thanks for your review Gengkun ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH linux-next 1/2] perf: Remove unnecessary parameter of security check 2025-01-05 2:45 ` Paul Moore 2025-01-07 1:52 ` Luo Gengkun @ 2025-02-25 21:59 ` Ingo Molnar 1 sibling, 0 replies; 9+ messages in thread From: Ingo Molnar @ 2025-02-25 21:59 UTC (permalink / raw) To: Paul Moore Cc: Luo Gengkun, peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen, x86, hpa, will, jmorris, serge, rostedt, mhiramat, mathieu.desnoyers, stephen.smalley.work, omosnace, linux-perf-users, linux-kernel, linux-arm-kernel, linux-security-module, linux-trace-kernel, selinux * Paul Moore <paul@paul-moore.com> wrote: > On Mon, Dec 23, 2024 at 1:57 AM Luo Gengkun <luogengkun@huaweicloud.com> wrote: > > > > It seems that the attr parameter was never been used in security > > checks since it was first introduced by: > > > > commit da97e18458fb ("perf_event: Add support for LSM and SELinux checks") > > > > so remove it. > > > > Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com> > > --- > > arch/x86/events/intel/bts.c | 2 +- > > arch/x86/events/intel/core.c | 2 +- > > arch/x86/events/intel/p4.c | 2 +- > > drivers/perf/arm_spe_pmu.c | 4 ++-- > > include/linux/lsm_hook_defs.h | 2 +- > > include/linux/perf_event.h | 10 +++++----- > > include/linux/security.h | 5 ++--- > > kernel/events/core.c | 14 +++++++------- > > kernel/trace/trace_event_perf.c | 4 ++-- > > security/security.c | 5 ++--- > > security/selinux/hooks.c | 2 +- > > 11 files changed, 25 insertions(+), 27 deletions(-) > > This patch seems fine to me from a LSM and SELinux perspective, but > I'd want to see an ACK from the perf folks before I merge this. A belated: Reviewed-by: Ingo Molnar <mingo@kernel.org> Patch still seems to apply fine. Thanks, Ingo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] perf: Remove unnecessary parameter of security check 2024-12-23 7:06 ` [PATCH linux-next 1/2] perf: Remove unnecessary parameter of security check Luo Gengkun 2025-01-05 2:45 ` Paul Moore @ 2025-02-26 19:13 ` Paul Moore 1 sibling, 0 replies; 9+ messages in thread From: Paul Moore @ 2025-02-26 19:13 UTC (permalink / raw) To: Luo Gengkun, peterz Cc: mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen, x86, hpa, will, jmorris, serge, rostedt, mhiramat, mathieu.desnoyers, stephen.smalley.work, omosnace, linux-perf-users, linux-kernel, linux-arm-kernel, linux-security-module, linux-trace-kernel, selinux, luogengkun On Dec 23, 2024 Luo Gengkun <luogengkun@huaweicloud.com> wrote: > > It seems that the attr parameter was never been used in security > checks since it was first introduced by: > > commit da97e18458fb ("perf_event: Add support for LSM and SELinux checks") > > so remove it. > > Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com> > Reviewed-by: Ingo Molnar <mingo@kernel.org> > --- > arch/x86/events/intel/bts.c | 2 +- > arch/x86/events/intel/core.c | 2 +- > arch/x86/events/intel/p4.c | 2 +- > drivers/perf/arm_spe_pmu.c | 4 ++-- > include/linux/lsm_hook_defs.h | 2 +- > include/linux/perf_event.h | 10 +++++----- > include/linux/security.h | 5 ++--- > kernel/events/core.c | 14 +++++++------- > kernel/trace/trace_event_perf.c | 4 ++-- > security/security.c | 5 ++--- > security/selinux/hooks.c | 2 +- > 11 files changed, 25 insertions(+), 27 deletions(-) Now that we have Ingo's Reviewed-by, I've gone ahead and merged patch 1/2 into lsm/dev, thanks everyone! -- paul-moore.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH linux-next 2/2] perf: Return EACCESS when need perfmon capability 2024-12-23 7:06 [PATCH linux-next 0/2] Fix perf security check problem Luo Gengkun 2024-12-23 7:06 ` [PATCH linux-next 1/2] perf: Remove unnecessary parameter of security check Luo Gengkun @ 2024-12-23 7:06 ` Luo Gengkun 2025-01-06 15:59 ` James Clark 1 sibling, 1 reply; 9+ messages in thread From: Luo Gengkun @ 2024-12-23 7:06 UTC (permalink / raw) To: peterz Cc: mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen, x86, hpa, will, paul, jmorris, serge, rostedt, mhiramat, mathieu.desnoyers, stephen.smalley.work, omosnace, linux-perf-users, linux-kernel, linux-arm-kernel, linux-security-module, linux-trace-kernel, selinux, luogengkun For perf_allow_kernel and perf_allow_cpu, both return EACCES when require CAP_PERFMON or CAP_SYS_ADMIN permissions, so update perf_allow_tracepoint to keep them the same. Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com> --- include/linux/perf_event.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 5d2ec4283ebf..c1bc0d7a275b 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -1685,7 +1685,7 @@ static inline int perf_allow_cpu(void) static inline int perf_allow_tracepoint(void) { if (sysctl_perf_event_paranoid > -1 && !perfmon_capable()) - return -EPERM; + return -EACCES; return security_perf_event_open(PERF_SECURITY_TRACEPOINT); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH linux-next 2/2] perf: Return EACCESS when need perfmon capability 2024-12-23 7:06 ` [PATCH linux-next 2/2] perf: Return EACCESS when need perfmon capability Luo Gengkun @ 2025-01-06 15:59 ` James Clark 2025-01-07 1:46 ` Luo Gengkun 0 siblings, 1 reply; 9+ messages in thread From: James Clark @ 2025-01-06 15:59 UTC (permalink / raw) To: Luo Gengkun Cc: mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen, x86, hpa, will, paul, jmorris, serge, rostedt, mhiramat, mathieu.desnoyers, stephen.smalley.work, omosnace, linux-perf-users, linux-kernel, linux-arm-kernel, linux-security-module, linux-trace-kernel, selinux, Peter Zijlstra On 23/12/2024 7:06 am, Luo Gengkun wrote: > For perf_allow_kernel and perf_allow_cpu, both return EACCES when require > CAP_PERFMON or CAP_SYS_ADMIN permissions, so update perf_allow_tracepoint > to keep them the same. > > Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com> > --- > include/linux/perf_event.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index 5d2ec4283ebf..c1bc0d7a275b 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -1685,7 +1685,7 @@ static inline int perf_allow_cpu(void) > static inline int perf_allow_tracepoint(void) > { > if (sysctl_perf_event_paranoid > -1 && !perfmon_capable()) > - return -EPERM; > + return -EACCES; > Is this necessary other than for consistency? If not it might be best to leave it inconsistent even if it's wrong. I see quite a few "if EPERM do this..." type things in Perf, so changing this would break error messages being shown to users. If anything, EPERM seems more correct because EACCESS is more about file access. Thanks James ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH linux-next 2/2] perf: Return EACCESS when need perfmon capability 2025-01-06 15:59 ` James Clark @ 2025-01-07 1:46 ` Luo Gengkun 0 siblings, 0 replies; 9+ messages in thread From: Luo Gengkun @ 2025-01-07 1:46 UTC (permalink / raw) To: James Clark Cc: mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen, x86, hpa, will, paul, jmorris, serge, rostedt, mhiramat, mathieu.desnoyers, stephen.smalley.work, omosnace, linux-perf-users, linux-kernel, linux-arm-kernel, linux-security-module, linux-trace-kernel, selinux, Peter Zijlstra On 2025/1/6 23:59, James Clark wrote: > > > On 23/12/2024 7:06 am, Luo Gengkun wrote: >> For perf_allow_kernel and perf_allow_cpu, both return EACCES when >> require >> CAP_PERFMON or CAP_SYS_ADMIN permissions, so update >> perf_allow_tracepoint >> to keep them the same. >> >> Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com> >> --- >> include/linux/perf_event.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h >> index 5d2ec4283ebf..c1bc0d7a275b 100644 >> --- a/include/linux/perf_event.h >> +++ b/include/linux/perf_event.h >> @@ -1685,7 +1685,7 @@ static inline int perf_allow_cpu(void) >> static inline int perf_allow_tracepoint(void) >> { >> if (sysctl_perf_event_paranoid > -1 && !perfmon_capable()) >> - return -EPERM; >> + return -EACCES; > > Is this necessary other than for consistency? If not it might be best > to leave it inconsistent even if it's wrong. I see quite a few "if > EPERM do this..." type things in Perf, so changing this would break > error messages being shown to users. > > If anything, EPERM seems more correct because EACCESS is more about > file access. I think so, from the perspective of capabilities and sysctl_perf_event_paranoid, EPERM is more appropriate. > > Thanks > James Thanks for your review. Actually, I am not sure if it's typo or intentional, so this patch is more like a consultation. It's okay to keep it the same so it doesn't torture the user. Thanks Gengkun ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-02-26 19:13 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-23 7:06 [PATCH linux-next 0/2] Fix perf security check problem Luo Gengkun 2024-12-23 7:06 ` [PATCH linux-next 1/2] perf: Remove unnecessary parameter of security check Luo Gengkun 2025-01-05 2:45 ` Paul Moore 2025-01-07 1:52 ` Luo Gengkun 2025-02-25 21:59 ` Ingo Molnar 2025-02-26 19:13 ` [PATCH " Paul Moore 2024-12-23 7:06 ` [PATCH linux-next 2/2] perf: Return EACCESS when need perfmon capability Luo Gengkun 2025-01-06 15:59 ` James Clark 2025-01-07 1:46 ` Luo Gengkun
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).