linux-kernel - Re: [PATCH v3 3/3] arm64: entry: Convert to generic entry

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: <f1883449-9bab-0896-6cc9-4340d2ca72aa@huawei.com>
Date: Thu, 22 Aug 2024 20:30:49 +0800
From: Jinjie Ruan <ruanjinjie@...wei.com>
To: Kevin Brodsky <kevin.brodsky@....com>
CC: <catalin.marinas@....com>, <will@...nel.org>, <oleg@...hat.com>,
	<tglx@...utronix.de>, <peterz@...radead.org>, <luto@...nel.org>,
	<kees@...nel.org>, <wad@...omium.org>, <rostedt@...dmis.org>,
	<arnd@...db.de>, <ardb@...nel.org>, <broonie@...nel.org>,
	<mark.rutland@....com>, <rick.p.edgecombe@...el.com>, <leobras@...hat.com>,
	<linux-kernel@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 3/3] arm64: entry: Convert to generic entry



On 2024/8/20 19:43, Kevin Brodsky wrote:
> On 29/06/2024 10:56, Jinjie Ruan wrote:
>> Currently, x86, Riscv, Loongarch use the generic entry. Convert arm64
>> to use the generic entry infrastructure from kernel/entry/*. The generic
>> entry makes maintainers' work easier and codes more elegant, which also
>> removed duplicate 150+ LOC. The changes are below:
>>
>>  - Remove TIF_SYSCALL_* flag, _TIF_WORK_MASK, _TIF_SYSCALL_WORK
>>
>>  - Remove syscall_trace_enter/exit() and use generic one.
>>
>>  - Remove *enter_from/exit_to_kernel_mode(), and wrap with generic
>>    irqentry_enter/exit().
>>
>>  - Remove *enter_from/exit_to_user_mode(), and wrap with generic
>>    irqentry_enter_from/exit_to_user_mode().
>>
>>  - Remove arm64_enter/exit_nmi() and use generic irqentry_nmi_enter/exit().
>>
>>  - Remove PREEMPT_DYNAMIC code, as generic entry will do it ok by
>>    implementing arch_irqentry_exit_need_resched().
> 
> This is looking good to me overall, moving to using the generic helpers
> is a clear improvement. I've tried my best to check that the new

Thank you very much for the review, It does make the code more clear.

> implementation is functionally equivalent to the old. Ignoring additions
> in the generic code (such as additional instrumentation_{begin,end}()
> pairs or assertions), this seems to be the case, with one exception. The
> MTE operations are done in a slightly different order on entry:
> 
> * mte_disable_tco_entry() was called *after* the generic lockdep/CT
> functions in __enter_from_user_mode(). It is now called before those
> generic operations - arch_enter_from_user_mode() called at the beginning
> of enter_from_user_mode().

The most special for arm64 are these MTE operations, the comment for
__enter_from_kernel_mode() and __enter_from_user_mode() said:

   " Handle IRQ/context state management when entering from user/kernel
mode. Before this function is called it is not safe to call regular
kernel code "

I guess it is the reason why mte_disable_tco_entry() was called *after*
the generic lockdep/CT functions, but the first version of commit
38ddf7dafaeaf ("arm64: mte: avoid clearing PSTATE.TCO on entry unless
necessary") call it in arch/arm64/kernel/entry.S, so I think the order
is not so important.

> 
> * Similarly mte_disable_tco_entry() and mte_check_tfsr_entry() was
> called after the generic operations in enter_from_kernel_mode(), and
> they are now called after - arch_enter_from_kernel_mode() called at the
> beginning of irqentry_enter().

This can be adjusted to be consistent.

> 
> I am not under the impression that these ordering changes are
> problematic, but I may be missing something.>
>> [...]
>>  
>> -/*
>> - * Handle IRQ/context state management when entering from kernel 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.
>> - *
>> - * 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)
>> -{
>> -	regs->exit_rcu = false;
> 
> exit_rcu in struct pt_regs is unused now that these functions are gone
> so it can be removed.

It is also consistent with ARM64.

> 
>> [...]
>>  
>> @@ -259,48 +74,6 @@ static void noinstr arm64_exit_el1_dbg(struct pt_regs *regs)
> 
> arm64_{enter,exit}_el1_dbg() have apparently no generic counterparts we
> can replace them with, but maybe we could align them with the generic
> functions some more? Specifically, I'm thinking about making them
> return/take an irqentry_state_t just like irqentry_nmi_{enter,exit}().
> This way we can get rid of struct pt_regs::lockdep_hardirqs, which is
> now only used by those functions.
> 
>>  		lockdep_hardirqs_on(CALLER_ADDR0);
>>  }
>>  
>> -#ifdef CONFIG_PREEMPT_DYNAMIC
>> -DEFINE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
> 
> The key is declared in asm/preempt.h, that declaration should also be
> removed.

Yes, you are right! commit 1b2d3451ee50 ("arm64: Support
PREEMPT_DYNAMIC") mentioned it:

    "Since arm64 does not yet use the generic entry code, we must define
our own `sk_dynamic_irqentry_exit_cond_resched`, which will be
enabled/disabled by the common code in kernel/sched/core.c. All other
preemption functions and associated static keys are defined there."

So if arm64 switched to generic entry, the code can be removed.

> 
> Kevin
> 
>> [...]
> 
>