| 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
| ||
|
Message-ID: <f10848f1-36d5-c954-2b55-d9cdaf5262bf@huawei.com>
Date: Tue, 30 Jan 2024 21:22:59 +0800
From: Tong Tiangen <tongtiangen@...wei.com>
To: Mark Rutland <mark.rutland@....com>
CC: Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
James Morse <james.morse@....com>, Robin Murphy <robin.murphy@....com>,
Andrey Ryabinin <ryabinin.a.a@...il.com>, Alexander Potapenko
<glider@...gle.com>, Alexander Viro <viro@...iv.linux.org.uk>, Andrey
Konovalov <andreyknvl@...il.com>, Dmitry Vyukov <dvyukov@...gle.com>,
Vincenzo Frascino <vincenzo.frascino@....com>, Andrew Morton
<akpm@...ux-foundation.org>, Michael Ellerman <mpe@...erman.id.au>, Nicholas
Piggin <npiggin@...il.com>, Christophe Leroy <christophe.leroy@...roup.eu>,
Aneesh Kumar K.V <aneesh.kumar@...nel.org>, "Naveen N. Rao"
<naveen.n.rao@...ux.ibm.com>, Thomas Gleixner <tglx@...utronix.de>, Ingo
Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
<dave.hansen@...ux.intel.com>, <x86@...nel.org>, "H. Peter Anvin"
<hpa@...or.com>, <linux-arm-kernel@...ts.infradead.org>,
<linux-mm@...ck.org>, <linuxppc-dev@...ts.ozlabs.org>,
<linux-kernel@...r.kernel.org>, <kasan-dev@...glegroups.com>,
<wangkefeng.wang@...wei.com>, Guohanjun <guohanjun@...wei.com>
Subject: Re: [PATCH v10 2/6] arm64: add support for machine check error safe
在 2024/1/30 21:07, Mark Rutland 写道:
> On Tue, Jan 30, 2024 at 06:57:24PM +0800, Tong Tiangen wrote:
>> 在 2024/1/30 1:51, Mark Rutland 写道:
>>> On Mon, Jan 29, 2024 at 09:46:48PM +0800, Tong Tiangen wrote:
>
>>>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>>>> index 55f6455a8284..312932dc100b 100644
>>>> --- a/arch/arm64/mm/fault.c
>>>> +++ b/arch/arm64/mm/fault.c
>>>> @@ -730,6 +730,31 @@ static int do_bad(unsigned long far, unsigned long esr, struct pt_regs *regs)
>>>> return 1; /* "fault" */
>>>> }
>>>> +static bool arm64_do_kernel_sea(unsigned long addr, unsigned int esr,
>>>> + struct pt_regs *regs, int sig, int code)
>>>> +{
>>>> + if (!IS_ENABLED(CONFIG_ARCH_HAS_COPY_MC))
>>>> + return false;
>>>> +
>>>> + if (user_mode(regs))
>>>> + return false;
>>>
>>> This function is called "arm64_do_kernel_sea"; surely the caller should *never*
>>> call this for a SEA taken from user mode?
>>
>> In do_sea(), the processing logic is as follows:
>> do_sea()
>> {
>> [...]
>> if (user_mode(regs) && apei_claim_sea(regs) == 0) {
>> return 0;
>> }
>> [...]
>> //[1]
>> if (!arm64_do_kernel_sea()) {
>> arm64_notify_die();
>> }
>> }
>>
>> [1] user_mode() is still possible to go here,If user_mode() goes here,
>> it indicates that the impact caused by the memory error cannot be
>> processed correctly by apei_claim_sea().
>>
>>
>> In this case, only arm64_notify_die() can be used, This also maintains
>> the original logic of user_mode()'s processing.
>
> My point is that either:
>
> (a) The name means that this should *only* be called for SEAs from a kernel
> context, and the caller should be responsible for ensuring that.
>
> (b) The name is misleading, and the 'kernel' part should be removed from the
> name.
>
> I prefer (a), and if you head down that route it's clear that you can get rid
> of a bunch of redundant logic and remove the need for do_kernel_sea(), anyway,
> e.g.
>
> | static int do_sea(unsigned long far, unsigned long esr, struct pt_regs *regs)
> | {
> | const struct fault_info *inf = esr_to_fault_info(esr);
> | bool claimed = apei_claim_sea(regs) == 0;
> | unsigned long siaddr;
> |
> | if (claimed) {
> | if (user_mode(regs)) {
> | /*
> | * APEI claimed this as a firmware-first notification.
> | * Some processing deferred to task_work before ret_to_user().
> | */
> | return 0;
> | } else {
> | /*
> | * TODO: explain why this is correct.
> | */
> | if ((current->flags & PF_KTHREAD) &&
> | fixup_exception_mc(regs))
> | return 0;
> | }
> | }
This code seems to be a bit more concise and avoids misleading function
names, which I'll use in the next version:）
> |
> | if (esr & ESR_ELx_FnV) {
> | siaddr = 0;
> | } else {
> | /*
> | * The architecture specifies that the tag bits of FAR_EL1 are
> | * UNKNOWN for synchronous external aborts. Mask them out now
> | * so that userspace doesn't see them.
> | */
> | siaddr = untagged_addr(far);
> | }
> | arm64_notify_die(inf->name, regs, inf->sig, inf->code, siaddr, esr);
> |
> | return 0;
> | }
>
>>>> +
>>>> + if (apei_claim_sea(regs) < 0)
>>>> + return false;
>>>> +
>>>> + if (!fixup_exception_mc(regs))
>>>> + return false;
>>>> +
>>>> + if (current->flags & PF_KTHREAD)
>>>> + return true;
>>>
>>> I think this needs a comment; why do we allow kthreads to go on, yet kill user
>>> threads? What about helper threads (e.g. for io_uring)?
>>
>> If a memroy error occurs in the kernel thread, the problem is more
>> serious than that of the user thread. As a result, related kernel
>> functions, such as khugepaged, cannot run properly. kernel panic should
>> be a better choice at this time.
>>
>> Therefore, the processing scope of this framework is limited to the user
>> thread.
>
> That's reasonable, but needs to be explained in a comment.
>
> Also, as above, I think you haven't conisderd helper threads (e.g. io_uring),
> which don't have PF_KTHREAD set but do have PF_USER_WORKER set. I suspect those
> need the same treatment as kthreads.
Okay, I'm going to investigate PF_USER_WORKER.
>
>>>> + set_thread_esr(0, esr);
>>>
>>> Why do we set the ESR to 0?
>>
>> The purpose is to reuse the logic of arm64_notify_die() and set the
>> following parameters before sending signals to users:
>> current->thread.fault_address = 0;
>> current->thread.fault_code = err;
>
> Ok, but there's no need to open-code that.
>
> As per my above example, please continue to use the existing call to
> arm64_notify_die() rather than open-coding bits of it.
OK.
Many thanks.
Tong.
>
> Mark.
> .
Powered by blists - more mailing lists