linux-kernel - Re: [PATCH -next v4 02/19] arm64: entry: Refactor the entry and exit for exceptions from EL1

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: <ZyDyS4TT6xgRIN1w@J2N7QTR9R3.cambridge.arm.com>
Date: Tue, 29 Oct 2024 14:33:47 +0000
From: Mark Rutland <mark.rutland@....com>
To: Jinjie Ruan <ruanjinjie@...wei.com>
Cc: oleg@...hat.com, linux@...linux.org.uk, will@...nel.org,
	catalin.marinas@....com, sstabellini@...nel.org, maz@...nel.org,
	tglx@...utronix.de, peterz@...radead.org, luto@...nel.org,
	kees@...nel.org, wad@...omium.org, akpm@...ux-foundation.org,
	samitolvanen@...gle.com, arnd@...db.de, ojeda@...nel.org,
	rppt@...nel.org, hca@...ux.ibm.com, aliceryhl@...gle.com,
	samuel.holland@...ive.com, paulmck@...nel.org, aquini@...hat.com,
	petr.pavlu@...e.com, viro@...iv.linux.org.uk,
	rmk+kernel@...linux.org.uk, ardb@...nel.org,
	wangkefeng.wang@...wei.com, surenb@...gle.com,
	linus.walleij@...aro.org, yangyj.ee@...il.com, broonie@...nel.org,
	mbenes@...e.cz, puranjay@...nel.org, pcc@...gle.com,
	guohanjun@...wei.com, sudeep.holla@....com,
	Jonathan.Cameron@...wei.com, prarit@...hat.com, liuwei09@...tc.cn,
	dwmw@...zon.co.uk, oliver.upton@...ux.dev,
	kristina.martsenko@....com, ptosi@...gle.com, frederic@...nel.org,
	vschneid@...hat.com, thiago.bauermann@...aro.org,
	joey.gouly@....com, liuyuntao12@...wei.com, leobras@...hat.com,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	xen-devel@...ts.xenproject.org
Subject: Re: [PATCH -next v4 02/19] arm64: entry: Refactor the entry and exit
 for exceptions from EL1

On Fri, Oct 25, 2024 at 06:06:43PM +0800, Jinjie Ruan wrote:
> These changes refactor the entry and exit routines for the exceptions
> from EL1. They store the RCU and lockdep state in a struct
> irqentry_state variable on the stack, rather than recording them
> in the fields of pt_regs, since it is safe enough for these context.

In general, please descirbe *why* we want to make the change first, e.g.

| The generic entry code uses irqentry_state_t to track lockdep and RCU
| state across exception entry and return. For historical reasons, arm64
| embeds similar fields within its pt_regs structure.
|
| In preparation for moving arm64 over to the generic entry code, pull
| these fields out of arm64's pt_regs, and use a seperate structure,
| matching the style of the generic entry code.

> Before:
> 	struct pt_regs {
> 		...
> 		u64 lockdep_hardirqs;
> 		u64 exit_rcu;
> 	}
> 
> 	enter_from_kernel_mode(regs);
> 	...
> 	exit_to_kernel_mode(regs);
> 
> After:
> 	typedef struct irqentry_state {
> 		union {
> 			bool    exit_rcu;
> 			bool    lockdep;
> 		};
> 	} irqentry_state_t;
> 
> 	irqentry_state_t state = enter_from_kernel_mode(regs);
> 	...
> 	exit_to_kernel_mode(regs, state);

I don't think this part is necessary.

> 
> No functional changes.
> 
> Suggested-by: Mark Rutland <mark.rutland@....com>
> Signed-off-by: Jinjie Ruan <ruanjinjie@...wei.com>
> ---
>  arch/arm64/include/asm/ptrace.h  |  11 ++-
>  arch/arm64/kernel/entry-common.c | 129 +++++++++++++++++++------------
>  2 files changed, 85 insertions(+), 55 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index 3e5372a98da4..5156c0d5fa20 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -149,6 +149,13 @@ static inline unsigned long pstate_to_compat_psr(const unsigned long pstate)
>  	return psr;
>  }
>  
> +typedef struct irqentry_state {
> +	union {
> +		bool	exit_rcu;
> +		bool	lockdep;
> +	};
> +} irqentry_state_t;

AFAICT this can be moved directly into arch/arm64/kernel/entry-common.c.

> +
>  /*
>   * This struct defines the way the registers are stored on the stack during an
>   * exception. struct user_pt_regs must form a prefix of struct pt_regs.
> @@ -169,10 +176,6 @@ struct pt_regs {
>  
>  	u64 sdei_ttbr1;
>  	struct frame_record_meta stackframe;
> -
> -	/* Only valid for some EL1 exceptions. */
> -	u64 lockdep_hardirqs;
> -	u64 exit_rcu;
>  };
>  
>  /* For correct stack alignment, pt_regs has to be a multiple of 16 bytes. */
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index c547e70428d3..68a9aecacdb9 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -36,29 +36,36 @@
>   * This is intended to match the logic in irqentry_enter(), handling the kernel
>   * mode transitions only.
>   */
> -static __always_inline void __enter_from_kernel_mode(struct pt_regs *regs)
> +static __always_inline irqentry_state_t __enter_from_kernel_mode(struct pt_regs *regs)
>  {
> -	regs->exit_rcu = false;
> +	irqentry_state_t ret = {
> +		.exit_rcu = false,
> +	};

I realise that the generic entry code calls this 'ret' in
irqentry_enter() and similar, but could we please use 'state'
consistently in the arm64 code?

[...]

>  /*
> @@ -190,9 +199,11 @@ asmlinkage void noinstr asm_exit_to_user_mode(struct pt_regs *regs)
>   * mode. Before this function is called it is not safe to call regular kernel
>   * code, instrumentable code, or any code which may trigger an exception.
>   */
> -static void noinstr arm64_enter_nmi(struct pt_regs *regs)
> +static noinstr irqentry_state_t arm64_enter_nmi(struct pt_regs *regs)
>  {
> -	regs->lockdep_hardirqs = lockdep_hardirqs_enabled();
> +	irqentry_state_t irq_state;

Likewise, please use 'state' rather than 'irq_state'.

In future we should probably have a separate structure for the NMI
paths, and get rid of the union, which would avoid the possiblity of
using mismatched helpers.

Mark.