linux-kernel - Re: [PATCH v2] arm64: armv8_deprecated: Checking return value for memory allocation

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 for Android: free password hash cracker in your pocket

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

Message-ID: <7b70fec7-e232-0d09-fd51-1fdd205823b8@huawei.com>
Date:   Tue, 8 Oct 2019 19:01:23 +0800
From:   Yunfeng Ye <yeyunfeng@...wei.com>
To:     Will Deacon <will@...nel.org>
CC:     <catalin.marinas@....com>, <will.deacon@....com>,
        <kstewart@...uxfoundation.org>, <gregkh@...uxfoundation.org>,
        <tglx@...utronix.de>, <info@...ux.net>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2] arm64: armv8_deprecated: Checking return value for
 memory allocation



On 2019/10/8 18:25, Will Deacon wrote:
> On Tue, Oct 08, 2019 at 10:33:17AM +0800, Yunfeng Ye wrote:
>> On 2019/10/7 23:37, Will Deacon wrote:
>>> On Mon, Oct 07, 2019 at 06:06:35PM +0800, Yunfeng Ye wrote:
>>>> @@ -617,25 +624,47 @@ static int t16_setend_handler(struct pt_regs *regs, u32 instr)
>>>>   */
>>>>  static int __init armv8_deprecated_init(void)
>>>>  {
>>>> -	if (IS_ENABLED(CONFIG_SWP_EMULATION))
>>>> -		register_insn_emulation(&swp_ops);
>>>> +	int ret = 0;
>>>> +	int err = 0;
>>>> +
>>>> +	if (IS_ENABLED(CONFIG_SWP_EMULATION)) {
>>>> +		ret = register_insn_emulation(&swp_ops);
>>>> +		if (ret) {
>>>> +			pr_err("register insn emulation swp: fail\n");
>>>> +			err = ret;
>>>> +		}
>>>> +	}
>>>
>>> Is there much point in continuing here? May as well just return ret, I
>>> think. I also don't think you need to print anything, since kmalloc
>>> should already have shouted.
>>>
>> The registration of each instruction simulation is independent. I think
>> that one failure does not affect the registration of other instructions.
> 
> Dunno, I think that if kmalloc() starts failing then it's time to give up!
> 
>> In addition, if return directly, is it need to unregister? Of course,
>> the first instruction registration can be directly returned, If the
>> following instruction registration fails, is it need unregister operation?
>> currently the unregistration of instruction simulation is not be implemented
>> yet.
> 
> That's an interesting one -- currently there isn't a way to unregister
> an emulation hook afaict. We could add unregister_insn_emulation() to
> remove the emulation hook from the insn_emulation list and free it, but
> I'm actually now starting to prefer your initial patch after all. The only
> way these failures will happen are either because the system is doomed
> or kmalloc fault injection is being used; so keeping things simple rather
> than add rarely executed complexity is probably best.
> 
>> The purpose of printing information is to replace the direct return, which
>> can distinguish which instruction failed to register. There is no need to print
>> information if it returns directly.
> 
> What do you expect people to do with that information?
> 
> Are you ok with me applying your original patch?
> 
I agree, it is simple. thanks.

> Will
> 
> .
>