[PATCH] arm64: hw_breakpoint: Save privilege of access control via ptrace

[PATCH] arm64: hw_breakpoint: Save privilege of access control via ptrace

[Tiezhu Yang]

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(-)

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;
 }
 
-- 
2.42.0

Re: [PATCH] arm64: hw_breakpoint: Save privilege of access control via ptrace

[Oleg Nesterov]

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.

Re: [PATCH] arm64: hw_breakpoint: Save privilege of access control via ptrace

[Tiezhu Yang]

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

Re: [PATCH] arm64: hw_breakpoint: Save privilege of access control via ptrace

[Oleg Nesterov]

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.

Re: [PATCH] arm64: hw_breakpoint: Save privilege of access control via ptrace

[Will Deacon]

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

Re: [PATCH] arm64: hw_breakpoint: Save privilege of access control via ptrace

[Tiezhu Yang]

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

Re: [PATCH] arm64: hw_breakpoint: Save privilege of access control via ptrace

[Oleg Nesterov]

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.

Re: [PATCH] arm64: hw_breakpoint: Save privilege of access control via ptrace

[James Clark]

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.
> 
>