| Message ID | 20240618071010.11214-1-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: hw_breakpoint: Save privilege of access control via ptrace | expand |
I don't understand what this patch does, but... On 06/18, Tiezhu Yang wrote: > > --- a/include/uapi/linux/perf_event.h > +++ b/include/uapi/linux/perf_event.h > @@ -467,6 +467,7 @@ struct perf_event_attr { > __u32 wakeup_watermark; /* bytes before wakeup */ > }; > > + __u8 bp_priv; > __u32 bp_type; Is it safe to add the new member in the middle of uapi struct? This will break userspace... Oleg.
On 06/19/2024 11:15 PM, Oleg Nesterov wrote: > I don't understand what this patch does, but... Thanks for your reply. ctrl->privilege in decode_ctrl_reg() is never be used later but it can and should be used in arch_build_bp_info(). arch/arm64/include/asm/hw_breakpoint.h static inline void decode_ctrl_reg(u32 reg, struct arch_hw_breakpoint_ctrl *ctrl) { ... ctrl->privilege = reg & 0x3; // it is never be used later but ... ... } arch/arm64/kernel/hw_breakpoint.c static int arch_build_bp_info(struct perf_event *bp, const struct perf_event_attr *attr, struct arch_hw_breakpoint *hw) { ... if (arch_check_bp_in_kernelspace(hw)) hw->ctrl.privilege = AARCH64_BREAKPOINT_EL1; // ... it can and should be used here else hw->ctrl.privilege = AARCH64_BREAKPOINT_EL0; // and here. ... } > > On 06/18, Tiezhu Yang wrote: >> >> --- a/include/uapi/linux/perf_event.h >> +++ b/include/uapi/linux/perf_event.h >> @@ -467,6 +467,7 @@ struct perf_event_attr { >> __u32 wakeup_watermark; /* bytes before wakeup */ >> }; >> >> + __u8 bp_priv; >> __u32 bp_type; > > Is it safe to add the new member in the middle of uapi struct? > This will break userspace... Let me put the new member "bp_priv" at the end of uapi struct perf_event_attr in the next version if you are OK with it. Thanks, Tiezhu
On 06/20, Tiezhu Yang wrote: > > On 06/19/2024 11:15 PM, Oleg Nesterov wrote: > >>--- a/include/uapi/linux/perf_event.h > >>+++ b/include/uapi/linux/perf_event.h > >>@@ -467,6 +467,7 @@ struct perf_event_attr { > >> __u32 wakeup_watermark; /* bytes before wakeup */ > >> }; > >> > >>+ __u8 bp_priv; > >> __u32 bp_type; > > > >Is it safe to add the new member in the middle of uapi struct? > >This will break userspace... > > Let me put the new member "bp_priv" at the end of uapi struct > perf_event_attr in the next version if you are OK with it. And add PERF_ATTR_SIZE_VER9 ? Well, you can safely ignore me, you should ask the maintainers ;) But to me the very idea of arm64-specific and "kernel only" member in perf_event_attr looks a bit strange. Oleg.
On Thu, Jun 20, 2024 at 11:08:07AM +0200, Oleg Nesterov wrote: > On 06/20, Tiezhu Yang wrote: > > > > On 06/19/2024 11:15 PM, Oleg Nesterov wrote: > > >>--- a/include/uapi/linux/perf_event.h > > >>+++ b/include/uapi/linux/perf_event.h > > >>@@ -467,6 +467,7 @@ struct perf_event_attr { > > >> __u32 wakeup_watermark; /* bytes before wakeup */ > > >> }; > > >> > > >>+ __u8 bp_priv; > > >> __u32 bp_type; > > > > > >Is it safe to add the new member in the middle of uapi struct? > > >This will break userspace... > > > > Let me put the new member "bp_priv" at the end of uapi struct > > perf_event_attr in the next version if you are OK with it. > > And add PERF_ATTR_SIZE_VER9 ? > > Well, you can safely ignore me, you should ask the maintainers ;) > > But to me the very idea of arm64-specific and "kernel only" member in > perf_event_attr looks a bit strange. Yeah, completely agreed. Will
On 06/20/2024 05:08 PM, Oleg Nesterov wrote: > On 06/20, Tiezhu Yang wrote: >> >> On 06/19/2024 11:15 PM, Oleg Nesterov wrote: >>>> --- a/include/uapi/linux/perf_event.h >>>> +++ b/include/uapi/linux/perf_event.h >>>> @@ -467,6 +467,7 @@ struct perf_event_attr { >>>> __u32 wakeup_watermark; /* bytes before wakeup */ >>>> }; >>>> >>>> + __u8 bp_priv; >>>> __u32 bp_type; >>> >>> Is it safe to add the new member in the middle of uapi struct? >>> This will break userspace... >> >> Let me put the new member "bp_priv" at the end of uapi struct >> perf_event_attr in the next version if you are OK with it. > > And add PERF_ATTR_SIZE_VER9 ? Yes, thank you. > Well, you can safely ignore me, you should ask the maintainers ;) > > But to me the very idea of arm64-specific and "kernel only" member in > perf_event_attr looks a bit strange. I noticed that there is a similar arm64-specific change in commit 09519ec3b19e ("perf: Add perf_event_attr::config3") according to the commit message, and it will be used for LoongArch later if this change is accepted. Thanks, Tiezhu
Again, I can't really comment, I know almost nothing about perf, but On 06/20, Tiezhu Yang wrote: > > On 06/20/2024 05:08 PM, Oleg Nesterov wrote: > > > >But to me the very idea of arm64-specific and "kernel only" member in > >perf_event_attr looks a bit strange. > > I noticed that there is a similar arm64-specific change in > commit 09519ec3b19e ("perf: Add perf_event_attr::config3") but this is another thing even if I have no idea what .config3 means. If nothing else, what do you think, say, tools/perf can do with ->bp_priv? What should sys_perf_event_open() do if bp_priv != 0 comes from user space? Nevermind, please forget, I leave this to you and maintainers. Oleg.
On 20/06/2024 11:36, Oleg Nesterov wrote: > Again, I can't really comment, I know almost nothing about perf, but > > On 06/20, Tiezhu Yang wrote: >> >> On 06/20/2024 05:08 PM, Oleg Nesterov wrote: >>> >>> But to me the very idea of arm64-specific and "kernel only" member in >>> perf_event_attr looks a bit strange. >> >> I noticed that there is a similar arm64-specific change in >> commit 09519ec3b19e ("perf: Add perf_event_attr::config3") > > but this is another thing even if I have no idea what .config3 means. > > If nothing else, what do you think, say, tools/perf can do with ->bp_priv? Yeah, including the tools side change in the same series might help to explain. It's not obvious what the end goal is from the commit message. Thanks James > > What should sys_perf_event_open() do if bp_priv != 0 comes from user space? > > Nevermind, please forget, I leave this to you and maintainers. > > Oleg. > >
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c index 722ac45f9f7b..06e34bcdcf92 100644 --- a/arch/arm64/kernel/hw_breakpoint.c +++ b/arch/arm64/kernel/hw_breakpoint.c @@ -486,15 +486,8 @@ static int arch_build_bp_info(struct perf_event *bp, /* Address */ hw->address = attr->bp_addr; - /* - * Privilege - * Note that we disallow combined EL0/EL1 breakpoints because - * that would complicate the stepping code. - */ - if (arch_check_bp_in_kernelspace(hw)) - hw->ctrl.privilege = AARCH64_BREAKPOINT_EL1; - else - hw->ctrl.privilege = AARCH64_BREAKPOINT_EL0; + /* Privilege */ + hw->ctrl.privilege = attr->bp_priv; /* Enabled? */ hw->ctrl.enabled = !attr->disabled; diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index 0d022599eb61..3b37c4a2e0d4 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -309,6 +309,7 @@ static struct perf_event *ptrace_hbp_create(unsigned int note_type, attr.bp_addr = 0; attr.bp_len = HW_BREAKPOINT_LEN_4; attr.bp_type = type; + attr.bp_priv = AARCH64_BREAKPOINT_EL0; attr.disabled = 1; bp = register_user_hw_breakpoint(&attr, ptrace_hbptriggered, NULL, tsk); @@ -352,6 +353,7 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type, attr->bp_len = len; attr->bp_type = type; attr->bp_addr += offset; + attr->bp_priv = ctrl.privilege; return 0; } diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h index 3a64499b0f5d..88dcaba421cc 100644 --- a/include/uapi/linux/perf_event.h +++ b/include/uapi/linux/perf_event.h @@ -467,6 +467,7 @@ struct perf_event_attr { __u32 wakeup_watermark; /* bytes before wakeup */ }; + __u8 bp_priv; __u32 bp_type; union { __u64 bp_addr; diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c index 6c2cb4e4f48d..3ad16b226e4f 100644 --- a/kernel/events/hw_breakpoint.c +++ b/kernel/events/hw_breakpoint.c @@ -754,6 +754,7 @@ static void hw_breakpoint_copy_attr(struct perf_event_attr *to, to->bp_addr = from->bp_addr; to->bp_type = from->bp_type; to->bp_len = from->bp_len; + to->bp_priv = from->bp_priv; to->disabled = from->disabled; }
Currently, decode_ctrl_reg() saves the privilege of access control which is not used anymore, arch_build_bp_info() checks whether bp virtual address is in kernel space to construct hw->ctrl.privilege, the process seems not reasonable. Add a member "bp_priv" in struct perf_event_attr to make a bridge between ptrace and hardware breakpoint, it can save the privilege of access control via ptrace for hardware breakpoint. Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- This patch is based on 6.10-rc4, cross compile tested only. This is a try and preparation for later patch on LoongArch. arch/arm64/kernel/hw_breakpoint.c | 11 ++--------- arch/arm64/kernel/ptrace.c | 2 ++ include/uapi/linux/perf_event.h | 1 + kernel/events/hw_breakpoint.c | 1 + 4 files changed, 6 insertions(+), 9 deletions(-)