| Message ID | 20241223070650.2810747-3-luogengkun@huaweicloud.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Fix perf security check problem | expand |
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
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
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); }
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(-)