linux-kernel - Re: [PATCH v2 1/3] perf: Add perf_event_attr::bp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240708073652.GE11386@noisy.programming.kicks-ass.net>
Date: Mon, 8 Jul 2024 09:36:52 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Tiezhu Yang <yangtiezhu@...ngson.cn>
Cc: Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>,
	Russell King <linux@...linux.org.uk>,
	Catalin Marinas <catalin.marinas@....com>,
	Oleg Nesterov <oleg@...hat.com>, Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	linux-arm-kernel@...ts.infradead.org,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] perf: Add perf_event_attr::bp_priv

On Sat, Jul 06, 2024 at 01:31:03PM +0800, Tiezhu Yang wrote:
> 
> 
> On 07/05/2024 06:34 PM, Will Deacon wrote:
> > On Fri, Jun 21, 2024 at 03:39:08PM +0800, Tiezhu Yang wrote:
> > > Add a member "bp_priv" at the end of the uapi struct perf_event_attr
> > > to make a bridge between ptrace and hardware breakpoint.
> > > 
> > > This is preparation for later patch on some archs such as ARM, ARM64
> > > and LoongArch which have privilege level of breakpoint.
> > > 
> > > Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
> > > ---
> > >  include/uapi/linux/perf_event.h | 3 +++
> > >  kernel/events/hw_breakpoint.c   | 1 +
> > >  2 files changed, 4 insertions(+)
> > > 
> > > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> > > index 3a64499b0f5d..f9f917e854e6 100644
> > > --- a/include/uapi/linux/perf_event.h
> > > +++ b/include/uapi/linux/perf_event.h
> > > @@ -379,6 +379,7 @@ enum perf_event_read_format {
> > >  #define PERF_ATTR_SIZE_VER6	120	/* add: aux_sample_size */
> > >  #define PERF_ATTR_SIZE_VER7	128	/* add: sig_data */
> > >  #define PERF_ATTR_SIZE_VER8	136	/* add: config3 */
> > > +#define PERF_ATTR_SIZE_VER9	144	/* add: bp_priv */
> > > 
> > >  /*
> > >   * Hardware event_id to monitor via a performance monitoring event:
> > > @@ -522,6 +523,8 @@ struct perf_event_attr {
> > >  	__u64	sig_data;
> > > 
> > >  	__u64	config3; /* extension of config2 */
> > > +
> > > +	__u8	bp_priv; /* privilege level of breakpoint */
> > >  };
> > 
> > Why are we extending the user ABI for this? Perf events already have the
> > privilege encoded (indirectly) by the exclude_{user,kernel,hv} fields in
> > 'struct perf_event_attr'.
> 
> IMO, add bp_priv is to keep consistent with the other fields
> bp_type, bp_addr and bp_len, the meaning of bp_priv field is
> explicit and different with exclude_{user,kernel,hv} fields.

In case it wasn't obvious, this structure has __u64 granularity. You
don't just add a __u8 to the end. Also, since you mention consistency,
you might have noticed those other bp_ fields are in a union on
config[12], so why can't this live in a union on config3 ?

> Additionally, there is only 1 bit for exclude_{user,kernel,hv},
> but bp_priv field has at least 2 bit according to the explanation
> of Arm Reference Manual. At last, the initial aim is to remove
> the check condition to assign the value of hw->ctrl.privilege.
> 
> https://developer.arm.com/documentation/ddi0487/latest/
> 
> 1. D23: AArch64 System Register Descriptions (Page 8562)
>    D23.3.11 DBGWCR<n>_EL1, Debug Watchpoint Control Registers, n = 0 - 63
>    PAC, bits [2:1]
>    Privilege of access control. Determines the Exception level or levels at
> which a Watchpoint debug
>    event for watchpoint n is generated.
> 
> 2. G8: AArch32 System Register Descriptions (Page 12334)
>    G8.3.26 DBGWCR<n>, Debug Watchpoint Control Registers, n = 0 - 15
>    PAC, bits [2:1]
>    Privilege of access control. Determines the Exception level or levels at
> which a Watchpoint debug
>    event for watchpoint n is generated.

That's all clear as mud for someone that don't speak arm. Can you please
provide a coherent reason for all this that does not rely on external
resources?