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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191008102511.pmkqcpf7spkogarp@willie-the-truck>
Date:   Tue, 8 Oct 2019 11:25:12 +0100
From:   Will Deacon <will@...nel.org>
To:     Yunfeng Ye <yeyunfeng@...wei.com>
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 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?

Will